diff --git a/devlog/_plan/260806_bug_grade_remediation/000_plan.md b/devlog/_plan/260806_bug_grade_remediation/000_plan.md new file mode 100644 index 000000000..a463ecda4 --- /dev/null +++ b/devlog/_plan/260806_bug_grade_remediation/000_plan.md @@ -0,0 +1,129 @@ +# 000 — Plan: bug-labelled backlog remediation + +Unit for the loop that converts the 2026-08-06 bug-grade triage into landed, +evidence-backed remediation. Base `origin/dev` = `ef1317871`, release `v2.10.2` +is published and is an ancestor of `origin/main` (npm `latest` = 2.10.2). + +## Objective + +Every `bug`-labelled open item reaches a **stated disposition**: fixed by us with +a genuine regression test, reviewed with `file:line` and the required change, or +explicitly parked with the named evidence that would unpark it. No item is closed +on suspicion and no PR is merged inside this loop. + +Being precise about the limit, because the amended audit caught the original +wording overselling it: this loop authors code for **one** defect (#1100's joint +contract). The other eleven G1 issues get a disposition, not a fix — several are +upstream, and several are waiting on a reporter capture no amount of local work +can substitute for. + +## Where the triage landed (25 items) + +Twelve issues and thirteen PRs carry the `bug` label. **No item was fake** — every +reported defect reproduces in code. What separates them is whether the *fix* is +sound. + +| Grade | Meaning | Items | +|---|---|---| +| G1 | Real defect, nobody is fixing it yet | issues #1102 #1100 #1024 #1017 #994 #904 #796 #1059 #418 #417 #241 #92 | +| G2 | Fix is real and correct | PRs #1095 #1085 (merge-ready) · #1111 #1056 #1047 (small change) | +| G3 | Bug real, fix dangerous | PRs #1036 #1092 #978 #1068 #557 | +| G4 | Premise or contract missing | PRs #1093 · #997 (borderline: fix right, proof absent) | + +`001_grade_matrix.md` carries the per-item evidence anchors. + +## Constraints + +- Bun-native TypeScript. `bun run typecheck` and the touched test files must be + green before any completion claim. +- Review language is English (repo review guideline), regardless of issue language. +- Every new regression test needs a red ablation: break the mechanism, watch the + test fail, restore, watch it pass. A green suite alone proves nothing. + +## Scope + +**IN** — this unit's docs; `tests/` additions we author; `src/` changes only for +defects we own; GitHub review comments and labels. + +**OUT (escalate, do not do)** — merging any PR; `git push`; closing contributor +PRs; weakening any security or capability gate; implementing the #1102 +loopback-peer exemption (a policy decision the maintainer owns); releases. + +## Work-phase map + +Dependency graph: **wp0 → wp1 is serial; wp2–wp5 are parallel lanes; wp6 +aggregates their settled outcomes.** The catalog contract runs first because it +is the only phase that changes shipped behavior, and it produces the vocabulary +("advertised capability is not proven capability") the review lanes reuse. + +| WP | Doc | Deliverable | +|---|---|---| +| wp0 | `000`, `001` | This roadmap, at diff-level | +| wp1 | `010` | #1100 catalog joint-contract regression test | +| wp2 | `020` | #1092 + #978 capability-gate reversal reviews | +| wp3 | `030` | #1036 + #1068 correctness-hazard reviews | +| wp4 | `040` | #557 + #1093 + #997 dispositions | +| wp5 | `050` | G2 merge-readiness dispositions | +| wp6 | `060` | G1 issue dispositions (the 11 issues wp1 does not fix) | + +wp1 consumes wp0's verified ordering finding, so that edge is serial. wp2–wp5 +have no dependency on each other and may run in any order; they are separate +phases because they fail for different reasons, not because one feeds the next. +wp6 is the aggregation barrier — it cannot close until the PR lanes settle, +because several G1 issues are dispositioned by pointing at a PR reviewed in +wp2/wp3 (#1017 → #1036, #994 → #1068). + +Running wp2/wp3 as one phase would blur why each PR is blocked: wp2 items reverse +a deliberate fail-closed decision, wp3 items delete or bypass an existing +protection. + +## The finding that shapes wp1 + +`normalizeRoutedCatalogEntry` deletes `supports_reasoning_summaries` from every +routed row (`src/codex/catalog/parsing.ts:353`) and `ensureStrictCatalogFields` +then defaults it to `false` (`:266`). Codex gates construction of the entire +Responses `reasoning` object on that flag, so a routed model advertises a full +effort ladder while the wire carries no effort — issue #1100. + +A direct experiment settles what is actually broken: + +``` +plain/ladder-model levels=[low,high,max,ultra] default=high summaries=false +optin/ladder-model levels=[low,high,max,ultra] default=high summaries=true +``` + +The per-model opt-in the `:352` comment says should exist **already exists** +(`modelSupportsReasoningSummaries`, resolved at +`src/codex/catalog/provider-fetch.ts:545`) and survives the delete, because +`applyCatalogModelMetadata` runs at `src/codex/catalog/sync.ts:268`, one line +*after* `normalizeRoutedCatalogEntry`. + +So the defect is narrower and more honest than "we strip the flag": the ordering +works, the escape hatch works, and **nothing asserts the two stay consistent**. +A future reordering of those two calls would silently disable effort propagation +for every routed provider that opted in, and no test would notice. That absent +joint contract is what wp1 buys, and it is squarely ours regardless of how the +upstream Codex gate behaves. + +wp1 deliberately does NOT flip the default to `true`. Advertising OpenAI-only +summary delivery for arbitrary providers is the exact overclaim the `:352` +comment refuses, and #1092/#978 are blocked in wp2/wp3 for the same class of +mistake. Fixing our own bug by committing the neighbours' bug would be incoherent. + +## Accept criteria + +- `c0` unit exists with `000`, `001` and one diff-level decade doc per phase. +- `c1` #1100 joint-contract test green, with recorded red-ablation output. +- `c2` #1092 and #978 reviews posted, each quoting the fail-closed source. +- `c3` #1036 and #1068 reviews posted with a reproducible hazard scenario. +- `c4` #557, #1093, #997 dispositions posted; #557 cites real failing tests. +- `c5` every G2 PR has a readiness verdict; all remain open and unmerged. +- `c6` each of the 11 remaining G1 issues carries one concrete disposition: + fix-owner named, reporter evidence requested, upstream handoff recorded, or + parked with the specific unblocking condition. + +## Terminal outcomes + +`DONE` all six criteria met with fresh evidence. `NEEDS_HUMAN` for any merge, +push, or the #1102 policy call. `BLOCKED` where a contributor or reporter must +act first. diff --git a/devlog/_plan/260806_bug_grade_remediation/001_grade_matrix.md b/devlog/_plan/260806_bug_grade_remediation/001_grade_matrix.md new file mode 100644 index 000000000..04b6fdc08 --- /dev/null +++ b/devlog/_plan/260806_bug_grade_remediation/001_grade_matrix.md @@ -0,0 +1,59 @@ +# 001 — Grade matrix (25 bug-labelled items) + +Research record. No diffs here (LEXICO-SPLIT-01). Verified against +`origin/dev` = `ef1317871`; two independent read-only review lanes produced the +PR rows and the main session re-verified the load-bearing claims by reading the +dev-side source directly. + +## Issues (12) — G1: real, unfixed + +| Issue | What breaks | Evidence anchor | What would move it | +|---|---|---|---| +| #1102 | Direct-spawned `codex app-server` never receives `OPENCODEX_API_AUTH_TOKEN`; 401 before any SSE frame | shim covers `app-server` (`src/codex/shim.ts:42,386,416`) but a direct spawn bypasses it; admission reads only the header (`src/server/auth-cors.ts:256,369`) | a maintainer policy decision on an opt-in loopback-peer exemption | +| #1100 | Routed models advertise an effort ladder; the wire carries no effort | `src/codex/catalog/parsing.ts:353,266`; `requestedEffort` set from parsed input at `src/server/responses/core.ts:1379` | one sanitized ingress body from the reporter | +| #1024 | Text-only models still receive images on one route | Zen path fixed (`a6f3b2fc2`, in `v2.10.2`); reporter's `TR` provider is not a built-in | the reporter's redacted `TR` config | +| #1017 | Cursor emits malformed `apply_patch` payloads | no structured-edit translation on dev (`src/adapters/cursor/protobuf-events.ts:314-348`) | PR #1036, once its hijack defect is fixed | +| #994 | Claude Code 400: `reasoning_content` must be passed back | replay metadata absent for `opencode-zen` (`src/providers/registry.ts:1753-1762`) | reporter's provider/model + wire capture | +| #904 | U+FFFD when writing Korean files | `eeef7a32a` shipped; tests cover astral surrogates, not the reported Hangul case | the original failing capture | +| #796 | Volcengine Ark 400 on tool turns | `d3abf4345` shipped with `tests/volcengine-ark-assistant-content.test.ts:66`; no live Ark validation | a live Ark retest | +| #1059 | ~207 Windows failures; leg is dispatch-only | `.github/workflows/ci.yml:371-400` | a Windows runner burn-down | +| #418 #417 #241 #92 | upstream Codex behavior | tracked upstream; relay transparency covered locally | upstream resolution | + +#1100 and #1102 received maintainer RCA comments on 2026-08-06 +(`5201923105`, `5201927225`). + +## PRs (13) + +### G2 — fix is real and correct + +| PR | Author | Readiness | Remaining blocker | +|---|---|---|---| +| #1095 | baileyh8 | merge-ready | removes forced non-streaming for `deepseek-v4-flash` (`src/providers/registry.ts:1254-1257`); synthesizes a terminal only after a complete item lifecycle; real upstream terminals stay authoritative | +| #1085 | n3wr1ch | merge-ready | replaces the unset `$OPENCODEX_API_KEY` reference that made Pi hide the provider; needs the stated security review | +| #1111 | Simon-Opopeee | small change | Copilot stream normalization is sound; the 64 MiB native-history bound is an unrelated concern and should be split | +| #1056 | WZBbiao | small change | opt-in native-alias workaround for #241, correctly bounded; split the unrelated drain-server timing edit | +| #1047 | baileyh8 | small change | syncs vision replacements into `_rawBody`, which passthrough actually serializes (`src/adapters/openai-responses.ts:1144-1188`) | + +### G3 — bug real, fix dangerous + +| PR | Hazard | Dev-side proof it is deliberate | +|---|---|---| +| #1036 | converts every call named `edit_file`/`multi_edit`, so a genuine client/MCP tool with that name is silently reinterpreted as `apply_patch` | the PR's own tool builder avoids shadowing those names, then the translator ignores that distinction | +| #1092 | injects a combo default effort for targets whose ladder is UNKNOWN | dev fails closed and logs `capability: unknown|unsupported` (`src/combos/request.ts:43-55`) | +| #978 | sends `thinkingLevel` to any model with a configured ladder | dev restricts it to two direct Flash ids (`src/adapters/google.ts:313-326`); a configured ladder is a UI contract, not a wire capability | +| #1068 | drops `OPENCODE_ZEN_TEXT_ONLY_MODELS`, which protects six models (`src/providers/registry.ts:372-379`), and uses obsolete generic DeepSeek constants superseded by per-model ladders (`:380-434`) | +| #557 | skips the preflight entirely on Windows; the PR claims "7000 pass / 0 fail" while the recorded Windows run has two failing update tests | + +### G4 — contract or proof missing + +| PR | Gap | +|---|---| +| #1093 | accepts any client-supplied `x-opencodex-ingress-span` matching a regex; no issuer is identified, and no policy for failures before adapter resolution. Also adds fields to persisted usage and `/api/logs` while claiming no contract change | +| #997 | fixes a real hazard (tests writing into the developer's real `~/.opencodex`, `src/config.ts:545-552`) but adds no test asserting the real home was untouched | + +## Cross-cutting observation + +#1092, #978 and #1068 are the same mistake wearing three costumes: each treats +*advertised* capability as *proven* capability, or removes a guard that encodes +the difference. The reviews in wp2/wp3 should name that shared principle rather +than reading as three unrelated nitpicks. diff --git a/devlog/_plan/260806_bug_grade_remediation/010_wp1_catalog_joint_contract.md b/devlog/_plan/260806_bug_grade_remediation/010_wp1_catalog_joint_contract.md new file mode 100644 index 000000000..dca2df271 --- /dev/null +++ b/devlog/_plan/260806_bug_grade_remediation/010_wp1_catalog_joint_contract.md @@ -0,0 +1,196 @@ +# 010 — wp1: routed-catalog reasoning joint contract (#1100) + +## Defect being pinned + +A routed catalog row can advertise `supported_reasoning_levels` while +`supports_reasoning_summaries` is `false`. Codex gates construction of the whole +Responses `reasoning` object on that flag, so Desktop renders the effort picker +and the wire carries nothing (#1100, upstream `openai/codex#30697`). + +Measured on this tree, no network: + +``` +plain/ladder-model levels=[low,high,max,ultra] summaries=false +optin/ladder-model levels=[low,high,max,ultra] summaries=true +``` + +Two facts follow, and they are the whole design of this phase: + +1. The escape hatch exists and works. `modelSupportsReasoningSummaries` + resolves at `src/codex/catalog/provider-fetch.ts:545` and reaches the entry + through `applyCatalogModelMetadata` (`src/codex/catalog/effort.ts:139-141`). +2. It works **only because of call ordering**, and only on one of two paths. + The template path in `src/codex/catalog/sync.ts` runs `applyReasoningLevels` + (`:266`), then `normalizeRoutedCatalogEntry` (`:267`, which deletes the flag), + then `applyJawcodeCatalogMetadata` (`:268`), then `applyCatalogModelMetadata` + (`:269`, which restores it). Swap the strip past the restore and every + opted-in provider silently loses effort propagation. + + The **no-template fallback** (`:291-310`) never calls + `normalizeRoutedCatalogEntry` at all, so the opt-in survives there + unconditionally. That asymmetry is why the test below must drive the template + path: a fallback-only test cannot observe the strip, and would pass under the + very reordering it claims to guard. + +Nothing in `tests/` asserts that ordering. `tests/codex-catalog.test.ts:2350` and +`:2387` assert `false` and `true` propagate, but neither pins the interaction +with the routed strip — both call `buildCatalogEntries(null, ...)`, i.e. the +fallback — so both keep passing under the broken order. That is precisely the +coverage gap this phase closes. + +## What this phase does NOT do + +It does not default routed rows to `supports_reasoning_summaries: true`. The +comment at `src/codex/catalog/parsing.ts:352` refuses to advertise OpenAI-only +summary delivery for arbitrary providers, and that refusal is correct — it is the +same principle that blocks #1092 and #978. This phase pins the contract that +exists; it does not widen a capability claim. + +## Scope + +IN: `tests/codex-catalog.test.ts` (MODIFY, append three tests). +OUT: any `src/` change; the default flag value; the upstream Codex gate. + +## Change map + +### MODIFY `tests/codex-catalog.test.ts` + +Append after the existing `#538` summary-delivery test (currently ending at +`:2389`), inside the same `describe`. `nativeTemplate()` is the existing helper +at `tests/codex-catalog.test.ts:937`; `buildCatalogEntries` and +`gatherRoutedModels` are already imported at `:5` and `:37`. + +```ts + test("routed strip does not defeat an explicit reasoning-summary opt-in (#1100)", async () => { + const models = await gatherRoutedModels({ + providers: { + ladder: { + adapter: "openai-responses", + baseUrl: "https://ladder.example.test/v1", + authMode: "key", + liveModels: false, + models: ["effort-model"], + modelReasoningEfforts: { "effort-model": ["low", "high", "max"] }, + modelSupportsReasoningSummaries: { "effort-model": true }, + }, + }, + }); + // MUST use a template: buildCatalogEntries(null, ...) takes the fallback + // branch (src/codex/catalog/sync.ts:291-310), which never runs the routed + // strip, so a null-template assertion cannot detect a reordering regression. + const entries = buildCatalogEntries(nativeTemplate(), [], models); + const routed = entries.find(e => e.slug === "ladder/effort-model"); + + // The ladder is advertised... + expect((routed?.supported_reasoning_levels as { effort: string }[]).map(l => l.effort)) + .toEqual(expect.arrayContaining(["low", "high", "max"])); + // ...and the opt-in survives normalizeRoutedCatalogEntry's delete, because + // applyCatalogModelMetadata runs after it (src/codex/catalog/sync.ts:266-269). + // If that order is ever swapped, Codex stops emitting reasoning.effort for + // every opted-in routed provider (#1100). + expect(routed?.supports_reasoning_summaries).toBe(true); + }); + + test("routed rows without an opt-in stay conservative about summaries (#1100)", async () => { + const models = await gatherRoutedModels({ + providers: { + plain: { + adapter: "openai-responses", + baseUrl: "https://plain.example.test/v1", + authMode: "key", + liveModels: false, + models: ["effort-model"], + modelReasoningEfforts: { "effort-model": ["low", "high", "max"] }, + }, + }, + }); + const entries = buildCatalogEntries(nativeTemplate(), [], models); + const routed = entries.find(e => e.slug === "plain/effort-model"); + + // Deliberate: we do not claim OpenAI-only summary delivery for an arbitrary + // provider just because it has an effort ladder. The consequence is #1100, + // and the supported remedy is the per-model opt-in asserted above. + expect(routed?.supports_reasoning_summaries).toBe(false); + }); + + test("the no-template fallback never applies the routed summary strip (#1100)", async () => { + const models = await gatherRoutedModels({ + providers: { + ladder: { + adapter: "openai-responses", + baseUrl: "https://ladder.example.test/v1", + authMode: "key", + liveModels: false, + models: ["effort-model"], + modelReasoningEfforts: { "effort-model": ["low", "high", "max"] }, + modelSupportsReasoningSummaries: { "effort-model": true }, + }, + }, + }); + // Pins the asymmetry itself: the fallback path (sync.ts:291-310) skips + // normalizeRoutedCatalogEntry, so this row is opt-in-true for a different + // reason than the template row above. Documented so a future unification of + // the two paths is a deliberate change rather than a silent one. + const routed = buildCatalogEntries(null, [], models).find(e => e.slug === "ladder/effort-model"); + expect(routed?.supports_reasoning_summaries).toBe(true); + }); +``` + +The second test documents the trade-off rather than asserting it is desirable — +it is the row that makes a future default flip a deliberate, visible decision +instead of an accident. + +## Red ablation (mandatory) + +In `src/codex/catalog/sync.ts`, move the strip after the metadata restore so it +runs last on the template path: + +``` +- normalizeRoutedCatalogEntry(e, model?.parallelToolCalls === true); +- if (model) applyJawcodeCatalogMetadata(e, model.provider, model.id, model.contextCap); +- applyCatalogModelMetadata(e, model); ++ if (model) applyJawcodeCatalogMetadata(e, model.provider, model.id, model.contextCap); ++ applyCatalogModelMetadata(e, model); ++ normalizeRoutedCatalogEntry(e, model?.parallelToolCalls === true); +``` + +Expected: the template opt-in test FAILS (`expected true, received false`); the +conservative test and the fallback test still pass — the fallback test staying +green is itself the proof that the template test is the one carrying the +contract. Restore, and all three pass. Record both outputs verbatim. + +If the opt-in test does NOT go red under this edit, the test is vacuous and must +not be committed. + +### Executed result + +Run on the landed tests, `bun test tests/codex-catalog.test.ts -t "1100"`: + +``` +=== ABLATION APPLIED: strip moved after metadata restore === +error: expect(received).toBe(expected) +Expected: true +Received: false +(fail) routed strip does not defeat an explicit reasoning-summary opt-in (#1100) +(pass) routed rows without an opt-in stay conservative about summaries (#1100) +(pass) the no-template fallback never applies the routed summary strip (#1100) + 2 pass, 1 fail +``` + +Exactly the predicted shape: only the template opt-in test goes red, and the +fallback test stays green in the same run — which is what proves the template +test is the one carrying the contract. After `git checkout -- src/codex/catalog/sync.ts` +the `src/` diff is empty and the file is back to 122 pass / 0 fail. + +## Verification + +- `bun test tests/codex-catalog.test.ts` — 119 baseline + 3 new, 0 fail. +- `bun run typecheck` — clean. +- Ablation output recorded above. + +## Accept criteria + +- All three tests present and passing. +- Ablation shows the TEMPLATE opt-in test genuinely red before restore, with the + fallback test still green in the same run. +- No `src/` file modified in the committed result. diff --git a/devlog/_plan/260806_bug_grade_remediation/020_wp2_capability_gate_reviews.md b/devlog/_plan/260806_bug_grade_remediation/020_wp2_capability_gate_reviews.md new file mode 100644 index 000000000..bb322837b --- /dev/null +++ b/devlog/_plan/260806_bug_grade_remediation/020_wp2_capability_gate_reviews.md @@ -0,0 +1,94 @@ +# 020 — wp2: capability-gate reversal reviews (#1092, #978) + +Both PRs fix a real annoyance by deleting the distinction between *unknown* and +*supported*. The reviews must make that shared principle explicit, because each +author will otherwise read the objection as a nitpick about their one file. + +## Shared principle to state in both + +A configured or advertised effort ladder is a **presentation** contract: it says +what OpenCodex offers a user. It is not evidence that the upstream endpoint +accepts that wire field. Turning presentation into wire behavior is how a 400 +reaches a user who changed nothing. + +## #1092 — combo default effort + +Verified on dev (`src/combos/request.ts:43-55`): when the chosen target does not +advertise the configured default, the effort is omitted and a debug line records +`capability: "unknown" | "unsupported"`. The two states are already distinguished +in the log, which shows the fail-closed behavior is deliberate, not an oversight. + +The PR keeps the UI fix (correct: one unknown member should not empty the picker, +`gui/src/combo-workspace-data.ts:11-36`) but also injects the default when the +ladder is `undefined`, and synthesizes absent catalog members with a 128k +context fallback so unknown targets appear usable. + +Failure mode to state: a combo containing a target with no discovery metadata can +now save `high` and send `reasoning.effort: "high"` upstream with no evidence the +target accepts a reasoning field at all. + +Required change: keep the picker populated, but carry `unknown` as its own state +through to request time — render it (disabled, or labelled "capability unknown") +rather than promoting it to supported, and leave `src/combos/request.ts` omitting +the field until capability is positively established. + +## #978 — Google `thinkingLevel` (verdict CORRECTED at wp2 P-phase) + +The narrow bug is real: dev emits `thinkingConfig` for two direct Flash ids only +(`src/adapters/google.ts:343-348`), while `gemini-3.1-pro-preview` advertises a +ladder at `src/providers/registry.ts:1217` and never receives the field. + +**An earlier grading of this PR as a "capability-gate reversal" was wrong, and +this doc corrects it before anything was posted.** The claim was that the PR +sends the field to any model with a ladder, including arbitrary configured +providers. Reading the actual diff and running both trees refutes the harmful +half of that. + +The PR's gate (`thinkingEligible`) is *narrower* than the summary suggests: it +excludes `cloud-code-assist`, excludes image-capable models so the +`responseModalities` fallback survives, keeps the Vertex freeze unless a ladder +is explicitly configured, and retains the hardcoded Flash slice for unladdered +configs. Its ladder source is `configuredReasoningEfforts` +(`src/reasoning-effort.ts:58-64`), which returns a value only from a registry +preset or an explicit user config — never a default. + +Measured on a custom Google-adapter provider, same probe on both trees: + +| Config | dev | PR #978 | +|---|---|---| +| custom provider, user-configured `modelReasoningEfforts` | `undefined` | `{thinkingLevel:"high"}` | +| custom provider, no ladder | `undefined` | `undefined` | +| provider-wide `reasoningEfforts` | `undefined` | `{thinkingLevel:"high"}` | + +So the widening is real but is **gated on the user having asserted a ladder for +that model**. That is an opt-in capability assertion, which is exactly the shape +wp1 concluded is the legitimate remedy for #1100 — not the unknown-is-supported +reversal that #1092 performs. Blocking it would contradict our own position. + +Disposition: **not blocked.** Post a review that (a) confirms the gate reads +correctly and names the four exclusions, (b) states plainly that a configured +ladder is a user capability assertion and the PR is right to treat it as one, +and (c) asks for one thing only — a docs note that configuring +`modelReasoningEfforts` for a Google-adapter provider now changes wire behavior, +since that consequence is not obvious from the field name. + +`bun test tests/google-hardening.test.ts` on the PR head: 24 pass, 0 fail. + +Also note the `enforce-target` failure is a stale-checkout mismatch (the run +cannot resolve `.github/scripts/pr-quality-state.cjs`), not the Google change; a +rebase clears it. This is the same root cause as #1068's gate crash — see `030` +— so both comments should describe it the same way rather than inventing two +different explanations for one artifact. + +## Deliverable + +Two posted PR comments, English. #1092 is a blocking review naming the dev-side +fail-closed line and the concrete failure mode. #978 is a non-blocking review +confirming the gate and asking for a docs note. No labels changed, no PR closed. + +## Accept criteria + +- Both comment URLs captured. +- The #1092 comment quotes the dev-side source it asks the author to preserve. +- The #978 comment records the corrected verdict with the measured evidence, and + does not ask for a rewrite the diff does not need. diff --git a/devlog/_plan/260806_bug_grade_remediation/030_wp3_correctness_hazard_reviews.md b/devlog/_plan/260806_bug_grade_remediation/030_wp3_correctness_hazard_reviews.md new file mode 100644 index 000000000..3fb888a9a --- /dev/null +++ b/devlog/_plan/260806_bug_grade_remediation/030_wp3_correctness_hazard_reviews.md @@ -0,0 +1,114 @@ +# 030 — wp3: correctness-hazard reviews (#1036, #1068) + +Both PRs remove or bypass a protection while fixing something else. Unlike wp2 +these are not policy disagreements — they are regressions with a nameable victim. + +## #1036 — Cursor structured edit → apply_patch + +The gap is real: the Cursor route advertises freeform `apply_patch` +(`src/adapters/cursor/tool-definitions.ts:135-141`) and dev has no structured-edit +translation (`src/adapters/cursor/protobuf-events.ts:314-348`), which is #1017. + +The hazard is that provenance is inferred from the tool NAME alone. The PR's own +`cursorStructuredEditTools` takes care not to shadow a real client tool called +`edit_file` or `multi_edit` — so the PR already knows the collision is possible — +but the translator then converts every call bearing those names. + +Scenario to state concretely: a user runs an MCP server exposing `edit_file`. +Their call is silently reinterpreted as a Codex `apply_patch`, so the edit either +lands somewhere unintended or fails with a patch error that names nothing the +user recognizes. The existing tests only exercise synthetic calls, so they give +confidence that this cannot happen while it can. + +Required change: tag synthetic tools at injection time and convert only calls +carrying that tag; add a regression test where a client tool named `edit_file` +passes through untranslated. The prior CHANGES_REQUESTED review already raised +synthetic-tool provenance — this is the same finding with a reproduction, so the +comment should reference it rather than restate it as new. + +## #1068 — DeepSeek reasoning replay for opencode-zen + +The replay gap is real: `opencode-zen` lacks `preserveReasoningContentModels` +while comparable OpenCode routes have it (`src/providers/registry.ts:1753-1762`), +and that is #994's shape. + +Two regressions in the diff: + +1. It replaces `noVisionModels` with a DeepSeek-only list, dropping + `OPENCODE_ZEN_TEXT_ONLY_MODELS` (`src/providers/registry.ts:372-379`), which + currently protects six models — `big-pickle`, `nemotron-3-ultra-free`, + `ling-3.0-flash-free`, `north-mini-code-free`, `laguna-s-2.1-free`, + `deepseek-v4-flash-free`. Five of those would start receiving images again. + That list was derived from a probe recorded in + `devlog/_plan/260805_bug_fix_stack/002_zen_modality_probe.md`, so it is + evidence, not a guess. Deleting it re-opens #1043. +2. It uses generic DeepSeek effort constants that `v2.10.2` replaced with + per-model ladders (`:380-434`, shipped for #1057), so a rebase either conflicts + or advertises the wrong tier. + +Required change: rebase onto dev, keep the full text-only list and add the +DeepSeek replay metadata alongside it, and use `deepseekThinkingEffortsFor` / +`deepseekReasoningMapFor`. + +### Measured at wp3 (set arithmetic on the real constants) + +``` +dev list: big-pickle, nemotron-3-ultra-free, ling-3.0-flash-free, + north-mini-code-free, laguna-s-2.1-free, deepseek-v4-flash-free +PR list: deepseek-v4-pro, deepseek-v4-flash, deepseek-v4-flash-free +lost: big-pickle, nemotron-3-ultra-free, ling-3.0-flash-free, + north-mini-code-free, laguna-s-2.1-free +``` + +Five models, not "some". `deepseek-v4-flash-free` sits in both sets, which is +almost certainly why the replacement looked safe to the author — worth saying in +the review so the correction reads as a shared observation rather than a scolding. +The fix is a union, not a rewrite. + +**Correction, from the plan audit.** An earlier draft of this doc told the review +to call #1068's failing `enforce-target` check substantive. That is wrong and must +not be written to the author. The current run fails inside the gate script itself +with `TypeError: parseGateState is not a function` — a repository-side gate +defect, not a verdict on this PR's code +(run 31081544753, job 92551264534). Contrast #978, whose failure IS a stale +checkout missing `.github/scripts/pr-quality-state.cjs`. + +The mechanism is known and already documented in-tree: the workflow's checkout +pins the BASE sha while the event loads the workflow YAML from the HEAD branch, +so a stale head's YAML calls into base scripts that do not export what it expects +— `parseGateState` is required at `.github/workflows/enforce-pr-target.yml:63` +and does exist and is exported on dev +(`.github/scripts/pr-quality-state.cjs:84,380`). The repository even carries a +regression test naming this exact crash string as the reason review events are +excluded (`.github/scripts/enforce-pr-target.test.cjs:51-60`). + +That makes both #1068 and #978 the same root cause with two symptoms: a stale +head against moved gate scripts. It is a staleness artifact, not a verdict on +either diff. + +So the review states the observable gate error, tells the author not to chase it, +and asks for a rebase onto current dev — after which the real question (the Zen +regression above) can be re-evaluated on a clean run. Blaming a contributor for a +gate artifact would be both wrong and corrosive. + +## Deliverable + +Two posted PR comments, English, each with the concrete victim of the regression +and the minimal change that keeps the fix. + +## Accept criteria + +- Both comment URLs captured. +- The #1068 comment names the protected models explicitly. +- The #1036 comment references the existing review rather than duplicating it. + +## Executed + +- #1036 → comment 5202680370 (3597 chars). Reproduction given as an MCP server + exposing `edit_file`, with both outcomes spelled out; asks for a request-scoped + synthetic flag plus the pass-through regression test. +- #1068 → comment 5202684051 (3107 chars). Carries the set diff above and a + one-line union fix; explicitly retracts the earlier "substantive CI failure" + framing to the author. +- `gh pr view` confirms both OPEN after posting. Neither merged, closed, or + relabelled. diff --git a/devlog/_plan/260806_bug_grade_remediation/040_wp4_own_pr_and_provenance.md b/devlog/_plan/260806_bug_grade_remediation/040_wp4_own_pr_and_provenance.md new file mode 100644 index 000000000..fab7df5fe --- /dev/null +++ b/devlog/_plan/260806_bug_grade_remediation/040_wp4_own_pr_and_provenance.md @@ -0,0 +1,94 @@ +# 040 — wp4: own-PR gap and missing contracts (#557, #1093, #997) + +These three share a shape: the change is defensible, the *claim* around it is not +yet supported. #557 is ours, so it gets the least charitable reading of the three. + +## #557 — npm cache recovery preflight (maintainer's own) + +The underlying failure is real: both update paths stop the proxy before running +npm (`src/update/index.ts:231-273`, `bin/ocx.mjs:239-250`), so a cache-permission +failure leaves the user stopped and un-upgraded. The Unix preflight is substantive. + +Two problems to record honestly: + +1. The preflight is skipped on Windows, and a test asserts that skip. Windows is + where npm cache ownership problems are most common, so the PR does not cover + its own motivating case on the platform that needs it most. +2. The description claims the full suite passes. It does not. + +### Measured at wp4 (run 30748759350, job 91498912876) + +``` + 2 fail +Ran 7008 tests across 482 files. [853.00s] + +(fail) GUI update execution decisions > persists installer-derived job fields + without raw cache paths or uid values +(fail) GUI update execution decisions > sanitizer redacts space-containing + profile paths (Wibias reproduction) + +175 | spawnStart: () => { throw new Error("must not spawn"); }, +error: must not spawn + at spawnStart (tests\update-job.test.ts:175:37) +``` + +Both failures share one cause: the preflight's restart-flow change reaches +`spawnStart` on a path whose test asserts nothing may restart the proxy. Whether +the test's expectation or the new flow is correct is the open question — it is +not re-runnable noise. + +Correction to an earlier draft of this doc: **all three legs are red** +(`ubuntu fail`, `windows fail`, `macos fail`), not "ubuntu segfault, macOS +cancelled". The comment must not attribute the other two legs to an unrelated +cause without triaging them, so it asks for separate triage instead. + +Because this touches dependency installation and update recovery, `MAINTAINERS.md` +requires a second-maintainer security review. Being the author does not exempt it — +state that in the comment so the record is unambiguous. + +Disposition: comment on our own PR correcting the verification claim and naming +the Windows gap. No merge, no push. + +## #1093 — ingress span provenance + +Neither field exists on dev (`src/server/request-log.ts:219-256`; +`src/server/responses/core.ts:1631-1641`), so this is additive. The problem is +that the header is accepted from any client if it matches a regex, while the +description calls the values "guard-issued". Nothing issues them. + +Three things the author must supply before review can proceed: + +1. Who issues `x-opencodex-ingress-span`, and why a client-supplied value may be + trusted into persisted logs. +2. The policy when a request fails before adapter resolution — auth rejection, + routing failure — since the attempt record is created after resolution. +3. A corrected description: the PR says no public contract changes while adding + fields to persisted usage records and `/api/logs`. + +Also still 138 commits behind after today's retarget; rebase before review. + +## #997 — usage-log fixture isolation + +The hazard is real and worth fixing: `addRequestLog` persists usage +(`src/server/request-log.ts:302-352`) and `getConfigDir()` resolves to the real +`~/.opencodex` when `OPENCODEX_HOME` is unset (`src/config.ts:545-552`), so a test +run can write into a developer's actual usage history. + +The per-test temp home and cleanup are complete for that file. What is missing is +a test that proves the promise: nothing asserts the real home was untouched. Ask +for one assertion — run with `OPENCODEX_HOME` pointed at a temp dir from an +unrelated cwd and assert no write reached the default path. + +This is the smallest ask of the three; frame it as "one assertion away", not as a +rejection. + +## Deliverable + +Three posted comments, English. The #557 one is self-directed and must not be +softer than the other two. + +## Accept criteria + +- Three comment URLs captured. +- The #557 comment cites the actual failing Windows tests, not the PR's claim. +- The #1093 comment lists the three prerequisites as a checklist the author can act on. diff --git a/devlog/_plan/260806_bug_grade_remediation/050_wp5_grade2_dispositions.md b/devlog/_plan/260806_bug_grade_remediation/050_wp5_grade2_dispositions.md new file mode 100644 index 000000000..38bcefb5e --- /dev/null +++ b/devlog/_plan/260806_bug_grade_remediation/050_wp5_grade2_dispositions.md @@ -0,0 +1,50 @@ +# 050 — wp5: Grade-2 merge-readiness dispositions + +Five PRs whose fix is real and correct. This phase records a readiness verdict +and the single remaining blocker for each, and posts it so the author knows +exactly what stands between their branch and review. + +**No merge happens in this phase.** Merging is a maintainer decision and is +explicitly out of scope for this loop; wp5 ends by escalating the merge call. + +## Verdicts (corrected at wp5 P against live state) + +The roadmap called #1095 and #1085 "merge-ready". Live `gh` state contradicts +that for #1095, and the correction matters because "merge-ready" is the phrase +that would have prompted a merge. + +| PR | Draft | Behind dev | Verdict | Remaining blocker | +|---|---|---|---|---| +| #1095 | **yes** | 341 | code-ready, NOT merge-ready | the fix is sound — removes forced non-streaming for `deepseek-v4-flash`, synthesizes a terminal only after a structurally complete item lifecycle, leaves real upstream terminals authoritative — but it is still a draft and 341 commits behind | +| #1085 | no | 85 | closest to mergeable | ready for review, but 85 behind and the credential-adjacent security review is still the gate | +| #1111 | yes | 0 | split required | Copilot normalization is sound and its test drives `handleResponses` end to end; the 64 MiB native-history bound is unrelated and belongs in its own PR. Author states the full suite was interrupted. On the latest dev commit, which is unusual and good | +| #1056 | yes | 44 | split required | opt-in native-alias workaround for #241, bounded to supported native ids with an honest display label; remove the unrelated `tests/native-profile-drain-server.test.ts` timing edit | +| #1047 | yes | 341 | rebase + full suite | syncs vision replacements into `_rawBody`, which passthrough actually serializes; only lightweight checks have run on this draft head | + +Four of the five are drafts. The repository's readiness gate keeps a draft in +draft until its author ticks the four-box checklist, and the gate verifies two of +those claims itself — green `ci`, and the branch at most 10 commits behind dev. +At 341, 85 and 44 behind, three of these would fail that check today. + +So the honest disposition for every one of them is "here is your single +remaining blocker", not "waiting on a maintainer". That leaves exactly zero PRs +where the maintainer is the bottleneck — a more useful thing to report than a +merge queue that does not exist. + +## Note on #1056 and #999 + +They are complementary, not competing: #1056 is the runtime workaround, #999 +documents the Desktop allowlist limitation. Neither supersedes the other, and +#241 stays open because the root cause is upstream. + +## Deliverable + +Five posted comments, each naming the one blocker that PR owns. No merge-decision +escalation, because the corrected state shows none is pending. + +## Accept criteria + +- Five comment URLs captured. +- `gh pr view` confirms all five remain open and unmerged after this phase. +- The draft/behind numbers match live `gh` output at posting time, not the + roadmap's assumption. diff --git a/devlog/_plan/260806_bug_grade_remediation/060_wp6_g1_issue_dispositions.md b/devlog/_plan/260806_bug_grade_remediation/060_wp6_g1_issue_dispositions.md new file mode 100644 index 000000000..c457e3a9f --- /dev/null +++ b/devlog/_plan/260806_bug_grade_remediation/060_wp6_g1_issue_dispositions.md @@ -0,0 +1,68 @@ +# 060 — wp6: G1 issue dispositions (the 11 wp1 does not fix) + +Added after the plan audit found the objective promising an outcome for every +bug-labelled item while the phase map scheduled work for only one issue. This +phase closes that gap honestly: a disposition is not a fix, and saying so is the +point. + +wp6 is the aggregation barrier — several dispositions point at a PR reviewed in +wp2/wp3, so it runs after those lanes settle. + +## Disposition vocabulary + +`fix-owned` we will author the fix · `awaiting-reporter` blocked on a specific +capture · `awaiting-contributor` blocked on a named PR · `upstream` tracked +outside this repo · `parked` real but nobody is on it, with the unblocking +condition stated. + +## Per-issue + +| Issue | Disposition | Action in this phase | +|---|---|---| +| #1100 | fix-owned (partial) | wp1's joint-contract test landed. Comment already posted with the RCA. Add: the per-model `modelSupportsReasoningSummaries: true` opt-in is a **usable workaround today** for a configured provider — that is new, actionable information the reporter does not have | +| #1102 | needs-human | RCA posted. The loopback-peer exemption is a policy call; escalate to the maintainer rather than deciding it here | +| #1024 | awaiting-reporter | the Zen half shipped in v2.10.2; only the reporter's `TR` provider config remains. Comment already asks for it — verify it is still the only blocker, do not re-ask | +| #1017 | awaiting-contributor | blocked on #1036, whose hijack defect wp3 documents. Cross-link so the issue reader can see why it is not simply open-and-ignored | +| #994 | awaiting-contributor + awaiting-reporter | #1068 covers the Zen replay half; the Claude-path capture is still missing. Cross-link #1068 | +| #904 | awaiting-reporter | fix shipped, tests cover astral surrogates but not the reported Hangul case. Needs the original failing capture | +| #796 | awaiting-reporter | fix + regression test shipped; needs a live Ark retest | +| #1059 | parked (ours) | Windows leg dispatch-only. Unblocking condition: a shard-by-shard burn-down. Confirm the existing status comment still reflects reality | +| #241 #417 #92 #418 | upstream | already labelled `upstream-tracking`. Verify each still carries a pointer to its upstream item; add one where missing | + +## Rule for this phase + +**Do not re-comment where a current comment already says the same thing.** Several +of these were dispositioned earlier today. Re-stating it adds noise to the +reporter's inbox and buys nothing. Only comment where this phase adds something +new — the #1100 workaround, and the #1017/#994 cross-links. + +## Deliverable + +A disposition table recorded here with the live state of each issue, plus the +small number of comments that carry genuinely new information. + +## Accept criteria + +- Every one of the 11 issues has a disposition and a stated unblocking condition. +- Each comment posted in this phase says something not already on the issue. +- No issue is closed in this phase. + +## Executed (live state verified per issue) + +Four comments posted, seven deliberately not. The restraint is the finding: most +of these already carry an accurate current comment, and re-stating it would have +added noise to a reporter's inbox while buying nothing. + +| Issue | Action | Why | +|---|---|---| +| #1100 | comment 5202766906 | NEW: the `modelSupportsReasoningSummaries: true` workaround, verified against the reporter's own provider shape (`GLM/glm-5.2 summaries=true` vs `GLMplain/glm-5.2 summaries=false`, generated locally, no network). Also states what the user asserts by setting it, and what we deliberately did not change | +| #1024 | comment 5202780631 | NEW: per-model answer to a follow-up that had gone unanswered since 08-04. Splits the four probes into fixed / deliberately-excluded / still-blocked, and surfaces a genuine disagreement — `mimo-v2.5-free` accepts images per the 08-05 probe, so "200 but blind" is a different defect from "rejects images" | +| #1017 | comment 5202769373 | NEW: cross-link to #1036 plus the specific hazard blocking it, so the issue does not read as abandoned | +| #994 | comment 5202771720 | NEW: cross-link to #1068, plus the one question that separates this from a lookalike — the report says "OpenCode models" without a model id, and the same 400 text has other causes | +| #1102 | none | RCA comment posted 08-06; the loopback-peer exemption is a maintainer policy call. Nothing new to add | +| #904 #796 #418 | none | each already carries an accurate maintainer comment naming the exact missing capture; re-asking is noise | +| #1059 | none | status comment current as of 08-06 | +| #241 #417 #92 | none | upstream trackers, each already labelled and cross-referenced | + +No issue closed. `#1024` in particular stays open on its `TR` half even though +the Zen half shipped — a partly-fixed issue is not a closed one. diff --git a/devlog/_plan/260806_bug_stack_absorb/000_plan.md b/devlog/_plan/260806_bug_stack_absorb/000_plan.md new file mode 100644 index 000000000..1c7de5c76 --- /dev/null +++ b/devlog/_plan/260806_bug_stack_absorb/000_plan.md @@ -0,0 +1,145 @@ +# 000 — Plan: stack-and-absorb the contributor bug fixes + +Second loop of the 2026-08-06 bug campaign. The first loop graded 25 items and +posted reviews; this one converts the reviews into landed work. + +Base `origin/dev` = `efdfd1cf5` (moved during loop 1 — #715 landed). Layer 0 is +already pushed and open as **#1119**. + +## Why we author replacements rather than wait — AND WHEN WE MUST NOT + +Four contributor PRs shared a shape: the defect is real, the diagnosis is right, +and the patch is one specific decision away from correct. The original intent was +to correct that decision ourselves, land it with attribution, and close theirs as +absorbed. **Attribution is not a courtesy here, it is accuracy**: in each case the +hard part — finding the defect — was theirs. + +The plan audit caught this campaign about to break its own rule, and the +correction is the most important thing in this document. + +**Measured 2026-08-06 by comparing LAST COMMIT time against OUR REVIEW time** — +the second audit round caught the first version of this table using +`updatedAt`, which moves when *we* comment and therefore proved nothing: + +| PR | Author | Last commit | Our review | Acted after? | +|---|---|---|---|---| +| #1092 | eachann1024 | 09:38:19Z | 09:09:51Z | **YES** — fail-closed guard restored with a comment keeping `unknown` distinct | +| #1068 | justjxke | 08:52:23Z | 09:13:15Z | **no** — commits predate the review by 21 minutes | +| #1036 | ZachDreamZ | 08-05 07:18Z | 09:12:51Z | no | +| #997 | Yuxin-Qiao | 08-06 02:51Z | 09:16:02Z | no | + +One author turned our review around in under half an hour. **Absorbing that would +be taking credit for work the author did in response to our own request** — the +opposite of why this campaign exists, and no amount of attribution text repairs +it. + +The rule: **we absorb only where the author has not acted after our review, AND +only after a stated response window has passed.** "Has not replied within an +hour" is not abandonment — people sleep, and #997's author was active at 02:51Z. +Absorption is for a correct diagnosis that would otherwise rot in a queue, not a +faster path to our own commit. + +**Response window: 72 hours from our review.** Before authoring or closing +anything we re-check the exact head. Any author who acts moves to the re-review +path. + +## Revised plan + +| WP | Layer | Disposition | +|---|---|---| +| s0 | devlog + #1100 joint contract | **done** — `codex/260806-bug-grade-remediation`, PR #1119 open | +| s1 | #1068 | **re-review, no absorb.** Its commits predate our review, and its guard/test state is internally inconsistent — that is a review finding, not something to take over | +| s2 | #1092 | **re-review, no absorb.** Author acted on our review within 30 minutes | +| s3 | #1036 Cursor edit provenance | **deferred absorb** — no response yet, but inside the 72h window. Re-review now; author the layer only if the window closes | +| s4 | #997 usage-log isolation | **deferred absorb** — same | + +So this loop authors **no absorbed layer today.** That is the honest outcome of +the two audit rounds: the campaign's premise — four stalled PRs — did not survive +contact with the live data. What remains is real and useful (re-reviews that carry +new findings, plus the PR-less issues), and pretending otherwise to justify the +original plan would be the failure mode this discipline exists to prevent. + +**These are independent PRs, not a stack.** The audit was right that calling them +dependency-ordered was false: s3 lives entirely in `src/adapters/cursor/`, s4 is +one test file, and neither reads the other's output. PHASE-SPLIT-01 asks for +dependency order, and inventing a chain where none exists would make every layer +wait on the one below it for no reason. Both branch from `origin/dev` directly. + +The `--update-refs` cascade machinery from the 260805 campaign applies to genuine +stacks; it is not needed here and pretending otherwise would add rebase risk. + +Before absorbing s3 or s4 we re-check the author's head one final time. An author +who acts between now and then moves to the re-review path, same as #1092 did. + +## Layers deliberately NOT absorbed + +- **#978** — the wp2 review corrected its grading from "dangerous gate reversal" + to an opt-in capability assertion and asked only for a docs note. It still needs + that author-side docs change; it is excluded because it is close to correct and + its author owns the remaining work, not because nothing is left to do. +- **#1095, #1085, #1111, #1056, #1047** — Grade 2. Each is correct and needs a + rebase or a split by its own author. Absorbing a correct PR because its author + has not rebased yet would be rude and would erase their authorship for no + technical gain. +- **#557** — ours, and blocked on a real design question (Windows coverage), not + on a missing rebase. +- **#1093** — blocked on a contract nobody has stated. There is nothing to absorb + until someone says what the data means. + +## Absorb protocol (applies to every layer) + +1. Confirm the response window has closed and the author's head still lacks the + fix. Then author the corrected change on a `codex/` branch **taken directly + from `origin/dev`** — s3 and s4 are independent, so stacking and the + `--update-refs` cascade do not apply and would only add rebase risk. Reserve + that machinery for a genuine dependency chain. +2. Regression test with a **red ablation** — break the mechanism, watch the test + fail, restore, watch it pass. Record both outputs in the layer's decade doc. +3. `bun run typecheck` + touched tests green before push. +4. Open our PR with the repo template, `Closes #` where one exists, and a + Credit line naming the original author and PR. +5. Close the contributor PR with a comment that states: what we kept from their + work, what we changed and why, and where it now lives. Never "superseded" with + no explanation. + +## Scope + +**IN** — `codex/` branches here; `src/` and `tests/` we author; this unit's docs; +our PRs; comments and closes on the four target PRs. + +**OUT (escalate)** — merging anything into `dev`; force-pushing another author's +branch; weakening any security or capability gate; the #1102 loopback-peer +exemption; releases. + +## Accept criteria + +- `k0` loop-1 devlog pushed and #1119 open with a diff-level decade doc per layer. +- `k1` #1068 re-reviewed against its current head, reporting the **failing** + guard/test contradiction (Pro and Flash are not in the registry list the router + merges) and the unmeasured modality question. No credit for a review response + that did not happen; no close. +- `k2` #1092 re-reviewed against its current head, crediting the author for + restoring the fail-closed guard within half an hour of our review, and asking + only for the unrelated `imageInput` scope to be split. No close. +- `k3`, `k4` #1036 and #997 confirmed to still lack the requested change, and + recorded as **deferred to the post-window unit**. Nothing authored, nothing + closed inside the 72h window. +- `k5` no authored layer this cycle, so no PR heads to verify beyond #1119. +- `k6` #1102 and #1059 reach an authored fix or an evidence-backed disposition. + +## Terminal outcomes + +The honest terminal for this unit is **not** `DONE`. The campaign was scoped to +absorb four stalled PRs; live data showed one author responding to our review +within 30 minutes and the other three inside a reasonable response window. The +absorb half is therefore `BLOCKED` on the contributors — by design, not by +obstruction — and the re-review half completes. + +- Re-reviews and issue dispositions → `DONE`. +- s3/s4 absorption → `BLOCKED` (72h response window), carried to a successor unit + `devlog/_plan/2608xx_bug_absorb_window/` created only if the window closes with + no author action. +- #1102 policy decision → `NEEDS_HUMAN`. + +Recording it this way rather than shrinking the criteria to fit what we finished +is the point: the plan was wrong about the world, and the ledger should say so. diff --git a/devlog/_plan/260806_bug_stack_absorb/010_s1_zen_deepseek_replay.md b/devlog/_plan/260806_bug_stack_absorb/010_s1_zen_deepseek_replay.md new file mode 100644 index 000000000..fc34f8cce --- /dev/null +++ b/devlog/_plan/260806_bug_stack_absorb/010_s1_zen_deepseek_replay.md @@ -0,0 +1,198 @@ +# 010 — s1: Zen DeepSeek replay (#1068) — no absorb; the head has a failing test + +> **Status: not authored — re-review instead.** Two corrections happened here and +> both matter. +> +> A first version of this banner said the author fixed it in response to our +> review. That was wrong: their commits landed at **08:52:23Z**, our review at +> **09:13:15Z**. The mistake came from reading `updatedAt`, which moves when *we* +> comment — so it can never show author activity. Always compare LAST COMMIT time +> against REVIEW time. +> +> The head does keep `noVisionModels: OPENCODE_ZEN_TEXT_ONLY_MODELS`, so the +> destructive replacement our review flagged is not in the current diff. But that +> is not agreement with the review; it is the state the branch was already in, and +> it now sits inconsistently against the tests the same commit added. That +> inconsistency is the finding below. +> +> We do not absorb this. It is inside the response window and the remaining work +> is a review conversation, not a takeover. + +## What the contributor got right + +justjxke diagnosed #994 correctly. `opencode-zen` and `opencode-free` reach the +SAME gateway (`https://opencode.ai/zen/v1`), and DeepSeek thinking mode requires +the assistant's original `reasoning_content` to be replayed on tool-call +continuations or the gateway answers 400. `opencode-free` carries that metadata; +`opencode-zen` does not. That asymmetry is the bug, and finding it was the work. + +## The one decision to correct + +The PR sets `noVisionModels` to the DeepSeek list, **replacing** +`OPENCODE_ZEN_TEXT_ONLY_MODELS` rather than extending it. Set arithmetic on the +real constants: + +``` +dev: big-pickle, nemotron-3-ultra-free, ling-3.0-flash-free, + north-mini-code-free, laguna-s-2.1-free, deepseek-v4-flash-free +PR: deepseek-v4-pro, deepseek-v4-flash, deepseek-v4-flash-free +lost: big-pickle, nemotron-3-ultra-free, ling-3.0-flash-free, + north-mini-code-free, laguna-s-2.1-free +``` + +Five models stop being marked text-only, so the proxy resumes forwarding image +parts and the gateway 400s the request — reopening #1043. `deepseek-v4-flash-free` +sits in both sets, which is almost certainly why the replacement looked safe. + +Second correction: the branch predates the per-model DeepSeek ladders that +shipped for #1057 in v2.10.2. `dev` now has `deepseekThinkingEffortsFor` and +`deepseekReasoningMapFor` (`src/providers/registry.ts:431,433`) because Flash and +Pro honor different tiers; a single shared array advertises the wrong ladder. + +## Reference implementation already in the tree + +`opencode-free` (`src/providers/registry.ts:1765-1781`) is exactly the shape we +need, on the same gateway: + +```ts +modelReasoningEfforts: Object.fromEntries(OPENCODE_FREE_DEEPSEEK_MODELS.map(id => [id, deepseekThinkingEffortsFor(id)])), +modelReasoningEffortMap: Object.fromEntries(OPENCODE_FREE_DEEPSEEK_MODELS.map(id => [id, deepseekReasoningMapFor(id)])), +preserveReasoningContentModels: OPENCODE_FREE_DEEPSEEK_MODELS, +``` + +So the corrected layer is: copy that pattern to `opencode-zen`, cover both the +`opencode-zen`-visible DeepSeek ids, and UNION the vision list instead of +replacing it. + +## Change map + +### MODIFY `src/providers/registry.ts` + +The `opencode-zen` entry (currently at `:1753-1762`) becomes: + +```ts + { + id: "opencode-zen", label: "opencode zen", baseUrl: "https://opencode.ai/zen/v1", + adapter: "openai-chat", authKind: "key", dashboardUrl: "https://opencode.ai/auth", + // Same opencode.ai/zen/v1 gateway as `opencode-free`: DeepSeek thinking mode requires the + // assistant's original reasoning_content to be replayed on tool-call continuations, or the + // gateway answers 400 (#994). Mirror the free tier's DeepSeek metadata; the ladders are + // per-model because Flash and Pro honor different tiers (#1057). + modelReasoningEfforts: Object.fromEntries( + ZEN_DEEPSEEK_MODELS.map(id => [id, deepseekThinkingEffortsFor(id)]), + ), + modelReasoningEffortMap: Object.fromEntries( + ZEN_DEEPSEEK_MODELS.map(id => [id, deepseekReasoningMapFor(id)]), + ), + preserveReasoningContentModels: ZEN_DEEPSEEK_MODELS, + // #1043: without this the proxy forwards image parts to text-only Zen models and + // the upstream rejects the whole request with a 400. The DeepSeek entries are + // UNIONED in, not substituted — dropping the probed list would re-open #1043 for + // five other models (see devlog/_plan/260806_bug_stack_absorb/010). + noVisionModels: OPENCODE_ZEN_TEXT_ONLY_MODELS, + }, +``` + +with a new constant beside the existing DeepSeek lists (near `:350`): + +```ts +/** DeepSeek thinking models reachable through the Zen gateway (keyed + free tiers). */ +const ZEN_DEEPSEEK_MODELS = [...DEEPSEEK_THINKING_MODELS, ...OPENCODE_FREE_DEEPSEEK_MODELS]; +``` + +`deepseek-v4-flash-free` is already in `OPENCODE_ZEN_TEXT_ONLY_MODELS`, so the +vision list needs no change at all — that is the point. The union is achieved by +*not touching* `noVisionModels`. + +### NEW `tests/opencode-zen-deepseek-replay.test.ts` + +Three assertions: + +1. every id in `ZEN_DEEPSEEK_MODELS` appears in the `opencode-zen` + `preserveReasoningContentModels`; +2. the per-model ladder for `deepseek-v4-flash` differs from `deepseek-v4-pro` + (proves the helpers are used rather than a shared array); +3. **the regression guard** — all six `OPENCODE_ZEN_TEXT_ONLY_MODELS` are still in + `opencode-zen.noVisionModels`, named individually so a future replacement + fails loudly with the missing model in the message. + +## Red ablation + +Replace `noVisionModels: OPENCODE_ZEN_TEXT_ONLY_MODELS` with +`noVisionModels: ZEN_DEEPSEEK_MODELS` — i.e. reproduce the contributor's exact +edit. Assertion 3 must fail naming `big-pickle`; assertions 1 and 2 stay green. +Restore, all three pass. + +If assertion 3 does not go red under that edit it is vacuous and must not ship. + +## Verification + +- `bun test tests/opencode-zen-deepseek-replay.test.ts` +- `bun test tests/provider-registry-parity.test.ts` (existing negative assertion + for `mimo-v2.5-free` must stay green — the union must not add it) +- `bun run typecheck` + +## Accept criteria + +*(superseded — see below)* + +## Revised deliverable: re-review, not absorb + +1. Verify on the CURRENT head (`ac63b73a5`) that the vision list is restored and + the per-model DeepSeek helpers are used rather than the obsolete generic + constants. +2. **The head's new test fails by construction — this is the load-bearing finding.** + The new test asserts `deepseek-v4-pro`, `deepseek-v4-flash` and + `deepseek-v4-flash-free` are all in `opencode-zen.noVisionModels`, while the + entry sets `noVisionModels: OPENCODE_ZEN_TEXT_ONLY_MODELS` — a list containing + only the `-free` one of those three + (`src/providers/registry.ts:350-379,1753-1762`). Routing merges the registry + list (`src/router.ts:254-282`), so the assertion cannot hold for Pro and Flash. + + Reproduced locally with the test's own `routeModel` configuration: + + ``` + deepseek-v4-flash-free: inNoVision=true + deepseek-v4-flash: inNoVision=false + deepseek-v4-pro: inNoVision=false + ``` + + So two of the three parameterized cases fail. It is latent rather than + reported because no GitHub check currently runs that suite on this PR — which + is exactly why the comment should name it rather than wait for CI to. +3. **The underlying question is unmeasured.** Whether Pro and Flash accept images + through Zen was never probed; the text-only list is a dated exception list from + a single 2026-08-05 probe, and Zen supplies no modality metadata. So the + comment asks for the probe or an explicit narrowing of the assertion — the + author is better placed to run it against their own Zen access than we are. + +No close. The PR stays theirs. + +## Executed — and the PR merged mid-phase + +Posted comment `5203704926` at 10:53Z naming the failing test with the reproduced +`routeModel` output. **#1068 had merged at 10:45:57Z** (by Wibias), eight minutes +earlier, and the merged code is not what I reviewed: + +```ts +noVisionModels: [...OPENCODE_ZEN_TEXT_ONLY_MODELS, ...DEEPSEEK_THINKING_MODELS], +``` + +That is the union the review asked for. Running the merged suite against +`origin/dev` gives **9 pass / 0 fail**, including the three-id case and a new +"Zen text-only free models (measured #1043) stay in noVisionModels" test pinning +the original list. Both of my objections were resolved before merge. + +Posted correction `5203721089` saying so plainly. The failure mode is worth +naming because it is cheap to avoid: I fetched a head, wrote a finding against +it, and posted after the PR had moved. **Re-fetch immediately before commenting** +— the same class of error as using `updatedAt` earlier in this unit. + +Residual carried forward, not a defect: whether `deepseek-v4-pro` and +`deepseek-v4-flash` genuinely reject images through Zen is still unmeasured. They +are now guarded on the safe assumption, which is the right default — a stripped +image degrades to a caption, an unguarded one 400s the whole request. + +#994 updated with the dev-only landing (merge `7d0c02dbe`, ancestor of +`origin/dev`) and deliberately left OPEN pending the reporter's provider +confirmation and a release. diff --git a/devlog/_plan/260806_bug_stack_absorb/020_s2_combo_picker_unknown.md b/devlog/_plan/260806_bug_stack_absorb/020_s2_combo_picker_unknown.md new file mode 100644 index 000000000..932abd7ad --- /dev/null +++ b/devlog/_plan/260806_bug_stack_absorb/020_s2_combo_picker_unknown.md @@ -0,0 +1,193 @@ +# 020 — s2: combo picker (#1092) — ABSORB WITHDRAWN, author fixed it + +> **Status: this layer is not being authored.** eachann1024 pushed at +> 2026-08-06T09:48Z, minutes after our review, and restored the fail-closed guard +> in `src/combos/request.ts` with a comment that keeps `unknown` distinct in the +> debug record — exactly what the review asked for. Absorbing that would be taking +> the author's response to our own request. The analysis below stands as the +> record; the deliverable is a re-review. + +## What the contributor got right — more than the review credited + +eachann1024 found the UI bug: `intersectComboEfforts` +(`gui/src/combo-workspace-data.ts:11`) maps an unknown ladder to `[]`, and an +empty member wipes the intersection, so ONE undiscovered target empties the whole +picker and the user cannot select an effort they are entitled to. + +Re-reading the diff for this layer turned up something the review missed and that +changes the shape of this correction: **they already built the honest UI.** The PR +adds `comboHasUnknownEffortTargets`, threads `hasUnknownTargets` into +`EffortSelect` from both the add-modal (`:45`) and the detail panel (`:63`), marks +the options, and adds a notice with copy that says out loud: + +> Some targets have no catalog effort ladder. Options stay selectable, but +> runtime omits the default for those unknown targets (fails closed). + +That copy describes exactly the behavior we want. The author understood the +trade-off and told the user about it — and then reversed the runtime guard anyway, +which made their own notice inaccurate. + +So this correction is smaller than "rewrite it": keep their UI work almost +wholesale, drop the runtime hunk, and the notice becomes true. + +## The one decision to correct + +`src/combos/request.ts:43` on `dev` fails closed for BOTH unknown and unsupported, +and records which one it was: + +```ts +capability: targetReasoningEfforts === undefined ? "unknown" : "unsupported", +``` + +The PR skips the guard when the ladder is `undefined` and hardcodes the field to +`"unsupported"`, so we start sending an effort to targets we know nothing about +and simultaneously lose the signal that would tell us how often that happens. +**We keep `src/combos/request.ts` exactly as it is on `dev`.** + +## A second, subtler correction + +The PR's intersection skips on `listed.length === 0` as well as `undefined`. That +conflates "we have no metadata" with "the provider advertises no efforts" — a +*known* empty ladder is information and must still constrain the intersection. +Our version skips only `undefined`. + +## Change map + +### MODIFY `gui/src/combo-workspace-data.ts` + +```ts + for (const target of complete) { + const key = `${target.provider.trim()}/${target.model.trim()}`; + const listed = modelEfforts.get(key); +- // Missing metadata must not invent a full ladder — runtime omits the combo default when +- // supportedLadderFor is undefined (#488 / Codex review). +- const member: string[] = listed === undefined +- ? [] +- : listed.filter((effort) => effortSet.has(effort)); ++ // Unknown ladder (`undefined`) is a WILDCARD for the picker only: it must not empty the ++ // selectable set, because one undiscovered member would otherwise hide efforts the other ++ // members genuinely support (#1092). This does NOT relax the runtime contract — the combo ++ // default is still omitted for an unknown target at request time (src/combos/request.ts:43), ++ // and the picker says so. A KNOWN-but-empty ladder is information, not absence, so it still ++ // constrains the intersection. ++ if (listed === undefined) continue; ++ const member = listed.filter((effort) => effortSet.has(effort)); + if (common === null) { + common = member; + } else { + const memberSet = new Set(member); + common = common.filter((effort) => memberSet.has(effort)); + } + } +- const commonSet = new Set(common ?? []); ++ // Every member unknown → nothing constrained the ladder → offer all of it. ++ if (common === null) return [...COMBO_EFFORTS]; ++ const commonSet = new Set(common); + return COMBO_EFFORTS.filter((effort) => commonSet.has(effort)); +``` + +Also add the small predicate the UI needs: + +```ts +/** True when any complete target has no catalog effort ladder (picker shows a fail-closed notice). */ +export function comboHasUnknownEffortTargets( + targets: readonly ComboTarget[], + modelEfforts: ReadonlyMap, +): boolean { + return targets + .filter((t) => t.provider.trim() && t.model.trim()) + .some((t) => modelEfforts.get(`${t.provider.trim()}/${t.model.trim()}`) === undefined); +} +``` + +### MODIFY the three GUI files + +Take the PR's `hasUnknownTargets` threading as-is: +`gui/src/components/combo-workspace-controls.tsx` (prop + the notice after the +existing unsupported-effort notice at `:77`), +`combo-workspace-add-modal.tsx:45`, `combo-workspace-detail-panel.tsx:63`. + +**Take the notice, drop the per-option suffix.** Appending `(unknown targets)` to +every option makes a five-item dropdown noisy for a condition that applies to the +combo, not to each effort. One line under the picker says it once. + +### MODIFY locales (6 files) + +Only the two keys this layer needs — `cws.field.defaultEffortUnknown` and the +amended `cws.field.defaultEffortHint`. The PR's other keys (`cws.copyPublicModel`, +`cws.capabilities`, image-capability copy…) belong to unrelated features in the +same PR and are out of scope here. + +### MODIFY `tests/combo-workspace-data.test.ts` + +Replace `intersectComboEfforts treats unknown members as having no selectable +efforts` (its premise is the bug) with: + +1. unknown member does not empty a known intersection; +2. all-unknown returns the full ladder; +3. **known-but-empty ladder still empties the intersection** — the case the PR's + version gets wrong; +4. `comboHasUnknownEffortTargets` true/false. + +### NEW test in `tests/combos.test.ts` (or nearest runtime combo test) + +The load-bearing one: with an `undefined` ladder, `concreteComboRequestBody` still +omits `reasoning.effort`. This is what makes the picker notice truthful and what +would fail if someone re-applies the PR's runtime hunk. + +## Red ablation + +Apply the PR's runtime change to `src/combos/request.ts` (skip the guard when +`undefined`). The new runtime test must fail with an injected effort present. +Restore, it passes. + +## Verification + +- `bun test tests/combo-workspace-data.test.ts` +- `bun test tests/combos.test.ts` +- `bun run lint:gui`, `bun run typecheck` +- GUI screenshot required by `enforce-target` (title/description mentions gui). + +## Accept criteria + +*(superseded — see below)* + +## Revised deliverable: re-review, not absorb + +On the current head (`0f9c7a042`) the runtime guard reads: + +```ts + // Fail closed for both unknown (`undefined`) and known-but-missing ladders. + // Unknown is kept distinct in debug so we can measure thin catalog rows without + // guessing the provider accepts a reasoning field. + if (!targetReasoningEfforts?.includes(defaultEffort)) { +``` + +That is the dev behavior restored, with the reasoning written down. Say so +plainly — an author who turns a review around in ten minutes should hear that it +landed, not silence. + +Two things still worth raising, neither blocking: + +1. **`imageInput` is unrelated scope.** The PR now also adds a combo + `imageInput: "auto" | "disabled"` field with validation, plus public-model + copy/preview UI and locale keys for both. That is a separate feature sharing a + branch with an effort-picker bug fix. Ask for a split so the picker fix can be + reviewed and land on its own. +2. **The known-but-empty case.** Verify the intersection still lets a *known* + empty ladder constrain the result — skipping on `listed.length === 0` as well + as `undefined` would conflate "provider advertises nothing" with "we have no + metadata", which is the same conflation the runtime guard just refused. + +No close. The PR stays theirs. + +## Executed + +Comment `5203708625`. Credits the sub-30-minute turnaround explicitly (review +09:09:51Z → commits 09:38:19Z), quotes the restored guard including the comment +keeping `unknown` distinct in debug, and notes the GUI notice copy was theirs +rather than ours. Two asks, neither blocking: split the unrelated `imageInput` +scope, and double-check that a *known but empty* ladder still constrains the +intersection. + +PR remains OPEN. Nothing absorbed, nothing closed. diff --git a/devlog/_plan/260806_bug_stack_absorb/030_s3_cursor_edit_provenance.md b/devlog/_plan/260806_bug_stack_absorb/030_s3_cursor_edit_provenance.md new file mode 100644 index 000000000..a08a6367c --- /dev/null +++ b/devlog/_plan/260806_bug_stack_absorb/030_s3_cursor_edit_provenance.md @@ -0,0 +1,152 @@ +# 030 — s3: Cursor structured edit with real provenance (corrects #1036, closes #1017) + +## What the contributor got right + +ZachDreamZ built the whole conversion: `translateStructuredEditCall` validates the +JSON, resolves several argument spellings (`file_path` / `filePath` / `path` …), +builds line-based hunks, rejects final-newline-only and no-op edits, handles +`multi_edit` arrays, and **drops with an explanatory error rather than emitting a +best-effort patch**. That last choice is the right one and is most of the value +here. The synthetic tool descriptions are unusually good — they tell the model +that `old_string` must match exactly once, because `apply_patch` rejects ambiguous +hunks. + +They also avoided shadowing: `cursorStructuredEditTools` filters out a candidate +whose bare name already exists in the client catalog. So the collision was +understood at injection time. + +## The one decision to correct + +That knowledge never reaches the translation. Both call sites gate on the name +alone: + +```ts +const translation = translateStructuredEditCall(responsesName, normalizedArgs); // stateless branch +const translation = translateStructuredEditCall(open.name, finalArgs); // completion path +``` + +So a genuine client or MCP tool named `edit_file` is converted anyway — the exact +case the injection filter was written to avoid. The user either gets their call +silently re-emitted as `apply_patch`, or a drop error naming a conversion they +never requested. + +**Do not derive provenance from `clientToolNames`.** A real client tool named +`edit_file` is in that set too; it is the collision, not the discriminator. + +## Where provenance actually lives + +The information exists at injection and must be carried forward: + +1. `src/adapters/cursor/request-builder.ts:251-265` — `createCursorRequest()` + filters the client catalog, then budgets. Compute the synthetic names from the + ORIGINAL `visibleTools` here, and intersect with `budget.tools` afterwards so + the set reflects what actually survived budgeting and was advertised. +2. `src/adapters/cursor/types.ts:10` — carry the list on `CursorRunRequest`. +3. `src/adapters/cursor/live-transport.ts:569-579` — seed the event state. +4. `src/adapters/cursor/protobuf-events.ts:127` — add the field: + +```ts + /** + * Bare names advertised as OUR synthetic structured-edit tools on THIS request. + * Provenance, not a name test: a client or MCP tool legitimately called + * `edit_file` must pass through untranslated (#1036 review). Absent set = no + * synthetic tools were advertised, so nothing converts. + */ + syntheticStructuredEditToolNames?: ReadonlySet; +``` + +Then both call sites become: + +```ts +const translation = state.syntheticStructuredEditToolNames?.has(name) + ? translateStructuredEditCall(name, args) + : undefined; +``` + +The stateless fallback at `protobuf-events.ts:314` has no state by design (direct +and unit callers). It **defaults to pass-through**: no provenance, no conversion. +That is the fail-closed direction — an unconverted structured call is a visible, +recoverable failure; a wrongly converted one silently edits a file. + +## Change map + +- MODIFY `src/adapters/cursor/tool-definitions.ts` — take the PR's + `CURSOR_EDIT_FILE_TOOL`, `CURSOR_MULTI_EDIT_TOOL`, schemas, + `isCursorStructuredEditToolName`, `cursorStructuredEditTools` as written. +- MODIFY `src/adapters/cursor/protobuf-events.ts` — take + `translateStructuredEditCall` and `dropStructuredEditCall` as written; add the + state field; gate both call sites on it. +- MODIFY `src/adapters/cursor/request-builder.ts`, `types.ts`, + `live-transport.ts` — thread the advertised-name set (this is the new work). +- NEW `tests/cursor-structured-edit.test.ts` — the PR's suite, PLUS the + regression below. + +## The regression test that would have caught this + +**Corrected at audit.** A first draft of this test passed a plain +`{name, args}` object and looked for `type === "toolCall"`. Neither matches the +real surface: `mapSyntheticMcpExecToToolEvents` takes a protobuf `McpArgs` and +returns `tool_call_start` / `tool_call_delta` / `tool_call_end` +(`src/adapters/cursor/protobuf-events.ts:314-348`), and it returns `[]` unless +`providerIdentifier === OCX_RESPONSES_TOOL_PROVIDER` (`:319`). The draft would +have asserted on an empty array and passed for the wrong reason. + +The contributor's own suite already builds these correctly, so reuse its shape — +encoded arg bytes, `providerIdentifier: "opencodex-responses"`, `toolCallId`: + +```ts + test("a client tool named edit_file is NOT converted when no synthetic tools were advertised (#1036 review)", () => { + // Provenance, not name matching: an MCP server exposing `edit_file` must reach the + // client untouched. Without the gate the user's call is either silently re-emitted + // as apply_patch or dropped with an error naming a conversion they never requested. + const args = create(McpArgsSchema, { + name: CURSOR_EDIT_FILE_TOOL, + toolName: CURSOR_EDIT_FILE_TOOL, + toolCallId: "call_collision", + providerIdentifier: "opencodex-responses", + args: { + file_path: encoder.encode(JSON.stringify("src/a.ts")), + old_string: encoder.encode(JSON.stringify("x")), + new_string: encoder.encode(JSON.stringify("y")), + }, + }); + // State WITHOUT syntheticStructuredEditToolNames: the client owns this name. + const state = createCursorProtobufEventState(); + state.clientToolNames = new Set([CURSOR_EDIT_FILE_TOOL]); + + const out = mapSyntheticMcpExecToToolEvents(args, "call_collision", { state }); + + const start = out.find(m => m.type === "tool_call_start"); + expect(start?.name).toBe(CURSOR_EDIT_FILE_TOOL); + expect(JSON.stringify(out)).not.toContain("*** Begin Patch"); + }); +``` + +The stateful branch emits through `recordToolCall`/`commitToolCall`, so if the +observed event names differ from `tool_call_start` there, assert on whatever that +path emits and on the ABSENCE of `*** Begin Patch` — the second assertion is the +load-bearing one and is shape-independent. + +## Red ablation + +Remove the provenance gate at both call sites (restore the PR's name-only form). +The test above must fail with the call converted to `apply_patch`. Restore, it +passes, and the PR's own conversion tests stay green throughout — proving the gate +narrows behavior without breaking the feature. + +If the test passes with the gate removed, it is not exercising the mapper — most +likely `providerIdentifier` or the arg encoding is wrong and the function returned +early. Fix the fixture before trusting the result. + +## Verification + +- `bun test tests/cursor-structured-edit.test.ts` +- `bun test tests/cursor-*.test.ts` (adapter suite unaffected) +- `bun run typecheck` + +## Accept criteria + +- Conversion happens only for provenance-tagged calls; stateless path passes through. +- The collision regression test present and proven red without the gate. +- `Closes #1017`; #1036 closed with attribution to ZachDreamZ, crediting the + conversion logic and the drop-with-explanation design we kept wholesale. diff --git a/devlog/_plan/260806_bug_stack_absorb/040_s4_usage_log_isolation.md b/devlog/_plan/260806_bug_stack_absorb/040_s4_usage_log_isolation.md new file mode 100644 index 000000000..9af1d9e36 --- /dev/null +++ b/devlog/_plan/260806_bug_stack_absorb/040_s4_usage_log_isolation.md @@ -0,0 +1,83 @@ +# 040 — s4: usage-log fixture isolation, with the proof (corrects #997) + +## What the contributor got right + +Yuxin-Qiao found a hazard that is easy to miss and genuinely unpleasant: +`addRequestLog` persists to `usage.jsonl`, and `getConfigDir()` falls back to the +real `~/.opencodex` when `OPENCODEX_HOME` is unset (`src/config.ts:549-552`). So +`bun test tests/management-api-logs-metrics.test.ts` run from outside the repo — +no `bunfig` preload — writes fixture rows into a developer's actual usage history +and they show up on the GUI Usage page. The isolation they wrote (temp home, +prior value restored including the `delete` branch, logs cleared, dir removed) is +correct and complete for that file. + +## The one thing to add + +Nothing tests the promise. All nine assertions in that file concern metrics, so +the safety property the PR exists to establish is the one property the suite +would not notice losing. A later reordering of `beforeEach`, or a new test that +constructs a logger before the hook runs, puts us back to writing into +`~/.opencodex` with a green suite. + +`resolveConfigDir` caches on the raw env value (`src/config.ts:547-554`), so it +re-resolves when `OPENCODEX_HOME` changes — the isolation works, but only because +of that cache key. That is exactly the kind of invariant worth pinning. + +## Change map + +### MODIFY `tests/management-api-logs-metrics.test.ts` + +Keep the contributor's `beforeEach`/`afterEach` verbatim — it is right. Add one +test that pins the property: + +```ts + test("logging never writes into the real OpenCodex home (#997)", async () => { + // The hazard this file's isolation exists to prevent: getConfigDir() falls back + // to ~/.opencodex when OPENCODEX_HOME is unset (src/config.ts:549-552), so an + // unisolated run persists fixture rows into a developer's real usage history. + // Assert the resolved target, not just that the tests pass. + const realHome = join(homedir(), ".opencodex"); + expect(testDir).not.toBe(realHome); + expect(getConfigDir()).toBe(testDir); + + addRequestLog({ /* minimal fixture row */ } as RequestLogEntry); + + expect(existsSync(join(testDir, "usage.jsonl"))).toBe(true); + // The real home must be untouched by this run. If it already exists on the + // developer's machine we cannot assert absence, so assert the write landed in + // the scratch dir and that the resolver never pointed at the real path. + expect(getConfigDir().startsWith(tmpdir())).toBe(true); + }); +``` + +Imports to add: `homedir` from `node:os`, `existsSync` from `node:fs`, +`getConfigDir` from `../src/config`. + +**Deliberate limitation, stated rather than hidden:** we cannot assert the real +`~/.opencodex` is absent, because on a real machine it usually exists and is not +ours to inspect. Asserting the resolved write target is inside `tmpdir()` is the +strongest honest claim. + +## Red ablation + +Comment out `process.env.OPENCODEX_HOME = testDir;` in `beforeEach`. The new test +must fail on `expect(getConfigDir()).toBe(testDir)` — resolving to the real home. +Restore, it passes. Record both. + +## Verification + +- `bun test tests/management-api-logs-metrics.test.ts` +- `bun run typecheck` + +## Follow-up worth noting, not doing here + +Other test files calling `addRequestLog` may carry the same hazard. A shared +helper beside `tests/helpers/isolated-codex-home.ts` (which does exactly this for +`CODEX_HOME`) would be the right home for it. Out of scope for this layer — +recorded so it is not lost. + +## Accept criteria + +- Contributor's isolation preserved unchanged. +- New assertion present and proven non-vacuous by the ablation. +- #997 closed with attribution to Yuxin-Qiao. diff --git a/devlog/_plan/260806_bug_stack_absorb/050_deferral_record.md b/devlog/_plan/260806_bug_stack_absorb/050_deferral_record.md new file mode 100644 index 000000000..677a53fec --- /dev/null +++ b/devlog/_plan/260806_bug_stack_absorb/050_deferral_record.md @@ -0,0 +1,48 @@ +# 050 — Deferral record: #1036 and #997 stay with their authors + +Measured 2026-08-06, comparing last commit against our review time (the method +this unit adopted after the `updatedAt` error): + +| PR | Last commit | Our review | Author response | +|---|---|---|---| +| #1036 | 2026-08-05T07:18:36Z | 2026-08-06T09:12:51Z | none yet | +| #997 | 2026-08-06T02:51:22Z | 2026-08-06T09:16:02Z | none yet | + +Both still lack the requested change — #1036 has no provenance tag on the +conversion, #997 has no assertion that the real home was untouched. Both were +verified by reading the current diffs, not inferred. + +**Neither is absorbed.** The response window is 72 hours from our review, i.e. +until 2026-08-09. Roughly two hours have passed, and #997's author was active at +02:51Z the same morning. Treating that as abandonment would be indefensible. + +## Why the window exists at all + +Because the alternative was demonstrated inside this very unit. The original plan +had four absorb targets. Live data showed: + +- #1092's author fixed our exact objection in under 30 minutes; +- #1068 merged with the union we asked for, while we were still writing the + replacement. + +Two of four would have been taken over while their authors were actively +responding. The window is what stops the campaign from optimizing for our commit +count instead of the repository. + +## What happens when the window closes + +Only if a head still lacks the fix on 2026-08-09: + +1. Re-check the exact head first — an author who acts in the meantime moves to + the re-review path. +2. Author the layer per `030` (#1036) or `040` (#997), on a branch taken directly + from `origin/dev`. These are independent, so no stack. +3. Open our PR with attribution naming the author and their PR, then close theirs + with a comment stating what we kept and what we changed. + +A successor unit `devlog/_plan/2608xx_bug_absorb_window/` gets created at that +point. It does not exist yet, and creating it now would presume the outcome. + +## Status + +`BLOCKED` on contributor response — by design. Both PRs remain open and theirs. diff --git a/devlog/_plan/260806_bug_stack_absorb/060_s6_pr_less_issues.md b/devlog/_plan/260806_bug_stack_absorb/060_s6_pr_less_issues.md new file mode 100644 index 000000000..003183c8f --- /dev/null +++ b/devlog/_plan/260806_bug_stack_absorb/060_s6_pr_less_issues.md @@ -0,0 +1,73 @@ +# 060 — s6: the issues with no PR (#1059, #1102) + +## #1059 — Windows suite: measured, not estimated + +The issue carried "~207 failures" from an aborted promotion run and had not been +re-measured since 2026-08-01. Dispatched the Windows leg against `dev` +(run `31095755263`); all four shards failed: + +| Shard | Result | +|---|---| +| 1/4 | **timed out** at the 15-minute cap — no counts recoverable | +| 2/4 | 8 fail | +| 3/4 | 21 fail | +| 4/4 | 47 fail | + +76 recoverable failures plus an unknown shard. Grouped by suite from 152 `(fail)` +lines: + +``` +32 injectCodexConfig integration (Design B) +30 Codex catalog sync hardening +16 Codex catalog restore +12 two real processes contend for one lock +12 server same-target 429 retry (end-to-end) +12 codex-journal + 6 Issue #702 expired forward replay state +``` + +**The finding that changes the plan:** roughly three quarters are one family — +Codex catalog and config *file writing*. The failing assertions are about atomic +publication and permission narrowing ("writes prepared bytes atomically with the +right...", "group-readable is narrowed back to owner-only", CAS txId conflicts), +which is where POSIX assumptions leak on Windows: `rename` over an existing file, +`chmod` bits, and advisory locking all behave differently. + +So the recorded plan ("management/server fixtures first, then platform process +semantics") is superseded by evidence: fix the write substrate first and ~78 +failures should fall together rather than shard by shard. + +Second defect, split out: **shard 1 times out**, so a quarter of the suite gives +no signal at all. The 15-minute cap was sized against a green suite; Windows +retries and filesystem waits exceed it. + +Posted as comment `5203988322`. Issue stays open — this is a measurement and a +re-prioritization, not a fix. + +## #1102 — waiting on the reporter, and on a policy call that is not ours + +RCA posted 07:52Z. The reporter (`comfuture`) has not replied; they have zero +comments on the thread since filing. + +Two things gate progress and neither is code: + +1. **Two questions to the reporter** — is their spawning host something they + control, and is the `0.0.0.0` bind actually serving remote clients that must + stay authenticated? The answer decides whether a documented credential path + suffices or a policy change is needed. +2. **A maintainer policy decision** — the only fix that helps a direct-spawned + app-server is an opt-in "trust loopback peers on a non-loopback bind". That + redefines the security boundary from "authenticate all callers when bound + remotely" to "trust any local transport peer", which includes any local + process and any tunnel terminator landing on loopback. + +Re-commenting today would add nothing: the RCA already states both, and the +workaround (`config.apiKeys` + the host passing `x-opencodex-api-key`) is already +documented in it. **Status: `NEEDS_HUMAN`** — carried to the user, not resolved +here. + +## Accept criteria + +- #1059 carries current measured data and a re-prioritized burn-down order. +- #1102 is explicitly escalated rather than silently left open. +- Neither issue closed. diff --git a/devlog/_plan/260806_bug_stack_absorb/070_push_on_top.md b/devlog/_plan/260806_bug_stack_absorb/070_push_on_top.md new file mode 100644 index 000000000..3a36dae4d --- /dev/null +++ b/devlog/_plan/260806_bug_stack_absorb/070_push_on_top.md @@ -0,0 +1,74 @@ +# 070 — Push on top of the contributor's branch + +The campaign spent two loops circling a problem it had invented. The user named +the answer in one line: **their PRs report `maintainerCanModify=true`, so push our +completion commit onto their branch.** Their commits stay, `git blame` stays +accurate, their PR is what merges, and attribution is the commit graph rather +than a paragraph. + +No replacement PR. No close. No "absorbed with credit to" prose. The thing that +made the earlier plan feel wrong was real, and this removes the cause instead of +managing it. + +## What landed + +### #1036 — done + +``` +b5e292928 bitkyc08-arch fix(cursor): gate structured-edit conversion on provenance, not tool name +33d617601 Agent59353 fix(cursor): address CodeRabbit + Codex review feedback on #1017 +d06de0f9c Agent59353 test(cursor): cover native-exec mcpArgs structured edit translation +500cd94f6 Agent59353 fix(cursor): never shadow an existing bare edit tool ... +f724063d3 Agent59353 fix(cursor): expose structured edit tools that convert to valid apply_patch calls (#1017) +``` + +Pushed with `--force-with-lease` pinned to their head, after re-verifying the +remote was unchanged. `gh pr view #1036` shows five commits with only the last +attributed to us. + +Change: `live-transport` records the bare names we actually advertised this +request (from `cursorStructuredEditTools`), the event state carries them, and +both translate call sites convert only for names in that set. The stateless +fallback passes through — it has no state to consult, and an unconverted call is +recoverable while a wrongly converted one edits a file. Their conversion logic is +byte-unchanged. + +Ablation: restoring the name-only gate gives 21 pass / 1 fail, red on exactly the +new collision test. + +### #997 — not ours to push + +The lease rejected our push as stale: the author had landed `3304d5c8` while we +worked, and **their version is stronger than the one we staged.** We pinned +`getConfigDir()`; they assert the resolved `usage.jsonl` receives the row *and* +that the default location lacks the request id — that survives a refactor of how +the path is resolved. + +Verified rather than overwritten: 10 pass on their head, and ablating their +isolation gives 9 pass / 1 fail on their own assertion. Comment posted saying no +changes requested. + +## The rule this produced + +**Re-fetch the contributor head immediately before preparing a commit, not before +pushing.** Three times in this campaign, acting on a stale head produced wasted or +wrong work: + +1. judging author activity by `updatedAt`, which moves when *we* comment; +2. posting a "failing test" finding on #1068 eight minutes after it merged with + the fix; +3. building a #997 commit against a head the author had already improved. + +The lease caught the third one. The first two reached a contributor as a wrong +public statement. Cheap check, expensive omission. + +## When push-on-top is the right move + +- `maintainerCanModify=true`, and the gap is a bounded correction rather than a + rewrite. +- The contributor's design survives intact — if our version would replace their + approach, it is a different PR and a conversation, not a commit on their branch. +- The remote head is re-verified immediately before pushing, with a lease pinned + to it. +- Any contract change we introduce is disclosed in a comment and explicitly open + to disagreement (here: the stateless pass-through). diff --git a/tests/codex-catalog.test.ts b/tests/codex-catalog.test.ts index 1f6896f8a..aff5093c8 100644 --- a/tests/codex-catalog.test.ts +++ b/tests/codex-catalog.test.ts @@ -2387,6 +2387,86 @@ describe("Codex catalog routed normalization", () => { expect(routed?.supports_reasoning_summaries).toBe(true); }); + // #1100: a routed row can advertise a full effort ladder while + // `supports_reasoning_summaries` is false, and Codex gates construction of the + // whole Responses `reasoning` object on that flag — so the Desktop picker shows + // an effort the wire never carries. The three tests below pin the contract that + // keeps the documented escape hatch working. + test("routed strip does not defeat an explicit reasoning-summary opt-in (#1100)", async () => { + const models = await gatherRoutedModels({ + providers: { + ladder: { + adapter: "openai-responses", + baseUrl: "https://ladder.example.test/v1", + authMode: "key", + liveModels: false, + models: ["effort-model"], + modelReasoningEfforts: { "effort-model": ["low", "high", "max"] }, + modelSupportsReasoningSummaries: { "effort-model": true }, + }, + }, + }); + // MUST pass a template: buildCatalogEntries(null, ...) takes the fallback + // branch (src/codex/catalog/sync.ts:291-310), which never runs the routed + // strip, so a null-template assertion cannot detect a reordering regression. + const entries = buildCatalogEntries(nativeTemplate(), [], models); + const routed = entries.find(e => e.slug === "ladder/effort-model"); + + expect((routed?.supported_reasoning_levels as { effort: string }[]).map(l => l.effort)) + .toEqual(expect.arrayContaining(["low", "high", "max"])); + // The opt-in survives normalizeRoutedCatalogEntry's delete only because + // applyCatalogModelMetadata runs after it (src/codex/catalog/sync.ts:266-269). + // Swap that order and every opted-in routed provider silently stops getting + // reasoning.effort from Codex. + expect(routed?.supports_reasoning_summaries).toBe(true); + }); + + test("routed rows without an opt-in stay conservative about summaries (#1100)", async () => { + const models = await gatherRoutedModels({ + providers: { + plain: { + adapter: "openai-responses", + baseUrl: "https://plain.example.test/v1", + authMode: "key", + liveModels: false, + models: ["effort-model"], + modelReasoningEfforts: { "effort-model": ["low", "high", "max"] }, + }, + }, + }); + const entries = buildCatalogEntries(nativeTemplate(), [], models); + const routed = entries.find(e => e.slug === "plain/effort-model"); + + // Deliberate, and the direct cause of #1100 for an unconfigured provider: we + // do not claim OpenAI-only summary delivery for an arbitrary endpoint just + // because it has an effort ladder. The supported remedy is the per-model + // opt-in asserted above. Pinned so that flipping this default is ever a + // deliberate decision rather than an accident. + expect(routed?.supports_reasoning_summaries).toBe(false); + }); + + test("the no-template fallback never applies the routed summary strip (#1100)", async () => { + const models = await gatherRoutedModels({ + providers: { + ladder: { + adapter: "openai-responses", + baseUrl: "https://ladder.example.test/v1", + authMode: "key", + liveModels: false, + models: ["effort-model"], + modelReasoningEfforts: { "effort-model": ["low", "high", "max"] }, + modelSupportsReasoningSummaries: { "effort-model": true }, + }, + }, + }); + // Pins the asymmetry itself: the fallback path skips + // normalizeRoutedCatalogEntry entirely, so this row is opt-in-true for a + // different reason than the template row above. Kept explicit so a future + // unification of the two construction paths is a visible change. + const routed = buildCatalogEntries(null, [], models).find(e => e.slug === "ladder/effort-model"); + expect(routed?.supports_reasoning_summaries).toBe(true); + }); + test("generated jawcode snapshot is restricted to mapped providers", () => { expect(resolveJawcodeProvider("kimi")).toBe("moonshot"); expect(resolveJawcodeProvider("nanogpt")).toBeUndefined();