diff --git a/devlog/_plan/260905_apply_patch_envelope_gap/040_wp1b_parser_guidance.md b/devlog/_plan/260905_apply_patch_envelope_gap/040_wp1b_parser_guidance.md new file mode 100644 index 00000000000..5177adef058 --- /dev/null +++ b/devlog/_plan/260905_apply_patch_envelope_gap/040_wp1b_parser_guidance.md @@ -0,0 +1,77 @@ +# 040 — wp1b: leave the parser guidance alone (rejected) + +Follow-on to `030_review_round.md`. Proposed while closing the loop, audited, and +**rejected before implementation**. No code changed. + +## What was proposed + +OpenCodex describes the `apply_patch` grammar in three places. The merged change +updated two; `src/responses/parser.ts:202` still reads "begin exactly with +`*** Begin Patch` (no trailing `***`)". The proposal was to align the third with the +other two, on the theory that a routed model should never see two descriptions of one +grammar. + +## Why that was wrong + +The premise was a category error, and the audit caught it. + +**They are not the same grammar in three places. They are two different tools with two +different repair policies.** + +- The two updated sites describe **nested** `tools.apply_patch(input)` inside code-mode + `exec`. There OpenCodex does not rewrite JavaScript, so a decorated marker really is + rejected. "Exactly, with no further asterisks" is true there. +- `parser.ts:202` describes the **top-level custom `apply_patch` tool**, whose payloads + are repaired: `repairFreeformToolInput` runs `normalizeApplyPatchDelimiters` for + exactly that tool. A decorated envelope on this path is *fixed*, not rejected. + +Confirmed by running the shipped function on one decorated envelope, both ways: + +``` +top-level apply_patch repaired? true -> "*** Begin Patch" +nested exec body repaired? false -> "*** Begin Patch ***" +``` + +The identical input is silently fixed on one path and left broken on the other. That is +the policy split, demonstrated rather than argued. + +Copying the strict wording onto the lenient path would have taught the model a +rejection this path does not perform. Alignment would have made the copy consistent and +the *meaning* wrong — the opposite of the goal. + +The original defect was a copy-hazard: printing `*** Begin Patch ***` as a copyable +literal. This site never did that; it names the decorated form only inside a +parenthetical prohibition. The reason the first pass skipped it still holds. + +## Two further defects in the proposal + +1. **The plan named the wrong test.** `040` claimed + `tests/responses-custom-tool-guidance.test.ts` asserts this description and would go + red first. It does not — it only checks that the text contains `*** Begin Patch`, + true of both wordings. The single red would have been + `tests/responses-parser.test.ts:111`, which pins `begin exactly with`. A plan whose + stated proof does not fire is a plan that cannot be verified. +2. **The replacement string was worse mechanically.** 128 -> 253 characters and two + U+2014 em-dashes in a model-facing schema string, where the sites it was meant to + match use commas. Length is harmless here (the Kiro limiter bounds injected system + instructions and tool descriptions, not parameter descriptions), but it was needless + Unicode added for no behavior change. + +## Dual injection, checked + +A model can receive both strings in one request, on the intersection catalog: a +top-level custom `apply_patch` plus a code-mode freeform `exec`. The nudge goes into the +system prompt while the parser text rides on `parameters.properties.input.description`. + +They do not contradict each other — both say start with `*** Begin Patch` and do not add +stars. The rewrite would not have removed a contradiction; it would only have made two +genuinely different call paths sound like one. + +## Outcome + +**NOOP.** `parser.ts:202` stays as it is. + +If this is ever revisited, the defensible version is a short ASCII mention of +`*** End Patch` that does not import the nested-exec strictness, together with an update +to `tests/responses-parser.test.ts:111`. That is not justified now. + diff --git a/devlog/_plan/260905_apply_patch_envelope_gap/050_delivery_record.md b/devlog/_plan/260905_apply_patch_envelope_gap/050_delivery_record.md new file mode 100644 index 00000000000..2b2c0d6ee07 --- /dev/null +++ b/devlog/_plan/260905_apply_patch_envelope_gap/050_delivery_record.md @@ -0,0 +1,96 @@ +# 050 — Delivery record + +Closes the unit. Everything below is verifiable from public git history. + +## What shipped + +PR #3498, squash commit `16c7f1ee1`, merged to `dev` and proven an ancestor of +`origin/dev`. Three commits on the branch, each responding to a review round. + +| Concern | Outcome | +|---|---| +| MODE A: raw envelope as the `exec` body | Repaired | +| MODE B: decorated envelope inside JavaScript | Refused, recorded | +| Injected guidance printing the forbidden literal | Reworded, effect unproven | +| `parser.ts` guidance (`040`) | Rejected as a category error | + +## End-to-end confirmation of the merged code + +Run against this checkout at the merged head: + +``` +MODE A recognized under code mode: apply_patch +compiled: const result = await tools.apply_patch("*** Begin Patch\n*** +refused under flat-bridge catalog: undefined +MODE B left alone (js body): undefined +``` + +Four behaviors in one run: the envelope is recognized, its decorated delimiters are +normalized on the way into the compiled call, a flat-bridge catalog is refused, and +JavaScript that merely contains an envelope is untouched. + +## MODE B still fails closed, checked against the adversarial shapes + +The reviewer that refused MODE B named the shapes a rewrite would corrupt. Each was run +against the shipped code: none resolves to a helper, and every one comes back +byte-identical. + +``` +SAFE block comment /*** ... ***/ +SAFE string literal +SAFE regex +SAFE helper call arg +SAFE concatenation +ALL MODE B SHAPES FAIL CLOSED AND BYTE-EXACT +``` + +The block-comment case is the one worth remembering: `/*** Begin Patch ***/` is a legal +JavaScript comment, and a lexical rewrite of the marker would leave it unclosed, turning +a text substitution into a control-flow change. + +## Review rounds + +Five reviewers across three rounds, and each round found something real. + +1. **Pre-implementation.** Three `xai/grok-4.6` investigators mapped the seam, the + safety case, and the prompt wording. MODE B came back + UNSAFE-RECOMMEND-PROMPT-FIX-ONLY. +2. **Design audit.** Two adversarial reviewers returned IMPLEMENT-WITH-CHANGES with six + required changes, including the streaming rewind the plan had missed. All applied. +3. **Post-push.** The Codex reviewer, CodeRabbit, and the maintainer independently + found the native SSE rewind; Codex and CodeRabbit both raised the missing code-mode + gate. Both fixed. + +The pattern worth keeping: every defect that mattered was found by someone auditing the +implementation against its own stated contract, not by adding more tests to the happy +path. + +## Verification + +- `bun run typecheck` clean. +- Focused suites only; the repository-wide suite was never run, per instruction. +- CI on exact head `16cfdf33e`: 23 pass, 0 fail, 1 skipping. +- Merged code re-verified in a scratch checkout: 190 pass, 0 fail. +- The native regression was mutation-tested — breaking the gate makes it fail, restoring + it makes it pass — so it is known to fail for the right reason. + +## Accepted residual risk + +A model that *quotes* a complete patch envelope, rather than intending to apply one, now +has it applied. No parse separates quotation from intent. This is the stated price of +reading "never valid JavaScript" as "meant `apply_patch`", and it is accepted +knowingly rather than solved. + +## Shipping claims, re-verified live at close + +Not asserted from memory. Re-checked against GitHub and git while closing the unit: + +``` +PR #3498 MERGED 2026-09-04T18:14:46Z 16c7f1ee12e91eb57e2b438a21ce72d9a46f7c11 +CI exact head 23 pass, 1 skipping, 0 fail +ancestry 16c7f1ee1 IS an ancestor of origin/dev +``` + +The `skipping` row is the Windows shard selector, which resolves to the four `test N/4` +legs that pass; it is not a suppressed failure. +