[Arieg419]

Issue for this PR

Closes #60442
Closes #60448

Type of change

• Bug fix

What does this PR do?

Apply the existing doom_loop policy across assistant messages, before local tools execute. Three identical calls now reach the permission gate using current agent/session rules. Allow, rejection feedback and continue_loop_on_deny retain their behavior; a new user message resets the history. The scan is bounded to 16 messages.

A file/version-aware guard also catches invalid read offsets that vary or have successful rereads between them. It includes completed reads from the current assistant response. Approval is shared only within the current invocation, avoiding duplicate prompts. Cancellation removes pending permission requests and prevents late execution after approval.

Provider-executed calls retain their event-time policy check, including persisted provider metadata. This stops subsequent continuation; it cannot undo a call already executed by the provider. Generic tool streaming and structured-output contracts are unchanged.

How did you verify your code works?

• Final combined prompt/read/processor/tools/compaction suites: 194 passed, 2 existing projector-disabled skips, 678 assertions, Bun 1.4.4.
• Final package typecheck, Prettier and git diff --check passed.
• Regression cases failed before their fixes. Temporary-file side effects prove that denied local calls do not execute. Coverage includes same-response and cross-response loops, approval/rejection, user/file boundaries, cancellation, terminal metadata and provider-executed calls.
• Both independent final reviews have no unresolved Critical or Important findings: correctness, integration. The reports preserve original findings, closure evidence and verification limits.

All provider responses in these tests are synthetic. This does not establish historical incident causation or repair old session rows. Related #53032 targets the newer dev architecture and includes a separate watchdog; this change targets v1. No active coordination files are affected.

Screenshots / recordings

Nonvisual exemption: tool execution, permissions and terminal-state handling only; no GUI or styling changes.

Draft head 922f9170d9bcb47349abd59cf2819a4649d00ae5 is 0 commits behind freshly fetched origin/v1; GitHub reports MERGEABLE/BLOCKED. Required tests and typecheck are action_required, awaiting maintainer approval. The publisher has read-only upstream permission. This is not a hosted-CI-pass or readiness claim.

Checklist

• I have tested my changes locally
• I have not included unrelated changes in this PR
• Combined behavior suites, typecheck, formatting and diff checks passed
• Both independent final reviews posted and findings resolved
• Required hosted CI passed on the current head

[Arieg419]

Independent GPT-6.1 Sol review (correctness). Workspace paths sanitized; findings and limitations preserved.

Independent correctness and integration review

Scope: four uncommitted files in the isolated PR checkout, base 006d837c5022ea5278162307c04c78412cfae9f3 (origin/v1). Read repository/package guidance, producer/processor changes, permission ordering, metadata persistence and provider-loop tests. All four source hashes match the local verification receipt.

Findings

No Critical or Important finding identified in the scoped patch.

• Ordinary permission denial remains recoverable: Read asks external-directory/read permissions before publishing readRange. The new processor condition requires DeniedError, tool read, and retained readRange metadata. The ordinary read-deny provider case verifies the absence of that marker and continued model recovery.
• Explicit doom_loop allow remains effective: the Read permission request can return normally, after which the ordinary out-of-range error is emitted. It is not classified as a permission block by the processor. The actual provider case reaches the next model response.
• continue_loop_on_deny=true retains continuation: processor derives shouldBreak from that existing setting and only assigns blocked=shouldBreak. The new DeniedError branch follows the same policy as existing interactive rejection handling.
• Metadata survives the error boundary: Read publishes the marker before asking doom_loop; failToolCall reloads the current persisted running part and retains its metadata in the terminal error part. It does not infer range failures from error prose or ordinary permission errors.
• Scope and reset are narrow: matching uses canonical path, stat fingerprint and observed line count; only error read parts after the latest visible user input count. Rotating offsets and successful rereads do not erase earlier matching failures. Changed-file and new-user tests cover reset behavior; canonical aliases share the same key.
• This adds no provider retries, data repair, mutation of file contents, new global state, protocol schema, runtime dependency or permission bypass. Permission requests and filesystem work remain in the existing interruptible Effect flow.

Evidence inspected

The verification receipt records pre-edit RED failures, four actual local provider-loop cases covering stop/ordinary-deny/allow/continue-policy, 17 final processor tests, earlier 119 read/retry/processor tests, final package typecheck, formatting and diff checks. These were inspected, not independently rerun during this read-only review. No live calls, product runtime operation or source writes were performed.

Limits and residual risk

