Repository navigation
feat(opencode): restore doom loop detection as internal plugin for v2 - #61328
metal-huang wants to merge 7 commits into
Conversation
![opencodejr[bot]](https://avatars.githubusercontent.com/in/5497873?s=60&v=4){kind=small}
There was a problem hiding this comment.
The detector works end to end. I ran OpenCode from source with the simulated model making the same read call once per step and doom_loop: ask. On the base, all reads ran with no prompt. With this PR, the 7th call asked for doom_loop. The new tests (65) pass and tsc -b for packages/core is clean.
Public API: no new or changed config keys, events, endpoints or plugin API. The behaviour of existing setups does change: the existing doom_loop permission is now actually requested. Users without an allow rule will see prompts, and unattended sessions get a denied tool call instead.
Concerns, most important first:
- The circuit breaker skips the permission system. Every other signal goes through
doom_loop, but the breaker at 12 low-novelty outputs interrupts the session even whendoom_loopis allowed. With*: allow, my run of identical reads stopped after 14 calls. The TUI showed nothing about why, and the model's final text was lost. Until the user sends a message, every tool call then fails. This contradicts the description ("every challenge goes through the existingdoom_looppermission ask"). Please route the breaker throughdoom_looptoo, or at least respectallow, and show the user why the session stopped. - A and C streaks never decay. They survive user messages and execution boundaries, and other calls in between don't reset them. Only a changed output (A) or a success of the same tool (C) resets them. In a long session where the user asks a few times to re-check an unchanged file, or
git statusstays clean, the 7th check (6 identical outputs earlier) prompts, even though there is no loop. Signals that are meant to catch runaway loops should probably be scoped to the current run or turn, or decay over time. - Design overlap with #53032 and #60443. Both also start raising
doom_loop, in the runner. Maintainers will need to pick one direction. Merging more than one would ask twice for the same loop. - Brittle coupling. The plugin depends on host details by matching strings: tool names (
execute,shell,question), theInvalid arguments for toolmessage prefix, and the runner's error type names infold.ts. If any of these change upstream, detection quietly misses or misfires. - Tests and test seam. 1,780 lines of tests for about 700 lines of code is a lot to maintain, and much of it pins the exact thresholds and wording. Please cut them down to the core cases: each signal firing, the reset or escape paths, and the history rebuild. The module-level
setMaxTrackedSessionsseam changes state shared by all plugin instances; injecting the cap would avoid that.
The TUI also shows these prompts as "Continue after repeated failures", which doesn't fit signals A and B (identical successful outputs).
| messageID: event.messageID, | ||
| }) | ||
| if (verdict.kind === "pass") return | ||
| if (verdict.kind === "circuit") { |
There was a problem hiding this comment.
This hard stop never consults the doom_loop rule, so it fires even with doom_loop: allow. I reproduced it with *: allow: identical reads stopped after 14 calls, the final assistant text never arrived, and the TUI said nothing about why. After that, every tool call fails until the user sends a message. Please route this through permission.assert like the other signals (or at least respect allow), and show the user why the session stopped.
|
|
||
| // User message: the circuit breaker's defined escape hatch. B resets, the breaker re-arms; A/C | ||
| // survive across user messages (a user interjection must not hand the loop a fresh counter). | ||
| export function userMessage(current: SessionState) { |
There was a problem hiding this comment.
Because A and C survive user messages and other calls in between never reset them, a streak builds up over the whole session. Re-checking an unchanged file or git status across several separate user requests eventually triggers a doom_loop ask with no loop going on. Consider resetting or decaying A and C per run or turn. A real loop rebuilds its streak within a few calls anyway.
| if (current.bStreak >= B_BLOCK && entry !== undefined && entry.streak >= A_WARN) { | ||
| return { kind: "assert", signal: "b", message: copy.blockB } | ||
| } | ||
| const failure = current.failures.get(call.tool) |
There was a problem hiding this comment.
C is keyed by tool name only, and only a success of the same tool resets it. Six different shell commands failing with the same normalized message (e.g. a generic non-zero exit or the same missing binary) would block the next shell call, even when its input is unrelated. Keying on signature, or on tool plus a normalized input, would avoid that.
There was a problem hiding this comment.
The only new commit merges dev. The doom-loop plugin, detector, fold and tests are unchanged, so the three earlier points (circuit breaker bypassing doom_loop, A/C streaks never decaying, C keyed by tool name only) are still open. On the merged head, the 65 doom-loop tests pass, tsc -b for packages/core is clean, and the doom_loop prompt still appears on the 7th identical read with the simulated model. The public API is unchanged by this push. Nothing new to raise.
…counters per turn
|
Thanks for the thorough review. All five points are addressed in
66 tests pass and |
There was a problem hiding this comment.
The circuit breaker now goes through doom_loop, and a user message resets A and C. I re-ran the circuit scenario with the simulated model (16 identical reads under *: allow): the run now completes, where before it was silently interrupted after 14 calls. The doom-loop tests pass (66) and tsc -b for packages/core is clean. C is still keyed by tool name only (open thread). The public API is unchanged by this push. Two small points on the new denial path are inline.
| source: { type: "tool", messageID: event.messageID, id: event.id }, | ||
| }), | ||
| ) | ||
| if (Exit.isSuccess(outcome)) { |
There was a problem hiding this comment.
Every failure is treated as a hard stop here, including a CorrectedError, where the user rejects with feedback for the model. That feedback is put in the Tool.Error, but the session is interrupted and the breaker made sticky right away, so the model never acts on it. Treating a correction like the other signals do (return the feedback, don't interrupt) would keep that escape hatch.
outcome.cause.toString() renders as Cause([Fail(Permission.BlockedError: The user rejected ...)]), so internal Effect text ends up in the tool error the model sees. Use the error's message instead, as the other signals do via mapError (e.g. Cause.squash or matching the failure).
There was a problem hiding this comment.
Thread 3 is addressed: failure streaks are now counted per call (tool plus input), both live and in the history rebuild, so unrelated calls of the same tool no longer add up. Thread 4 is addressed: when the user rejects the circuit-breaker prompt with feedback, the model gets that feedback and the session isn't stopped or locked, and the message uses the error text instead of the internal Cause([...]) string. Threads 1 and 2 were addressed in the previous push and are unchanged. The doom-loop tests pass (67) and tsc -b for packages/core is clean. A re-run of the 16-read circuit scenario with *: allow completes normally. The public API is unchanged by this push. Nothing new to raise.
There was a problem hiding this comment.
This push only merges dev. The doom-loop plugin, detector, fold and tests are unchanged. On the merged head, the 67 doom-loop tests pass, tsc -b for packages/core is clean, and the doom_loop prompt still appears on the 7th identical read with the simulated model. The earlier points were addressed in previous pushes. The public API is unchanged by this push. Nothing new to raise.
|
One more distribution path, independent of the upstream decision: the same implementation is now published as a standalone npm plugin, opencode-doom-loop-detector, so users can install it today. It uses only the public plugin API, has 45 tests of its own, and needs no runner changes. https://www.npmjs.com/package/opencode-doom-loop-detector Install: |
There was a problem hiding this comment.
This push only merges dev. The doom-loop plugin, detector, fold and tests are unchanged. On the merged head, the 67 doom-loop tests pass and tsc -b for packages/core is clean. The earlier points were addressed in previous pushes. The public API is unchanged by this push. Nothing new to raise.
Issue for this PR
Closes #59339
Type of change
What does this PR do?
This is offered for design review; I can adapt the implementation to whichever direction the team prefers.
v2 currently has no doom-loop detection: the
doom_loopaction is still accepted by the v1 config schema and rendered by the TUI, but no code produces it. The v1 detector also missed rotations, because it required three consecutive identical calls inside one assistant message.This PR restores detection as a zero-dependency detector module plus a thin internal plugin, with no runner changes:
Every challenge goes through the existing
doom_looppermission ask, so intentional polling keeps an escape hatch. A user message resets only B and the breaker; A/C counters deliberately survive, so an interruption does not hand the loop a fresh counter. Memory is bounded with LRU caps and a 10k line store; state is rebuilt from durable history after a restart.Relation to #53032: different shape — a pure module plus a thin plugin rather than runner changes. Its consecutive-only predicate does not catch rotations: running it over the 199-call incident sequence gives zero firings.
Scope: tool-call loops only. Text/reasoning loops (#15271, #46030, #46492) and harness/compaction loops (#15533, #52697) are out of scope and linked as related in the issue. AST normalization of bash inputs is deliberately deferred to keep the core zero-dependency; it can return as an optional layer if maintainers want parse capability exposed to plugins.
How did you verify your code works?
packages/coretypecheck is clean.Visual
Screenshots / recordings
Not a UI change. The report above includes stills of the incident timeline and of the same 199 calls replayed under the v1 baseline and the three new signals.
Checklist
Added by the opencode bot (AI-generated).
Fixes #61198
Related: #45442, #59567