[Arieg419]

Issue for this PR

Closes #60608. Related #60442 for the current v2 runner.

Type of change

• Bug fix

What does this PR do?

Fixes two read-tool failure modes in v2:

• A selected blank line was displayed as 0 lines. Text pages now retain their selected-line count, preserving blank lines while genuinely empty files still report zero lines.
• Repeated invalid offsets could keep cycling when the requested offset changed. After two settled invalid reads of the same file contents since the last delivered user input, the next invalid read asks the existing doom_loop permission. The identity uses a hash of bytes already scanned at EOF; file changes reset the match. Valid rereads remain available.

Explicit allow retains ordinary offset errors. Configured denial or rejection of the guard prompt stops the step and releases its execution claim. Feedback and shutdown retain their existing distinct behavior. Already-running parallel calls can finish before their failures count. Permissions documentation describes those boundaries.

This covers neither generic identical-call loops/watchdogs (#53032), directory EOF offsets (#60029), nor the separate v1 implementation (#60443). Both v2 fixes share the reader and its tests. No active coordination files are affected.

How did you verify your code works?

• Behavioral regressions failed before their fixes: blank-only pages, EOF identity, changing offsets, renamed tools, provider-retry precedence, and rejected-prompt claim retention.
• Combined head: 514 tests passed across ten affected read, permission and session suites. Core Bun and official TypeScript checks passed.
• Root lint passed (zero errors; 1,370 warnings). All 35 repository typecheck tasks passed, including both Astro packages, with a task-local NODE_PATH and turbo typecheck --concurrency=3 --env-mode=loose. The unmodified default environment had failed to resolve @tsconfig/bun/tsconfig.json; no dependency, lockfile or config change was needed.
• Independent adversarial reviews cleared both fixes after repairing alias matching, denial/retry precedence and rejected-prompt claim retention. Final combined review passed 84 focused tests and four independent blank-page/EOF/guard scenarios with no actionable findings. Correctness review, integration review, finding resolutions, and combined review.
• Diff checks passed. Tests use synthetic provider streams and real temporary files; no installed service or live provider was exercised. Windows path-case equivalence was not tested on a Windows host.

Screenshots / recordings

Screenshot evidence remains blocked for /v2/docs/permissions/: the actual Cloudflare-backed Astro preview exited during dependency optimization with a missing deps_ssr/base-*.js chunk. Both desktop and narrow capture await a renderable preview. Docs typechecking passed. Local diagnostics are retained in /tmp/opencode-v2-reliability-20261030/; these are not reviewer screenshot evidence. Owner/next step: author restores the docs preview and attaches captures. UI review remains incomplete.

Visual

<svg xmlns="http://www.w3.org/2000/svg" width="240" height="72" viewBox="0 0 240 72">
  <style>@keyframes pause{0%,40%{opacity:.45}60%,100%{opacity:1}}.pause{animation:pause 3s ease-in-out infinite}@media(prefers-reduced-motion:reduce){.pause{animation:none}}</style>
  <g fill="none" stroke="currentColor" stroke-width="1.5">
    <rect x="8" y="8" width="84" height="56" rx="6" opacity=".5"/>
    <path d="M21 22h56M21 36h10M21 50h56" opacity=".6"/>
    <path d="M106 36h28m-6-6 6 6-6 6"/>
    <rect class="pause" x="148" y="16" width="82" height="40" rx="7"/>
    <path class="pause" d="M181 27v18m15-18v18" stroke-width="4"/>
  </g>
</svg>

Checklist

• I have tested my changes locally
• I have not included unrelated changes in this PR

Current head 89b6074860 is zero commits behind fetched dev (6b4329c885) and GitHub reports MERGEABLE/BLOCKED. The check, check-test, check-nix, and check-bot-scope workflows require maintainer approval (action_required); PR triage passes. This remains a draft pending hosted CI, documentation screenshots and upstream review. The submitting account has read-only upstream access, so it cannot approve fork workflows or perform an admin merge.

[Arieg419]

Blank-page read fix: independent review

Result: no actionable findings. The four-file patch is surgical and preserves the parser's existing line, byte-budget, EOF, and continuation semantics.

• Original base/head: 8f0fc21e718e8e1ea60aee6ca04875c35cb7a876 / c85d4ca09ce08399fac265be907633d249226b52.
• Reviewed rebased base/head: 65c0d83f4dcfb40813c64658fbcbb86d3ba53ec1 / 2ee19919c1369ef8038281a3ecb34413215d4075.
• Both patches have stable patch ID eca01be2e0b811062188518e01c8c46b5c56aab8.

Correctness and compatibility

packages/core/src/tool/read-filesystem.ts:86 adds the selected-line count to the producer-owned TextPage output. Its sole production constructor assigns result.entries.length at :254, preserving the distinction between zero selected lines and one selected blank line even though both serialize to an empty string.

packages/core/src/tool/plugin/read.ts:197 uses the count only to decide whether an empty page contains a selected line. Other pages retain the established split/join formatting; whole-file, media, and directory branches are unchanged. The count describes selected returned lines, not the file's total lines or UTF-8 bytes.

Repository-wide TextPage/text-page consumer inspection found only the production read path, its tests, and the TUI's permissive legacy JSON-envelope parser (packages/tui/src/mini/tool.ts:169). The TUI reads existing type/content fields and ignores the new property. The in-repo constructed TextPage fixtures were updated. Durable Tool.Success events retain normalized model content and metadata, not a TextPage machine-output object (packages/core/src/session/runner/publish-llm-event.ts:617); historical sessions therefore do not require the new field. The patch does not change the public Protocol or Server HttpApi, so the client-generation rule does not apply.

The required output field is additive for produced values but is a source-level change for external code that manually constructs TextPage; no such external consumers were inspected. No compatibility alias or speculative fallback is warranted by the repository evidence.

Independent verification

• bun run test -- test/tool-read-filesystem.test.ts test/tool-read.test.ts from packages/core: exit 0, 61 pass, 0 fail, 157 assertions. This ran before the rebase; the reviewed patch is unchanged.
• A separate inline oracle against the actual implementation on rebased head: exit 0, 150 cases passed, covering empty files, one/multiple blank lines, LF/CRLF, Unicode, terminal-newline presence, offsets 0–4, limits 1–3, and out-of-range errors. Expected lines were computed from the source text independently of TextPage serialization.
• A second oracle on rebased head: exit 0, 33 cases passed, covering reads across the 256 KiB first-chunk boundary, UTF-8 byte-budget truncation, large blank-only pages, noninitial offsets, EOF, and exact next-offset/truncated values.
• git diff <rebased-base>..HEAD --check: exit 0. Working tree was clean at review completion.

An initial one-off oracle command used the removed Effect.either API and failed in the review harness before completing its assertions; it was corrected to Effect.exit and both completed oracle runs passed. This was not a product failure.

Limits

No provider calls, live runtime tests, UI checks, edits to the committed tree, root full check, or independent typecheck were performed by this reviewer. Parent-reported red/green/repetition/typecheck evidence is not counted as independent review execution. Root-check Astro configuration diagnostics remain for the parent to characterize against baseline.

What's Next In This Stream

Complete the parent's current-head required checks and PR freshness gates. The separate invalid-offset/read-loop guard remains owned by v2_offset_scope and was not included in this review.

[Arieg419]

Invalid-offset read guard: independent correctness review

No Critical or Important correctness defects found in the pinned patch. One Minor documentation mismatch remains below. This is a focused invalid-offset guard, not generic repeated-tool or successful overlapping-read detection.

• Worktree: isolated v2-read-loop worktree
• Base: 8f0fc21e718e8e1ea60aee6ca04875c35cb7a876
• Reviewed head: 39574b164d9f1329eaf24bb929e58ca528735a0d
• Review was read-only; no files in the worktree were changed.

Findings

Minor — permission documentation contradicts the new behavior. services/www/src/docs/content/permissions.mdx:139 says that doom_loop is not a current V2 Core permission action. The new read guard invokes it at packages/core/src/tool/read-range.ts:67 and the base policy now asks at packages/schema/src/agent.ts:48. Replace the obsolete statement with the actual narrow behavior: third settled invalid-offset attempt for the same path/content fingerprint since promoted user input asks, explicit allow retains ordinary errors, configured deny stops execution. This does not block the runtime fix.

Correctness assessment

• EOF identity: the fingerprint is calculated only when the parser has no selected lines for a noninitial offset, after scanning to EOF (read-filesystem.ts:252–262). It hashes existing read chunks, without an extra file read. It does not substitute size or mtime for content identity. Empty files, blank files, Unicode, long lines, and multi-chunk reads produced matching SHA-256 values in independent checks.
• History: read-range.ts:28–62 starts with the owning assistant, pages backwards through canonical projections without hydrating media, and stops at a promoted user message. The descending-order cursor moves towards older rows. Queued input and synthetic context do not clear history. Files, sessions, content versions, and total line counts remain distinct.
• Aliases: counting validated owned metadata at read-range.ts:40–46 no longer depends on the advertised tool name. The earlier alias defect is closed; the committed integration test and independent helper oracle both pass.
• Failures: ordinary successful reads, running calls, invalid-schema arguments, directories, media, and binary failures do not acquire or satisfy invalid-offset history. Alternate filename recovery invokes the same read wrapper with the alternate target, preserving target identity.
• Permissions: the default rule now reaches a real ask request; explicit allow retains the original offset error. Configured deny raises a specific ToolFailure subclass, which the runner recognizes without treating it as a human rejection. Correction remains a model-visible tool error. Genuine ask rejection is classified separately and ends with Execution.Failed, releasing the execution claim; shutdown follows the existing interruption path and retains the claim for recovery.
• Settlement: range policy failure is recorded only after provider/tool settlement (step.ts:187–189), avoiding concurrent fragment flushing. It blocks compaction/recovery/retry/continuation paths (:174, :203, :211, :283) and reaches StepFailedError (:293). The earlier retry bypass and default-reject claim findings are closed by inspection and passing committed regression tests.
• Parallel boundary: only settled terminal failures count. A parallel batch can therefore overshoot the nominal third-call threshold before its terminal events are committed; the next request sees those failures. This limitation is explicit in the code comment (read-range.ts:64) and is not presented as strict per-call admission. The implementation adds no in-memory execution loop or speculative reservation machinery.

Independent verification

• bun run test -- test/tool-read-range.test.ts test/tool-read-filesystem.test.ts test/tool-read.test.ts, from packages/core: exit 0, 77 pass, 0 fail, 230 assertions.
• Separate inline oracle using the actual ReadRange helper: exit 0, 12 cases passed, including alias names, current-response failures, file/version/session isolation, promoted-user boundaries, synthetic context, multi-page history, and exclusion of running or successful calls. It also checks that the gate progress marker appears exactly when permission is checked.
• Separate inline oracle using the actual filesystem parser and memory environment: exit 0, 14 invalid-offset cases passed, spanning empty/blank/CRLF/Unicode text, a long line, a 262 KiB blank sequence, and a large multi-chunk Unicode file. SHA-256, total line counts, and exact contiguous read offsets matched independent expectations. A separate binary-file assertion confirmed that an out-of-range request still produces BinaryFileError instead of range history.
• The 471-test result, package typechecks, formatting, and root-check progress supplied by the parent are not counted as independent executions in this review.

Limits

No provider calls, installed-app lifecycle operations, Windows execution, live external model sessions, root full check, or independent TypeScript compilation were performed. The committed claim test exercises the real step and real execution coordinator through a deliberately minimal test runner, not a paid provider.

Path identity reuses FSUtil.resolve. On Windows that utility preserves requested spelling for case-only realpath differences (packages/util/src/fs-util.ts:108–110); casing aliases may therefore form separate counters. This is an unverified cross-platform corner, not a measured failure on the current macOS target. The content digest describes the bytes observed by the read; it does not introduce an atomic filesystem snapshot under concurrent external writes.

What's Next In This Stream

Correct the Minor documentation mismatch if included in scope, finish the parent's required checks, rebase onto the latest target branch, and verify the final PR head. Reconfirm patch equivalence after rebase; changes to this reviewed implementation need another focused review. Generic repeated-tool/stall behavior remains separately covered by upstream PR #53032.

Supplemental review: documentation and parallel coverage

Reviewed 39574b164d9f1329eaf24bb929e58ca528735a0d..2810b9fd166b10e668b727c7e4649c4fba15f0cb independently. It changes only the permissions document and a focused read-range test; the reviewed runtime and schema implementation are unchanged.

The Minor documentation finding is closed. The action table now includes doom_loop, the prose explains the actual invalid-offset threshold/default/allow/deny/reject/parallel behavior, the obsolete exclusion is removed, and the ordered base-policy example contains the new ask rule. No Critical, Important, or remaining Minor findings were identified in the supplemental diff.

The new parallel-pair test exercises concurrent read execution, checks both ordinary terminal failures, verifies that the next settled-history read is gated, asserts one terminal state for each distinct call ID, and confirms session isolation. It does not claim strict per-call admission within a larger parallel batch.

Independent verification: bun run test -- test/tool-read-range.test.ts --test-name-pattern 'parallel pair' from packages/core exited 0: 1 pass, 0 fail, 18 filtered, 6 assertions. Supplemental git diff --check exited 0; git diff ... -- packages/core/src packages/schema/src was empty. Previous oracles were not rerun because the runtime patch did not change.

Remaining work belongs to the parent: latest-base rebase, required current-head checks, and final PR freshness verification. The platform/evidence limits in the original review still apply.

Parent verification after rebase

The final submitted head is 8afd6fdf8b4e06a6a8f18da0d4035b4725fe6812 on base 6b4329c88579c5b100620c8e72e8ca97b1baf3d0. Its combined patch matches the reviewed runtime and supplement (stable patch ID 389d0b74ada1888ef6402fc14d6e2b7f51f47a1d). The parent reran 508 affected tests and both Core typecheckers successfully on that head. This is local verification; hosted CI and docs rendering remain separate incomplete gates.

Combined PR verification

This reviewed guard was cherry-picked unchanged into the existing v2 read fix PR after GitHub rejected a second PR at its three-open-PR contributor cap. Combined runtime head: 89b6074. The parent passed 514 affected tests, both Core typecheckers, root lint and all 35 repository typecheck tasks (with task-local NODE_PATH passed through Turbo). A further independent combined review passed 84 focused tests and four additional integration scenarios, with no actionable findings.

[Arieg419]

Guard independent integration review

Verdict

No actionable Critical or Important findings at reviewed commit 39574b164d9f1329eaf24bb929e58ca528735a0d, against base 8f0fc21e718e8e1ea60aee6ca04875c35cb7a876.

Review worktree: isolated v2-read-loop worktree. Source was clean and pinned to that head during inspection and test execution. No worktree files, dependencies, configuration or services were changed by this review.

This verdict applies to the explicit contract: the gate activates when two matching invalid-range failures have already settled since promoted user input. It is not an atomic admission limiter for an initial parallel burst.

Supplemental commit 2810b9fd166b10e668b727c7e4649c4fba15f0cb was inspected read-only after this verdict. It changes only the guard test file and permissions documentation; runtime source is unchanged. No additional Critical or Important finding arose. The parent is rebasing and owns execution of the new test on the combined final head; this reviewer did not repeat unchanged suites or claim a measured result for the supplemental test.

Independent verification

From packages/core, with Bun 1.4.4 on PATH:

bun run test test/tool-read-range.test.ts test/session-runner.test.ts test/session-execution.test.ts

Exit 0: 296 pass, 0 fail, 1423 assertions, three files, 8.97s.

Log: local author test log; not hosted CI.

git diff --check also exited 0. Official TypeScript and full root gate checks were parent-owned and still running when this review began; this report does not claim their result or remote/CI readiness. The parent owns rebasing onto the latest base and confirming the final combined head.

Earlier findings closed

1. Renamed read tools: ReadRange now counts validated leaf-emitted range metadata rather than requiring provider-visible tool name read. The real registry/publisher test advertises the read definition as view and verifies the third invalid range is gated.
2. Provider retry precedence: rangeFailure suppresses context-overflow recovery, full-continuation recovery, retry-decision invocation, and the post-settlement Continue path. The terminal typed StepFailedError is returned before propagating provider stream failure. The regression uses a retry-enabled preparation and a transport failure carrying recovery: "retry-full" after the actual tool call; it requires the saved assistant's doom_loop failure and typed stop.
3. Default prompt rejection and execution claims: the runner only marks gate-reaching call IDs, recognizes an actual Permission.DeclinedError on one of those calls, and returns a typed permission.rejected failure when neither location closure nor interruption indicates teardown. The lifecycle tests exercise the actual read leaf, Permission service, SessionModelRequest decline recovery, SessionStep, and real SessionExecution layer/coordinator. Rejecting the actual prompt removes the durable execution claim; closing the execution scope while the prompt is pending preserves the claim.

Integration assessment

• Permissions: default policy is doom_loop: ask; explicit session allow and deny override through normal permission evaluation. Explicit allow permits repeated invalid ranges, consistent with user policy. Approve, reject, reject-with-feedback and cancellation use real Permission replies rather than fabricated outcomes. Feedback remains model-visible tool failure and does not trigger the plain-rejection stop. Pending permission requests are removed after reply/interruption.
• Lifecycle and terminal events: actual lifecycle tests require exactly one tool failure, one step failure, and one execution terminal for both rejection and shutdown. Rejection is a typed failed execution that releases the claim. Shutdown remains interrupted and recoverable. Publisher settlement prevents later unsettled-tool cleanup from issuing a duplicate terminal for an already settled call; Step.Ended is excluded once the assistant has a failure.
• History boundaries: queries are Session-scoped, descend from the current assistant message, and stop at a promoted user message. Synthetic context and queued input do not reset the detector. The paging test places prior failures beyond the first 16-message page. Canonical projected metadata supports reconstruction without process-global counters or another durable ledger.
• Identity/progress: matching uses canonical resolved path, actual EOF-byte SHA-256 and total line count. Content mutation with preserved size/mtime is covered; other files/sessions do not collide. Valid reads do not erase invalid-range history. Hashing reuses bytes already scanned to EOF and does not reread the file.
• Architecture/simplicity: policy stays in the built-in read leaf; the registry is not given a second authorization path. The helper captures existing store/policy dependencies, and the runner adds only per-attempt failure/gate state needed for settlement and termination. No new in-memory model loop, universal tool-loop framework or runtime-wide permission behavior change was introduced.

Precise limits

• Only settled matching failures count. Concurrent invalid reads that begin before two prior failures settle can complete without the gate; once the threshold is visible, later calls use the permission policy. No hard bound on first-burst parallel executions is claimed. The supplemental test now schedules a real pair with Effect.all(..., { concurrency: 2 }), joins their settlement, then requires the third call to gate. It also verifies the three projected calls are unique terminal errors and a second session's first invalid read is not blocked. This closes the previously missing dedicated concurrent-call coverage note for the stated settled-failure boundary. It does not assert simultaneous admission with a barrier or count raw terminal events for that pair; the earlier rejection/shutdown tests directly count terminal events.
• The claim fixture replaces the outer drain with one real SessionStep and uses TestLLM plus a snapshot double. It proves the production step-to-execution lifecycle integration, not a live provider, daemon, deployed session, or a wholly unmocked multi-step drain.
• Default-ask feedback/approval and cancel behavior have leaf-level coverage; actual execution-claim tests cover rejection and shutdown. A browser UI prompt rendering test was not run or needed for this backend change.
• The supplemental permission docs accurately name the narrow invalid-text-offset trigger, two prior completed failures, default ask/explicit allow/deny behavior, and already-running parallel-call boundary. They remove the stale claim that doom_loop is absent from V2 and include the new default agent rule.
• Directory EOF behavior, default read caps, generic loop detection, and the separate blank-line serialization fix remain separate lanes. This review does not credit this patch with those behaviors.

What's next in this stream

Parent owns latest-base rebase, any resulting integration checks, the outstanding official/root check results, and draft PR preparation. No other implementation work was started by this reviewer.

Parent verification after rebase

The final submitted head is 8afd6fdf8b4e06a6a8f18da0d4035b4725fe6812 on base 6b4329c88579c5b100620c8e72e8ca97b1baf3d0. Its combined patch matches the reviewed runtime and supplement (stable patch ID 389d0b74ada1888ef6402fc14d6e2b7f51f47a1d). The parent reran 508 affected tests and both Core typecheckers successfully on that head. This is local verification; hosted CI and docs rendering remain separate incomplete gates.

Combined PR verification

This reviewed guard was cherry-picked unchanged into the existing v2 read fix PR after GitHub rejected a second PR at its three-open-PR contributor cap. Combined runtime head: 89b6074. The parent passed 514 affected tests, both Core typecheckers, root lint and all 35 repository typecheck tasks (with task-local NODE_PATH passed through Turbo). A further independent combined review passed 84 focused tests and four additional integration scenarios, with no actionable findings.

[Arieg419]

Read-range guard: adversarial review and resolution

All three findings are resolved in guard head 8afd6fd and retained in the combined PR head 89b6074. Both independent final reviews cleared the runtime and supplement; parent current-head tests and Core typecheckers passed.

Finding	Required resolution	Status
Configured stop can still enter a provider retry/continuation path.	Stop must take precedence, with a mixed denied-read/retryable-provider-failure regression.	Resolved; regression passes
History matching uses advertised tool name read, missing supported context-hook aliases.	Identify prior real read failures by owned metadata; test renamed native read tool.	Resolved; regression passes
Default ask rejection can be classified as shutdown and preserve a resumable execution claim.	Distinguish actual guard rejection from location shutdown, retain feedback continuation, verify terminal events and claim release through real execution.	Resolved; regression passes

The guard counts settled failures. Calls already in flight within one provider batch may finish before subsequent calls observe durable metadata. This is a declared concurrency boundary, not a hard in-flight admission cap.

[Arieg419]

Combined read fixes: final integration review

No actionable findings. Combining the blank-page serialization fix and the invalid-offset guard introduces no observed interaction defect. The changes remain independently owned by their existing parser success/error branches.

• Worktree: isolated v2-read-validation worktree
• Base: 6b4329c88579c5b100620c8e72e8ca97b1baf3d0
• Reviewed head: 89b6074860f9df20b91c00ad30b72610f12a8ddc
• Read-only review; no worktree edits. Working tree was clean at completion.

Integration assessment

packages/core/src/tool/read-filesystem.ts keeps two distinct line counts: the successful TextPage's lines is result.entries.length, while an OffsetOutOfRangeError's lines remains the full file's line count. The selected-line count is populated only after the invalid-offset branch, so a valid selected blank line is not mistaken for an invalid offset or an empty file. EOF fingerprints still hash the already-read chunks only when a noninitial offset selects no entries.

packages/core/src/tool/plugin/read.ts invokes ReadRange only on the specific OffsetOutOfRangeError with an available fingerprint. Successful blank pages instead reach toModelContent, where output.lines disambiguates an empty string containing one selected blank line from zero selected lines. This field is not used as a file-revision key or confused with totalLines in guard history.

The guard/history/permission/runner code and documented limits match the previously reviewed guard implementation. The default-policy documentation issue remains closed. Earlier alias, retry-continuation, and genuine-rejection execution-claim findings remain addressed.

Independent verification

• bun run test -- test/tool-read-range.test.ts test/tool-read-filesystem.test.ts test/tool-read.test.ts from packages/core: exit 0, 84 pass, 0 fail, 248 assertions. This exercises the combined production parser, output schema, read tool, guard history, permissions, and runner settlement regressions.
• A separate inline integration oracle on the combined head passed 4 scenarios: an empty file, one blank line, a noninitial blank EOF line, and a 262,150-line blank file spanning the first-chunk boundary. Each scenario alternated three valid/blank reads with three invalid offsets and verified selected-line model formatting, full-file EOF line count, SHA-256 identity, exclusion of successful reads from failed history, and one permission check on the third invalid attempt.
• git diff 6b4329c885..89b6074860 --check: exit 0.

The first inline oracle invocation had a JavaScript parenthesis syntax error before execution; the corrected invocation completed and passed. This was a review-harness error, not a product failure.

Limits and next step

No provider calls, developer-runtime lifecycle actions, full 500-test rerun, independent typecheck, Windows execution, or PR/CI freshness check were performed. The broader checks belong to the parent. The prior limits remain: simultaneous calls can overshoot within a batch before their failures settle; Windows case-alias behavior is unverified; fingerprints describe bytes observed by a read rather than an atomic filesystem snapshot.

Finish the parent's current-head checks and final PR freshness verification. If the base moves again, preserve this runtime patch or review any resulting semantic changes before delivery.