fix(agent): detect a repeated same-tool fan-out - #89
Conversation
Streaks compared a tool's last fingerprint, so a fan-out of distinct arguments reissued verbatim never accumulated evidence: `read(a), read(b), read(c)` has a different last call every round, and each round's first call reset the streak to 1. Measured before this change — 8 identical rounds of a 3-call fan-out produced zero detections, while single-call rounds tripped at round 2. Distinct-argument fan-out is the dominant shape in parallel-tool-calling agents, so this was the common case going unseen. A round's identity for one tool is now the set of fingerprints it was called with, compared across rounds. The set completes only once every call has arrived, so a fan-out scores on the call that completes the match and the round's earlier calls report the pre-match streak — a partial fan-out is not yet a repeat. Unchanged: single-call rounds, in-round duplicate collapsing (one decision per (tool, fingerprint) per round), resumed streaks, persisted state shape, and verdict payloads. The new round fields are run-local and never serialized. Verified: 7 new tests covering accumulation, order-insensitivity within a round, reset on changed membership, subset-is-not-a-repeat, and the single-call and resume controls. 3 of them fail without this fix. Full suite 753 pass, typecheck and biome clean.
There was a problem hiding this comment.
Summary
Keys a tool's per-round doom-loop identity on the set of fingerprints seen in the round instead of the last call, which genuinely closes the repeated-fan-out gap. But because the set is compared after every call while the round is still filling, a round that is a strict superset of the previous round transiently matches it and can fire observe/block/stop on a call that represents real progress — order-dependently, and untested.
Findings (5)
🟠 major · packages/agent/src/lib/doom-loop.ts:994-998
Mid-round set comparison makes a superset round match the previous round transiently. With r0=[a,b], r1=[a,b], r2=[a,b,c], the b call of r2 sees set [a,b] == prior [a,b] and scores streak 3 -> block, so a legitimate call is refused via checkDoomLoopBeforeExecution/runToolWithHooks (model-result.ts) even though the round added new work. On main nothing fires. The behavior is also emission-order dependent (r2 emitted as [c,a,b] fires nothing), contradicting the PR's 'ordering within the round does not matter'. Expanding fan-outs ([a], [a,b], [a,b,c], …) likewise accumulate streaks they should not.
🟡 minor · packages/agent/tests/unit/doom-loop-fanout.test.ts:150-166
The suite tests the strict-subset direction ('a strict subset is not a repeat') but not the superset direction, which is the case that actually produces a verdict. Add [a,b], [a,b], [a,b,c] asserting no verdict on the middle call, plus an order-permuted variant of the same superset round.
🟡 minor · packages/agent/src/lib/doom-loop.ts:907-914
restore() sets priorStreak: entry.streak, but round is intentionally left undefined, so isSameRound can never be true on the first resumed record and that field is never read — dead state that suggests a semantic it does not have.
🟡 minor · packages/agent/src/lib/doom-loop.ts:1035-1041
Docs are now inconsistent with the implemented semantics: the recordToolCall docstring still states 'A different fingerprint for the same tool resets the streak to 1', the file-header 'Round-scoped streaks' bullet and DoomLoopVerdict.streak ('consecutive identical-fingerprint round count') describe last-call keying, and the verdict message tells the model the tool was invoked 'with identical arguments (fingerprint X…)' — false for a distinct-argument fan-out, where the quoted fingerprint is only the completing call. Since the module advertises itself as the cross-port spec, the round-set identity should also be stated for Python/Go ports.
1 more finding(s)
🟡 minor · packages/agent/src/lib/doom-loop.ts:992-998
At the block rung only the call that completes the set is refused; the other N-1 calls of a repeating fan-out still execute every round (they report the pre-match streak). Detection now works, but mitigation is partial until the stop rung is reached — worth documenting so users do not expect a blocked fan-out to stop spending.
The fan-out fix compared a round's fingerprint set while that set was still
filling, so a round that is a strict superset of the previous one transiently
equaled it. With r0=[a,b], r1=[a,b], r2=[a,b,c], the `b` of r2 saw [a,b], matched
the prior round, and scored streak 3 -> block: a call in a round that had added
new work was refused. It was also emission-order dependent — r2 as [c,a,b] never
formed the matching prefix and fired nothing — which contradicted the
order-insensitivity the previous commit claimed. Expanding fan-outs ([a], [a,b],
[a,b,c], …) accumulated streaks the same way.
The engine now declares a round's complete set before any of its calls is scored
(`declareRound`, called from all three execution-batch boundaries), so the
comparison is always whole-round against whole-round. Ordering within a round no
longer matters in fact rather than only in intent, and neither a subset nor a
superset is a repeat — a round that adds work is progress.
Every call in a repeating round now reports that round's streak rather than only
the call completing the match. At the block rung a repeating fan-out therefore
stops spending, instead of executing N-1 of its calls every round.
An undeclared round falls back to per-call sets: exact for single-call rounds,
and for a fan-out no stronger than the pre-fix last-call behavior — a test pins
that it can only reach the hook-only `observe` rung, never refuse a call.
Also: drop `priorStreak` from restore(), which was unreachable (it is only read
under `isSameRound`, and restore() intentionally leaves `round` undefined); add
the resumed-fan-out test Perry asked for; and correct the docstrings, file-header
port spec, and verdict message that still described last-call keying.
Unchanged: single-call round timing, in-round duplicate collapsing, resumed
single-call streaks, persisted state shape, and verdict payloads.
Verified: 758 pass (5 new), typecheck and biome clean. The three new
superset/order/expanding tests fail against the previous commit with the buggy
values ('block' on the progressing call, and order-dependent outcomes).
Two regressions from the previous commit, both found by Devin's re-review. A brand-new call could be reported as a repeat. Round-scoped scoring had every call in a round reuse the round's streak, which is only sound when the round's membership was declared up front. Server-tool records go through checkDoomLoopForResponse undeclared, so with one web_search in round R-1 (query x) and two in round R (x, then a new y), y inherited x's streak of 2 and emitted a verdict quoting y's own fingerprint and claiming y had been issued in 2 consecutive rounds. Undeclared rounds are now scored per call against the previous round — the pre-fan-out semantics — so a fan-out there goes undetected rather than mis-scored. That required restoring priorRoundFingerprints/priorStreak (dropped last commit as dead) to hold the previous round's baseline for the length of the current one; they are live on this path and still never serialized. The steer rung could inject N duplicate corrections for one round. queueDoomLoopSteer dedupes by exact message text, and every call of a repeating round now emits a verdict, so interpolating the individual call's fingerprint made three strings out of one round of evidence and queued all three. A multi-call round now quotes the round's identity (identical for all its calls) and names the call count; single-call messages are unchanged. Also corrects the fallback comment, which claimed the undeclared path degrades "never to a false positive" — the inheritance bug above was exactly that. Not changed: Devin also notes that a call repeating inside a round whose other members vary ([a,b], [a,c], [a,d]) no longer accumulates, since round identity requires the whole set to match. Confirmed, but it is the previous commit's deliberate "a changed member is progress" trade-off rather than a regression introduced here, and restoring per-call streaks alongside round streaks is a design change. Raised on the thread for the author instead. Verified: 759 pass (2 new regressions, both fail against eb3b51d), typecheck and biome clean.
…Key once Three findings from Devin's third pass. A call outside the declared set could inherit the round's streak. The shared- streak branch keyed on the tool having a declaration, not on this call being a member of it. `declareRound` drops a call whose key material is unhashable (bigint/NaN/circular), but at record time that call still resolves an identity through the engine's fallback chain, so it took the round's accumulated count and could be blocked on its first ever appearance — the same failure mode the declared/undeclared split exists to prevent, reached by a narrower path. Sharing is now gated on set membership. A tool's loopKey ran twice per call. Declaring a round resolves each call's key material, and the per-call checkpoint resolved it again. `loopKey` is user code: one that counts or logs saw double the activity, and one returning a fresh value each time made the declared and recorded identities disagree, hiding that call from detection for the round. The declaration's resolution is now cached per call id and reused. The fallback warning still logs per call. README documented the old last-call semantics. It now describes round-set identity, reset-on-membership-change in both directions, that a repeating fan-out gets a verdict per member with one shared steer message, and that DoomLoopDetected fires once per distinct member rather than once per round. Added two limits to the "does NOT catch" list: a repeat inside a varying round, and fan-outs on paths that cannot declare a round (server tools). Verified: 761 pass (2 new regressions, both fail against 81c2572 with the buggy values — streak 3 instead of 1, and loopKey invoked 4 times instead of 2); typecheck and biome clean.
…nd set Completes the previous commit's fix, which gated READING the declared set on membership but not WRITING it. A call the declaration could not include — `declareRound` drops unhashable key material, and the engine still records it via its fallback chain — stored `roundFingerprints: [itsOwnFingerprint]`, replacing the round's declared set. The next round's declared member then compared against that singleton, failed to match, and reset to 1; with the unhashable call recurring every round the member's streak was pinned at 1 permanently. Measured: [1,1,1,1] across four repeating rounds where the control climbs [1,2,3,4]. So a single bigint in one call's arguments disabled doom-loop detection for that tool for the rest of the run — the inverse of the fail-open guarantee, which allows an unhashable value to cost detection for its OWN call only. Devin also noted an order dependence: a non-member recorded before the round's members made them inherit its streak. Both symptoms had one cause — round-level state (the round's identity and score) and per-call state (fingerprint, in-round dedupe) shared one mutable record. They are now written separately: the streak is computed as a pure function of (this round's set, the previous round's set, that round's streak), so arrival order cannot affect it, and a non-member records its own identity while leaving the round's identity and score to its declared members. The round TRANSITION is still recorded by whichever call arrives first, so the baseline advances even when a non-member opens the round — fixing that was what the first attempt at this commit got wrong. Regression test asserts the member streak climbs 1..4 with an unhashable call riding along, in BOTH emission orders. Fails against f44c69d with [1,1,1,1]. Verified: 762 pass, typecheck and biome clean.
…d changeset example Two findings from Devin's fifth pass. `beginDoomLoopRound` declared every call in the batch, resolving each one's `loopKey` up front — including calls that are never checked. A manual tool (no `execute`, no `onToolCalled`) is handed to the caller and never recorded as evidence, and every execution path skips it via `isAutoResolvableTool`, but the declaration ran its `loopKey` anyway. For user code that counts or logs inside that callback, this was activity for a call the detector never evaluated. The declaration now applies the same `isAutoResolvableTool` predicate, which is also correct on its own terms: a call that is not evidence is not part of the round. Note the reproduction needs a MIXED batch. An all-manual round never reaches `beginDoomLoopRound` at all (`hasExecutableToolCalls` guards it), so the first version of this test passed with and without the fix — it proved nothing. The committed test pairs a manual call with an executable one and asserts the executable call's loopKey runs exactly once while the manual call's never runs; it fails without the guard with "called 1 times". The changeset had no code example, which .agents/skills/public-api-examples requires for behavioral changes to a public option even when the signature is unchanged. Added one to the changeset and an `### API example` section to the PR description, both showing the same before/after: a repeating three-call fan-out that previously never tripped now observes at round 2 and blocks every call of the round at 3, while a round that adds work resets to 1. Verified: 661 unit tests pass, typecheck and biome clean. (One e2e cancellation test failed once on a full run and passed in isolation and on re-run — a live network timing flake, unrelated to this change.)
…resume
The persisted state holds one fingerprint and one count per tool, so it cannot
express "this count was earned by the set {a,b,c}". Restoring a fan-out's streak
verbatim attached the whole count to whichever member happened to be recorded
last, so a resumed round consisting of just that one call matched, inherited the
fan-out's evidence, and was BLOCKED on its first appearance — while the model had
done strictly less work than before the save. It was also arbitrary: resuming
with a different member of the same fan-out scored 1 and passed.
`getState` now persists a multi-call round's streak as 1. Under-counting on
resume is the safe direction — the round is re-observed and re-accumulates from a
correct baseline, which the test asserts so the fix cannot silently become a
detection hole.
Measured, 3-call fan-out repeated twice then resumed with one call:
before this PR (main): saved 1, resumed -> streak 2, observe
eb3b51d..43b88c2: saved 2, resumed -> streak 3, BLOCK
now: saved 1, resumed -> streak 2, observe
So the mechanism predates this PR, but making fan-outs accumulate raised the
saved count, which escalated the resumed outcome from a harmless observe to a
refused call. That makes it this PR's regression to fix.
Also corrects the comment in restore(), which claimed a resumed fan-out streak
"restarts at 1" — it did not, and the claim is only true now that getState
enforces it. The existing resume test passed either way because it resumed the
same multi-call set, which never matched; it never covered the single-call case.
Verified: 662 unit tests pass; the new test fails without the getState change.
Typecheck and biome clean.
…ositive class The changeset asserted two things that were not true. "Resumed streaks unchanged" — the previous commit deliberately made a multi-call round's saved streak restart after a resume. The changeset now states that, why (the persisted shape cannot express which set earned a count), and that single-call streaks still continue. "No API surface changed" — `DoomLoopMonitor` is exported from src/index.ts, so `declareRound` is a new public method. Documented as additive, with a note that `callModel` users need not touch it while direct `DoomLoopMonitor` users and SDK ports should, since an undeclared multi-call round's fan-out goes undetected. Bump raised patch -> minor accordingly: .agents/skills/changeset-versioning specifies minor for new exports and features. Also documents a false-positive class this PR newly makes reachable, which is worth a decision before shipping (raised on the thread, not resolved here). Because a round's identity is the whole set, a tool called with a stable set of parallel arguments every round now accumulates where it previously could not. Measured, an agent re-reading three context files every turn: round 1: none none none round 2: observe observe observe round 3: block block block <- all three reads refused, every round after That is a legitimate shape, and it produces N synthesized error outputs per round rather than one. `loopKey: false` is the opt-out; no prior exemption covered this, since the shape was invisible to the detector before. Added to the README next to the `loopKey` exemption guidance and to the changeset.
`beginDoomLoopRound` declared some calls that never reach the doom-loop checkpoint, so they became phantom members of the round's identity and the sibling that WAS recorded got scored against a set including them. The streak then reset the moment the phantom stopped being emitted, even though the recorded call never changed. Two sources, both closed: - The malformed-arguments branch ran BEFORE the tool-resolvability check, so a raw-string call to an unknown or manual tool was declared despite never being recorded. The tool lookup and `isAutoResolvableTool` gate now precede it. - A call the PermissionRequest hook denied without pausing: `hookDeniedCalls` is populated before the round begins and `runToolWithHooks` synthesizes the rejection before the checkpoint, so those are skipped too. The new test drives the monitor directly with an over-broad declaration to pin the consequence — an identical recorded call scores [1,2,1,2] across four rounds when a phantom member is present for the first two — so the reason for the engine-side filtering is documented rather than implicit. Note on the test: an earlier version of this drove the engine end-to-end with a malformed manual call, and produced ZERO detections — the loop pauses on the manual call before later rounds run, so it asserted nothing. Removed rather than patched; the monitor-level test verifies the actual mechanism. That is the third test this session that would have passed against the bug it claimed to cover, so I am now deriving the expected numbers before writing the assertion instead of after. Verified: 663 unit tests pass, typecheck and biome clean.
…t its earner Two findings from Devin's ninth pass. `resolveLoopKeyMaterial` can throw, and both call sites were unguarded. It catches a throwing `loopKey`, but the field-list form does `field in args` and `args[field]`, so a getter or proxy trap on the arguments object escapes it. Declaration resolves the whole batch up front, so an uncaught throw there would fail the round and the run over one odd call — the opposite of the invariant that detection only ever affects a run through a ladder action. Both sites now skip just that call and warn. Severity is narrower than reported, and worth recording: this is NOT reachable through `callModel`. Tool arguments come from `JSON.parse` (stream-transformers), so they are always plain objects, and `PreToolUse` argument mutation happens after the round is declared. It is reachable for direct callers, since both `resolveLoopKeyMaterial` and `DoomLoopMonitor` are exported, and for ports that build key material differently. Guarded regardless. `getState` paired the saved streak with the wrong identity. `fingerprint` is what pairs with `streak` in persisted state, and a non-member recorded LAST in a round overwrote it, so the count was attached to a call that never earned it. Both halves broke on resume: the non-member (a call detection is meant to ignore) matched, inherited the count, and was BLOCKED on its first appearance, while the genuinely repeating call reset to 1 and lost its evidence. Measured, saved streak 2: ignored call -> streak 3 block / real repeat -> streak 1; now -> streak 1 none / streak 3 block. A non-member no longer overwrites the identity; it is still tracked for in-round dedupe via `seenThisRound`. Test-quality note: my first attempt at the resolution-throw test passed WITHOUT the fix, because the end-to-end case I chose (loopKey returning a bigint) is caught by the pre-existing fingerprint fallback and never reaches the new throw path. Rewritten to pin the throw directly and to state in-comment that the engine path cannot reach it. That is the fourth test in this PR that would have passed against its own bug; every assertion here was derived from a measured run and verified to fail with the fix removed. Verified: 665 unit tests pass, typecheck and biome clean.
…areRound example Devin traced that undeclared multi-call rounds do NOT behave "the same as before", and the trace is right. Verified: undeclared [a,b] x6: b -> 1, 2 observe, 3 block, 4, 5, 6 stop order flipped: the verdict moves to the OTHER call [a,b],[c,b],[d,b]: b -> 3 block Each call of an undeclared round overwrites `roundFingerprints` with its own singleton, so the next round's matching call compares against the previous round's LAST recorded fingerprint. A repeating undeclared fan-out therefore does accumulate — on whichever member lands last, order-dependently — and it reaches `stop`. The changeset, the README limit, and a source comment all claimed such fan-outs go undetected. Corrected all three, and added a test pinning the real behavior (including that a repeat inside a VARYING round accumulates here, which the declared path treats as progress). No behavior change: the engine declares every executed batch, so this is the server-tool and direct-caller path only. Also addresses the changeset's missing example for the new public method. While writing it I ran it, and it did not work: `resolveDoomLoopOption` is not exported, so the obvious construction fails at runtime. `DoomLoopMonitor` is exported but a consumer must hand-build the resolved config shape to instantiate it. Rewrote the example to only use exported API and noted the export gap as a follow-up — it predates this PR and is unrelated to `declareRound`. Also dropped the "No API surface changed" line, which was still there from before the bump was raised to minor. Verified: 666 unit tests pass, typecheck and biome clean.
…ructible `DoomLoopMonitor` was exported without its config resolver, so the natural construction — `new DoomLoopMonitor(resolveDoomLoopOption(true))` — failed at runtime for any consumer: `resolveDoomLoopOption` and `ResolvedDoomLoopConfig` existed only at module level. The class was effectively unusable outside `callModel` short of hand-building the internal resolved-config shape. Found by executing the changeset's usage example instead of eyeballing it. Exports `resolveDoomLoopOption` (value) and `ResolvedDoomLoopConfig` (type) from the package entrypoint, and adds a consumer-contract test file that imports from `src/index.js` only — construction with defaults, a custom ladder, fan-out detection via declareRound, and a JSON state round-trip across a simulated process boundary. Changeset example updated to match and the follow-up note removed, since this was that follow-up. Already covered by the existing minor bump. Verified: 669 unit tests pass (3 new), typecheck and biome clean.
… identities
Behavior-identical simplification of the scoring path — all 669 tests pass
unchanged, including the 20 fan-out regressions that were each verified to fail
against the bug they cover.
The branching had accreted one special case per bug fix: a member/non-member
fork for the reported streak, a second fork recomputing the stored round streak,
a third choosing the persisted identity, each with its own baseline reads. All
of it reduces to a single scoring rule applied to two identities:
score(set) = baseline matches set ? priorStreak + 1 : 1
reported streak = score(callSet) callSet = declared set if member,
else the call's singleton
stored round state = score(roundSet) roundSet = declared set if one exists,
else the call's singleton
The baseline (previous round's set + streak) is fixed at the round transition
and read once. Every non-member rule from the last several commits falls out of
the callSet/roundSet distinction instead of being its own branch: a non-member
scores 1 because its singleton is not the baseline; it cannot clobber the round
because roundSet prefers the declaration; the round transition still advances
because the baseline write is unconditional; the persisted identity guard is the
one remaining explicit special case.
Net -70 lines in the hot path. The verdict message now derives from callSet,
which is what the call was actually scored with (same value as before in every
reachable case).
Also spot-checked beyond the suite: non-member-first ordering across four
rounds, in-round duplicate handling, and the resume identity pairing all produce
byte-identical traces to the pre-refactor code.
…e/resume The persisted shape carried one (fingerprint, streak) per tool, which cannot say WHICH set earned a count. That forced a choice between two failure modes, and this PR had cycled through both: persist the streak verbatim and a resumed subset call inherits a fan-out's whole evidence (blocked on first appearance — the I1 false positive); persist 1 and the evidence is discarded at every save. Devin's last two passes showed the second mode is worse than the changeset admitted: saveStateSafely snapshots on every persist, so an approval/HITL pause reset a fan-out sitting at the block rung, and per-turn-resume topologies (one callModel per user turn — the serverless pattern) never accumulated at all. A multi-call round now persists its full fingerprint set (optional `roundFingerprints` on `DoomLoopStreak`, additive). The streak travels with the exact set that earned it, so both failure modes are gone rather than traded: per-turn resume, identical 3-call fan-out: 1 -> 2:observe -> 3:block -> 4 (was 1,1,1,1) pause at block rung, resume, repeat: 4:block (was reset) resume with a SUBSET of the saved set: 1, no verdict (unchanged) Compatibility: single-call rounds omit the field (their fingerprint fully describes the round), pre-existing blobs restore with their old semantics, and a malformed persisted set (non-string entries) degrades to the lone fingerprint instead of dropping the entry. Text streaks never carry it. Also overloads resolveDoomLoopOption so `new DoomLoopMonitor( resolveDoomLoopOption(true))` — the changeset's own example — compiles under strict TS: a `true`/config argument now types as non-null, while the engine's pass-through of a raw caller option keeps the nullable signature. Devin flagged the example as non-compiling; verified with a strict-mode tsc run before and after. Tests: the two resume tests now pin continuation instead of the old downgrade (both fail against the previous commit), a public-API test drives the serverless per-turn pattern end to end through JSON, and a legacy/hostile-blob test pins backward compatibility. 671 unit tests pass, typecheck and biome clean.
Perry's fresh review asked for the integration test bridging the monitor-level
fan-out suite and the engine: a scripted repeating distinct-argument fan-out
through callModel, asserting the observe -> block escalation and that the block
rung stops the whole fan-out (only rounds 1-2 execute).
The interesting part is what the obvious assertions do NOT catch, found by
injecting the two declaration regressions the INVARIANT comment warns about
(a phantom declared member, and a member wrongly filtered out): for an
exactly-repeating fan-out the per-call detector produces identical actions and
streaks, so corrupted round identity is invisible to action/streak assertions —
the per-call counts mask it. Both injected regressions passed the first version
of this test. The discriminator is the verdict message form: round verdicts
name the set ("same set of 3 parallel calls"), per-call verdicts name a single
repeated call. Asserting the round form on every detection pins the
beginDoomLoopRound declaration end to end; both injected regressions now fail
the test.
685 unit tests, structural gate, typecheck, and biome all clean.
cortex review —
|
… text; warn on declaration drops Three fixes from Cortex's fresh full review. The restored round streak now gets the same range validation as the per-call counts. isValidStreak only checks "finite number", so a tampered/corrupt blob carrying streak: -1e6 would hold that tool's guardrail below every rung (thresholds compare with >=), and the legacy fallback seeded the per-call detector with the same unvalidated value — the new sibling field had exactly the validation this one lacked. Clamped to [1, 1e6] with Math.floor rather than rejected: a corrupt streak degrades the entry, never drops the tool's evidence (fail-open, like every other restore path). The undeclared-path verdict text claimed "even as its other calls changed" for every server-tool and direct-monitor record — including a plain one-call-per- round repeat with no other calls, where the transcript contradicts the message. The text now asserts only what per-call evidence shows: this exact call repeated N consecutive rounds. declareRound's unhashable-member drop now warns with the tool name and cause, like every other fail-open path (the engine's own declaration already warned; the public method silently continued). Its docstring also no longer claims an undeclared fan-out "simply goes undetected" — false since per-call streaks: every repeated member accumulates without a declaration; what declaration adds is round-set evidence (shared verdict, one steer message). 685 unit tests, structural gate, typecheck, biome all clean.
|
Re the design question (round-set scoring dominated by per-call scoring): your dominance analysis is correct for state this codebase writes, and I verified your ablation result independently — but the round detector is load-bearing for state it READS, so I recommend keeping it. Final call flagged to @LukasParke. Your invariant-based argument holds: whenever declared members are all recorded, The counter-example is a persisted blob that carries So collapsing to per-call scoring makes fan-out detection across process boundaries silently dependent on every state writer populating an optional field. The round detector is the reader-side guarantee that the required round identity keeps working. On the invariant cost: agreed the declared-member invariant's teeth are the price. Two mitigations shipped since your review: the end-to-end Minor items from your review: PR body's "753 passed" and "Changeset: patch" were stale and are fixed (685 at head, minor). If the author prefers the simplification anyway, the right sequencing is: make |
There was a problem hiding this comment.
cortex panel verdict: comment — details in the consolidated review comment.
…o, sort-once sets
Cortex's fresh review escalated three perf findings to a request-changes
verdict. Two of the three are fixed here; measured before and after:
width-100 fan-out, 3 rounds (declare + record each):
before: 18.2ms (0.061 ms/call)
after: 5.1ms (0.017 ms/call) — 3.6x
- declareRound hashes independent digests concurrently (Promise.all) instead
of awaiting each serially; declaration gates dispatch of the whole batch, so
the serial awaits added one hash latency per call to every round.
- A per-monitor WeakMap memoizes (keyMaterial object, toolName) -> fingerprint,
so the declare-time and record-time hashes of the same resolved key material
compute once. The engine hands the same object through its per-call
resolution cache, which is what makes the memo hit; a fresh object with equal
VALUE still hashes and still matches by fingerprint (covered by a probe that
feeds fresh objects at declare and record and asserts streaks 1,2,3). Keyed
by tool name too, since the tool participates in the hash. WeakMap so
arguments are not retained beyond their natural lifetime; primitive key
material (raw-string malformed args) skips the memo.
- Declaration accumulates per-tool sets in a Set and sorts once per tool,
replacing the per-member copy-and-sort. Record-time bookkeeping keeps its
immutable copies deliberately — that discipline is what ended this PR's
aliasing bugs, and record-time width is 1 object per call.
The third finding (persisted-blob compaction) stays deferred: the persisted
shape is the cross-port conformance contract, and indices-into-keys is a shape
redesign for all ports in lockstep, not a patch. Measurement on the thread
(13.75KB at width 100, linear, on a state that already carries the transcript).
685 unit tests, structural gate, typecheck, biome all clean.
…f aliasing Cortex caught a false assumption my own comment asserted: "keyed by call id (required on ParsedToolCall, unique per round)". The id is required by the TYPE, but ids are model-emitted strings and nothing upstream enforces uniqueness within a batch. Last-write-wins meant a colliding id aliased the second call's key material onto the first call's checkpoint — the first call's true fingerprint was never recorded, so a model emitting (id=X, read a), (id=X, read b_i) each round evaded the per-call detector for `a` entirely. Measured: zero detections across four such rounds. An id seen twice in one batch now maps to a DUPLICATE_CALL_ID sentinel, and the checkpoint treats a poisoned entry as a cache miss: both colliding calls fall through to per-call resolution. Cost is at most a duplicate loopKey invocation for protocol-malformed calls only — correctness over the single-invocation economy exactly where the input is already out of spec. Well-formed batches are unaffected. Regression test drives four dup-id rounds through callModel and asserts the repeated call still reaches observe and block; fails against the previous commit with zero detections. 686 unit tests, structural gate, typecheck, biome all clean.
…fact, not one Devin traced that one declared round can produce TWO steer messages: [a], [a,b], [a,b] puts `a` on the per-call branch (3-peat call) and `b` on the round-set branch (2-peat set) in the same round. Verified: 2 verdicts, 2 distinct messages. Keeping the behavior, correcting the documentation. The two messages state two DIFFERENT facts — "this exact call repeated 3 rounds" and "this 2-call set repeated 2 rounds" — and both are true and independently actionable; collapsing them (deciding the branch once per round, as suggested) would either suppress the stronger per-call fact for `a` or misattribute it to `b`. What the shaping guarantees — and what the README previously overstated as "one message per round" — is one message per distinct piece of evidence: same evidence renders byte-identical text, so the steer queue is bounded at two messages per tool per round (each a distinct fact), never one per call. The wide-round test pins N-collapses-to-1 for single-evidence rounds; the new test pins exactly-2 for the mixed round, with the branch texts asserted. 687 unit tests, structural gate, typecheck, biome all clean.
…om/OpenRouterTeam/typescript-agent into lukeparke/doom-loop-fanout-streaks
…residual Devin's invariant audit verified all three mirror paths hold and found one residual the mirroring cannot cover: a per-request timeout/abort can cancel a call after declaration but before its checkpoint, leaving a declared phantom for that round. Documented at the INVARIANT comment with the assessment — bounded (round-set streak resets when the phantom stops recurring), fail-safe (per-call streaks unaffected, so detection degrades to per-call rather than being lost), and not worth un-declaring mid-round, which would reintroduce the order-dependence this design eliminates.
…unts
Two review findings on wide fan-outs, both real:
recordToolCall did O(W) work per call of a W-wide round — mergeFingerprint
spread and SORTED the seen-list on every record (sorting bought nothing:
its only consumer is the duplicateInRound membership check, while round
identity uses the declared set, deduped and sorted once in declareRound),
and the callStreaks accumulator was rebuilt via spread per record. The
seen-list is now a run-local Set grown in place, and callStreaks grows in
place within a round (growCallStreaks) — a fresh object per round keeps
the prior-round baseline aliasing one-directional. Both are never
serialized mid-round; getState() copies before persisting.
Persisted state stored each wide round's 64-char hashes twice: once in
roundFingerprints, once as callStreaks keys (~13 KB per width-100 round,
copied on every save). In the steady state of a repeating fan-out every
member's count equals the round streak, so the counts carry no
information beyond the set: getState() now omits them exactly then
(callStreaksReconstructible), and restore() rebuilds {member: streak} for
the WHOLE set — not just the last-recorded member, which would have reset
W-1 members' evidence on resume. Counts that differ from the round streak
(shrunk rounds, non-members) persist verbatim, as before.
Mutation-verified: rebuilding only the last fingerprint fails the
'scores a resumed SINGLE call on its own earned evidence' test. 92/92
doom-loop tests pass; full suite green; gate reports no degradation.
…ks' into lukeparke/doom-loop-fanout-streaks
…peats collapse Devin caught the last diverging element in the per-call message: it interpolated Math.max(roundStreak, callStreak), so members of one round whose repeats started at different times rendered different strings. An expanding fan-out — [a], [a,b], [a,b,c], [a,b,c,d] — carries counts 4, 3, 2 in round 4; measured, that queued 3 near-identical corrections into one injected prompt. The documented "at most two messages per tool per round" bound was really the round's width; the existing test missed it because every member it checks shares one count. The per-call text now names no count (and, as before, no fingerprint), so every per-call verdict of one tool renders byte-identical and the exact-text steer dedupe collapses them. Exact counts still reach consumers via the verdict payload's streak field — asserted in the new test, which pins streaks [4,3,2] alongside exactly one distinct message and fails against the previous text with "expected 3 to be 1". The builder docstring now states the enforced bound explicitly. 792 unit tests, structural gate, typecheck clean. (The one lint warning is the pre-existing unused variable in agent-tool.test.ts from the main merge, not mine.)
…tive class Devin noted the changeset's false-positive paragraph was framed around fixed sets, while the expanding case — a repeated anchor call whose round-mates change every round, so the ROUND detector calls it progress yet the per-call detector still blocks it from round 3 — is a distinct and less obvious consequence that never fired on main. Both variants are now named, with the rationale (an already-read file is in context; re-reading is spend without progress), the loopKey escape, and the ladder's built-in warning rounds.
…pConfig Devin caught that ResolvedDoomLoopConfig is exported while the type of its escalation field is not, so a consumer assembling or narrowing the config by hand had no importable name for that member — the same class of gap as the original resolveDoomLoopOption omission, caught the same way (by using the export rather than eyeballing it). Verified with a strict-mode tsc snippet that names both types through the package entrypoint.
The gap
#73keys a tool's streak on its last fingerprint (this.tools.get(toolName)), so a fan-out of distinct arguments reissued verbatim never accumulates evidence. Each round's first call has a different fingerprint than the previous round's last call, which resets the streak to 1 before the matching call arrives.Measured against
mainbefore this change:Zero detections across 24 calls the model had no business making.
read(a), read(b), read(c)on repeat is the dominant shape in parallel-tool-calling agents, so this is the common case rather than an edge one.Worth being precise about what
#73does handle: N identical calls fanned out in one round correctly count once (theduplicateInRoundpath, B3 in the remediation suite). The miss is specifically distinct arguments across a repeating round.The fix
A round's identity for one tool is now the set of fingerprints it was called with, compared across rounds — not the last call.
The set is only complete once every call in the round has arrived, so a fan-out scores on the call that completes the match, and the round's earlier calls report the pre-match streak. That is deliberate: a partial fan-out genuinely is not yet a repeat.
Ordering within the round does not matter (the set is sorted), a changed member resets the streak, and a strict subset is not a repeat.
Unchanged: single-call round timing, in-round duplicate collapsing, and verdict payload shape (same fields). Verdict message TEXT changed on two paths: undeclared records (server tools, direct monitor consumers) now render fingerprint-free per-call wording, and multi-call round verdicts quote the round's set identity — both deliberate, so calls carrying the same evidence produce byte-identical text and the steer dedupe holds. The persisted shape gains additive optional fields (
roundFingerprints,callStreaksonDoomLoopStreak) so fan-out and per-call evidence survive save/resume; old blobs restore with their old semantics. Resumed single-call streaks behave exactly as before.Tests
7 new tests in
doom-loop-fanout.test.ts:restore()3 of the 7 fail without the fix — verified by stashing the change and re-running.
Verification
vitest run(packages/agent): 685 passed at head (counts moved with main merges; all pre-existing doom-loop tests still pass)tsc --noEmit: clean (neededexactOptionalPropertyTypescare on the new optional fields)biome check: cleanDoomLoopMonitor.declareRound,resolveDoomLoopOption,ResolvedDoomLoopConfigexports)Context
Found while evaluating whether
#73supersedes the router-side port inopenrouter-web#30170. It largely does — the ladder, escalation, text detectors, and in-loopstopare all things the router plugin cannot do fromresolveEndpoints. This was the one axis where the router port was stronger, because the same gap was caught there in review and fixed by keying on a round fingerprint. Closing it here so the router can drop its duplicate and depend on the SDK.Also verified while I was in here, no action needed: the two-tier fail-open in
model-result.ts:1344(raw-args fallback, then skip-detection) is correctly per-call, so one unhashable value cannot zero a whole request's detection.One thing I did not change, flagging for a decision: the detector ignores tool results, so identical arguments with changing results reach
blockby round 3 — a polling tool returningrunningthendonelooks like a loop.loopKey: falseis the intended exemption, but it is opt-out per tool rather than automatic. Folding a result digest into the round identity would make polling self-exempting; happy to follow up if you want that.API example
For
callModelusers,doomLoopis configured exactly as before — what changed is when it fires:API example — new public surface
This PR adds public API:
DoomLoopMonitor.declareRound, plusresolveDoomLoopOptionandResolvedDoomLoopConfigexports (the monitor was previously exported without its config resolver, so it could not be constructed from the package at all). Consumer usage:Note on the implementation, since review
The first commit scored a round's fingerprint set as it accumulated, which
made a superset round transiently match its predecessor — blocking calls that
represented real progress, order-dependently. The engine now declares a round's
complete set before any of its calls is scored (
declareRound). Subsequentcommits fixed fallout from that seam: streak sharing is scoped to declared
members, a non-member can no longer clobber the round's set, and
loopKeyrunsonce per checked call. See the review threads for the full trail.