diff --git a/devlog/_plan/260829_cursor_tool_continuation_pairing/040_phase5_checkpoint_suffix_gap.md b/devlog/_plan/260829_cursor_tool_continuation_pairing/040_phase5_checkpoint_suffix_gap.md index 4ccf260094..105664cdf9 100644 --- a/devlog/_plan/260829_cursor_tool_continuation_pairing/040_phase5_checkpoint_suffix_gap.md +++ b/devlog/_plan/260829_cursor_tool_continuation_pairing/040_phase5_checkpoint_suffix_gap.md @@ -62,7 +62,7 @@ the fix was restored: | covered history is not replayed a second time | no — double-replay guard | | an id reused in covered history yields no invocation line | no — ambiguity guard | | native composer keeps checkpoint results off the root prompt | no — native-path guard | -| an ambiguous id resolved from full history is not re-resolved from the suffix | yes — but against `size > 0`, not against the threading | +| an ambiguous id resolved from full history is not re-resolved from the suffix | yes — against the threading *and* against `size > 0` | ### Why the lookup uses `??` and not a `size > 0` check @@ -89,8 +89,19 @@ covered history yields no invocation line" passes under *both* variants, because is ambiguous within the suffix too. The distinction only shows up when the ambiguity is visible in full history but not in the suffix, which is what the added case constructs. -Two assertions fail without the threading and pass with it; the other three are guards that -must hold either way, and they document what the widened lookup must *not* break. +**Three** of the six assertions fail without the threading and pass with it; the other three are +guards that must hold either way, and they document what the widened lookup must *not* break. + +An independent final-gate review corrected this count. The original text said two of five, which was +wrong on both numbers: the sixth test was added after the table was written, and it fails against a +missing threading too, not only against a `size > 0` fallback. Without the threading `knownCalls` is +`undefined`, so the suffix-only index sees one candidate and names `echo SECOND` for a result whose +output is `FIRST` — the same wrong label, reached by a different route. Measured at `1241a8d5c`: +reverting only the call-site threading gives **16 pass / 3 fail**. + +The fix is therefore better covered than the first version of this record claimed. Recorded because a +reader who reverts the threading expecting two failures would not know whether they were looking at a +stale doc or a real drift. Two shapes needed care while writing them: @@ -129,6 +140,37 @@ callers select on role first. So the `toolResult` branch inside `contentText` is dead for these paths, and the two patched sites are the complete set. `request-builder.ts` has its own `toolResultToText` for the text `messages` channel; it is a different channel with no invocation line by design and is out of scope here. + +### Two gaps that enumeration missed + +The final-gate review found the argument above correct about `contentText` but the surrounding claim +overstated: "only two functions attach an invocation line" is true, yet it is not the same statement as +"every site that emits a result envelope has been accounted for". Both items below are **pre-existing** +and neither is induced by the checkpoint cut. + +**A fourth emission site, line ~1025.** The `conversationTurns` native branch resolves its call from +suffix-local `pendingToolCalls` and, on a miss, falls through to a bare `toolResultToText(message)` +with no invocation line. It never consults `knownCalls`. Measured: full replay and checkpoint produce +byte-identical bare output on the same interleaved input, so the cut does not induce it. + +**The two builders gate on different predicates.** `rootPromptMessages` uses +`cursorNeedsExternalToolContinuation`; `conversationTurns` uses `isCursorExternalWireModel`. These +disagree for exactly one model: + +| Model | `cursorNeedsExternalToolContinuation` | `isCursorExternalWireModel` | +|-------|--------------------------------------|------------------------------| +| `composer-2.5` | true | **false** | +| `grok-4.6-high` | true | true | +| `composer-2.5-fast` | false | false | + +So for `composer-2.5` the map is threaded in and then ignored by the turn builder. Measured on an +interleaved history: `ROOT invoked=true`, `TURN_STEP invoked=false`. + +The asymmetry was inherited from #2900, where the root gate was deliberately widened to +`cursorNeedsExternalToolContinuation` (audit 001 F2) while the turn gate was left alone. Whether +`composer-2.5` turn steps should also name the invocation is a behaviour question about a native +model's replay, not a checkpoint-indexing bug, so it is not folded in here — it belongs to a unit that +can verify the native path end to end rather than being changed on inference. - `bun x tsc --noEmit` — exit 0. - Full suite on `ssh lidge`; no local full-suite run was used as a gate. diff --git a/devlog/_plan/260829_cursor_tool_continuation_pairing/050_phase6_native_turn_orphan.md b/devlog/_plan/260829_cursor_tool_continuation_pairing/050_phase6_native_turn_orphan.md new file mode 100644 index 0000000000..688f59e888 --- /dev/null +++ b/devlog/_plan/260829_cursor_tool_continuation_pairing/050_phase6_native_turn_orphan.md @@ -0,0 +1,243 @@ +# 050 — Phase 6: the native turn branch still emits an orphaned result + +Depends on: `11d33597f` (#2910), `cfb70c972` (#2913), `6906049c6` (#2919), all on `origin/dev`. + +## Why this exists + +The final-gate review of #2910 flagged a fourth emission site as a MINOR finding and I deferred it, +on the grounds that it was pre-existing and not induced by the checkpoint cut. Both of those are +true. What I did not check before deferring is whether it produces **the same defect this whole unit +is about** — an emitted result envelope that names no invocation. + +It does. Measured on `origin/dev`: + +```text +no-interleave [composer-2.5] steps=["toolCall"] +interleaved [composer-2.5] steps=["toolCall","BARE_TEXT_ENVELOPE(invoked=false)"] +no-interleave [composer-2.5-fast] steps=["toolCall"] +interleaved [composer-2.5-fast] steps=["toolCall","BARE_TEXT_ENVELOPE(invoked=false)"] +``` + +So this is not a cosmetic gap in a doc table. It is the orphaned-result condition, reachable today, +on the native path. + +## Root cause + +`conversationTurns` handles a native `toolResult` by looking for its call in `pendingToolCalls`, a map +populated **only while walking the current turn**: + +```ts +const priorCall = pendingToolCalls.get(message.toolCallId); +if (priorCall) { + current.steps.push(toolCallStep(priorCall, requestScope, message)); // paired: call + result together + pendingToolCalls.delete(message.toolCallId); +} else { + current.steps.push(/* … */ toolResultToText(message) /* … */); // bare: no invocation named +} +``` + +A user message closes the current turn (`flush()`), which clears `pendingToolCalls`. So when history +interleaves a user message between a call and its result — an ordinary shape, not a contrived one — +the lookup misses and the `else` fires. That branch calls `toolResultToText(message)` with **no second +argument**, even though the function has accepted an optional `call` since #2900: + +```ts +function toolResultToText( + message: OcxToolResultMessage, + call?: Extract, +): string +``` + +`turnCalls` — the full-history index this unit already threads in — is in scope at that line and holds +exactly the call the fallback could not find. + +## The change — after TWO audits corrected it + +> **Audit round r2 returned VERDICT: FAIL** on the version of this plan below the first correction, +> with three BLOCKERs. This section records what was wrong, because the same reasoning error keeps +> recurring in this unit and the record is the only thing that makes it visible. +> +> The rewritten design is in "Design v3" further down. Everything between here and there is history. + +**The first version of this plan was wrong, and the audit gate caught it before implementation.** It +proposed resolving the fallback from `turnCalls`. That cannot work, and the reason is worth recording +because it is the same class of mistake this unit keeps making — reasoning about the code instead of +measuring it. + +`turnCalls` is gated on the external predicate: + +```ts +const turnCalls = externalModel ? (knownCalls ?? toolCallsByCallId(messages)) : undefined; +``` + +And the `toolResult` handler returns early for external models, *before* the `pendingToolCalls` +lookup exists. So the two sets are disjoint by construction: + +| Model | `isCursorExternalWireModel` | `turnCalls` | Reaches the `else` branch? | +|-------|------------------------------|--------------|----------------------------| +| `grok-4.6-high` | true | populated | **no** — external branch handles it | +| `composer-2.5` | false | `undefined` | yes | +| `composer-2.5-fast` | false | `undefined` | yes | + +Measured: `grok-4.6-high` already emits `ENVELOPE(invoked=true)` on the interleaved history, through +the external branch. Every model that reaches the fallback has `turnCalls === undefined`, so +`turnCalls?.get(...)` is unconditionally `undefined` there. The proposed one-line change would have +been **inert**, shipped green, and looked like a fix. + +The actual change is a separate index that does not ride the external gate: + +```ts +// Native fallback: the call is real history, just not in THIS turn's pending map. +const nativeCalls = knownCalls ?? toolCallsByCallId(messages); +… +const fallbackCall = nativeCalls.get(decodeCursorCallId(message.toolCallId)); +… toolResultToText(message, fallbackCall) … +``` + +Deliberately narrow: + +- The `if (priorCall)` paired path is untouched. When call and result sit in one turn, Cursor gets a + real `toolCallStep` carrying both halves, which is strictly better than text and must not change. +- Only the `else` branch — already a text envelope today — gains a line inside it. +- `knownCalls` is reused when the checkpoint path supplied it, so the covered-history lookup from + `040` applies here too rather than being re-derived from a slice. +- Ambiguity handling is inherited: `toolCallsByCallId` drops any id two different invocations claim, so + a reused id still yields no invocation line rather than a confidently wrong one. + +### Cost + +This indexes history for native models, which previously skipped it. Measured in `040` at 0.27 ms per +encode on a 401-message thread, against blob serialization and SHA-256 hashing already in the same +encode. Verified again for the native path in this phase. + +## Audit r2: three BLOCKERs against the design above + +An independent auditor copied `src/` to a scratch tree, applied the exact patch this plan proposed, +and ran both trees through the real encoder. Findings, each reproduced: + +**B1 — the fix would name a FUTURE call for a stale result.** `toolCallsByCallId` carries no +positional information, but `pendingToolCalls` was inherently backward-looking: it only ever held +calls already walked in the current turn. Replacing it with a whole-history index removes that bound. +Measured with a result at index 1 and its id's call at index 3 (`echo LATER`): + +| tree | output | +|------|--------| +| base | `TEXT invoked=false` | +| patched | `TEXT invoked=true — invoked: exec_command with {"cmd":"echo LATER"}` | + +The ambiguity guard does not catch this, because one call for an id is not ambiguous. I re-derived it +independently: `resultIndex=1`, `callIndex=3`, `callIndex > resultIndex` is true. This is precisely the +failure the index's own doc comment calls unacceptable — "an early result could be labelled with a +later command… a wrong invocation is worse than none". The root path escapes it only because it skips +results at or after `activeUserIndex`; the turn path has no such bound. + +**B2 — the added line can make a request fail to encode.** The turn path stores one blob per step with +no truncation guard. The root path has `truncateToolResultBlob`; `toolCallStep` degrades by dropping +images; this `else` branch has neither, and `storeCursorBlob` throws `CursorBlobAdmissionError` +unconditionally on rejection. With the entry ceiling lowered to reach the boundary cheaply, a +large-but-legal argument plus a result that fits in base threw `entry_too_large` in the patched tree. +The plan's cost section discussed only the 2 KB argument cap, never the step-blob total. + +**B3 — the "no-index guard" test row was false, and it was the row that would have caught B1.** +`nativeCalls` was unconditional, so no model stays un-indexed. Measured `invoked=false → true` for +`composer-2.5-fast`, `auto`, and `auto-intelligence`. A test asserting "unchanged" would have failed +immediately and been quietly rewritten to match observed output — the exact mechanism that produced +three partial fixes in this unit already. + +Plus: the affected set is wider than this plan listed. `isCursorNativeWireModel` returns true for +`auto` and `default` as well as `composer-*`, so `auto` and `auto-intelligence` reach the branch too. + +## Design v3 + +Three constraints, one per BLOCKER. + +**Positional bound (B1).** The fallback accepts a call only when it appears *before* the result in +history. That needs an index carrying position, so `toolCallsByCallId` gains a variant that records the +message index of each first binding, and the fallback compares against the result's own index. A call +at a later index yields no invocation line — the honest degradation the existing code already prefers. + +**The bound must compare within ONE coordinate system, and this is the trap.** `040` threads a +**full-history** index into a **sliced** replay: `buildPreparedCursorRunRequest` builds +`toolCallsByCallId(request.rawMessages)` and hands it to `conversationTurns`, which then iterates +`rawMessages.slice(suffixStart)` using slice-local positions. Comparing a full-history `callIndex` +against a slice-local `resultIndex` compares two different origins. Worked example with +`suffixStart = 4`, the call at full index 1 and the result at full index 4: + +| comparison | result | +|------------|--------| +| full vs full (correct) | `1 < 4` → accept | +| full vs slice-local (naive) | `1 < 0` → **reject** | + +A naive bound therefore drops the invocation line for a legitimately earlier call — silently +re-creating, on the checkpoint path, the exact orphan #2910 was merged to fix. So the fallback must +either receive the suffix offset and compare `callIndex < suffixStart + localIndex`, or the index must +be built over the same message array the loop walks. Whichever is chosen, a test must pin the +checkpoint case specifically, because a unit test on full replay alone cannot see this. + +**Byte budget (B2).** The rendered step is measured against `cursorBlobMaxEntryBytes()` before it is +stored. If naming the invocation would not fit, the envelope is emitted **without** the invocation +line rather than throwing: the result output is the payload, the invocation line is a convenience, and +that ordering is already established by the root path's `PROBE a huge argument must not evict the +result output` test. + +**Honest scope (B3).** The change affects every model that reaches this branch — `composer-2.5`, +`composer-2.5-fast`, `auto`, `auto-intelligence` — and the tests must assert that, not the opposite. +No test claims a model is unchanged when it is not. + +What stays untouched: the `if (priorCall)` paired path. The auditor confirmed it is byte-identical +across all five models in the patched tree, and it produces a real `mcpToolCall` protobuf step +carrying both halves, which is strictly better than any text envelope. + +### The 363-B question, answered rather than inherited + +The previous draft asserted safety by inheritance. The specific guard forbids a `[Tool Call]` marker, +and `toolInvocationLine` emits none — confirmed, no `[Tool Call]` string appears in any patched turn +step. But the auditor named a shape that does not exist on the external path: the text envelope now +sits directly beside a genuine `mcpToolCall` step describing the same call, so the same invocation is +described twice in one turn. Given that `040` already records a live `composer-2.5` run fabricating a +`[Tool Result]` envelope as chat, that duplication is not obviously harmless. + +This is why the phase does **not** widen the gate and does not proceed on inference. The narrow +question — a result whose call is genuinely absent from the current turn gets its invocation named, +bounded by position and by bytes — is decidable from the wire. Whether a native model should see the +same call described twice is a live-behaviour question, and it is deferred with that reason stated. + +## The predicate question, deliberately not answered here + +`turnCalls` is gated on `isCursorExternalWireModel`, while the root builder gates on the wider +`cursorNeedsExternalToolContinuation`. They disagree for `composer-2.5` (true vs **false**), so this +change alone will not name the invocation for that model's turn steps. + +Widening the turn gate to match would change what a *native* model receives on its resume path, and +this unit has already shipped three partial fixes by reasoning about the Cursor wire instead of +measuring it. The gate stays as it is; the asymmetry stays recorded in `040`. What this phase fixes is +the case where the index already exists and was simply not consulted. + +## Tests + +In `tests/cursor-tool-result-invocation.test.ts`, driven red before the fix: + +Two rows of the previous table were factually wrong and audit r2 rejected them: one named an +"external" model when every model reaching this branch is native by `isCursorExternalWireModel`, and +one asserted native turn steps were "unchanged" when the patch changes them for four models. A test +that asserts the opposite of what the code does gets quietly rewritten to match observed output, which +is how this unit shipped three partial fixes. + +| Test | Without the fix | +|------|-----------------| +| a native result separated from its call by a user message names its invocation in the turn step | **red** | +| a result whose id's call appears LATER in history gets no invocation line | **red** — B1 bound | +| naming the invocation is dropped, not thrown, when the step would exceed the entry ceiling | **red** — B2 budget | +| a call and result inside one turn still pair into an mcpToolCall step, not text | green — paired-path guard | +| every model reaching the branch is named explicitly (`composer-2.5`, `composer-2.5-fast`, `auto`, `auto-intelligence`) | green — scope is asserted, not assumed | +| `grok-4.6-high` is unaffected, because the external branch handles it before this code | green — disjointness guard | +| on the CHECKPOINT path, a call before `suffixStart` is still accepted by the positional bound | **red** — coordinate-system guard | + +The second and third rows are the ones that did not exist before the audit, and they are the two that +encode its BLOCKERs as executable checks rather than prose. + +## Verification + +- Focused `bun test` on the cursor files. +- `bun x tsc --noEmit`. +- Full suite on `ssh lidge`; no local full-suite run as a gate. diff --git a/devlog/_plan/260829_cursor_tool_continuation_pairing/060_phase7_positional_bound.md b/devlog/_plan/260829_cursor_tool_continuation_pairing/060_phase7_positional_bound.md new file mode 100644 index 0000000000..a05efb763a --- /dev/null +++ b/devlog/_plan/260829_cursor_tool_continuation_pairing/060_phase7_positional_bound.md @@ -0,0 +1,278 @@ +# 060 — Phase 7: bound the invocation lookup by position + +Depends on: `11d33597f` (#2910), `cfb70c972` (#2913), `6906049c6` (#2919) on `origin/dev`. +Supersedes the implementation intent of `050`; that unit's own defect is now the smaller half of this +one. + +## Why this replaces 050 + +`050` set out to name the invocation on a native turn-branch fallback. Two independent audit rounds +failed it (r2 and r3), and the second one found something that outranks the thing `050` was trying to +fix: **the mislabel is already on the wire, in code merged today.** + +Measured on the tracked tree with **no patch applied** — a result whose own output is `EARLY-OUT`, +labelled as having been produced by a command that runs later in history: + +```text +grok-4.6-high => invoked: exec_command with {"cmd":"echo LATER"} | output=EARLY-OUT +composer-2.5 => invoked: exec_command with {"cmd":"echo LATER"} | output=EARLY-OUT +``` + +This is the failure mode `toolCallsByCallId`'s own doc comment calls unacceptable: "an early result +could be labelled with a later command — a wrong invocation is worse than none, since it is the kind +of mislabel the model cannot detect." The index implements the *ambiguity* half of that comment and +not the *ordering* half. + +So the priority inverts. A missing invocation line on a native turn step is a cosmetic gap; a **wrong** +invocation line on the shipped external root path is the defect this unit exists to prevent, and I +introduced it in #2900. + +## Root cause + +`toolCallsByCallId` carries no position. It keeps the first call for an id and drops ids claimed by +two different invocations, but nothing constrains *where* the winning call sits relative to the result +being labelled. The comment's parenthetical — "results follow their call, so the first binding is the +one an earlier result belongs to" — is an assumption about history order, not something the code +checks. + +`050`'s draft claimed the root path escaped this via `activeUserIndex`. That is false, and audit r3 +disproved it: `activeUserIndex` is `-1` whenever the last raw message is a `toolResult`, and otherwise +it bounds the loop end, never the call index. I re-measured it above on the shipped tree. + +## Reachability, stated honestly + +This needs a history where a result's id is first claimed by a *later* call. It is not the common +shape: Codex normally replays a full thread in which the call precedes its result. + +My first draft framed the precondition as id reuse. Audit r4 corrected that — **the actual +precondition is only "a result precedes its call in serialized order"**, and it is reachable without +any id reuse at all: a result serialized before the assistant message declaring its call was measured +being labelled from that later message, with two distinct ids. Routes: + +- a result emitted before its own call in the serialized order — no reuse required; +- an id reused by a retry after the original call has left the replayed window. + +It also does not need a contrived trailing shape. On the ordinary trailing-`toolResult` continuation — +the single most common shape this proxy sees — `activeUserIndex` is `-1` and the loop walks the entire +history, so nothing bounds the lookup at all. + +I have **not** produced this from a live `codex exec` run, and I am not claiming a live repro. What is +demonstrated is that the encoder produces a confidently wrong label when given the shape, on the path +that ships today. Given that a mislabel is undetectable downstream by design, that is worth closing on +its own terms rather than waiting for a user to hit it. + +## The change + +Give the index position, and require the call to precede the result. + +`toolCallsByCallId` gains a companion that records the message index of each first binding. Both +emission sites already know the result's index — the root loop has `i` (it already passes +`messageIndex: i` into `pushDeduped`), and the turn loop can carry it. A call at an index **not less +than** the result's index yields no invocation line: the same honest degradation the ambiguity path +already takes. + +### The coordinate-system trap + +Both audits converged on this and it is the reason a naive bound is worse than none. `040` threads a +**full-history** index into a **sliced** replay: the checkpoint path builds +`toolCallsByCallId(request.rawMessages)` and hands it to builders that iterate +`rawMessages.slice(suffixStart)`. A full-history call index compared against a slice-local result +index compares two different origins: + +| `suffixStart = 4`, call at full index 1, result at full index 4 | comparison | outcome | +|---|---|---| +| correct | `1 < 4` | accept | +| naive (full vs slice-local) | `1 < 0` | **reject** | + +Audit r3 measured both directions of this on a patched tree: it rejects valid pairings *and* can +accept for the wrong reason. Rejecting silently re-creates, on the checkpoint path, the exact orphan +#2910 was merged to fix. + +The bound therefore compares in one coordinate system. Two options looked available: + +1. build the index over the **same array the loop walks**; +2. keep the **full-history** index and have the loop convert its local index to full space before + comparing. + +**Option 1 is self-contradictory and this plan initially chose it.** The checkpoint site threads a +full-history index *precisely because the call can sit outside the slice* — that is what #2910 fixed. +Rebuilding the index over the slice removes that call from the index altogether, so the invocation is +lost for exactly the shape phase 5 closed. Worked through with `suffixStart = 2`, the call at full +index 1 and the result at full index 3: + +| option | call in index? | comparison | outcome | +|--------|----------------|------------|---------| +| 1 — rebuild over slice | **no** | n/a | invocation LOST, re-breaks #2910 | +| 2 — full index + offset | yes | `1 < 2 + 1 = 3` | named, and ordering enforced | + +So the design is **option 2**: the index stays full-history, and each emission site converts the +position it walks into full-history space before comparing. The root path already has the full-history +`i` when it is not slicing; the checkpoint path must add its `suffixStart`, which means that offset has +to be passed to the builders alongside `knownCalls` rather than inferred. + +That is one more parameter than option 1 would have needed, and it is the price of not re-breaking the +previous phase. Recording the wrong first choice because "no offset to thread" is exactly the kind of +simplicity argument that produced the last three partial fixes. + +### The three origins ADD — write the expression, not the parts + +There are three offsets in play, and the comparison position is their **sum**: + +```text +resultFullIndex = knownCallsOffset + start + w + + knownCallsOffset : checkpointSuffixStart, or 0 on full replay + start : historyMessageStart in conversationTurns, or 0 + w : the loop's own position within the array it walks +``` + +Audit r5 implemented this plan and then mutation-tested the two readings the earlier prose permitted. +Both typecheck cleanly and passed all 274 cursor tests **plus the five rows the table below had at the +time**. Row 6 exists because of this measurement, so it is the one row they do not pass — see the note +under the table. + +| variant | 274 cursor tests | live behaviour | +|---------|------------------|----------------| +| `start + w` (drops `knownCallsOffset`) | 273 pass, 1 fail | caught | +| `knownCallsOffset + w` (drops `start`) | **274 pass** | **live orphan** | +| `knownCallsOffset > 0 ? offset + w : start + w` | **274 pass** | **live orphan** | + +The shape that exposes the two survivors is checkpoint **and** root pruning together: +`suffixStart = 1` with a large turn inside the suffix forcing `historyMessageStart = 3`. Correct +arithmetic names the call; both survivors emit no invocation line — re-creating on the checkpoint path +the exact orphan #2910 fixed, which is the failure this document spends its longest section warning +about. + +So the expression is normative. An implementer who derives only one term ships something that looks +green from every angle this plan would otherwise check. + +**Storage mechanism, so review does not relitigate it:** `toolCallsByCallId` returns a bare `Map`, so +positions go in a side table keyed by the returned map — a `WeakMap>` — rather +than changing the return type and every caller. There are four `toolCallsByCallId(` invocations +(`rg -c` on the file) across two builders and the checkpoint site. Audit r6 built the side table exactly +as specified and confirmed the return type and all existing call sites stay unchanged, with positions +recorded on first binding and deleted alongside the ambiguity drop. + +**Loop rewrite caution:** converting `conversationTurns`' `for…of` to an indexed loop should keep an +`if (!message) continue;` guard, matching the existing root loop. Audit r6 corrected my stated reason: +`noUncheckedIndexedAccess` is **not** enabled in this repo, so `walked[w]` types as `OcxMessage` and no +narrowing is lost — removing the guard still typechecks. Confirmed: `grep -c noUncheckedIndexedAccess +tsconfig.json` returns 0. So the guard is a runtime-consistency choice, not a strictness requirement, +and an implementer who tests the original justification would find it did not hold. + +## Tests + +**Exactly one row is red without the fix.** Saying "every row must fail first" would be the same +overclaim `040` was audited for twice: the accept-side rows exist to stop the bound from becoming a +blanket refusal, and a guard that is green before *and* after is doing its job. What matters is that no +row is **vacuous** — every row must be red under at least one wrong implementation. + +| Test | Unpatched | Red under | +|------|-----------|-----------| +| a result whose id's call appears LATER in history gets NO invocation line | **red** — names `echo LATER` today | the defect itself | +| the same history with the call EARLIER still names it | green | a bound that refuses everything | +| on the checkpoint ROOT path, a call before `suffixStart` is still named | green | `same-array`, `naive` | +| on the checkpoint TURN path, the same call is still named | green | `same-array`, `naive` | +| an id ambiguous in FULL history but not in the suffix yields no line | green | `same-array` | +| on the checkpoint TURN path with root pruning too (`suffixStart` > 0 **and** `historyMessageStart` > 0) the call is still named | green | `knownCallsOffset + w`, `start + w`, ternary | + +Row 5 is the one audit r4 said was missing, and it is the most important guard in the table. The +plain "an ambiguous id yields no line" row I originally listed does **not** catch suffix-narrowing — +measured green under `same-array` — because the ambiguity is visible in the slice too. The guard has to +construct ambiguity that full history sees and the suffix does not. That test already exists in the +tree as `an ambiguous id resolved from full history is not re-resolved from the suffix`, added in +#2919, so this phase must keep it green rather than write a new one. + +Rows 3 and 4 are split because audit r4 showed the single row as worded was satisfiable by the root +path alone, which would let a turn-path regression through. + +Row 6 is the one audit r5 proved was missing, and it must assert against the **turn** path. Audit r6 +built it both ways on identical history and only the turn form discriminates: + +| row 6 asserts against | correct | `knownCallsOffset + w` | ternary | `start + w` | +|---|---|---|---|---| +| turn path | pass | **fail** | **fail** | **fail** | +| root path | pass | pass | pass | fail | + +The reason is structural, not fixture luck. `historyMessageStart` is an *output* of +`rootPromptMessages`, assigned only after its loop finishes, while that loop walks full-history `i` from +zero — so the root path's expression reduces to `knownCallsOffset + 0 + i` and `knownCallsOffset + w` is +*identical* to the correct one there. No root-path test can ever separate them. Only +`conversationTurns` carries `start = historyMessageStart` into its slice. + +This is the same defect rows 3 and 4 were split to avoid, in the one row that must not have it: a +table that cannot distinguish a correct derivation from a plausible wrong one is the shape of every +earlier failure in this unit. Row 6 is also the only row whose preconditions must be checked rather +than assumed — r6 instrumented it and confirmed `offset=1 start=1 w=2`, both offsets genuinely +non-zero, so the row exercises the composition instead of being incidentally satisfied. + +### Measured across implementations + +Audit r4 implemented every coordinate option behind one knob and ran identical tests: + +Test counts below differ by **file scope**, not because the suite grew — r4 measured the three files +this unit touches (124 tests: `cursor-tool-result-invocation` 19, `cursor-tool-continuation` 12, +`cursor-blob` 93), r5 and r6 widened to seven and nine cursor files respectively. The three-file figure +is the one this phase gates on, and it is reproducible with +`bun test tests/cursor-tool-result-invocation.test.ts tests/cursor-tool-continuation.test.ts tests/cursor-blob.test.ts`. + +| implementation | new rows | three-file cursor suite | +|----------------|----------|-------------------------| +| shipped (no bound) | row 1 red | 124 pass | +| `same-array` (this plan's first choice) | row 3/4 red | **121 pass, 3 fail** | +| `naive` (condemned by r2/r3) | row 3/4 red | 122 pass, 2 fail | +| **`offset`** (the design above) | **all green** | **124 pass** | + +`same-array` is worse than the option two earlier audits already rejected: besides losing the +out-of-slice call, it narrows the ambiguity evidence and emits `invoked: … echo SECOND` for a result +whose output is `FIRST` — a fresh instance of the wrong-label defect, on the checkpoint path. + +### A third coordinate origin + +`conversationTurns` iterates `messages.slice(start, historyEnd)` with a `for…of` over **values**, so it +has no index at all today, and `start` is `historyMessageStart` — non-zero on the full-replay path after +root pruning. The loop-local position is therefore `start + w`, not `w`. Audit r4 confirmed this third +origin produces no mislabel on its own, so it is an implementation trap rather than a live defect, but +an implementer who reads only the `suffixStart` discussion above will walk straight into it. + +## Scope + +The bound lives in the shared lookup, so it covers every consumer at once — the external root path +(where the mislabel is live), the external turn path, and the checkpoint variants of both. + +The native turn-branch fallback from `050` is **not** included. Audit r3 showed the affected id set is +48 wire ids rather than the four `050` listed, that the paired `mcpToolCall` step already describes the +same call so the envelope is not as orphaned as `050` claimed, and that a raw-vs-decoded id keying +asymmetry between `pendingToolCalls` and the index is unaccounted for. That is a separate phase with +its own measurements, not a rider on a correctness fix. + +## Verification + +### Implementation notes: row 6 took five fixtures to make discriminate + +The plan predicted row 6 would catch a dropped `start` term. Getting a fixture that actually does took +five attempts, and the failures are worth recording because each one looked correct: + +| attempt | why it did not discriminate | +|---------|------------------------------| +| `suffixStart = 1`, 400 KiB filler | cut left the call INSIDE the slice, so no covered call was exercised | +| `suffixStart = 2`, 400 KiB filler | 400 KiB is under the 512 KiB root budget, so nothing pruned and `start` stayed 0 | +| `suffixStart = 2`, 600 KiB filler | call was in the COVERED region, where its position is below the offset and the under-count cannot cross it | +| call adjacent to result, 600 KiB | correct shape, but the assertion pooled roots **and** turn steps | +| same, asserting the TURN step only | **discriminates** | + +The fourth is the instructive one. Pooling both sources hid the mutation exactly as the plan's own +analysis said it would: the root path has no `start` term to drop, so it keeps naming the call and an +either-source assertion stays green. Instrumenting the loop gave `offset=1 start=1 w=2`, so under the +mutation the result's computed position was 3 while its call sits at 3 — `3 >= 3` rejects, the turn step +loses its invocation line, and the root step still has one. + +The condition was derived rather than guessed after the third failure: dropping `start` under-counts a +walked message by exactly `start`, so it flips the decision only when the call is inside the slice and +`w_result - w_call <= start`. + +- Focused `bun test` on the cursor files; row 1 driven red first, and each guard row driven red against + the wrong implementation it exists to catch. +- `bun x tsc --noEmit`. +- `bun run privacy:scan` — the declared CI gate in `AGENTS.md`, omitted from the first draft of this list. +- Full suite on `ssh lidge`; no local full-suite run as a gate. diff --git a/src/adapters/cursor/protobuf-request.ts b/src/adapters/cursor/protobuf-request.ts index 0ea1c91fc4..14f7f1c4e6 100644 --- a/src/adapters/cursor/protobuf-request.ts +++ b/src/adapters/cursor/protobuf-request.ts @@ -214,6 +214,12 @@ function rootPromptMessages( * which is where the defect this line prevents actually reappeared in live use. */ knownCalls?: Map>, + /** + * Full-history index of `rawMessages[0]` for this call. Non-zero only on the checkpoint path, where + * only a suffix is replayed but `knownCalls` still spans full history; the positional bound needs + * both sides in the same space (devlog 260829 060). + */ + knownCallsOffset = 0, ): { ids: Uint8Array[]; byteLength: number; @@ -320,7 +326,9 @@ function rootPromptMessages( // #1920: the prefix must reflect the NORMALIZED error state (an empty // node_repl result is an error even when the runtime said isError=false). const prefix = normalizedToolResult(message, contentToText(message.content)).isError ? "[Tool Error]" : "[Tool Result]"; - const text = `${prefix}\n${toolResultToText(message, replayedCalls?.get(decodeCursorCallId(message.toolCallId)))}`; + // The bound compares in full-history space: this loop's `i` is already full-history on the + // full-replay path, and `knownCallsOffset` re-bases it when only a suffix is replayed. + const text = `${prefix}\n${toolResultToText(message, callBefore(replayedCalls, decodeCursorCallId(message.toolCallId), knownCallsOffset + i))}`; pushDeduped(toolResultRootPayload(text), "toolResult", { messageIndex: i, text }, text); } } @@ -748,6 +756,42 @@ function toolInvocationLine(call: Extract>, + Map +>(); + +/** + * The indexed call for `callId`, but only when it appears BEFORE `resultIndex` in history. + * + * `toolCallsByCallId` has no ordering constraint, so it would happily name a call that runs LATER than + * the result being labelled — a result whose own output is `EARLY-OUT` was measured on the shipped tree + * as `invoked: exec_command with {"cmd":"echo LATER"}`. That is the mislabel the index's own comment + * calls worse than no label, because nothing downstream can detect it (devlog 260829 060). + * + * `resultIndex` MUST be in full-history space. The checkpoint path replays a suffix and the turn + * builder starts at `historyMessageStart`, so a caller composes `knownCallsOffset + start + local` + * before calling; comparing a full-history call index against a slice-local result index silently + * drops legitimate pairings and re-creates the orphan #2910 fixed. + */ +function callBefore( + calls: Map> | undefined, + callId: string, + resultIndex: number, +): Extract | undefined { + const call = calls?.get(callId); + if (!call || !calls) return undefined; + const position = callPositions.get(calls)?.get(callId); + if (position === undefined || position >= resultIndex) return undefined; + return call; +} + /** * Index assistant tool calls by decoded call id so a replayed result can name its invocation. * @@ -762,8 +806,10 @@ function toolInvocationLine(call: Extract> { const calls = new Map>(); const ambiguous = new Set(); - for (const message of messages) { - if (message.role !== "assistant" || !Array.isArray(message.content)) continue; + const positions = new Map(); + for (let index = 0; index < messages.length; index++) { + const message = messages[index]; + if (!message || message.role !== "assistant" || !Array.isArray(message.content)) continue; for (const part of message.content) { if (part.type !== "toolCall") continue; const callId = decodeCursorCallId(part.id); @@ -771,6 +817,7 @@ function toolCallsByCallId(messages: readonly OcxMessage[]): Map>, + /** Full-history index of `rawMessages[0]`; see {@link rootPromptMessages}. */ + knownCallsOffset = 0, ): Uint8Array[] { const messages = request.rawMessages; if (!messages?.length) return []; @@ -967,7 +1018,15 @@ function conversationTurns( pendingToolCalls.clear(); }; - for (const message of messages.slice(start, historyEnd)) { + const walked = messages.slice(start, historyEnd); + for (let w = 0; w < walked.length; w++) { + const message = walked[w]; + // `for…of` gave this for free; keep it explicit so the indexed loop behaves identically. + if (!message) continue; + // Full-history position of this message: the slice offset the caller passed, plus where this + // loop starts inside `rawMessages`, plus the local step. All three terms are needed — dropping + // `start` still passes every test except the checkpoint-plus-pruned-root case (devlog 060). + const fullIndex = knownCallsOffset + start + w; if (message.role === "assistant") { if (!current) continue; for (const part of message.content) { @@ -1004,7 +1063,7 @@ function conversationTurns( const prefix = normalized.isError ? "[Tool Error]" : "[Tool Result]"; // Name the invocation here as well, for the same reason the root replay does: a result with // no visible originating call reads as an interrupted attempt (devlog 260829 000_rca). -const call = turnCalls?.get(decodeCursorCallId(message.toolCallId)); + const call = callBefore(turnCalls, decodeCursorCallId(message.toolCallId), fullIndex); const invocation = call ? `${toolInvocationLine(call)}\n` : ""; current.steps.push(storeCursorBlob(toBinary(ConversationStepSchema, create(ConversationStepSchema, { message: { @@ -1170,8 +1229,10 @@ function buildPreparedCursorRunRequest( // Index calls from the FULL history, not the suffix: the cut can fall between a call and // its result, and a result replayed without its invocation is the orphaned-result defect. const fullHistoryCalls = toolCallsByCallId(request.rawMessages); - const suffixRoots = rootPromptMessages(suffixRequest, requestScope, fullHistoryCalls); - const suffixTurns = conversationTurns(suffixRequest, requestScope, suffixRoots.historyMessageStart, fullHistoryCalls); + // `suffixStart` re-bases the replayed slice into full-history space, which is the space + // `fullHistoryCalls` positions live in. Without it the positional bound compares two origins. + const suffixRoots = rootPromptMessages(suffixRequest, requestScope, fullHistoryCalls, suffixStart); + const suffixTurns = conversationTurns(suffixRequest, requestScope, suffixRoots.historyMessageStart, fullHistoryCalls, suffixStart); const suffixSystemCount = systemPromptBlobs(suffixRequest).length; const suffixHistoryIds = suffixRoots.ids.slice(suffixSystemCount); const suffixHistorySerialized = suffixRoots.serialized.slice(suffixSystemCount); diff --git a/tests/cursor-tool-result-invocation.test.ts b/tests/cursor-tool-result-invocation.test.ts index 545c1e4ef0..2c87fe0942 100644 --- a/tests/cursor-tool-result-invocation.test.ts +++ b/tests/cursor-tool-result-invocation.test.ts @@ -446,3 +446,110 @@ describe("cursor checkpoint continuation names the invocation from covered histo expect(root).not.toContain("echo SECOND"); }); }); + +/** + * devlog 260829 060: the index that names an invocation had no ordering constraint, so it would name a + * call that runs LATER in history than the result being labelled. Measured on the shipped tree, a + * result whose own output was `EARLY-OUT` came out as + * `invoked: exec_command with {"cmd":"echo LATER"}` — the mislabel the index's own comment calls worse + * than no label, because nothing downstream can detect it. + * + * The bound compares positions in FULL-HISTORY space. That matters because two call sites replay less + * than the whole history: the checkpoint path replays a suffix, and the turn builder starts at + * `historyMessageStart`. The comparison position is therefore `knownCallsOffset + start + local`, and + * dropping any term passes almost every test here — which is why the last case exists. + */ +describe("cursor invocation lookup is bounded by history position", () => { + const FWD = "call_fwd"; + + /** Result at index 1; the call claiming its id is at index 3. */ + function forwardHistory(): OcxMessage[] { + return [ + { role: "user", content: "start", timestamp: 1 }, + { role: "toolResult", toolCallId: FWD, toolName: "exec_command", content: "EARLY-OUT", isError: false, timestamp: 2 }, + { role: "user", content: "next", timestamp: 3 }, + { + role: "assistant", + content: [{ type: "toolCall", id: FWD, name: "exec_command", arguments: { cmd: "echo LATER" } }], + timestamp: 4, + }, + { role: "user", content: "answer", timestamp: 5 }, + ]; + } + + test("a result whose call appears LATER in history gets no invocation line", () => { + const root = resultRoot(encode(forwardHistory(), "grok-4.6-high")); + expect(root).toBeDefined(); + expect(root).toContain("EARLY-OUT"); + expect(root).not.toContain("invoked:"); + expect(root).not.toContain("echo LATER"); + }); + + test("the turn step is bounded too", () => { + const step = turnStepTexts(encode(forwardHistory(), "grok-4.6-high")) + .find(text => text.startsWith("[Tool Result]")); + if (step) { + expect(step).not.toContain("invoked:"); + expect(step).not.toContain("echo LATER"); + } + }); + + // The bound must not become a blanket refusal: without this, a lookup that returns nothing at all + // would satisfy the case above and look correct. + test("the ordinary call-then-result order is still named", () => { + const root = resultRoot(encode(history(), "grok-4.6-high")); + expect(root).toContain("invoked: exec_command with"); + expect(root).toContain("echo AAA"); + }); + + test("a call before the checkpoint cut is still named on the root path", () => { + const root = resultRoot(encodeCheckpoint(history(), "grok-4.6-high", 2)); + expect(root).toContain("invoked: exec_command with"); + }); + + /** + * The one case that needs all THREE offset terms. Audits r5, r6 and r7 each measured that a bound + * computing `knownCallsOffset + local` — dropping `historyMessageStart` — passes every other + * assertion in this file and the whole cursor suite, while emitting a live orphan here. + * + * It cannot be caught on the root path: `historyMessageStart` is an OUTPUT of `rootPromptMessages`, + * assigned after the loop that would use it, so that loop always walks full-history `i` from zero and + * the dropped term is identically zero there. Only `conversationTurns` carries a non-zero `start`. + * + * Both offsets must actually be non-zero for the case to bite, so the history forces a checkpoint cut + * AND enough root pressure to prune, and the assertion is on the TURN step. + */ + test("checkpoint plus root pruning still names the call on the turn path", () => { + const CK = "call_ck3"; + // CURSOR_EXTERNAL_ROOT_BYTE_LIMIT is 512 KiB; this must exceed it to force any pruning, so + // historyMessageStart lands above zero. A 400 KiB message left it at 0 and made the case toothless. + const bulky = "Z".repeat(600 * 1024); + const messages: OcxMessage[] = [ + { role: "user", content: "first", timestamp: 1 }, + // Pruned from the root, which is what pushes historyMessageStart above zero. + { role: "user", content: bulky, timestamp: 2 }, + { role: "user", content: "carry on", timestamp: 3 }, + { + role: "assistant", + content: [{ type: "toolCall", id: CK, name: "exec_command", arguments: { cmd: "echo COVERED" } }], + timestamp: 4, + }, + { role: "toolResult", toolCallId: CK, toolName: "exec_command", content: "COVERED-OUT", isError: false, timestamp: 5 }, + { role: "user", content: "answer", timestamp: 6 }, + ]; + // Derived rather than guessed: dropping `start` under-counts a walked message's position by + // exactly `start`, so it flips the decision only when the call is INSIDE the slice and + // (w_result - w_call) <= start. The call must therefore sit next to its result in the replayed + // region, not in the covered region — three earlier fixtures put it in the covered region, where + // the call's position is below the offset and the under-count can never cross it. + const bytes = encodeCheckpoint(messages, "grok-4.6-high", 1); + // Assert on the TURN step specifically. Pooling roots and turn steps together hid the mutation: + // the root path has no historyMessageStart term to drop (it is an OUTPUT of rootPromptMessages, + // assigned after the loop that would use it), so the root keeps naming the call and an + // either-source assertion stays green. Only the turn step discriminates. + const step = turnStepTexts(bytes).find(text => text.includes("COVERED-OUT")); + expect(step).toBeDefined(); + expect(step).toContain("invoked: exec_command with"); + expect(step).toContain("echo COVERED"); + }); +});