• Detection uses the current model-visible Tool.Context history, not a durable session-wide counter. Compaction and injected/new visible user boundaries can reset the horizon. Failures emitted together in one provider response may not all be present in each concurrent tool's history snapshot; the next provider turn can see the persisted batch. Do not describe this as a strict global third-call guarantee independent of tool batching/compaction.
• A stat fingerprint plus line count is not a cryptographic content version. Same-size, same-line-count changes with unchanged/insufficiently precise timestamps can share a key. This is already disclosed and is proportionate to this narrow guard; no adversarial file-integrity claim is warranted.
• TOCTOU between stat, reading and realpath remains possible on a concurrently modified file. The patch changes gating metadata, not the existing file-read/permission ownership model.
• Explicit allow and continuation-on-deny intentionally permit further attempts; this is not a universal loop killer or a measured model-performance improvement.
• Historical stale rows are not repaired. No claim of live transcript replay is supported by the fixture evidence.

Conclusion: the narrow invalid-range recovery guard and denial-policy integration are coherent with the inspected evidence, subject to the history/stat limitations above. Normal PR review and hosted CI remain separate gates.

[Arieg419]

Independent GPT-6.1 Sol review (integration). Workspace paths sanitized; findings and limitations preserved.

OpenCode read-range patch: independent integration/simplicity review

Reviewed four uncommitted files in read-range-loop against base origin/v1 006d837, repository instructions, surrounding read authorization/tool lifecycle, generic doom-loop permission path, and supplied verification manifest. All four source SHA256 values match the local verification receipt. Read-only review; no test rerun, network/runtime calls or source writes.

Verdict

No Critical or Important finding identified. The patch is scoped and understandable; no broader feature expansion recommended.

Equivalence and behavior

• The failure group is canonical real path + stat-derived file version + observed line count, rather than exact offset/input. Rotating invalid offsets and intervening successful rereads therefore still reach the third-failure gate. File changes and latest-user boundary reset the group. Successful/binary/not-found/directory/read-permission failures have no readRange marker and do not enter the count.
• Both earlier failures must be completed error tool parts. The current tool's streamed marker is attached while running, so it is not accidentally counted as a prior failure. Third matching failure triggers the permission request as intended.
• Canonical path and target stat group symlink aliases without introducing path hashing or a new persistent cache. The single-use logic remains inside ReadTool, following repository style; a separate framework/helper would add complexity without current reuse.

Permission integration

• Existing external-directory and read permission checks still happen before file reading and before range metadata publication.
• New request uses the established doom_loop permission and read tool pattern/always key, consistent with existing generic loop checks. It does not grant read access or bypass ordinary read permissions.
• Narrow processor condition handles DeniedError only for read parts carrying the internally published readRange marker. This marker is published after the ordinary read ask and before the range-specific doom_loop ask, so normal read denials remain recoverable under existing behavior.
• Explicit doom_loop allow still reaches the ordinary out-of-range error. continue_loop_on_deny remains governed by existing ctx.shouldBreak policy. RejectedError/Question behavior is preserved.

Tests and evidence limits

The supplied evidence records RED for rotating offsets and processor continuation, then green read/retry/processor and four policy integration cases plus typecheck/format/diff checks. Tests exercise actual read/processor modules and a local synthetic TestLLMServer; they distinguish denied loop termination from normal read denial, explicit loop allow and continue-on-deny behavior. The manifest correctly notes which119-test run preceded the narrow processor change and records a fresh17-test processor run and four final policy cases afterward. I inspected source/tests and manifest, not a new independent execution.

No claim is made that the installed runtime was replaced, a live transcript replay was performed, historical stale tool rows were repaired, or model performance improved. Full historical/session execution outside the current model-visible history is not covered.

Stat identity is a practical version heuristic, not proof of byte equality: same-size content changes with identical/coarse/restored timestamps and unchanged observed line count can remain in one group. Current metadata strategy and manifest acknowledge this limitation. I do not recommend adding full-file hashing or persistent ownership machinery to this bounded fix.

Counting is limited to current Tool.Context history after the latest user boundary, including its compaction horizon. Explicit permission/config choices may intentionally permit continued attempts. These are documented behavior limits, not hidden completion claims.

[github-actions]

Thanks for updating your PR! It now meets our contributing guidelines. 👍

[Arieg419]

Independent GPT-6.1 Sol correctness review. Publisher verification: the reviewed frozen patch was committed unchanged as 922f9170d9bcb47349abd59cf2819a4649d00ae5; its delta from e425ef0d matches SHA256 5634c0574939c6509e329367f5b662134fe26add55904dfb8e73ff4907dac87c. The final eight-file change passed the five-suite run: 194 tests, 2 existing skips, 678 assertions. Package typecheck, Prettier and diff checks passed. Local checkout path redacted; reviewer findings and limits are preserved. This does not claim hosted CI is green.

