fix(bridge,adapters): a turn that did not finish is not completed - #1995
Merged
Conversation
buildResponseJSON defaulted status to "completed" whenever no error/incomplete event
was present, so a turn whose adapter simply stopped emitting came back as a success.
The worst shape was a tool call left open: the response carried a function_call with
arguments '{"code":"tru' and status completed, inside a turn also marked completed.
A caller trusting either field would try to execute half-written JSON.
The same events WITH tool_call_end already produced status failed and item status
incomplete, and the streaming path already reported response.incomplete/adapter_eof,
so the bridge knew how to reject this - it just never ran that path when the stream
stopped on its own.
A buffered turn with no adapter terminal now resolves to incomplete with
incomplete_details.reason adapter_eof (the same string streaming uses), and an open
tool call is flushed as incomplete rather than completed. Explicit done/error/incomplete
outcomes are untouched, including each event's own reason.
This closes the default itself. 010 and 040 removed two Cursor-side routes to it; any
adapter that ends a stream without a terminal is now covered, including future ones.
bridge.ts is shared by every provider, so this was verified against a full-suite
baseline captured before the change (12761 pass / 0 fail across 826 files).
Audit follow-up. The #422 guard suppresses the compaction item for a truncated turn, but it could only see explicit error/incomplete events. A stream that stopped without ANY terminal slipped past it: a probe produced status incomplete / adapter_eof while still emitting a compaction item, which becomes the conversation's replacement history. That is exactly the hazard #422 exists to prevent, reached by a route that did not exist when the guard was written. The guard now also requires sawTerminal. Verified red-before-green, and the completed and explicit-failure paths are pinned by their own tests so the suppression cannot widen silently.
…action history Audit round 2 found the #422 guard still had two open routes, both verified before fixing: 1. Streaming emitted the compaction item BEFORE reading done.stopReason, so a max_tokens or content_filter turn shipped a half-written summary and then declared itself incomplete. The buffered path had always checked this; streaming had the same hazard one branch over. 2. Google's parseResponse ignored finishReason entirely and emitted a clean done, so a buffered MAX_TOKENS turn was reported as completed - and on a compaction turn its partial summary became replacement history. The streaming path already mapped MAX_TOKENS and the safety reasons to a stopReason; the buffered path did not. Both now match. Verified red-before-green: 5 of the new tests fail with either fix reverted. Suppression is pinned in both directions so it cannot widen - a clean compaction turn must still ship exactly one item, since codex-rs fatals on zero.
…ocabulary Audit round 3 found the #422 guard was still bypassable, and reproduced it: Command Code emits done(stopReason: "length"), which neither bridge recognized, so a truncated compaction turn shipped its partial summary and reported completed. Anthropic forwards raw stop_reason values like "refusal" the same way. Only openai-chat and google normalized to the canonical pair the guard matched. stopReason is an open-ended string, so the guard cannot depend on which adapter produced the event. isTruncatedStopReason now recognizes the canonical names plus the raw OpenAI/Anthropic/Gemini vocabularies, and both the streaming and buffered guards use it. The buffered path keeps the raw reason alongside the narrowed one, since the narrowed value exists only to map onto incomplete_details. Unknown reasons stay non-truncated on purpose: this must never turn a healthy turn into a failure, and an unrecognized value is far more likely an ordinary stop. That is pinned by its own test, as is the requirement that a clean turn still ships EXACTLY ONE compaction item on both paths - codex-rs fatals on zero, and the previous test only checked presence, so a duplicate would have passed. Verified red-before-green: 5 of the new tests fail with canonical-only matching restored.
…ment Round 4 caught a regression I introduced one round earlier. Recognizing raw stop reasons suppressed the compaction item for values like "length" and "refusal", but the terminal mapping stayed canonical-only - so those turns reported response.completed with ZERO compaction items, which codex-rs treats as fatal. Suppressing the item without also downgrading the turn was worse than the bug it replaced. The classifier now returns WHICH incomplete_details reason a truncation maps to, and both bridges use that single decision for suppression and for status. A truncated turn is incomplete on both paths, for every vocabulary. Also fixed: matching claimed to be case-insensitive but lowercased the input against uppercase-only entries, so "SAFETY" matched while "Safety" and "safety" did not. All entries are stored lowercase and compared lowercase. Vocabulary extended with the values round 4 documented from vendor references: Anthropic model_context_window_exceeded, AI SDK content-filter and error, and the Gemini enums MALFORMED_RESPONSE, UNEXPECTED_TOOL_CALL, IMAGE_SAFETY and LANGUAGE. Unknown reasons remain non-truncated, pinned by a test over end_turn, stop, stop_sequence, tool_use, STOP and tool-calls: a false positive costs a compaction item, and zero is fatal.
… a failure Two remaining bridge-side gaps from round 5. Anthropic forwards pause_turn verbatim, and the classifier read it as a normal completion. Anthropic documents it as a long-running turn the client is expected to CONTINUE, so whatever was produced is by definition unfinished and must not become replacement history. The AI SDK's error finish reason means generation FAILED upstream, not that it stopped. Carrying it as done+stopReason forced a bad choice: read as a clean completion, or (once classified) mislabel an upstream error as a content filter and reject it from the replay cache for the wrong reason. Command Code now emits a proper error terminal, so the turn reports failed with an accurate cause and error is no longer in the truncation table. The two remaining findings are adapter-side erasure - Kiro's disabled completion mode and ordinary Google mode both drop finish reasons before the bridge can see them. Those are separate subsystems with their own truncation handling and are recorded as follow-ups rather than folded in here.
…turn and keeps usage Round 6 found both of these, and both were mine. Removing 'error' from the shared truncation table was only safe for Command Code, which converts it upstream. Anthropic-compatible providers forward stop_reason verbatim, so a probe with stop_reason 'error' produced a clean done and both bridge paths reported completed with a compaction item installed. Anthropic now emits an error terminal on both the buffered and streaming paths. The Command Code error terminal I added last round dropped usage, so a failed turn looked free in accounting and reported zeros to the client. Both error terminals now carry usage: a turn that failed still consumed tokens. The new tests drive the REAL adapter parsers. The previous suite constructed the downstream error event by hand, which is why it stayed green while the adapter itself still emitted a clean done - the gap the audit named.
Anthropic has THREE terminal paths and I had fixed two. The third exists for compatible providers that close after message_delta without message_stop, and it bypasses emitDone entirely - so it still emitted done(stopReason: error) and the bridge reported completed with a compaction item installed. Same rule now applies there. Test hardening from the same review, both cases where a green test was protecting nothing: - The Anthropic streaming test was titled "carrying usage" but never asserted usage, so removing it would have kept the test green. It now asserts input and output tokens, and a new case covers the EOF route end to end. - The Command Code coverage hand-constructed the downstream error event instead of driving the real parser. tests/command-code-error-finish.test.ts now exercises parseStream and parseResponse directly, and pins that an ordinary finish still completes and that a length finish is still a truncation rather than an error. Verified non-vacuous: dropping usage from either error terminal turns the suite red.
The campaign shipped against f64c063. dev has moved 104 commits since, and two of our 18 files were touched there too. A per-file sweep found exactly one SEMANTIC collision and one textual one; the other 16 paths have zero dev commits. live-transport.ts is the semantic one: dev fixed the SAME clean-EOF defect in the opposite shape (a fail-closed error EVENT via finalizeTurnEvents, with CodeRabbit explicitly rejecting a throw) while our 54f68da raises a typed transport error. An independent read-only investigation traced both shapes through the streaming and buffered Responses paths, including our own phase-050 bridge changes, and dev's shape survives: the error sets both errorEvent and sawTerminal, so buildResponseJSON already returns failed with no adapter_eof and no compaction history. What our branch still contributes there is emittedTerminal, which f145fd5 depends on, plus one extra guard so EOF finalization cannot append a second terminal after a mapper error. google.ts is textual: dev's six commits are request-side identity work, so the buffered finishReason hunk applies unchanged at :946 instead of :939. Docs only. No code moved in this commit.
…accepted Round r1-20260818030046 attacked the integration plan before any rebase step ran and returned FAIL. Every finding was re-checked against the tree; none was waved through. Three changed the plan structurally. F3 needed a new work-phase. Choosing dev's error-EVENT shape for clean-EOF (010) is correct on its own merits, but finalizeTurnEvents' open-tool branch emits no usage while the thrown path attaches partialUsage. So the shape choice would have silently traded away token reporting on truncated turns. resolvedTurnUsage is already in that module and already used by the done branch, so wp2b/015 fixes the omission with a test that asserts usage separately from shape — the campaign already shipped one test titled "carrying usage" that never checked usage. F1 is real and is NOT fixed here. Every Cursor model sits in noVisionModels (registry.ts:978-982), so the vision sidecar replaces tool-result images with text before the adapter runs (core.ts:2225-2243, vision/index.ts:252-259,565-581). The 020 encoder work is correct and currently unreachable in production. Fixing it means role-aware vision policy plus an end-to-end test, and dropping Cursor from noVisionModels alone is unsafe because user images are still flattened. Recorded as follow-up 1 with the overstated capability claim corrected. F2 reframes the merge. MAINTAINERS.md:48-49 requires maintainer approval and successful required CI; the user waived CI and granted admin merge. That is owner authority, not compliance, and lidge is Linux-only. 040 now records it as an owner-authorized exception and forbids the readiness note from claiming "policy-compliant". F4 killed the three-PR stack: ten devlog commits precede the first code commit and docs interleave after each phase, so the advertised split needs cherry-picking and would not be this history. One honest PR instead, with the reasoning written down. F5 moved privacy:scan and audit:high before the PR (they are in release.ts:374,380) and requires the docs determination before the template box is ticked. F6 corrected the counts: 28 paths, not 18; 32 commits, not 31.
… not the capability Audit r1 finding F1 proved the decode unit overstated phase 020. The encoder does emit real McpImageContent, and that part is right. But every Cursor model is in noVisionModels (registry.ts:978-982), so the vision sidecar replaces tool-result images with text descriptions before the adapter runs, or strips them fail-closed when no sidecar plan exists (core.ts:2225-2243, vision/index.ts:252-259,565-581). The phase test calls encodeCursorRunRequest directly, so it never crosses that preprocessing and proves encoder support only. Correcting the wording rather than the code is deliberate: dropping Cursor from noVisionModels would leave user images with neither an image nor a description, because request-builder.ts:206-214 still flattens them. The fix is role-aware vision policy plus an end-to-end regression, which is its own unit.
r2 re-verified all six r1 closures and found three that were real but incomplete. The important one is the usage resolver in WP2b. My first draft said resolvedTurnUsage(state), which is the CLEAN-turn resolver: it falls back to the session carry-forward and then the request estimate, so it returns a number even when this turn produced no token signal. partialUsageFromEventState exists exactly because that is wrong for a failure — its own comment says a carry-forward belongs to an earlier successful turn and cannot prove a first-frame failure consumed anything. So the draft would have traded reporting 0 tokens for reporting someone else's tokens. 015 now moves that helper down into protobuf-events.ts (the dependency already runs that way; the reverse would cycle) and adds the negative regression case r2 asked for. Also confirmed the patch site: message-mapper.ts:29 forwards usage on an error event, so no mapper change is needed. Second: every moving-ref number in the plan was already stale. dev went from 87f7f97 (+104) to e1bdbc1 (+124) while I was writing. The counts are now pinned to immutable SHAs, the rebase re-reads origin/dev instead of trusting the doc, and the collision set was re-checked at the later head — still the same two paths. Third: the plan deferred F1 while its write scope conditionally allowed WP2b to edit vision policy. That contradiction is removed; vision is out of scope. And the honesty cleanup was incomplete — request-builder.ts and two test files still said tool-result images "reach Cursor". They now say the encoder emits them, and name the noVisionModels preprocessing that stops anything from reaching Cursor today. Comment-only in src/ and tests/: bun test tests/cursor-request-builder.test.ts tests/cursor-tool-result-image.test.ts -> 48 pass / 0 fail.
…ore the stacked PR r3 was a fresh reviewer (r1/r2's was retired; a final gate must not reuse a contaminated one). Every CODE resolution passed: the merged EOF block typechecks and preserves dev's guard precedence, the WP2b helper move creates no cycle, the mapper and bridge really do carry usage on a failed turn, and the collision set is still just two files at dev e1bdbc1. The five findings were all in the workflow. F3 is the one worth reading. r1 correctly killed a FABRICATED adapter/bridge/docs split, and I drew the wrong conclusion from it — "no honest split exists" instead of "that split was the wrong one". The campaign's own phase boundaries are already clean commits: dfb6fb8 ends the Cursor wire work (17 commits), 6d97442 ends the CANCEL provenance work (3), and the rest is bridge/adapter terminals (15). The file sets separate cleanly and the dependency is real — PR2's guard reads PR1's emittedTerminal, and PR3's bridge logic is what makes PR1/PR2's error events reportable at all. So the user's stacked PR request is back, with a per-layer verification table because AGENTS.md:178-180 wants each layer's own evidence. F1 and F2 are both about not trusting a cached ref or a shared checkout. dev moved twice while I was planning, so the base is now pinned with git ls-remote (the live head, the way scripts/release.ts:327-335 does it) and re-read before every merge in the stack. And the remote gates no longer checkout -f the shared lidge checkout, which would have discarded any tracked uncommitted work there; they use a dedicated /tmp/ocx-* worktree, which is already that host's pattern. F4: build:gui is not N/A just because no gui/ path changed — prepublishOnly runs it unconditionally and it also runs prepare:package. It moves into both gate phases, and the readiness note must say publication still needs Cross-platform CI plus Service lifecycle at the exact SHA. F5: the plan cached origin/main = 474584b, which is now 0013b23 while the v2.24.2 tag still points at the old SHA. Same root cause as F1.
r4 was dispatched as an explorer role so the SubagentStop observer actually records the verdict; r2 and r3 had to be aborted as inconclusive because their reviewers ran outside that hook's matcher. Every code judgment held again — PR1 and PR2 tests genuinely do not depend on PR3 code, so the stack is real — and all four findings were about how the plan EXECUTES. F1 caught the pin being decorative: 020 recorded a live SHA while 010 still said git rebase origin/dev. And 040's rule that every merge must see dev equal to the original VERIFIED_BASE is false the moment PR1 lands, since PR1's merge result IS the new dev. It defined PR2's updated expectation and forgot PR3. EXPECTED_DEV now evolves through the stack, read live with ls-remote each time, with an explicit stop-and-rebase if it moved. dev drifted 87f7f97 -> e1bdbc1 -> 1645bb9 during planning alone. F2 is the one that would have produced mislabeled PRs. The commit ranges were miscounted (6d97442..HEAD is 16, not "15 + WP2b"), the top range also edits three PR1-owned files via the r2 honesty corrections, and the rebase rewrites the very boundary SHAs 030 named as branch points. So 030 is rewritten around subsystem OWNERSHIP with a real procedure: map rewritten boundaries by subject line, move the late PR1-owned edits below the PR1 boundary, land WP2b inside PR1 before cutting the branch, then re-verify every layer's file set and refuse to open a PR whose contents do not match its title. F3: 54f68da's resolution drops CursorStreamTruncatedError from the import line that f145fd5 also edits, so a second conflict is guaranteed. Its functional context survives intact, so 010 now carries the literal resolved import line. F4: cursor-interaction-query.test.ts is WP2b's own contract test and was gated in PR3 while WP2b lands in PR1 — PR1 could have merged with the re-export untested.
…of re-slicing r5 verified r4's four closures: the evolving base pin, the import-line resolution, and WP2b's test placement all held. r4-F2's did not, and the reviewer was right that the sequence could not be executed as written. The procedure had four concrete defects: steps 2-3 put commits on cursor-call-wire before step 4 created it (and step 4 created it at a point that excluded them), no restack of PR2/PR3, a claim that retargeting could make an existing commit a no-op, and no handling for two late doc edits. The partition also still leaked — be1b881 edited two decode docs that PR1 owns, and cursor-interaction-query.test.ts was in PR1's Owns set while 015 explicitly leaves that file unchanged, so an exact file-set match was impossible by construction. The real problem is that four rounds all tried to re-slice a finished linear history, which forces commits to move between layers, and every mechanism for moving them broke a different invariant. So 030 now builds the stack FORWARD: each layer branch is cut from its base and its files are checked out from FINAL (the rebased tip), then committed as that layer's contribution. PR1 union PR2 union PR3 is then identical to cursor-call by construction, and step 4 checks that mechanically instead of by reading. The late doc edits stop being a special case because step 1 takes the whole decode directory at its final state. live-transport.ts is the one file needing care, since FINAL's copy already contains PR2's classifyTurnFailure — PR1 takes that file from the wire-work boundary commit, whose subject r5 confirmed is unique in the range. 030 also states a fallback out loud: if the union check fails for a genuine interleaving reason, open one PR and say why. A fifth iteration on the split is not on the table. r5's third finding was a literal one: 010 wrote an escaped \$(...) that zsh would assign as command text and a literal \$VERIFIED_BASE passed to git rebase. Fixed.
… and it was a category error r6 found four blockers in the forward-construction procedure. PR3 was not an ancestor of PR2, so a stacked PR would have shown the whole original series. PR1's pathspecs were overbroad: taking cursor-errors.ts from FINAL hands PR1 PR2's CursorUnexpectedCancelError, and taking the whole decode directory hands it PR2's and PR3's phase docs. WP2b's live-transport.ts portion landed in PR2 because PR1 deliberately took that file from an earlier boundary. And the union check proved nothing — a whole-range --stat is fixed by its endpoints and stays identical no matter which layer content lands in. That is four consecutive failures on the same question, each on a different mechanism, which under LOOP-REPAIR-01 is the point to stop patching the answer and re-read the question. The error was mine. I had been requiring the layers to be subsystem-pure — each touching only its own files — which I inferred from r4 F2 noting that the top commit range also edits PR1-owned files. A stacked PR does not promise subsystem purity; it promises reviewable increments in dependency order. Purity requires moving content between commits, the history is final, and every mechanism for moving it broke a different git invariant. r3 F3 had the right answer and I over-corrected it away. 030 now cuts the rebased history at two existing commits. Ancestry is automatic, the union equals the branch by construction, and no commit moves — which closes all four r6 findings at once instead of patching them individually. Step 3 proves it with two --is-ancestor checks and three range counts that must sum to the whole. Consequence: WP2b and cursor-interaction-query.test.ts are both in PR3, where WP2b lands chronologically. r4 F4's principle stands and is satisfied in the other direction. PR1 is still correct without it — PR1 makes a truncated turn reportable, PR3 makes it report tokens. PR3's body names both that and the late honesty corrections to PR1-owned files, so a reviewer is not left wondering why a bridge PR touches a cursor comment.
…sh was missing r7 measured the branch-pointer stack instead of taking it on faith: 39 commits, zero merges, boundary subjects unique, ranges partitioning exactly 17 + 3 + 19 = 39, and branch creation adding pointers only. All four properties the previous four schemes fought for, obtained by dropping the subsystem-purity requirement that caused them. The blocker was a hole between phases. 020 has lidge fetch origin/cursor-call and build a worktree at the new SHA, and 010 said the audit happens "before pushing", but no push command existed anywhere between them. lidge would have fetched the pre-rebase branch and either failed on an unknown revision or silently tested stale code without the rebase or WP2b. 010 now pushes with --force-with-lease and asserts ls-remote equals rev-parse; 020 re-confirms the SHA on lidge before installing. Also renamed 030's prose base from EXPECTED_DEV to VERIFIED_BASE — 040 defines EXPECTED_DEV as the evolving merge-time variable, and 030's own commands already used VERIFIED_BASE. r7 also judged the layering honest: PR1 gates independently, PR2's dependency on PR1's emittedTerminal is real, and PR3's breadth is disclosed rather than hidden. Running cursor-eof-terminal.test.ts in both PR1 and PR3 is meaningful because PR1 checks terminal shape and PR3 adds the usage cases, with PR1's toMatchObject tolerating the new field. Seven rounds, 27 findings, all absorbed. The roadmap cycle is done.
…of gap r7 walked the whole sequence and found nothing. Its verdict could not be recorded because the goalplan's activeWorkPhaseId was null, and the review observer discards any sign-off whose round targets a work-phase that is not active (review-observer.ts:99-101). That is why r2 through r6 all had to be aborted as inconclusive after their findings were absorbed — the findings were real, the FSM record was not. Fixed. r8 then re-confirmed independently and found one thing worth having: step 3 asserted wire -> cancel -> tip but never VERIFIED_BASE -> wire, and the counts do not cover that gap because rev-list --count A..B counts commits reachable from B and not A even when A is not an ancestor of B. The reviewer demonstrated it against dev at 1645bb9, where --is-ancestor exits 1 while the three counts still sum. So a stack could have passed step 3 with its bottom not on the verified base — exactly the class of defect this plan kept failing on. The assertion is added with its reasoning inline so nobody deletes it as redundant later. Eight rounds, 26 findings, all absorbed. Four of them attacked the same question and the fourth failure was the signal the question was wrong, which is recorded in 009.
A suffixed 010a is not a numbered plan document, and cxc review-round rejects it as a plan path.
…rong tip r8 confirmed r7's two fixes and found one more phase-boundary hole. 010 step 7 pushes the rebase tip and 020 verifies whatever origin/cursor-call points at, but the declared order is WP2 -> WP2b -> WP3 and WP2b changes code after that push while carrying only local checks. lidge would have authoritatively verified a tree without WP2b, and PR3's WP2b implementation would have reached dev backed by nothing but a local bun test. 015 now ends with its own force-with-lease push and SHA assertion, and 020 states which of the two pushes it consumes. The earlier push stays as a post-rebase checkpoint. Same class as r7's finding: a phase boundary where the artifact one side produces is not the artifact the other side reads. That is why both survived rounds of reading each document on its own terms — neither doc is wrong alone. r8's second finding was already closed by 045f193, which added the missing VERIFIED_BASE -> cursor-call-wire ancestry assertion; the reviewer saw the branch move mid-read and said so.
… path cxc review-round open requires a numbered document; 00A sorted correctly but did not match the numeric pattern.
review-round open only accepts NNN_*.md; 009b and 011 collided with that check and with each other.
… PR heads r10's job was to sweep every phase boundary for the defect class the previous two rounds each found: one phase produces an artifact, the next reads a different one. It found a third instance, and this one was invisible to the check meant to catch it. 020 verifies one specific SHA. 030 cut branches from cursor-call, a mutable ref, without asserting it still equaled that SHA, and 040 checked only the live dev base before each merge, never the PR's own head. So a force-push to any PR head would merge commits no gate had seen — and 040's post-merge ancestry check still passes, because the verified tip stays an ancestor of a superset. The fix threads one named SHA through three phases. 020 records VERIFIED_TIP after WP2b's push. 030 step 0 refuses to cut branches unless cursor-call still equals it, and step 5 records each PR's expected head. 040 asserts headRefOid against that before every merge, with the reasoning inline so it does not get deleted as redundant with the base check — the base check proves dev has not moved and says nothing about what the PR points at. Three rounds, three instances, same shape: r7 (nothing pushed the rebase before remote verification), r8 (the push preceded WP2b), r10 (the verified tip was never bound to what merges). Each document was correct alone; the defect lived in the seam. 014 records the general lesson so the next unit inherits it. r10 also confirmed r8's fix: 015 ends with its own force-with-lease push and SHA assertion, 020 consumes that later push, both snippets parse correctly under zsh, and the remaining handoffs are coherent.
Sweeping for the r7/r8/r10 defect class one more time surfaced the last unbound handoff: 050 took <MERGED_DEV_SHA> as a placeholder with nothing tying it to what 040 actually merged. A fresh read of origin/dev there would gate whatever the branch happens to point at, so a green result could be attributed to someone else's push. 040 now records MERGED_DEV from ls-remote after PR3 lands and asserts VERIFIED_TIP is an ancestor of it; 050 says explicitly that it gates that SHA and not a re-read. That closes every artifact boundary in the plan: VERIFIED_BASE (rebase target), VERIFIED_TIP (the verified tree), PR1_HEAD/PR2_HEAD/PR3_HEAD (what merges), EXPECTED_DEV (the evolving merge base), and now MERGED_DEV (what the release gates run against).
…that run r13 did what twelve prior rounds had not: it executed the snippets. Four blockers, all one kind — the right check described in prose, not expressed as something a shell would run. PR1_HEAD = <PR1_TIP> is not an assignment; the reviewer's probe exited 127 with command not found, because NAME = value runs NAME as a command. 040 printed headRefOid instead of comparing it, so the gate depended on someone eyeballing a value. PR1_TIP and PR2_TIP were read by eye and never asserted against the branches actually created — the ancestry and count checks prove topology, not identity. And VERIFIED_TIP was captured after the gates rather than before, which reopens inside WP3 the exact drift r10 closed between phases. All five are now executable: 020 captures VERIFIED_TIP from ls-remote before building the worktree at that SHA and asserts HEAD matches; 030 binds the boundary SHAs with git log | grep -F | cut and asserts the created branch tips equal them; 040 merges behind a test against the recorded head. MERGED_DEV now comes from gh pr view --json mergeCommit — the merge itself — with a separate assertion that dev still points there, so a concurrent push cannot be attributed to this campaign. Probed the extraction against the real tree: it returns dfb6fb8 and 6d97442, exactly the two boundary commits 030 names. Four rounds running (r7, r8, r10, r13) found the same failure mode at different altitudes: a binding that lives in prose instead of in a command. A seam is only real when it is a test.
…t tested Two more instances of the class r7, r8 and r10 already found: a phase boundary where the artifact one side produces is not the artifact the other side reads. 020 created the lidge worktree at an unnamed SHA, ran every gate, and only then introduced VERIFIED_TIP in a section that claimed to come before the gates. If cursor-call moved during the eight-minute suite, 030 would bind to a tree nobody tested. The capture now happens first, reads the live remote, asserts local and remote agree, builds the worktree at that SHA, and re-asserts HEAD inside it before installing — and the worktree path carries the SHA. The second one is worse in principle: 020's per-layer table said what each layer runs and never where. Running PR1's tests at the stack tip proves nothing about PR1, because that tree already contains PR2 and PR3, so a PR1 test could pass on code a PR1 reviewer never sees. The layer branches only exist from 030 step 2, so WP3 is now explicitly ordered — stack-tip gates, then cut and prove, then one pinned worktree per layer head, then open the PRs each citing its own run. Five instances of one class across five rounds is worth naming, so 016 tabulates them: each document reads correctly alone, and the defect only appears when you ask at every boundary what the next phase actually binds to.
…ose-to-command sweep
The r13 confirmation reviewer caught that my own fix had been nullified: 040 carried
TWO "Hand MERGED_DEV to WP6" sections, and the second one — the original, left in
place — reassigned MERGED_DEV from a fresh ls-remote of dev, overwriting the corrected
binding to PR3's merge commit two paragraphs above it. Deleted.
Three remaining prose bindings became commands. 040's EXPECTED_DEV ladder used := and
English ("before PR1: live dev == EXPECTED_DEV"), which is a legend, not a gate; it is
now a real per-layer sequence that tests the base, tests the PR head, merges, and
advances EXPECTED_DEV from that layer's merge commit rather than from a fresh read.
040's Procedure block printed the live ref instead of comparing it. And 020 printed
ls-remote and said the result "is" VERIFIED_BASE instead of assigning it.
Probed the whole chain under zsh: parses clean, and the assertions pass against the
live refs (VERIFIED_BASE=1645bb924, DEVDIR derived correctly).
Five rounds found this same class — a seam described correctly in prose that no shell
would enforce. This commit is the last of them: every named artifact in the plan is
now captured by a command and asserted where it is consumed.
…lan, not reading it r14 executed every shell fragment in the unit against a scratch zsh with real SHAs, simulating only the mutating commands. Three High findings. The gates in 020 were bare local commands sitting under a section that had just pinned a lidge worktree. Copied into a shell they run wherever the operator happens to be, so the phase could report green for a tree that is not VERIFIED_TIP — the exact failure r10 and r12 were about. They are now a loop that runs each gate over ssh inside the worktree and re-asserts HEAD first. The per-layer block could not run at all: it referenced $PR1_HEAD before 030 assigns it, and it contained a literal <that layer's files from the table>, which is a zsh parse error rather than an instruction. It now runs after 030 step 4, spells the test lists out as variables, and wraps the work in a run_layer function that pins, asserts, installs, typechecks and tests. The third is the sixth artifact-chain gap. 040 asserted whether dev moved and whether each PR head moved, but never whether the PR still points at dev. A retarget to main, or to a parent branch that has since merged, passes both checks and gh pr merge would merge into that base. The pre-merge check is now three assertions together: base is dev, head equals the recorded SHA, live dev equals EXPECTED_DEV. r14 also ran the focused Cursor tests itself: 71 pass, 0 fail, typecheck exit 0. And it confirmed the stack partition at its anchor: 17 + 3 + 31 = 51, no duplicate subjects, no merges.
r14 audited by executing every snippet under zsh -n plus the read-only extractions. Six of nine named artifacts failed, after thirteen rounds of reading had found none. VERIFIED_BASE was captured twice: 010 pins it before the rebase, 020 captured it again afterwards, so a second ls-remote would overwrite the pin with a newer dev and every later assertion would compare against a base the campaign never rebased onto. 020 now inherits and asserts it. git branch cursor-call-wire <PR1_TIP> fails zsh -n outright — the angle-bracket form is not shell syntax, and 030 had captured the variables then not consumed them. Fixed in both the branch creation and the git show --stat confirmations. 040 carried three merge procedures that disagreed with each other. One referenced $PRN, $PRN_HEAD and $PR_NEXT, none of which any phase assigns. One omitted both the head assertion and the EXPECTED_DEV update. r14's sharpest point: the angle-bracket forms in 040 parse only because zsh treats them as redirections, which is worse than failing, because they run and bind nothing. There is now one merge_layer function taking the PR number and its expected head, asserting base + head + EXPECTED_DEV before merging and advancing EXPECTED_DEV from the merge commit. Probed the whole chain under zsh: parses clean and returns dfb6fb8 and 6d97442, the two boundary commits 030 names. Also cleaned up after the auditor: it executed one 020 snippet during isolation and left worktree /tmp/ocx-L- and branch ocx-L- on lidge. Both removed; nothing was pushed or edited there.
Two 018 files had appeared for the same round — one for r14's code-block pass, one for its named-variable pass. Merged: 018_audit_r14.md now carries both, and the duplicate is gone. The second pass is the one worth keeping in the same place as the first, because it shows the same method finding different things depending on what you enumerate. The code-block pass found fragments that could not run; the variable pass found bindings that ran and bound nothing — including angle-bracket forms in 040 that zsh silently reads as redirections rather than rejecting.
Seven audit rounds (r7, r8, r10, r12, r13, r14, r15) found the same defect class at different altitudes: a binding written as prose that no shell would enforce. Each round I fixed the instances it named and the next round found new ones — VERIFIED_BASE captured twice, git branch cursor-call-wire <PR1_TIP> failing zsh -n, three contradictory merge ladders, <pr3> that zsh silently reads as a redirection, PR1_TIP consumed at line 88 and assigned at line 94, PR numbers referenced but never assigned anywhere at all. Seven rounds of one failure is not seven mistakes. It is one wrong idea: I was trying to make a document behave like a program. A markdown file cannot enforce that a variable is assigned before it is read. It cannot fail. Every fix was a promise that the next reader would execute the fragments in the right order with the right values in scope, and every audit proved that promise unenforceable by construction. cursor-call-integration.zsh is now the executable form of 010/020/030/040/050. Under set -euo pipefail an unset variable is a hard error, so the entire class is impossible. State persists to .tmp/ so a compaction costs nothing and each step re-reads what earlier steps recorded. Every assertion is a test or an ||die — no printed value for an operator to eyeball. Steps are idempotent. Nothing merges or pushes unless the operator names that step. Verified rather than asserted: parse OK; pin actually recorded VERIFIED_BASE=1645bb924 and it persisted; cut, merge and release_gates each refused with FATAL and exit 1 naming the missing variable; record_prs rejected two arguments; an unknown step exited 1. Those refusals are the point — the ordering the prose could only request, the script enforces. The decade docs keep what a script cannot carry: why dev's error-event EOF shape beat ours, why WP2b uses partialUsageFromEventState rather than resolvedTurnUsage, why the stack splits where it does, why the merge is an owner-authorized exception rather than policy compliance. Each now carries a banner naming its script step and saying that if the two disagree, the script is right.
Probing my own script found two defects the reviewer would have. save appended, so re-running a step left two rows for one artifact. source takes the last, but anything reading the file by eye takes the first — the same one-value-two-readings hazard the whole script exists to kill. It now rewrites the key and also exports it into the current shell, so a step that saves then reads sees its own value. step_push and step_record_prs never called load_state, so they ran with nothing loaded. Harmless today because neither reads an earlier artifact, but it is exactly the omission that becomes a bug the moment one of them does. Verified: three consecutive pin runs leave one line, not three; record_prs stores all three numbers and state reads them back.
The script lives three levels down (devlog/_plan/<unit>/) and ROOT counted two, so it treated devlog/ as the repo root and wrote its state to devlog/.tmp/. That directory is also gitignored, which is why nothing leaked and why the bug was invisible — a wrong path that happens to be safe is worse than one that fails, because it survives. ROOT now comes from git rev-parse --show-toplevel with an explicit failure if the script is run outside a repo. Verified: state lands in .tmp/cursor-call-integration.env, git check-ignore confirms it is ignored, two consecutive pin runs leave one line, and the stale devlog/.tmp copy is moved aside rather than deleted. Third self-inflicted defect found by running the script instead of reading it. That is the argument for having written it.
Round 15 audited the script instead of the prose and found seven; running it myself found three more. The worst: LIDGE_HOME=~/Developer/opencodex expands LOCALLY under zsh, so every ssh command was sending /Users/jun/... to a Linux host. The remote gates could not have run at all. Now single-quoted so the remote shell expands it. merge could not resume: it restarted at PR1 on every invocation, so a disconnect between gh pr merge and save would re-attempt an already-merged PR. merge_layer now reads PR state first — MERGED adopts its merge commit and returns, OPEN proceeds, anything else is fatal — and retarget_to_dev is equally idempotent. Layers were never verified at their own heads, which AGENTS.md:178-180 requires and 020/030 both specify. cut pushed the branches and moved on. That is now verify_layers, with PR1_TESTS/PR2_TESTS as real variables, and merge refuses without LAYERS_GREEN_AT. Smaller: || true on worktree creation swallowed real failures and accepted a dirty tree sitting at the right HEAD (remote_worktree now creates only when missing and asserts clean); neither worktree was ever removed though both phases require it (cleanup step); the conflict scan missed a lone =======; push and verify did not require VERIFIED_BASE; re-running pin silently invalidated downstream artifacts (now needs --repin); and the state file is sourced, so save now refuses anything that is not a plain token. Plus my own three: save appended instead of replacing keys, ROOT counted .. wrong and wrote state into devlog/.tmp/, and push/record_prs never called load_state. Ten defects in 200 lines, none of which fifteen rounds of reading had surfaced.
…icked it Round 16 ran the script and found eight more defects. The worst was mine from round 15's fix: [[ "$v" == [A-Za-z0-9._/-]## ]] needs EXTENDED_GLOB, which set -euo pipefail does not enable, so every save died and pin could not even record a base. zsh -n passes it because the syntax is valid — only running it shows every match failing. A security check that silently rejects everything is worse than no check. LAYERS_GREEN_AT was presence-only: it stored PR2_HEAD and merge asked only whether it was non-empty, so verifying old layers then re-cutting new ones inherited the marker. It now stores PR1_HEAD+PR2_HEAD and merge compares against the current heads. --repin invalidated nothing — the guard refused a silent re-pin while the override left every downstream artifact looking valid. It now clears them, which is what makes the guard mean anything. (And the driver never passed $@ to step_pin, so --repin could not reach the guard at all.) The MERGED resume path adopted a merge unchecked: a PR merged by anyone, from any head, into any base would have been accepted as campaign output. It now asserts base, head, and that the merge commit contains the verified head. PR3's merge not being on dev was a log line; now fatal, since the release gates would otherwise run on a commit that never landed. Also: cleanup saved worktree paths only after all gates passed, so a failed run left them unreachable; release_state did not require the release gates to have run on that SHA; and rebase restarted instead of continuing, which would have discarded the resolution of the two conflicts 010 expects. Verified: pin records, record_prs stores three numbers, the re-pin guard refuses, and pin --repin clears every downstream key.
…tten after them Round 16's second pass found seven more, and one of them generalizes. MERGED_DEV was saved before its ancestry proofs. A disconnect there, or a failing assertion, left a durable key that release_gates trusts on the next invocation — reopening the exact bypass the proof existed to close. It is now the last thing merge does, and that is the rule for every save in the file except the worktree paths, which are deliberately the opposite: they are cleanup handles, so they must exist before the thing that might fail. The MERGED resume path did not prove its predecessor. It checked base, head and that the merge contained the verified head, but not that it was merged onto the EXPECTED_DEV this campaign produced — so a merge made after an unrelated commit landed on dev would have been adopted as the next layer. The merge commit's first parent must now equal EXPECTED_DEV. --repin cleared CC_WORKTREE and DEV_WORKTREE without removing them first, orphaning them on lidge with no handle left; it now removes them before forgetting. A failed per-layer gate left an untracked worktree; LAYER_WORKTREE is recorded before the gates and cleared on success. subject_sha could return two SHAs if two commits shared a subject, producing a two-line "SHA" that every later assertion would compare against. It now fails unless exactly one matches. Gate markers were not evidence — 020 and 050 want the command, its output and the SHA. Every gate now appends a receipt to .tmp/cursor-call-receipts.log, including on failure. And record_prs verified nothing; it now checks each PR points at its layer head and carries all three template sections. The body's substance stays the agent's to write; its structure is checkable, so it is checked.
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
✅ Deterministic PR hygiene checks passed. |
This was referenced Aug 18, 2026
Closed
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff4b0bb4e6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| log "a rebase is in progress — continuing it" | ||
| git rebase --continue | ||
| else | ||
| git rebase "$VERIFIED_BASE" |
There was a problem hiding this comment.
When step_rebase is invoked from any branch other than cursor-call—the script never checks out or validates that branch—one-argument git rebase "$VERIFIED_BASE" rewrites the currently checked-out branch instead. The subsequent assertion examines cursor-call, not the branch that was actually rebased, so it can even pass when cursor-call already contains the base. Validate the current branch/worktree before continuing or explicitly pass cursor-call as the branch to rebase, including for the in-progress-rebase path.
Useful? React with 👍 / 👎.
Comment on lines
555
to
+561
| const stopReason = typeof event.rawFinishReason === "string" ? event.rawFinishReason : typeof event.finishReason === "string" ? event.finishReason : undefined; | ||
| // The AI SDK's `error` finish reason means the generation failed upstream, not that it | ||
| // stopped. Reporting it as a `done` left the bridge to infer failure from a stop-reason | ||
| // string, which either read as a clean completion or (once classified) mislabelled an | ||
| // upstream error as a content filter and rejected it from the replay cache for the | ||
| // wrong reason. | ||
| if (stopReason === "error") { |
There was a problem hiding this comment.
When a Command Code finish event contains the AI SDK's normalized finishReason: "error" together with a provider-specific rawFinishReason such as "internal_error", this comparison sees only the raw value and emits a done terminal. The bridge then reports the failed generation as completed and may install partial compaction history. Test event.finishReason === "error" independently before choosing which reason to forward.
AGENTS.md reference: src/AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
Comment on lines
+953
to
+958
| const finishReason = candidates?.[0]?.finishReason as string | undefined; | ||
| const stopReason = finishReason === "MAX_TOKENS" | ||
| ? "max_tokens" | ||
| : ["SAFETY", "RECITATION", "BLOCKLIST", "PROHIBITED_CONTENT", "SPII"].includes(finishReason ?? "") | ||
| ? "content_filter" | ||
| : undefined; |
There was a problem hiding this comment.
For buffered Google responses ending with IMAGE_SAFETY, LANGUAGE, MALFORMED_RESPONSE, or UNEXPECTED_TOOL_CALL—all classified as truncations by the new shared helper—this allowlist drops the raw reason and emits a clean done. Because the bridge never receives the original value, it reports the turn as completed and a compaction request can install partial output as replacement history. Forward the raw finish reason or normalize through the shared classifier instead of maintaining this narrower list.
AGENTS.md reference: src/AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
| // Suppression and terminal status must agree: withholding the compaction item while | ||
| // still reporting success hands codex-rs a completed response with zero compaction | ||
| // items, which it treats as fatal. | ||
| if (truncationReasonFor(event.stopReason)) { |
There was a problem hiding this comment.
When emptyCompletionRetry is enabled and a newly recognized raw truncation such as Anthropic model_context_window_exceeded produces no visible content, the upstream guard still recognizes only max_tokens and content_filter; it suppresses this terminal, makes a second billable identical request, and ultimately replaces the intended response.incomplete with empty_completion_retry_failed. The same shared truncation predicate used here must be applied by the guard before it decides that an empty terminal is retryable.
Useful? React with 👍 / 👎.
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Bridge and adapter terminal semantics: a turn that did not finish must not be reported as if it did.
buildResponseJSONdefaulted tocompletedwhen an adapter produced no terminal at all. The worst shape it could return was an open tool call with truncated JSON, markedfunction_call/status: completed, inside acompletedturn — a turn that never finished, presented as one that did. Eight review rounds found the rest of the family:sawTerminalflag rather than an assumption.openai-responsesproviders — passthrough assumes native compaction support #422). The streaming path was emitting the compaction item before readingstopReason; Google'sparseResponsewas droppingfinishReasonentirely, so aMAX_TOKENScutoff read as a clean completion.length,refusal, and friends) are recognized throughsrc/responses/truncated-stop-reason.tsinstead of a hard-coded list.completedwith zero compaction items, which codex-rs treats as fatal. Suppression and status now come from one decision (truncationReasonFor), so they cannot disagree.pause_turnis classified as unfinished, and an AI SDKerrorfinish is a real failure: Command Code and Anthropic emit error terminals with usage on all terminal paths, including Anthropic's EOF-without-message_stop.Also carries WP2b (the EOF truncation error now reports the tokens the turn consumed, via the failure-specific
partialUsageFromEventStaterather than the clean-turn resolver — a carry-forward belongs to an earlier successful turn and must not be billed here) and the campaign's own late honesty corrections to PR1-owned comments.Stacked on #1994.
Verification
Full gate run on
ssh lidgeat this SHA (ff4b0bb4e) in a worktree pinned to it:These are shared-runtime files (
bridge.ts,google.ts,anthropic.ts,command-code.ts), which is why the full isolated suite is the gate rather than a focused subset.Checklist
devlog/_plan/260817_cursor_toolcall_decode/050_*.mdand the integration unit; correctness in a failure path, not a documented feature.)privacy:scangreen; no request bodies logged.)