From c03cd584e154b2228e03d22d547b352cee61b329 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Thu, 6 Aug 2026 18:03:27 +0900 Subject: [PATCH 01/11] docs(devlog): bug-grade remediation roadmap for the 25 bug-labelled items MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase-0 docs-only cycle for the loop that turns the 2026-08-06 bug triage into landed remediation. Twelve issues and thirteen PRs carry the `bug` label; none is a false report, so the roadmap is organized by whether the proposed FIX is sound rather than by whether the defect is real. The load-bearing finding is in 010. A routed catalog row advertises a full effort ladder while `supports_reasoning_summaries` is stripped (catalog/parsing.ts:353) and defaulted false (:266), which is #1100. But the per-model opt-in the strip's own comment says "can be added" already exists and already survives, because applyCatalogModelMetadata runs at catalog/sync.ts:269, after the strip at :267. So the defect is narrower and more honest than "we strip the flag": the ordering works and nothing asserts it stays that way. wp1 buys that joint contract and deliberately does not flip the routed default to true — advertising OpenAI-only summary delivery for arbitrary providers is the same overclaim that blocks #1092 and #978. Two adversarial audit rounds preceded this commit. Round 1 caught the planned regression test as vacuous: it called buildCatalogEntries(null, ...), which takes the fallback branch (sync.ts:291-310) and never runs the strip, so the ablation could not have failed. The amended plan drives the template path via nativeTemplate() and keeps a third test pinning the fallback asymmetry. The ablation was then proven empirically rather than argued: reordering the two calls flipped the template row true->false while the fallback row stayed true, and restore left an empty diff. Round 1 also caught the objective promising an outcome for every labelled item while only one issue had scheduled work; wp6 (060) now gives each remaining G1 issue a disposition and an unblocking condition, and the objective says plainly that this loop authors code for exactly one defect. Round 1's third finding corrected a misdiagnosis of our own making: #1068's failing gate is not a verdict on its diff. The workflow requires parseGateState (enforce-pr-target.yml:63), dev exports it (pr-quality-state.cjs:84,380), and an in-tree regression test names this exact crash as the head-YAML/base-script mismatch (enforce-pr-target.test.cjs:51-60) — the same root cause as #978's stale checkout. Blaming a contributor for a gate artifact would have been wrong. No src/ or tests/ changes in this commit; implementation starts at wp1. --- .../260806_bug_grade_remediation/000_plan.md | 129 +++++++++++++ .../001_grade_matrix.md | 59 ++++++ .../010_wp1_catalog_joint_contract.md | 176 ++++++++++++++++++ .../020_wp2_capability_gate_reviews.md | 65 +++++++ .../030_wp3_correctness_hazard_reviews.md | 88 +++++++++ .../040_wp4_own_pr_and_provenance.md | 71 +++++++ .../050_wp5_grade2_dispositions.md | 35 ++++ .../060_wp6_g1_issue_dispositions.md | 48 +++++ 8 files changed, 671 insertions(+) create mode 100644 devlog/_plan/260806_bug_grade_remediation/000_plan.md create mode 100644 devlog/_plan/260806_bug_grade_remediation/001_grade_matrix.md create mode 100644 devlog/_plan/260806_bug_grade_remediation/010_wp1_catalog_joint_contract.md create mode 100644 devlog/_plan/260806_bug_grade_remediation/020_wp2_capability_gate_reviews.md create mode 100644 devlog/_plan/260806_bug_grade_remediation/030_wp3_correctness_hazard_reviews.md create mode 100644 devlog/_plan/260806_bug_grade_remediation/040_wp4_own_pr_and_provenance.md create mode 100644 devlog/_plan/260806_bug_grade_remediation/050_wp5_grade2_dispositions.md create mode 100644 devlog/_plan/260806_bug_grade_remediation/060_wp6_g1_issue_dispositions.md 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..3e5614242 --- /dev/null +++ b/devlog/_plan/260806_bug_grade_remediation/010_wp1_catalog_joint_contract.md @@ -0,0 +1,176 @@ +# 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. + +## 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..9d3e228a7 --- /dev/null +++ b/devlog/_plan/260806_bug_grade_remediation/020_wp2_capability_gate_reviews.md @@ -0,0 +1,65 @@ +# 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` + +Verified on dev (`src/adapters/google.ts:313-326`): `thinkingConfig` is emitted +for two direct Flash ids only. The narrow bug is real — `gemini-3.1-pro-preview` +advertises a ladder at `src/providers/registry.ts:1212-1221` and never receives +the field. + +The PR replaces the id gate with "any model that has an effort ladder", which +reaches arbitrary custom Google/Vertex-compatible providers a user configured. + +Required change: extend the allowlist with the specific models whose acceptance +of `generationConfig.thinkingConfig.thinkingLevel` can be cited, or introduce an +explicit per-model capability flag. The tests prove serialization, which is not +the same as proving upstream acceptance — say so plainly. + +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, each naming the dev-side fail-closed line, the +concrete failure mode, and the required change. No labels changed, no PR closed. + +## Accept criteria + +- Both comment URLs captured. +- Each comment quotes the dev-side source it asks the author to preserve. +- Neither comment asks for a rewrite where a narrowing would do. 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..7f2603abd --- /dev/null +++ b/devlog/_plan/260806_bug_grade_remediation/030_wp3_correctness_hazard_reviews.md @@ -0,0 +1,88 @@ +# 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`. + +**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. 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..a45e1620e --- /dev/null +++ b/devlog/_plan/260806_bug_grade_remediation/040_wp4_own_pr_and_provenance.md @@ -0,0 +1,71 @@ +# 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 "7000 pass / 0 fail". The recorded Windows run has two + failing update-job tests, both unexpectedly reaching `spawnStart` under the + altered restart flow. The Ubuntu failure is a Bun 1.3.14 segfault after the + suite and is not attributable to this patch; macOS was cancelled. + +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..9bd7b1126 --- /dev/null +++ b/devlog/_plan/260806_bug_grade_remediation/050_wp5_grade2_dispositions.md @@ -0,0 +1,35 @@ +# 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 + +| PR | Verdict | Remaining blocker | +|---|---|---| +| #1095 | merge-ready | none technical; needs the maintainer's merge decision. Removes forced non-streaming for `deepseek-v4-flash` and synthesizes a terminal only after a structurally complete item lifecycle; real upstream terminals stay authoritative | +| #1085 | merge-ready pending security review | replaces an unset env reference with a non-secret loopback placeholder; credential-adjacent, so the stated security review is the gate | +| #1111 | split required | Copilot normalization is sound and its test drives `handleResponses` end to end; the 64 MiB native-history bound is an unrelated fix and belongs in its own PR. Author also states the full suite was interrupted | +| #1056 | 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 | rebase + full suite | syncs vision replacements into `_rawBody`, which passthrough actually serializes; only lightweight checks have run on the current draft head | + +## 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, then an escalation to the maintainer naming the two PRs +that are merge-ready and the decision required. + +## Accept criteria + +- Five comment URLs captured. +- `gh pr view` confirms all five remain open and unmerged after this phase. +- The escalation states plainly that the agent did not merge. 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..63f648305 --- /dev/null +++ b/devlog/_plan/260806_bug_grade_remediation/060_wp6_g1_issue_dispositions.md @@ -0,0 +1,48 @@ +# 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. From c3b4d18e52c978f4c1d2c45d493d958c0ab5c2fb Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Thu, 6 Aug 2026 18:06:51 +0900 Subject: [PATCH 02/11] test(catalog): pin the routed reasoning joint contract (#1100) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A routed catalog row advertises a full effort ladder while `supports_reasoning_summaries` is stripped (catalog/parsing.ts:353) and defaulted false (:266). Codex gates construction of the entire Responses `reasoning` object on that flag, so Desktop renders an effort picker whose selection never reaches the wire. That is #1100. The escape hatch already exists: `modelSupportsReasoningSummaries` resolves at catalog/provider-fetch.ts:545 and survives the strip, but only because `applyCatalogModelMetadata` runs at catalog/sync.ts:269, two lines after `normalizeRoutedCatalogEntry` at :267. Nothing asserted that ordering, so a future reorder would silently disable effort propagation for every opted-in routed provider with a green suite. Three tests, and the choice of construction path is the whole point. The two existing summary tests (:2350, :2387) call `buildCatalogEntries(null, ...)`, which takes the fallback branch (sync.ts:291-310) and never runs the strip — so they keep passing under the broken order. A plan audit caught the first draft of this commit making exactly that mistake. The opt-in and conservative tests now pass `nativeTemplate()` to reach the template branch, and a third test keeps the `null` path deliberately, pinning the asymmetry between the two so a future unification is a visible change rather than a silent one. Red ablation, run rather than asserted. Moving the strip after the metadata restore produces: Expected: true / Received: false (fail) routed strip does not defeat an explicit reasoning-summary opt-in (pass) routed rows without an opt-in stay conservative about summaries (pass) the no-template fallback never applies the routed summary strip Only the template test goes red; the fallback staying green is what proves the template test carries the contract. Restore leaves an empty src/ diff. No src/ change: this does not fix #1100 for a reporter whose provider has no opt-in configured, and it deliberately does not flip the routed default to true — claiming OpenAI-only summary delivery for arbitrary providers is the same overclaim that blocks #1092 and #978. It pins what we have and makes the supported workaround load-bearing. tests/codex-catalog.test.ts: 119 -> 122 pass, 0 fail. typecheck clean. --- .../010_wp1_catalog_joint_contract.md | 20 +++++ tests/codex-catalog.test.ts | 80 +++++++++++++++++++ 2 files changed, 100 insertions(+) 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 index 3e5614242..dca2df271 100644 --- 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 @@ -162,6 +162,26 @@ 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. 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(); From e465dff79c31f4c814c52305bbda7edb8d4ac6ec Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Thu, 6 Aug 2026 18:11:11 +0900 Subject: [PATCH 03/11] =?UTF-8?q?docs(devlog):=20wp2=20ledger=20=E2=80=94?= =?UTF-8?q?=20#1092=20blocked,=20#978=20verdict=20corrected?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two reviews posted. They ended up in different places than the roadmap predicted, and the correction is the point of this commit. #1092 confirmed as planned. `src/combos/request.ts:43` fails closed on dev for both an unknown and an unsupported ladder, and the debug record carries `capability: unknown | unsupported` — someone deliberately made those two states separable. 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 lose the one signal that would tell us how often that happens. Review asks for the picker fix to land as written while `undefined` stays fail-closed. #978 graded wrong in the roadmap, corrected before anything was posted. It was filed as a capability-gate reversal on the strength of its title. The diff is narrower: `thinkingEligible` excludes cloud-code-assist, excludes image models so `responseModalities` still fires, keeps the Vertex freeze absent an explicit ladder, and retains the hardcoded flash slice. Its ladder source, `configuredReasoningEfforts` (src/reasoning-effort.ts:58-64), returns a value only from a registry preset or explicit user config — never a default. Probing a custom google-adapter provider on both trees: dev sends nothing in all three configs; the PR sends `thinkingLevel` only where the user asserted a ladder. That is opt-in, which is the same shape wp1 called the legitimate remedy for #1100, so blocking it would contradict our own position. Review is non-blocking with one docs ask: configuring `modelReasoningEfforts` now changes wire behavior, and that is not guessable from the field name. Both PRs remain open and unmerged. Comments 5202651774 and 5202655589. --- .../020_wp2_capability_gate_reviews.md | 65 ++++++++++++++----- 1 file changed, 47 insertions(+), 18 deletions(-) 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 index 9d3e228a7..bb322837b 100644 --- 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 @@ -32,20 +32,47 @@ 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` - -Verified on dev (`src/adapters/google.ts:313-326`): `thinkingConfig` is emitted -for two direct Flash ids only. The narrow bug is real — `gemini-3.1-pro-preview` -advertises a ladder at `src/providers/registry.ts:1212-1221` and never receives -the field. - -The PR replaces the id gate with "any model that has an effort ladder", which -reaches arbitrary custom Google/Vertex-compatible providers a user configured. - -Required change: extend the allowlist with the specific models whose acceptance -of `generationConfig.thinkingConfig.thinkingLevel` can be cited, or introduce an -explicit per-model capability flag. The tests prove serialization, which is not -the same as proving upstream acceptance — say so plainly. +## #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 @@ -55,11 +82,13 @@ different explanations for one artifact. ## Deliverable -Two posted PR comments, English, each naming the dev-side fail-closed line, the -concrete failure mode, and the required change. No labels changed, no PR closed. +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. -- Each comment quotes the dev-side source it asks the author to preserve. -- Neither comment asks for a rewrite where a narrowing would do. +- 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. From 9ad26e0d4a0ff298de59d303e576f66a6d255f5f Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Thu, 6 Aug 2026 18:14:23 +0900 Subject: [PATCH 04/11] =?UTF-8?q?docs(devlog):=20wp3=20ledger=20=E2=80=94?= =?UTF-8?q?=20#1036=20name=20hijack,=20#1068=20loses=20five=20vision=20gua?= =?UTF-8?q?rds?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both hazard claims re-derived from the real diffs rather than carried on the roadmap grading, after wp2 showed a grading could be wrong. Both held. #1036: `translateStructuredEditCall` returns early only on `isCursorStructuredEditToolName(toolName)`, and both call sites pass a wire-supplied name — `responsesName` in `mapSyntheticMcpExecToToolEvents`, `open.name` in the completion path. No request-scoped check asks whether the synthetic tools were advertised at all. The diff even adds a comment describing exactly that flag, so the intent exists and only the enforcement is missing. A user running an MCP server that exposes `edit_file` either has their call silently re-emitted as `apply_patch`, or gets a drop error naming a conversion they never requested. Every new test drives our own synthetic caller, so the uncovered case is the one that can actually go wrong. #1068: the `noVisionModels` line is replaced, not extended, and set arithmetic on the real constants gives the cost precisely — big-pickle, nemotron-3-ultra-free, ling-3.0-flash-free, north-mini-code-free and laguna-s-2.1-free stop being marked text-only, reopening #1043. `deepseek-v4-flash-free` sits in both sets, which is almost certainly why the replacement looked safe. The fix is a union, so the review says that rather than asking for a rewrite. Also flagged: the branch predates the per-model DeepSeek ladders that shipped for #1057 in v2.10.2. The CI-framing correction from wp2 was applied to both comments: the `enforce-target` failure is a repository-side gate artifact, and the #1068 comment retracts the earlier "substantive" framing to the author directly rather than quietly dropping it. Comments 5202680370 and 5202684051; both PRs remain open and unmerged. --- .../030_wp3_correctness_hazard_reviews.md | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) 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 index 7f2603abd..3fb888a9a 100644 --- 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 @@ -50,6 +50,21 @@ 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 @@ -86,3 +101,14 @@ and the minimal change that keeps the fix. - 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. From 5fe3a365b471ffe56ced3f6b5ef84ba54d90da98 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Thu, 6 Aug 2026 18:17:19 +0900 Subject: [PATCH 05/11] =?UTF-8?q?docs(devlog):=20wp4=20ledger=20=E2=80=94?= =?UTF-8?q?=20#557=20claim=20disproved,=20#1093=20contract,=20#997=20proof?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three dispositions, and the harshest one is on my own PR. #557 says the full suite passes. It does not. Run 30748759350 job 91498912876 ends `2 fail / Ran 7008 tests across 482 files`, and both failures resolve to the same line: 175 | spawnStart: () => { throw new Error("must not spawn"); }, error: must not spawn at spawnStart (tests\update-job.test.ts:175:37) The preflight's restart-flow change reaches `spawnStart` on a path whose test asserts nothing may restart the proxy. That is a real question about which side is correct, not re-runnable noise. This doc also corrects itself: an earlier draft claimed the ubuntu failure was a Bun segfault and macOS was cancelled — all three legs are red, so the comment asks for separate triage rather than attributing them. That is the second roadmap claim overturned by direct verification in this loop, after #978, which is why every claim is now re-derived before it reaches a contributor. The PR also skips the preflight entirely on Windows, with a test pinning the skip, so the platform where npm cache permission failures are most common is the one platform uncovered — while the same PR breaks two Windows tests. Being the author is not an exemption from the second-maintainer security review `MAINTAINERS.md` requires for dependency-installation changes. #1093: `INGRESS_SPAN_RE` is a bare format check applied to a raw client header, so the description's "guard-issued" framing is unsupported — a regex is not provenance. Asked for three things: who issues the span and why a client value may be persisted, what happens for requests that fail before adapter resolution, and a corrected contract claim, since the diff adds fields to persisted usage and `/api/logs` while saying it changes no public contract. #997: the isolation is complete for the file, including the delete-when- previously-unset branch. But all nine assertions test metrics, so the safety property the PR exists to establish is the one thing the suite would not notice losing. Asked for a single assertion pinning the real home. Comments 5202707676, 5202714727, 5202711453. All three remain open. --- .../040_wp4_own_pr_and_provenance.md | 31 ++++++++++++++++--- 1 file changed, 27 insertions(+), 4 deletions(-) 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 index a45e1620e..fab7df5fe 100644 --- 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 @@ -14,10 +14,33 @@ 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 "7000 pass / 0 fail". The recorded Windows run has two - failing update-job tests, both unexpectedly reaching `spawnStart` under the - altered restart flow. The Ubuntu failure is a Bun 1.3.14 segfault after the - suite and is not attributable to this patch; macOS was cancelled. +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 — From ffbe73f5745028b3456dbce05494ce3927c1d2ed Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Thu, 6 Aug 2026 18:20:59 +0900 Subject: [PATCH 06/11] =?UTF-8?q?docs(devlog):=20wp5=20ledger=20=E2=80=94?= =?UTF-8?q?=20no=20PR=20is=20actually=20waiting=20on=20a=20maintainer?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The roadmap called #1095 merge-ready. Live state says it is a draft, 341 commits behind `dev`. That is the third roadmap claim overturned by direct verification in this loop, after #978 and the #557 CI attribution, and the pattern is consistent: the volatile facts — draft state, commits behind, CI outcome — decay fastest, so each is now re-derived at the P of its own work-phase before it can reach a contributor. Measured per PR (`gh pr view` + `git rev-list --count ..origin/dev`): #1095 draft 341 behind #1085 ready 85 behind #1111 draft 0 behind #1056 draft 44 behind #1047 draft 341 behind Four of five are drafts, and three would fail the readiness gate on the at-most-10-behind check alone. So the deliverable changed: instead of escalating a merge decision, each comment names the one blocker that PR owns. There is no merge queue waiting on a maintainer, and reporting one would have been worse than useless. The technical verdicts stand on re-reading the diffs. #1095's terminal repair synthesizes completion only after a structurally complete item lifecycle and leaves real upstream terminals authoritative. #1085 removes an unset secret reference for a loopback-only client without widening the remote-export refusal. #1111's Copilot normalization is sound but carries an unrelated 64 MiB history bound that should be its own PR. #1056 is a bounded opt-in workaround for an upstream Desktop filter, with an unrelated drain test edit to drop. #1047 syncs vision replacements into the `_rawBody` that passthrough actually serializes. Comments 5202740738, 5202742778, 5202744803, 5202747758, 5202750228. All five remain open; zero PRs merged by this loop. --- .../050_wp5_grade2_dispositions.md | 39 +++++++++++++------ 1 file changed, 27 insertions(+), 12 deletions(-) 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 index 9bd7b1126..38bcefb5e 100644 --- a/devlog/_plan/260806_bug_grade_remediation/050_wp5_grade2_dispositions.md +++ b/devlog/_plan/260806_bug_grade_remediation/050_wp5_grade2_dispositions.md @@ -7,15 +7,29 @@ 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 - -| PR | Verdict | Remaining blocker | -|---|---|---| -| #1095 | merge-ready | none technical; needs the maintainer's merge decision. Removes forced non-streaming for `deepseek-v4-flash` and synthesizes a terminal only after a structurally complete item lifecycle; real upstream terminals stay authoritative | -| #1085 | merge-ready pending security review | replaces an unset env reference with a non-secret loopback placeholder; credential-adjacent, so the stated security review is the gate | -| #1111 | split required | Copilot normalization is sound and its test drives `handleResponses` end to end; the 64 MiB native-history bound is an unrelated fix and belongs in its own PR. Author also states the full suite was interrupted | -| #1056 | 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 | rebase + full suite | syncs vision replacements into `_rawBody`, which passthrough actually serializes; only lightweight checks have run on the current draft head | +## 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 @@ -25,11 +39,12 @@ documents the Desktop allowlist limitation. Neither supersedes the other, and ## Deliverable -Five posted comments, then an escalation to the maintainer naming the two PRs -that are merge-ready and the decision required. +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 escalation states plainly that the agent did not merge. +- The draft/behind numbers match live `gh` output at posting time, not the + roadmap's assumption. From 2fc5261ef7cf731ca4ae424c3ada0b2e704f79f5 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Thu, 6 Aug 2026 18:24:30 +0900 Subject: [PATCH 07/11] =?UTF-8?q?docs(devlog):=20wp6=20ledger=20=E2=80=94?= =?UTF-8?q?=20four=20issue=20comments,=20seven=20deliberate=20silences?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Aggregation barrier for the twelve bug-labelled issues. Four got a comment; seven did not, and the restraint is the substance — each of those already carries an accurate current comment naming the exact missing evidence, so re-stating it would have added noise to a reporter's inbox and bought nothing. #1100 gets the actionable half this issue never had: setting `modelSupportsReasoningSummaries: true` restores the flag Codex gates the whole `reasoning` object on. Verified against the reporter's own provider shape rather than asserted — generating the catalog for a custom openai-chat provider on their BigModel.cn base URL yields `summaries=true` with the key and `false` without it, on identical ladders. The comment also states what the user asserts by setting it, and why we did not simply flip the routed default, which would trade their problem for failures at providers that reject the shape for users who configured nothing. #1024 had a detailed reporter follow-up unanswered since 08-04, and the per-model answer is not uniformly good news. One probe is fixed, one is still blocked on their `TR` config, and one is a disagreement worth having: `mimo-v2.5-free` is deliberately absent from the text-only list because the 2026-08-05 probe found it *accepts* images, with a negative assertion in `tests/provider-registry-parity.test.ts` pinning that. "200 but blind" is a different defect from "rejects images", and adding it would silently replace a real image with a caption. The comment says so and names the capture that would change our mind, rather than quietly agreeing or quietly ignoring it. #1017 and #994 get cross-links to the PRs that block them (#1036, #1068) plus the specific hazard holding each one, so neither reads as abandoned. #994 also gets the question that separates it from a lookalike: the report says "OpenCode models" with no model id, and that 400 text has other causes. No issue closed. #1024 stays open on its `TR` half even though the Zen half shipped — partly fixed is not fixed. Comments 5202766906, 5202780631, 5202769373, 5202771720. --- .../060_wp6_g1_issue_dispositions.md | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) 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 index 63f648305..c457e3a9f 100644 --- 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 @@ -46,3 +46,23 @@ small number of comments that carry genuinely new information. - 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. From 4d70b8285c9a74645b9a5d20e411e1ae36d1f02d Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Thu, 6 Aug 2026 19:34:54 +0900 Subject: [PATCH 08/11] docs(devlog): stack-and-absorb roadmap, and why it became a wait MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second loop of the bug campaign, scoped to author corrected replacements for four contributor PRs and close theirs as absorbed. Three adversarial audit rounds refuted the premise, and the plan now records that instead of the outcome it was written to produce. The decisive finding: I judged "has the author responded to our review" by `updatedAt`, which moves when WE comment and therefore can never show author activity. Comparing last-commit time against review time gives the real picture: #1092 commits 09:38:19Z review 09:09:51Z -> acted, in under 30 minutes #1068 commits 08:52:23Z review 09:13:15Z -> predates the review #1036 commits 08-05 review 09:12:51Z -> no response yet #997 commits 02:51Z review 09:16:02Z -> no response yet Under the wrong reading I was about to close #1092 as absorbed — taking credit for work its author did in direct response to my own request — and to credit #1068 with agreeing to a review it had not yet read. Neither is a process nit; both would have been visible to the contributor. So the plan changes shape. #1092 and #1068 become re-reviews. #1036 and #997 get a stated 72-hour response window with a mandatory head re-check before anything is authored or closed; "has not replied within an hour" is not abandonment, and #997's author was active at 02:51Z. This loop therefore authors no absorbed layer at all, and says so. #1068's re-review carries a finding neither side has: its new test asserts all three DeepSeek ids are in `noVisionModels`, but routing merges the registry list, which holds only the `-free` one. Reproduced with the test's own routeModel config — Pro=false, Flash=false, Flash-free=true — so two of three cases fail. Latent because no check currently runs that suite. Also corrected: the layers were called dependency-ordered when they share no files, so s3/s4 are independent heads off `origin/dev` and the `--update-refs` cascade is reserved for genuine chains; and #978's exclusion no longer claims it is "already correct" when it needs an author-side docs change. Terminal outcome is deferred, not DONE. Shrinking the criteria to match what finished would have hidden exactly the thing worth recording. --- .../_plan/260806_bug_stack_absorb/000_plan.md | 145 ++++++++++++++ .../010_s1_zen_deepseek_replay.md | 169 ++++++++++++++++ .../020_s2_combo_picker_unknown.md | 182 ++++++++++++++++++ .../030_s3_cursor_edit_provenance.md | 152 +++++++++++++++ .../040_s4_usage_log_isolation.md | 83 ++++++++ 5 files changed, 731 insertions(+) create mode 100644 devlog/_plan/260806_bug_stack_absorb/000_plan.md create mode 100644 devlog/_plan/260806_bug_stack_absorb/010_s1_zen_deepseek_replay.md create mode 100644 devlog/_plan/260806_bug_stack_absorb/020_s2_combo_picker_unknown.md create mode 100644 devlog/_plan/260806_bug_stack_absorb/030_s3_cursor_edit_provenance.md create mode 100644 devlog/_plan/260806_bug_stack_absorb/040_s4_usage_log_isolation.md 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..6fdfdfb11 --- /dev/null +++ b/devlog/_plan/260806_bug_stack_absorb/010_s1_zen_deepseek_replay.md @@ -0,0 +1,169 @@ +# 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. 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..288e92630 --- /dev/null +++ b/devlog/_plan/260806_bug_stack_absorb/020_s2_combo_picker_unknown.md @@ -0,0 +1,182 @@ +# 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. 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. From 8221c00bb0be25942c54951a7a3dcbfc4a4ba860 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Thu, 6 Aug 2026 19:57:49 +0900 Subject: [PATCH 09/11] docs(devlog): re-review ledger, and two PRs that stay with their authors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Records what the stack-and-absorb campaign actually produced once live data replaced its assumptions. #1068 merged at 10:45:57Z while I was writing its review, and the merged code is not what I reviewed: `noVisionModels: [...OPENCODE_ZEN_TEXT_ONLY_MODELS, ...DEEPSEEK_THINKING_MODELS]` — the union the review asked for. Running the merged suite against `origin/dev` gives 9 pass / 0 fail, including a new test pinning the six probed text-only models. My 10:53Z comment claiming a failing test was true of the head I had fetched and false of what landed, so it got a public correction eight minutes later. That is the second process error in this unit, and both are now written down rather than quietly fixed. First: judging author activity by `updatedAt`, which moves when WE comment. Second: commenting against a stale fetched head. Each produced a wrong public statement to a contributor. The rules are re-fetch immediately before commenting, and compare last-commit time against review time. #1092's author restored the fail-closed guard within 30 minutes of the review, with a comment keeping `unknown` distinct in debug. Credited explicitly; asked only to split the unrelated `imageInput` scope. #1036 and #997 are deferred, not absorbed. Their last commits (08-05 07:18Z, 08-06 02:51Z) predate our reviews and neither carries the requested change, but roughly two hours have passed and #997's author was active that morning. 050 states a 72-hour response window with a mandatory head re-check before anything is authored or closed. The window exists because this unit demonstrated the alternative: two of four absorb targets were being actively fixed by their authors while we drafted replacements. #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. Zero contributor PRs closed by this loop. --- .../010_s1_zen_deepseek_replay.md | 29 +++++++++++ .../020_s2_combo_picker_unknown.md | 11 +++++ .../050_deferral_record.md | 48 +++++++++++++++++++ 3 files changed, 88 insertions(+) create mode 100644 devlog/_plan/260806_bug_stack_absorb/050_deferral_record.md 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 index 6fdfdfb11..fc34f8cce 100644 --- 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 @@ -167,3 +167,32 @@ If assertion 3 does not go red under that edit it is vacuous and must not ship. 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 index 288e92630..932abd7ad 100644 --- 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 @@ -180,3 +180,14 @@ Two things still worth raising, neither blocking: 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/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. From 77002f142ab8c5716f75c2af23941905fe5ef656 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Thu, 6 Aug 2026 20:22:03 +0900 Subject: [PATCH 10/11] docs(devlog): measure the Windows suite instead of restating the plan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #1059 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) and waited for all four shards: shard 1/4 TIMED OUT at the 15-minute cap — no counts recoverable shard 2/4 8 fail shard 3/4 21 fail shard 4/4 47 fail Grouping the 152 recoverable `(fail)` lines by suite shows they are not spread evenly: 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 About 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 exactly where POSIX assumptions leak on Windows: rename-over-existing, chmod bits, and advisory locking. So the recorded burn-down order ("management/server fixtures first") is superseded by evidence: fix the write substrate first and roughly 78 failures should fall together rather than shard by shard. Shard 1's timeout is split out as its own defect. A shard that cannot finish gives no signal on a quarter of the suite, so no claim is made about whether the total improved against the ~207 estimate — that figure stays labelled unverified rather than contradicted. #1102 gets no new comment: the reporter has not answered the RCA's two questions, and the only fix that helps a direct-spawned app-server is an opt-in "trust loopback peers on a non-loopback bind" — a security boundary change that is the maintainer's call, not an agent's. Recorded as NEEDS_HUMAN and carried to the user. --- .../060_s6_pr_less_issues.md | 73 +++++++++++++++++++ 1 file changed, 73 insertions(+) create mode 100644 devlog/_plan/260806_bug_stack_absorb/060_s6_pr_less_issues.md 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. From e00ce78be477b267998c7f4d4a8b2fa370d46382 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Thu, 6 Aug 2026 21:22:03 +0900 Subject: [PATCH 11/11] docs(devlog): push on top of the contributor's branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two loops circled a problem the campaign 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, blame stays accurate, their PR merges, and attribution is the commit graph rather than a paragraph. No replacement PR, no close, no "absorbed with credit to" prose. #1036 landed that way. Four Agent59353 commits, then ours on top, pushed with a lease pinned to their head after re-verifying the remote. The change is bounded: live-transport records the bare names we actually advertised this request, the event state carries them, and both translate call sites convert only for names in that set. Their conversion logic is byte- unchanged. The stateless fallback passes through, since it has no state to consult and an unconverted call is recoverable while a wrongly converted one edits a file — disclosed in the PR comment as a judgment call open to disagreement. Ablation: restoring the name-only gate gives 21 pass / 1 fail, red on exactly the new collision test. #997 was not ours to push. The lease rejected it as stale: the author landed their own fix mid-flight, and theirs is stronger than what we staged — we pinned getConfigDir(), they assert the resolved usage.jsonl receives the row and the default location does not. Verified instead of overwritten, 10 pass with a 9/1 ablation on their own assertion. That is the third time in this campaign that acting on a stale head produced wasted or wrong work: updatedAt as an activity signal, a "failing test" comment posted eight minutes after #1068 merged with the fix, and a commit built against a head the author had already improved. The lease caught the third; the first two reached a contributor as a wrong public statement. The rule is now written down — re-fetch immediately before preparing a commit, not before pushing. --- .../070_push_on_top.md | 74 +++++++++++++++++++ 1 file changed, 74 insertions(+) create mode 100644 devlog/_plan/260806_bug_stack_absorb/070_push_on_top.md 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).