-
Notifications
You must be signed in to change notification settings - Fork 1.1k
docs(devlog): correct the red-test arithmetic and name two enumeration gaps #2919
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -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. | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Hyphenate “end-to-end.” Here, “end to end” modifies “native path.” Use “end-to-end” for correct technical wording. Proposed wording- native path end to end rather than being changed on inference
+ native path end-to-end rather than being changed on inference📝 Committable suggestion
Suggested change
🧰 Tools🪛 LanguageTool[grammar] ~173-~173: Use a hyphen to join words. (QB_NEW_EN_HYPHEN) 🤖 Prompt for AI AgentsSource: Linters/SAST tools |
||||||
| - `bun x tsc --noEmit` — exit 0. | ||||||
| - Full suite on `ssh lidge`; no local full-suite run was used as a gate. | ||||||
|
|
||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Describe the six rows as tests, not assertions.
The table at Lines 58-65 contains six test cases. The measured result at Line 100 is a separate 16-pass/3-fail aggregate. Calling the six rows “assertions” makes the corrected test evidence unclear. Use “tests in this block” here.
Proposed wording
📝 Committable suggestion
🤖 Prompt for AI Agents