Combined loop-guard correctness review

Verdict: no remaining Critical or Important finding in the frozen final working patch reviewed below. This narrow rereview supersedes the earlier e425 history-delegation verdict. It is source/local-contract review, not CI or PR-readiness approval.

Checkout: isolated read-range-loop worktree
Base: 006d837
Frozen content: HEAD e425ef0 plus working diff SHA2565634c0574939c6509e329367f5b662134fe26add55904dfb8e73ff4907dac87c (git diff e425ef0d...).
Full base-to-reviewed-content diff SHA256: 067669534fcae2b373b829869314f1300c936ba0d85deffd6d6f4fb7167d8d0e.

Eight-file scope: session/processor.ts, session/tools.ts, tool/read.ts, and their prompt, processor-effect, tools, compaction and read tests under packages/opencode. Reviewer made no tracked source/test edits.

Final approval and history semantics

• The generic gate returns false when no repeated-call permission decision was needed, and true only after that call's doom_loop permission returns successfully. Denied/rejected decisions throw and never produce a checked approval. Abort is checked before and after the race, so cancellation cannot hand an approval to execution.
• The registry resolver creates a fresh context for each actual execution and carries that invocation's checked result in ctx.extra. ReadTool suppresses only the duplicate range ask for that current invocation. No decision is inferred from prior failure metadata; the earlier history-based generic delegation is removed. Unapproved subsequent calls cannot reuse a previous call's result.
• ReadTool merges captured parts after the latest user boundary with fresh MessageV2.parts(ctx.messageID), keyed by partID with fresh parts winning. This prevents stale captured state from hiding failures and prevents the same part being counted twice. It covers all failures within one assistant message as well as mixed and separate assistant history.
• Only terminal read errors with the same canonical path, version identity and line count contribute. The current running read does not count itself. The captured latest-user slice plus current-assistant binding preserves the user boundary; version changes continue to reset the specialized count. Actual ToolRegistry already provides Database.Service, and the direct read test layer now supplies that dependency explicitly.
• Resolver context.ask retains its reviewed AbortSignal race and pending-request cleanup. Generic beforeToolCall retains the same cancellation semantics while returning its checked result. Successful repeated reads still use the generic gate.
• Host-tool execution remains gated in all5resolver boundaries. Generic AsyncIterable return shapes and StructuredOutput policy remain unchanged. Provider-executed reports retain reactive event-time policy, including persisted providerExecuted metadata; this does not claim to prevent work the remote provider has already done.

Findings closed and credit

• Original merge overlap: third identical invalid read asked twice after one approval. Independent RED at392c17f5 reproduced the second request. Final per-call approval handoff resolves this, and both identical invalid and valid read contracts pass.
• Original range cancellation: cancelling rotating invalid-read permission left a request listed. Independent RED at392c17f5 reproduced it. Resolver context.ask cancellation now removes it; late approval fails and the session becomes idle.
• Follow-up review I2 exposed the insufficiency of the earlier e425 history-based delegation and captured-only range history. Writer's actual ReadTool RED cases reached6/5/6calls instead of3 for same/mixed/varied histories. The final implementation removes that delegation, passes an actual current-call permission decision, and merges current persisted parts. Those findings were not discovered by this reviewer and the old e425 review is superseded, not silently treated as proof of this final content.

Historical e425 review: /tmp/opencode-combined-loop-correctness-review-e425-20261030.md. Original RED observations: /tmp/opencode-combined-loop-correctness-review-red-observations-20261030.md and the session tool transcript. The original outside-source probe log paths were overwritten by GREEN reruns and must not be cited as RED files; root/writer permanent regression artifacts retain separate baseline evidence.

Independent final verification

Pinned Bun1.4.4 with isolated HOME/OPENCODE_TEST_HOME/XDG config/data/state/cache; local synthetic fixtures and real local ReadTool execution only:

bun1.4.4 test test/session/prompt.test.ts test/tool/read.test.ts --test-name-pattern 'invalid reads reach|asks only once|pending invalid-range|pending doom loop|changed file or new user' --timeout30000

Result:8pass,0fail,23assertions; Bun exit0. Exact GREEN output: /tmp/opencode-per-call-correctness-green-20261030.log. Exit receipt: /tmp/opencode-per-call-correctness-green-exit-20261030.txt.

These cases cover same/mixed/varied assistant histories, one approval for identical invalid and valid reads, both generic and specialized pending permission cancellation, and changed-file/new-user reset. The frozen working diff digest was checked unchanged after the run. git diff --check: exit0.

Writer additionally reports20focused cases passing with99assertions, package typecheck exit0, formatting and diff checks passing. Root/writer are running the full5-file suite on the final content. Those broader checks were not repeated by this reviewer.

Limits and next gate

• Root owns the final commit identity, full suite, typecheck evidence, reviewer integration, CI and branch freshness. If the frozen source content changes, this content-specific verdict needs updating.
• Same/mixed/varied fixtures use a local LLM service seam to execute the real resolver/ReadTool in controlled event order. They are not claims of live external-provider performance. Earlier independent AI SDK public-seam tests cover actual bash side-effect prevention, interactive policies and cancellation; see /tmp/opencode-doom-loop-correctness-review-20261030.md.
• Independent checks ran on macOS. Native host execution reaches the same resolver boundaries by source trace; no separate native behavioral campaign ran here.
• No live runtime, protected user configuration, historical DB or research path was modified. No commit, push, PR write or external message was made by this reviewer.

[Arieg419]

Independent GPT-6.1 Sol integration review. Publisher verification: the reviewed frozen patch was committed unchanged as 922f9170d9bcb47349abd59cf2819a4649d00ae5; its delta from e425ef0d matches SHA256 5634c0574939c6509e329367f5b662134fe26add55904dfb8e73ff4907dac87c. The final eight-file change passed the five-suite run: 194 tests, 2 existing skips, 678 assertions. Package typecheck, Prettier and diff checks passed. Local checkout path redacted; reviewer findings and limits are preserved. This does not claim hosted CI is green.

Combined loop integration review

PR: #60443

Initial reviewed head: 392c17f5de8f91c004cf0e9487389fb73aa30618; narrow re-review head: e425ef0d304991042690858bd11a63a41ed52100; final frozen source is that head plus working diff SHA256 5634c0574939c6509e329367f5b662134fe26add55904dfb8e73ff4907dac87c, against 006d837c5022ea5278162307c04c78412cfae9f3 (origin/v1). Read-only review of seven changed files, focused on simplicity, integration, cancellation, permission policy, and the read-range guard. No checkout files changed.

Finding I1: provider-executed calls lost their existing loop guard (closed at e425ef0)

packages/opencode/src/session/processor.ts removes the entire tool-call event-time doom-loop check, while its replacement is invoked only from local Tool.execute wrappers in packages/opencode/src/session/tools.ts.

Provider-executed tools do not invoke those wrappers. src/session/llm/native-runtime.ts explicitly skips local dispatch for event.providerExecuted; the AI SDK adapter also preserves this field on tool-call events. Consequently, repeated provider-executed tool calls never reach beforeToolCall. The previous event-time check did cover them.

Preserve an event-time check for provider-executed calls only. It cannot prevent the already executed server-side call, but must stop subsequent continuation when the loop policy denies it. Client-owned calls should retain the new pre-execution gate.

Independent reproduction

Temporary fixture: /tmp/opencode-provider-loop-review.test.ts.

Log: /tmp/opencode-provider-loop-review-20261030.log.

From packages/opencode, with task-local Bun 1.4.4:

/tmp/opencode-infra-toolchain-20261030/bun-darwin-aarch64/bun test /tmp/opencode-provider-loop-review.test.ts --preload ./test/preload.ts --timeout 30000

The existing processor fixture seam emits three identical lookup calls with providerExecuted: true, with doom_loop: deny configured and no local tools. Actual result: continue, and all three tool rows are completed. Expected result: stop. Command exited 1: 0 passed, 1 failed. This uses a local synthetic provider event stream and an isolated test database, not a live provider.

I1 closure

At e425ef0d304991042690858bd11a63a41ed52100, the processor's tool-call handler uses the updated persisted providerExecuted marker to apply the event-time gate, while local tools remain gated before execution. This also preserves a previously retained provider-execution marker when a later event omits it.

Reran the exact unchanged temporary reproduction above on that head: exit 0, 1 passed, 0 failed. It now returns stop, with the first two provider tool rows completed and the third an error. Green log: /tmp/opencode-provider-loop-review-green-20261030.log.

Finding I2: range delegation skips the current provider-turn history (closed in final frozen patch)

At e425ef0d, gateToolCall delegates a third identical local read to ReadTool when the two preceding persisted errors carry readRange. However, SessionTools.resolve passes ReadTool the pre-turn msgs snapshot from SessionPrompt.loop; the current assistant message is created afterward and is absent from that array.

If the preceding two invalid reads completed within the same provider turn, the generic gate sees those errors and delegates, while ReadTool cannot see either error. The third identical invalid read therefore receives neither doom-loop gate. This reintroduces an extra provider continuation under an explicit deny policy.

Delegate only when the prior range failures are available to ReadTool's history; retaining the generic gate for prior failures in the current assistant message is one small option. Mixed current/prior-turn cases need the same care.

Independent I2 reproduction

Fixture: /tmp/opencode-same-turn-read-review.test.ts.

Log: /tmp/opencode-same-turn-read-review-20261030.log.

From packages/opencode, task-local Bun 1.4.4:

/tmp/opencode-infra-toolchain-20261030/bun-darwin-aarch64/bun test /tmp/opencode-same-turn-read-review.test.ts --preload ./test/preload.ts --timeout 30000

This uses the actual SessionTools and ReadTool executors, sequentially within one synthetic provider stream, against an isolated three-line file and test database. No historical rows are seeded or modified. Three identical out-of-range requests all persist under the same assistant message with ordinary range errors despite doom_loop: deny; the policy only activates in the following provider turn. The test observes six tool rows rather than the expected three, exits 1, with 0 passed and 1 failed. This is a local fixture reproduction, not live-provider evidence.

I2 closure and final narrow review

The final frozen patch removes the delegation heuristic. The generic gate returns a boolean only after that invocation matched the exact-call threshold and permission succeeded. The registry captures it in a newly constructed invocation context as extra.doomLoopPermissionChecked; ReadTool accepts only literal true when skipping its secondary permission request. The flag is neither persisted nor reused across invocations, so a one-time approval cannot become a durable grant.

For invalid ranges, ReadTool also fetches the current assistant's persisted parts and merges them with captured history after its newest user boundary, deduplicating by part ID with fresh parts winning. This makes current-turn and mixed-turn failures visible while retaining canonical-path, version, and line-count grouping. The database requirement is declared, the direct read harness now supplies Database.node, and the production ToolRegistry already supplies it. Ordinary successful reads do not issue the new current-parts query.

Independently reran the exact unchanged I2 fixture against the frozen patch: exit 0, 1 passed, 0 failed, 2 assertions; exactly three calls, with the third denied. Green log: /tmp/opencode-same-turn-read-review-green-20261030.log.

Also ran a second temporary fixture changing only offsets from identical 10 to 10/11/12 within the same provider turn: exit 0, 1 passed, 0 failed, 2 assertions; again exactly three calls and the third denied. Fixture: /tmp/opencode-varied-same-turn-read-review.test.ts; green log: /tmp/opencode-varied-same-turn-read-review-green-20261030.log.

The exact-call denial occurs before the third ReadTool body; the varied-offset case reaches the version-aware range gate. Both are local synthetic provider-stream tests using the actual registry/read executors and isolated test data. I inspected the writer's same/mixed/varied regression cases and paired one-approval cases; I did not independently repeat the full suite or mixed-history case.

Other reviewed contracts

• Client-owned calls enter beforeToolCall before plugin hooks and tool bodies, including standard tools, MCP resource tools, and ordinary MCP tools.
• The permission wait is raced against the call abort signal; interruption removes the pending permission through Permission.ask finalization, and the post-wait signal check prevents execution after cancellation wins a reply race.
• Current session rules are merged with agent rules. Explicit allow and continue_loop_on_deny preserve their intended control paths.
• The exact-call guard anchors on the current persisted tool part and uses a bounded 16-message window after the newest user boundary. It retains the existing JSON serialization equality policy rather than introducing semantic argument normalization.
• The read-range guard remains independent: canonical file identity and version group invalid offsets across messages despite successful rereads. Ordinary read permission denials do not gain the range marker.
• No tool input schema, provider generator contract, SDK-generated file, or structured-output generator contract changed.
• I found no additional Important/Critical issue in these areas. The writer's paired valid/invalid read tests cover one-time approval without a duplicate prompt; I inspected that per-invocation coordination but did not independently repeat those paired tests.

Status

I1 and I2 are closed with recorded RED-to-GREEN evidence. No remaining Important/Critical finding in the final frozen source identified above. Root owns final combined checks, immutable commit/publication, and CI readiness. The generic equality policy remains exact JSON serialization with a bounded history window; provider-side execution of the current event cannot be undone by the retained event-time gate. These fixtures are not live-provider evidence or a measured performance claim. Broad suites were not repeated for this review. No checkout files were edited by the reviewer.