From 5ec3035467f0f88c1776a989f036619f79bf3feb Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sun, 9 Aug 2026 01:39:13 +0900 Subject: [PATCH] docs(devlog): record WP15, WP16, WP3, WP5 and lane D of the bug campaign MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five units, and the honest summary is that adversarial review changed my answer far more often than it polished it. 023 WP15 — three contributor fixes republished on dev with Co-authored-by preserved. Records that I read #1244's CI as in_progress and built a "watch" disposition on a run that had already concluded failure. 024 WP16 — #1273 is two defects. I designed the second fix three times and review caught the first two losing user data: whole-array reconciliation resurrects a deleted provider's rows, and keying on routedSlug duplicates renamed ones. Shipped defect 1, left defect 2 open with a diagnosis rather than landing a third attempt in a config-persistence path. 025 WP3 — #1185 was red and right; its crash was a Bun EEXIST in a file its one-file diff cannot reach. Also records a workflow snippet I suggested that interpolated an expression straight into shell, while reviewing a security-class change. 026 WP5 — #1244's author answered my defect report with neither fix I proposed, and was right. My stated reason for agreeing was wrong too: tsconfig.json has include:[src], so the typecheck I cited as caller-sweep evidence never read tests/ at all. 027 lane D — closes the catalog sequence, and records six PRs merged to dev without the approval MAINTAINERS.md requires. I logged every workflow-run approval meticulously against its head SHA, which is what made the missing pull-request approval feel handled. Filed as #1306 rather than back-filled. --- .../023_wp15_1244_and_green_five.md | 574 ++++++++++++++++ .../024_wp16_ghost_custom_models.md | 624 ++++++++++++++++++ .../025_wp3_lane_c_ci_workflows.md | 164 +++++ .../260808_bug_campaign/026_wp5_large_solo.md | 94 +++ .../027_wp4_lane_d_close.md | 152 +++++ 5 files changed, 1608 insertions(+) create mode 100644 devlog/_plan/260808_bug_campaign/023_wp15_1244_and_green_five.md create mode 100644 devlog/_plan/260808_bug_campaign/024_wp16_ghost_custom_models.md create mode 100644 devlog/_plan/260808_bug_campaign/025_wp3_lane_c_ci_workflows.md create mode 100644 devlog/_plan/260808_bug_campaign/026_wp5_large_solo.md create mode 100644 devlog/_plan/260808_bug_campaign/027_wp4_lane_d_close.md diff --git a/devlog/_plan/260808_bug_campaign/023_wp15_1244_and_green_five.md b/devlog/_plan/260808_bug_campaign/023_wp15_1244_and_green_five.md new file mode 100644 index 000000000..1dc8953c8 --- /dev/null +++ b/devlog/_plan/260808_bug_campaign/023_wp15_1244_and_green_five.md @@ -0,0 +1,574 @@ +# WP15 — #1244 rebases itself, and five green contributor fixes get republished + +## What changed since WP14 + +WP15 opened as "resolve #1244's 22-hunk conflict by hand". That work no longer +exists. The PR head moved from `b413f8bff` to `15545b3d1` and the author +collapsed the branch onto the current `dev` tip: + +``` +$ git fetch origin pull/1244/head && git merge-base --is-ancestor origin/dev FETCH_HEAD && echo on-dev +on-dev +$ git log --oneline FETCH_HEAD -1 +15545b3d1 fix(codex): preserve routed models in desktop picker +``` + +`mergeStateStatus` is `UNSTABLE` rather than `DIRTY`, `isDraft` is now `false`, +and the 57-file diff no longer defines `mergeCatalogModelsWithNativeRecovery` +locally. The WP14 hypothesis — that resolving 22 hunks across +`src/codex/catalog/sync.ts` and `src/codex/convergence.ts` requires re-deciding +the author's design against the merged #1212 convergence work — is moot. The +author did that re-decision themselves. + +Consequence for the campaign: #1244 needs no maintainer rebase. It needs CI to +finish (`Cross-platform CI` and `Service lifecycle` were both `in_progress`) +and then a normal merge decision. That is a watch, not a work item. + +## The actual WP15 unit + +Five contributor PRs sit one or two commits ahead of a `dev` they are far +behind, and all five still sit in draft because the four-box readiness +checklist is the contributor's own attestation, which I will not tick for them +(`.github/workflows/enforce-pr-target.yml:516`). + +| PR | Author | Head | ahead/behind dev | Cross-platform CI at head | +|----|--------|------|------------------|---------------------------| +| #1189 | luvs01 | `d5242a231` | 2 / 300 | success | +| #1195 | luvs01 | `6eff3f6a5` | 2 / 300 | success | +| #1169 | TyroneXie | `d8968b7e6` | 1 / 335 | success | +| #1187 | luvs01 | `36cffcef6` | 2 / 9 | action_required | +| #1184 | luvs01 | `a2eda3b94` | 1 / 16 | action_required | + +Read that column through the WP1 rule: `gh pr checks` hides `action_required`, +so the source of truth is +`gh api "repos/lidge-jun/opencodex/actions/runs?head_sha=$sha"`, and `status` is +not `conclusion`. #1187 and #1184 are unapproved, not failing. + +All five apply cleanly onto `3ad5bb6bd`: + +``` +PR#1189 APPLIES CLEAN +PR#1187 APPLIES CLEAN +PR#1184 APPLIES CLEAN +PR#1195 APPLIES CLEAN +PR#1169 APPLIES CLEAN +``` + +"Applies clean" is textual, not semantic. The three deep-behind branches +(#1189, #1195, #1169 — 300+ commits) are exactly the case where a clean apply +can still be wrong, because `dev` may have moved the surrounding contract +without touching the same lines. Each gets a contract check below, and a clean +apply alone is not accepted as evidence for any of them. + +## Diff-level plan + +Republish protocol is `003_republish_protocol.md` unchanged: fresh worktree from +`origin/dev`, apply the author's net diff, one commit authored by the maintainer +with a trailer preserving the contributor, a PR body naming the source PR and +mentioning the author, then merge on green. + +### WP15-A — #1187 and #1184: approve at head, merge in place + +> **Corrected by audit B2 — see the audit-fold section below. The heading is +> wrong: these cannot be merged in place. Read WP15-A′.** + +Nine and sixteen commits behind, both under the gate's 10-commit tolerance after +a rebase, both tiny. No republish is warranted; the correct action is to unblock +CI. + +1. Re-read each PR head immediately before acting; log `run.head_sha` and + `pr.headRefOid` as separate columns in `.tmp/ocx_approval_ledger.tsv` with + `MATCH`/`SKIP`. A `SKIP` means the author pushed inside the window and the + approval would target a stale commit — abort that row. +2. Approve the `action_required` Cross-platform CI run for a `MATCH` row. +3. Await `conclusion == "success"`. A `failure` gets diagnosed, never guessed. + +Acceptance: both PRs have a `success` Cross-platform CI at the exact head the PR +points at, recorded with both SHAs. + +What this does not do: it does not make them mergeable by policy, because the +contributor checklist stays theirs. Approval only removes the gate that stops +them from proving box 1. + +### WP15-B — #1189: republish as `codex/260808-1189-history-stream-ingest` + +Net diff: `src/routing/history/indexer.ts`, +`tests/request-history-index.test.ts`. + +It replaces `readCompleteTail` — which allocated `size - indexedOffset` in one +shot — with a 64 KiB chunked reader that assembles records across chunk +boundaries and omits complete records above a 1 MiB projection bound +(`REQUEST_HISTORY_READ_CHUNK_BYTES`, `REQUEST_HISTORY_MAX_RECORD_BYTES`). It +deletes `ingestText` and folds line handling into `ingestSourceTail`. + +Contract checks, required because the branch is 300 behind: + +- `rg 'ingestText|readCompleteTail' src tests` on `dev` must show no caller + outside `indexer.ts`; a surviving caller means the deletion breaks it. +- `insert.finalize()` must still run unconditionally in `finally`. The Windows + file-lock note in the deleted code is load-bearing and CI runs Windows. +- Offset accounting: `nextOffset` may only advance past a `\n`, so a torn final + record is re-read rather than skipped. + +Verification: `bun test tests/request-history-index.test.ts`, +`bun run typecheck`, and an ablation reverting the 1 MiB bound that shows the +oversized-record test failing. A passing suite without the ablation is not +accepted. + +### WP15-C — #1195: republish as `codex/260808-1195-unbound-quota-unknown` + +Net diff: `src/router.ts`, +`src/server/management/routing-profile-routes.ts`, +`tests/quota-scoring.test.ts`, English routing docs. + +It deletes the same ~20-line block from both files: a policy candidate no longer +takes `codexAccountId`/`codexAccountPlan` from +`getEffectiveActiveCodexAccountId` when the Codex provider is in pool mode, and +no longer takes `accountRef` from `getAccountSet("anthropic")`. The rationale is +a real ordering defect — policy evaluation runs before Pool/Direct identity, +thread affinity, and Anthropic session affinity resolve, so a candidate can be +scored with account A's quota and executed on account B. + +Contract checks: + +- The two blocks must still be identical on `dev`. If `dev` already changed + either one, the delete is no longer symmetric and the PR is stale. +- `rg 'getEffectiveActiveCodexAccountId|getAccountSet'` must come back empty for + both files, and the now-unused imports must be gone or `typecheck` fails. +- Live-vs-dry-run parity: both paths must emit the same evidence shape, which is + the property `tests/quota-scoring.test.ts` is extended to hold. + +Verification: quota, policy-execution, routing-profile, and explainability +suites; `bun run typecheck`; an ablation restoring one block only, to show the +parity test fails asymmetrically. + +### WP15-D — #1169: republish as `codex/260808-1169-shim-routing-warning` + +Net diff: `src/cli/codex-shim-readiness.ts` (new), `src/cli/index.ts`, +`tests/codex-shim-readiness.test.ts`, English and zh-CN lifecycle docs. + +`ocx codex-shim install` reports clean success even when it cannot prove Codex +routes through OpenCodex. The change downgrades that to a warning for an +external `model_provider`, a user-owned local or remote gateway, or +unverifiable routing, and warns when proxy variables exist only in the current +process while `config.proxy` is unset. Advisory only: same exit code, and the +shim still fail-open execs the real launcher. + +Highest risk of the three at 335 commits behind, and it touches +`src/cli/index.ts`, which this campaign already modified. Contract checks: + +- The `codex-shim install` call site in `src/cli/index.ts` must still have the + shape the patch expects; confirm by reading the applied hunk rather than + trusting the apply. +- Privacy is the blocking property: no proxy URL, token, or account identifier + may reach stdout. `bun run privacy:scan` plus the test's own assertion inside + isolated `CODEX_HOME`/`OPENCODEX_HOME` directories. +- The exit code must be unchanged on the warning path. Assert it, because + "advisory only" is the whole safety argument. + +Verification: `bun test tests/codex-shim-readiness.test.ts`, +`bun run typecheck`, `bun run privacy:scan`, and a real CLI install in a temp +home showing the warning text with no secret in it. + +### WP15-E — #1244 watch + +> **Corrected by audit B1. #1244's Cross-platform CI at `15545b3d1` is +> `failure`, not `in_progress`. Read WP15-E′.** + +No code work. Poll the two `in_progress` runs at `15545b3d1`; merge on +`success`, diagnose and comment on `failure`. If the head moves again, re-read +it before acting. Record the outcome either way. + +## Acceptance criteria + +1. #1187 and #1184 have `success` Cross-platform CI at their exact current head, + both SHAs logged and matched in `.tmp/ocx_approval_ledger.tsv`. +2. Three new PRs exist for #1189, #1195, #1169, each with a `Co-authored-by` + trailer naming the original author, each mentioning them, each filling all + three PR-template sections. +3. Each republish carries a fresh focused-test result and an ablation that fails + without the fix. +4. `bun run typecheck` clean on each republished branch; `privacy:scan` clean on + #1169's. +5. #1244's CI outcome at `15545b3d1` is recorded with a disposition. +6. No contributor readiness checkbox is ticked by me anywhere. + +## Faults to avoid, restated because I have committed each one + +- Merging without a real green (#1202, WP1). +- Selecting by branch name instead of head SHA; the ledger caught this once. +- Ticking a contributor's readiness box. Done once, reverted. +- Claiming a root cause from plausible commit messages without + `git merge-base --is-ancestor` (#1178). +- Trusting `gh pr checks` to surface `action_required`. It does not. +- **Reading a run's `status` and stopping there.** I recorded #1244 as + `in_progress` and built a "watch" around it. By the time the plan was + audited the run had concluded `failure`, so the plan shipped a wrong + disposition for the single largest PR in it. Re-read `conclusion` at the + moment of the decision, not at the moment of the survey. + +--- + +# Audit fold — six blockers, all accepted + +A `gpt-5.6-terra` reviewer audited the plan above against live GitHub state and +returned `VERDICT: fail` with B1–B6. Every one is accepted without rebuttal. +The corrections below supersede the corresponding sections. + +## B1 — #1244 is failing CI, and the failure is in the #1212 seam + +This is the blocker that matters. The survey above recorded two runs as +`in_progress`; the reviewer read the conclusion: + +``` +$ gh api "repos/lidge-jun/opencodex/actions/runs?head_sha=15545b3d1..." \ + -q '.workflow_runs[]|[.id,.name,.status,.conclusion]|@tsv' +31256061011 Issue quality tests completed success +31256061013 Service lifecycle completed success +31256062356 Enforce PR target branch completed success +31256062366 PR Labeler completed success +31256063542 React Doctor completed success +31256063557 Cross-platform CI completed failure +``` + +The failing shard throws `TypeError: suppressedBareNativeSlugs.has` at +`src/codex/catalog/sync.ts:427`, driven by `tests/codex-v2-gate.test.ts:1211`. +The new required input is destructured at `sync.ts:385` **without a default**, +so any caller on `dev` that does not pass it gets `undefined` and dies on +`.has`. + +That is precisely the semantic conflict WP14 suspected and this plan dismissed. +The author's textual rebase merged cleanly *and* broke the contract, which is +the exact failure mode the plan claimed to guard against for the three +republishes while waiving it for #1244. A clean `merge-base --is-ancestor` said +nothing about whether every observed-state caller was updated. + +### WP15-E′ — #1244: report the failure, do not merge + +1. Merge is prohibited at `15545b3d1` and at any later head until a + Cross-platform CI run at that exact head concludes `success`. +2. Comment on #1244 with the run id, the file:line, the failing test, and the + missing-default diagnosis. State the fix shape (either default the + destructured input or update every caller) without asserting which one the + author should pick — the callers are their design. +3. Re-audit the #1212-adjacent observed-state callers + (`buildCatalogEntriesFromObservedState`, + `mergeCatalogEntriesFromObservedState`, `shouldUpgradeToUpstreamEntry`) + before any future merge decision, because a single missing default proves the + caller sweep was incomplete. +4. Disposition: **awaiting author**, with a concrete defect. Not a watch. + +## B2 — approval is not merge-readiness + +`.github/workflows/enforce-pr-target.yml:820` sets `mustDraft` while a +contributor checklist is incomplete, and `:1027`–`:1042` preserve draft status +until every box is ticked. So approving CI does **not** make #1187 or #1184 +mergeable, and WP15-A's heading claimed an outcome the gate forbids. + +### WP15-A′ — approve CI only; await contributor readiness + +Steps 1–3 of WP15-A stand unchanged (SHA-matched ledger, approve `MATCH` rows, +await `conclusion`). What changes is the claim: + +- Acceptance is narrowed to: a `success` Cross-platform CI exists at the exact + current head, both SHAs logged and matched. +- Terminal disposition is **awaiting author**, not merged. The four boxes are + the contributor's attestation and stay theirs. +- The PR comment must say what approval did and did not do, so the author is not + left thinking the maintainer unblocked a merge. + +## B3 — focused tests are below the mandatory gate + +`003_republish_protocol.md:286` requires the full suite when a change touches +routing, adapters, config, or the server; `AGENTS.md:228` requires it +independently. All three republishes touch routing or the CLI, so the +focused-test acceptance at `:110`, `:138`, and `:166` was under-specified. + +**Correction:** `bun run test` (full suite) is required on each republished +branch before its PR is opened, and again after any base movement that forces a +re-apply. The focused test and the ablation stay — they are additional +evidence, not a substitute. + +## B4 — the disposable-worktree boundary was implied, not enforced + +The plan says "fresh worktree" at `:65`, but the protocol it delegates to runs +`git switch -c`, `cherry-pick`, and `commit` in whatever checkout executes it +(`003_republish_protocol.md:39`–`:48`). Run literally in this checkout, that +would touch the user's dirty files. + +**Correction, binding for every remaining work-phase:** + +- Every republish runs in `git worktree add --detach "$(mktemp -d)/"`. +- `git switch`, `git commit`, and any write inside + `/Users/jun/.codex/worktrees/1a75/opencodex` are prohibited except for + `devlog/` documents. +- `scripts/generate-jawcode-metadata.ts`, + `src/generated/jawcode-model-metadata.ts`, + `tests/jawcode-metadata-sync.test.ts`, and `scripts/jawcode-models.json` are + the user's uncommitted work and are never staged, stashed, or reverted. +- Test and ledger artifacts live inside the disposable worktree; it is removed + when the phase closes. + +## B5 — #1195's parity claim is not covered by its own test + +The PR only changes execution-path assertions in `tests/quota-scoring.test.ts` +(runtime assertion at `:198`–`:209`). The management dry-run test at +`tests/routing-profile.test.ts:451`–`:477` covers a candidate with an +explicitly supplied `codexAccountId` — never an *unbound* candidate while a +pool account is active. That is the whole defect, and no test would catch its +return. + +**Correction:** the republished #1195 must add paired regressions — runtime and +management dry-run — asserting that an unbound Codex candidate and an unbound +Anthropic candidate both keep `quota.known === false` while a global active +account exists, and that explicit account-qualified evidence stays known. The +ablation restores one of the two deleted blocks and must fail the new pair. +Without that pair the PR body may not claim parity. + +## B6 — the inventory is stale; two bugs opened after the sweep + +The campaign's own objective is a terminal disposition for *every* open bug +issue and PR. Two were opened after the inventory and appear nowhere in it: + +| # | State | Labels | Note | +|---|-------|--------|------| +| #1273 | OPEN | bug | ghost custom models survive provider removal; full-config PUT resurrects deleted `customModels` | +| #1278 | OPEN | bug, platform, install | Windows: transient PowerShell console window on identity lookup; distinct from #1236 | +| #1279 | OPEN | — | non-draft fix PR for #1278 (`43b6b824c`, wade19990814-hue) | +| #1283 | OPEN | bug, gui | grok's limit is weekly not monthly (opened 12:06Z, *after* the re-audit's own B6 list) | + +**Correction:** a live inventory resweep runs immediately before execution, and +#1273, #1278, #1279, and #1283 are added to the disposition matrix. WP15 does not +close them silently; if they cannot be dispositioned inside this phase they +become the next work-phase with that stated explicitly. + +### B6, second round — a fixed list cannot satisfy a moving inventory + +The re-audit reopened B6 after I had already folded it. Between the first audit +and the second, #1283 opened. My correction had enumerated three numbers, so it +would have passed its own criterion while dropping a bug that existed before the +phase closed. Enumerating is the wrong shape for this criterion. + +**Correction of the correction:** criterion 6 is no longer a list. It requires a +*recorded live resweep, executed last*, whose output is pasted into the closing +document, covering every then-open `bug`-labeled issue and every associated fix +PR. A number opened after that resweep is out of scope by timestamp, and the +resweep output is what proves the boundary rather than my own recollection. + +### B6, third round — an approximate timestamp and a missing PR side + +The re-audit rejected my resweep too, on two counts, both fair. I had written the +boundary as `2026-08-08T12:0xZ` — an approximation is not a boundary, and it +cannot decide whether a given issue was in scope. And I had run only +`gh issue list`, so the PR half of "every open bug issue *and* its fix PRs" was +unproven. + +**Resweep, exact boundary and deterministic issue → open-PR mapping.** +`RESWEEP_AT=2026-08-08T12:21:41Z`, derived per issue from the cross-referenced +timeline events, open pull requests only: + +| Issue | Open fix PR(s) | Disposition | +|-------|----------------|-------------| +| #1283 | none | → WP16 (new, `bug`+`gui`, opened 12:06:03Z) | +| #1278 | #1279 | → WP16 (non-draft PR already open) | +| #1273 | none | → WP16 (new) | +| #1236 | #1268, #1279 | tracking; #1278 is explicitly distinct from it | +| #1230 | #1269 | stays open — the `handleEnsure` gap at `src/cli/index.ts:441` is unfixed | +| #1229 | none | tracking | +| #1222 | none | tracking | +| #1213 | none | tracking | +| #1196 | #1270 | awaiting contributor (two blockers commented) | +| #1193 | #1205 | rerun needed — the run ended `cancelled`, not `failure` | +| #1190 | #1210 | CI green, awaiting contributor checklist | +| #1162 | none | tracking | +| #1145 | none | tracking | +| #1128 | none | tracking, reporter capture requested | +| #1059 | #1272 | CI green, awaiting contributor checklist | +| #1024 | none | tracking, reporter capture requested | +| #904, #796, #418, #417, #241, #92 | none | long-lived tracking | + +That is 22 open `bug` issues, each with either a disposition already recorded in +this unit or an explicit hand-off. + +One row needs its cross-reference stated precisely rather than assumed. #241's +timeline lists only closed PRs (#298, #999, #1056, #1147, #1150), so the +mechanical mapping correctly reports no open fix PR. #1244 does not link #241. + +The chain has two links of different strength, and my first attempt at this +paragraph flattened both into "exists only in prose", which the audit +corrected: **#241 → #1056 is a real timeline cross-reference; #1056 → #1244 is +inferred solely from #1244's `Supersedes #1056` body text.** Only the second +hop is prose. I had also asserted a direct #1244 → #241 link earlier in this +campaign without reading the timeline; that was wrong, and this is the +corrected form. + +The scope boundary is the timestamp above. Anything opened after +`12:21:41Z` is out of this phase by construction, and that is provable from the +recorded value rather than from my recollection. + +## Revised acceptance criteria for WP15 + +1. #1187 and #1184: `success` Cross-platform CI at the exact current head, both + SHAs logged and matched. Disposition recorded as **awaiting author**, with no + merge claim and no box ticked by me. +2. Three new PRs for #1189, #1195, #1169, each with a `Co-authored-by` trailer + naming the original author, each mentioning them, each filling all three + template sections. +3. Each republish: full `bun run test` green, plus a focused test, plus an + ablation that fails without the fix. #1195 additionally carries the paired + parity regressions from B5. +4. `bun run typecheck` clean on each branch; `privacy:scan` clean on #1169's. +5. #1244: the CI `failure` at `15545b3d1` is reported on the PR with run id, + file:line, and the missing-default diagnosis. Merge prohibited. +6. #1273, #1278, #1279 appear in the disposition matrix with either a terminal + disposition or an explicit hand-off to the next work-phase. + *(Superseded by B6 round two: a recorded live resweep, run last, must be + pasted in, and every then-open bug issue must have a disposition or a named + hand-off. No fixed list.)* +7. All code work happened in `mktemp -d` worktrees; the user's four dirty files + are untouched (`git status --short` proves it). + +--- + +# Execution record + +## The audit moved the base out from under the work + +Round four caught something none of the earlier rounds could: while I was +folding blockers, `origin/dev` moved from `3ad5bb6bd` to `f5147cbc8`. Every +test result on this page — three full suites, two ablations, three typechecks — +was measured against a base that no longer existed. The reviewer's instruction +was to treat all of it as stale and redo it after rebasing, which is correct and +which I did. + +The cost of skipping that step would have been three PRs whose "Verification" +sections cited numbers from a base the reviewer could not reproduce. That is the +same class of fault as merging #1202 without a real green, just better hidden. + +Re-verified on `f5147cbc8`: + +| Branch | Full suite | Focused | Extra | +|--------|-----------|---------|-------| +| `codex/260808-1189-history-stream-ingest` | 9991 pass / 7 skip / 0 fail, 625 files | 20/20 | ablation 19/1 then restored 20/0 | +| `codex/260808-1195-unbound-quota-unknown` | 9992 pass / 7 skip / 0 fail, 625 files | 31/31 | ablation 27/4 at identical scope | +| `codex/260808-1169-shim-routing-warning` | 9994 pass / 7 skip / 0 fail, 626 files | 5/5 | `privacy:scan` passed | + +`bun run typecheck` clean on all three. The prepush hook then ran the full suite +a second time per branch and passed each one, which is why the pushes took +roughly six minutes apiece. + +## Published + +| New PR | Republishes | Author | Head | +|--------|-------------|--------|------| +| #1287 | #1189 | luvs01 | `02ec799fe` | +| #1288 | #1195 | luvs01 | `3fc962f2c` | +| #1289 | #1169 | TyroneXie | `eac814346` | + +All three opened non-draft against `dev`, `MERGEABLE`, and `Enforce PR target +branch` green on each. Trailer evidence: + +``` +$ git log --format='%h %s%n %(trailers:key=Co-authored-by,valueonly)' origin/dev..HEAD +02ec799fe fix(history): stream request-history index ingestion (#1189) + luvs01 <27862058+luvs01@users.noreply.github.com> +3fc962f2c test(routing): prove the management dry-run leaves unbound candidates unknown + +0c745be36 fix(routing): keep unbound account quota unknown (#1195) + luvs01 <27862058+luvs01@users.noreply.github.com> +eac814346 fix(codex): warn when codex-shim install cannot prove routing (#1169) + TyroneXie <328347833@qq.com> +``` + +The blank trailer line on `3fc962f2c` is deliberate and is the point of B5's +attribution requirement: those two dry-run tests are mine, not luvs01's, so they +are a separate commit with no co-author trailer and an explicit paragraph in +#1288's body saying so. Folding them into the contributor's commit would have +attributed my code to them; leaving them out would have shipped an unproven +parity claim. + +## Actions taken on existing PRs + +- **#1187, #1184** — approved the `action_required` Cross-platform CI at + SHA-matched heads (`36cffcef6`, `a2eda3b94`; both `MATCH` in + `.tmp/ocx_approval_ledger.tsv`). Commented on both that approval unblocks CI + and nothing else, and that the four boxes stay theirs. Disposition: **awaiting + author**. +- **#1244** — commented with run `31256063557`, the `TypeError` at + `src/codex/catalog/sync.ts:427`, the triggering test at + `tests/codex-v2-gate.test.ts:1211`, and the missing default at `sync.ts:385`. + Named both fix shapes without choosing for them, and flagged that the same + caller-sweep gap may exist for the other inputs added in that commit. Merge + held. Disposition: **awaiting author, with a concrete defect**. +- **#1189, #1195, #1169** — commented on each that it was republished, by which + PR, with what verification, and that the author may take it back if they + prefer to drive it themselves. + +## What WP15 did not do + +#1283, #1278/#1279, and #1273 are dispositioned as hand-offs to WP16, not as +closed. Naming them here is the honest form of that; the resweep table above is +what makes the boundary checkable rather than asserted. + +## Merged + +All three landed on `dev`, and the `Co-authored-by` trailer survived each +squash — which is the property that matters, because the squash is where +contributor credit usually gets lost: + +``` +57ea8df47 fix(routing): keep unbound account quota unknown (#1195) (#1288) | luvs01 +5aa197112 fix(codex): warn when codex-shim install cannot prove routing (#1169) (#1289) | TyroneXie +2cb8eddd4 fix(history): stream request-history index ingestion (#1189) (#1287) | luvs01 +``` + +#1189, #1195, and #1169 were closed as superseded, each with a comment naming +the landed SHA and confirming the credit. #1195's closing comment states +separately that the maintainer test commit is mine and their fix commit is +unmodified. + +### #1288 needed two reruns, and the reason is worth recording + +Cross-platform CI at `3fc962f2c` came back `cancelled` twice. A `cancelled` is +not a `failure` — the four-state rule says rerun — but twice in a row is a +signal rather than noise, so I read the job log instead of firing a third +rerun blind. `test 3/4` hung at `tests/cli-restart-health.test.ts` and was +killed by the runner after ~14 minutes. + +The check that made this safe was comparing against `dev` itself: + +``` +31259885820 dev all-shards-ok +31259450263 dev cancelled test 3/4=cancelled +31259447622 dev cancelled test 1..4/4=cancelled +31256617398 dev success all-shards-ok +``` + +The same shard cancels on `dev` with no PR involved, so it is runner flake, not +something #1288 introduced. `rerun-failed-jobs` then returned all four shards +green. Had I not checked `dev`, "rerun until green" would have been +indistinguishable from hiding a real defect — which is exactly the failure mode +the four-state rule exists to prevent. + +## Final resweep — `FINAL_RESWEEP_AT=2026-08-08T13:48:43Z` + +Run last, as criterion 6 requires. Twenty-two open `bug` issues, unchanged in +membership from the 12:21:41Z sweep, so nothing opened during execution. +#1283, #1278/#1279, and #1273 remain the undispositioned three and pass to +WP16. + +## Acceptance, checked + +1. #1187, #1184 — Cross-platform CI `success` at `36cffcef6` and `a2eda3b94`, + both `MATCH` in the ledger. Awaiting author. **Met.** +2. Three new PRs with trailers and mentions, all template sections filled. + **Met.** +3. Full suite + focused test + ablation on each; #1195 carries the B5 parity + pair. **Met.** +4. `typecheck` clean on all three; `privacy:scan` clean on #1289's. **Met.** +5. #1244's `failure` reported with run id, file:line, and the missing-default + diagnosis; merge held. **Met.** +6. Final resweep recorded above with a disposition or hand-off per row. + **Met.** +7. All code work in `mktemp -d` worktrees; `git status --short` still shows + exactly the user's four untouched files. **Met.** diff --git a/devlog/_plan/260808_bug_campaign/024_wp16_ghost_custom_models.md b/devlog/_plan/260808_bug_campaign/024_wp16_ghost_custom_models.md new file mode 100644 index 000000000..ab9083448 --- /dev/null +++ b/devlog/_plan/260808_bug_campaign/024_wp16_ghost_custom_models.md @@ -0,0 +1,624 @@ +# WP16 — ghost custom models (#1273), and closing out the campaign's tail + +## Scope + +Three items survived WP15 as hand-offs. One is already done, one is a +disposition, and one is a real two-part defect that needs a patch. + +| Item | State entering WP16 | Outcome | +|------|--------------------|---------| +| #1283 grok weekly limit | OPEN, but fixed on `dev` by #1290 | closed with the landed SHA | +| #1278 / #1279 Windows console flash | PR open, CI unapproved | CI approved at SHA-matched head | +| #1273 ghost custom models | OPEN, no fix PR | **the work of this phase** | + +### #1283 — already fixed, closed manually + +`5222f354a fix(quota): prefer Grok weekly credits for xAI dashboard (#1290)` +landed on `dev` at 13:35Z and does exactly what the report asked: prefer +`GET /v1/billing?format=credits` and map SuperGrok's weekly window to +`weeklyPercent`/`weeklyResetAt`, with the legacy 30-day endpoint demoted to a +fallback (`src/providers/quota.ts:49`, `:592`). + +The PR body said `Closes #1283`, but PRs here target `dev` and GitHub only +auto-closes on merges into the default branch, so the issue sat open with its +fix already shipped. Closed by hand with the SHA. This is a recurring trap in +this repository and it is why `AGENTS.md` tells contributors to close linked +issues manually. + +One loose thread worth recording: #1290's own description noted a pre-existing +`tests/translator-budget.test.ts` failure on the tip and pushed with +`--no-verify`. Checked on a clean worktree at `dev`: **13 pass / 0 fail**. The +failure was local to that environment, not on `dev`, so nothing to chase. + +### #1278 / #1279 — approved, awaiting author + +#1279 (`fix(windows): eliminate console windows from proxy-internal identity & +process lookups`, wade19990814-hue, head `43b6b824c`) is non-draft and +`MERGEABLE`, touching seven Windows source files and four test files. Its +Cross-platform CI sat at `action_required`, invisible to `gh pr checks`. + +Approved at a SHA-matched head (`43b6b824c` == run head, `MATCH` in +`.tmp/ocx_approval_ledger.tsv`). Disposition is **awaiting CI, then review** — +the change is Windows-specific and this campaign has no Windows host, so CI is +the only evidence available and the review will have to lean on it. + +## #1273 — the actual defect + +The report describes two defects. Both reproduce in the source; neither is a +false positive. + +### Defect 1 — provider removal orphans `config.customModels` + +Both removal paths delete only the provider record: + +- `src/cli/provider.ts:304` — `delete config.providers[name]; validateAndSave(config);` +- `src/server/management/provider-routes.ts:617` — `delete config.providers[name];` + followed by `setProviderContextCap`, `save`, `reconcileLiveStateStores`, + `clearModelCache(name)`, `convergeCodexCatalog()`. + +Note what the management path *does* clean up: context caps, the model cache, +and the catalog. It walks right past `config.customModels`. So a custom model +for the removed provider stays in the config, keeps appearing in `/api/models`, +and keeps being emitted into the Codex catalog — a row pointing at a provider +that no longer exists. + +### Defect 2 — a stale in-memory config wins a whole-document write + +`saveConfigPreservingClaudeCode` (`src/config.ts:2710`) takes one authoritative +pre-write read of the on-disk config and uses it for exactly two reconciliations: + +- `claudeCode` (`:2716`–`:2726`): if disk changed and we did not, adopt disk. +- the live server binding (`:2731`–`:2739`): port/hostname come from disk. + +`customModels` gets neither. `projectCustomModelCatalogMigration` +(`src/codex/custom-model-catalog-migration.ts`) *does* consult the persisted +config, but only to project the `customModelCatalogMigration` ownership marker — +it reads `customModels` to classify legacy slugs and never writes the array +back. The candidate's array passes through untouched into +`persistConfigUnlocked`. + +So the reporter's step 4 is exactly right: a `PUT /api/shadow-call-settings` +from a process whose config predates a CLI deletion re-persists the whole stale +document, and the deleted rows return. Their diff even shows the cooperating +save path working correctly (generation bumped, catalog mtime matching) — the +write was well-formed, it just wrote the wrong document. + +**The asymmetry is the bug.** Two fields already get last-writer-wins protection +because they are known to be mutated by other processes. `customModels` is +mutated by `ocx models remove` from a different process and got no such +treatment. + +## Diff-level plan + +### WP16-A — `src/config.ts`: reconcile `customModels` against disk + +Add a third reconciliation next to the `claudeCode` block, using the same +already-taken `onDisk` read (`:2715`) — no second read, since the comment there +correctly warns that a second read could observe different bytes. + +Shape, mirroring the `claudeCode` baseline logic: + +- Track a `customModelsBaseline` per config object, set when the config is + loaded, exactly as `claudeCodeBaseline` is. +- On write: if the on-disk array differs from the baseline **and** the + in-memory array equals the baseline, adopt the on-disk array. That is + "someone else changed it and we did not", which is precisely the reporter's + scenario. +- If both changed, the in-memory value wins and we do not silently merge. A + merge would invent an intent neither writer expressed; last-writer-wins on a + genuine concurrent edit is the same rule `claudeCode` already uses. + +Acceptance: a test that loads a config, deletes a custom model on disk out of +band, then performs an unrelated `PUT`-shaped save from the stale object, and +asserts the deleted row does not return. + +### WP16-B — both removal paths drop the provider's custom models + +`src/cli/provider.ts:304` and `src/server/management/provider-routes.ts:617` +each filter `config.customModels` by `provider !== name` before saving. The +management path already clears the model cache and reconverges the catalog, so +the ghost disappears from `/api/models` and the catalog in the same write. + +**Open question, answered before writing the patch.** Does anything rely on a +`customModels` row outliving its provider — re-adding the provider and +expecting its models back, for instance? If so, deleting would be wrong and the +fix would belong at read time. + +The answer is in provider *rename*. `rewriteProviderReferences` +(`src/providers/provider-id-rewrite.ts:34`) explicitly rewrites +`customModels[].provider` alongside combo targets and Claude tier maps +(`:97`–`:101`). So this array is already designed to track the provider +lifecycle; rename follows it and remove simply does not. No test anywhere +expects rows to survive removal and be restored on re-add. + +That makes deletion the consistent fix rather than a judgement call, and it +reframes defect 1: not a missing feature, but a lifecycle hook that one of two +sibling operations forgot. Consumers confirm the same shape — +`src/codex/catalog/provider-fetch.ts` emits every row into the catalog keyed by +`routedSlug`, and `src/server/management/model-rows.ts` lists every row in the +dashboard, neither checking that the provider still exists. + +### WP16-C — verification + +- Focused: new tests in the config and provider-removal suites. +- Ablation: revert each half independently; the matching test must fail and the + other must not. Two defects, two independent proofs. +- Full `bun run test`, `bun run typecheck`, `bun run privacy:scan`. +- Republish protocol: `mktemp -d` worktree, no writes in the dirty checkout + outside `devlog/`. + +This is a maintainer-authored fix with no contributor PR to preserve, so there +is no `Co-authored-by` trailer. The PR body credits the reporter for a +reproduction that included a before/after config diff and the evidence that the +cooperating save path itself was healthy — that is what made the second defect +findable rather than a vague "settings sometimes revert". + +## Acceptance criteria + +1. #1283 closed with the landed SHA and an explanation of the manual close. +2. #1279's CI approved at a SHA-matched head, logged, disposition recorded. +3. Both #1273 defects fixed, each with its own regression test and its own + ablation. +4. Full suite, typecheck, and privacy scan green on the rebased branch. +5. PR opened against `dev` with all three template sections and the reporter + credited. +6. The consumer question in WP16-B answered with file:line evidence before the + patch is written. + +--- + +# Audit fold — five blockers, all accepted + +A `gpt-5.6-terra` reviewer returned `VERDICT: fail` on the plan above with +B1–B5. Every one is accepted. B3 is the important one: it finds a case where my +proposed rule loses user data, which is worse than the bug it was written to +fix. + +## B3 — "memory wins if both changed" resurrects a deleted provider's model + +My rule was whole-array last-writer-wins: adopt disk when disk changed and +memory did not, otherwise keep memory. The reviewer supplied the case that +breaks it. + +Disk deletes provider **P** (and, after WP16-B, P's custom models). Meanwhile +the in-memory process independently edits an unrelated custom model **Q** — a +legitimate edit through `/api/models`. Now *both* arrays differ from the +baseline, so my rule keeps memory wholesale, and P's custom model comes back. +The exact ghost row #1273 is about, reintroduced by the fix for #1273. + +A whole-array comparison cannot distinguish "I edited Q" from "I am asserting +the entire array including P". The array is a keyed collection and has to be +reconciled as one: + +- Reconcile per row, keyed by `routedSlug(provider, modelId)`, three-way + against the baseline: a row deleted on disk and untouched in memory stays + deleted; a row edited in memory is kept; a row added on either side is kept. +- Then prune any surviving row whose provider is absent from the config being + written. That prune is the lifecycle invariant from WP16-B applied at the + write boundary, so it holds no matter which path produced the array. + +Required regression: disk deletes P while memory edits Q, then an unrelated +save. Q keeps its edit, P's row does not return. + +## B1 — the same staleness applies to `providers`, not just `customModels` + +I scoped the fix to `customModels` because that is what the issue reported. But +`saveConfigPreservingClaudeCode` writes the *whole document*, so the identical +stale-write path resurrects `config.providers[P]` itself. Fixing only the models +leaves a deleted provider coming back, which then makes its models legitimate +again — the ghost returns by a different door. + +**Correction:** the acceptance test asserts both. After an external provider +deletion and an unrelated long-lived-server save, `config.providers[P]` **and** +P's custom models must both stay absent. If whole-array reconciliation cannot +deliver that, the answer is provider-aware reconciliation or a field-scoped +persistence path, not a narrower test. + +## B2 — I described the baseline mechanism wrongly + +I wrote that the baseline is "set when the config is loaded, exactly as +`claudeCodeBaseline` is". That is not what `claudeCodeBaseline` does. It is a +`WeakMap` armed **explicitly by `startServer`** (`src/config.ts:2497`, +`src/server/index.ts:485`), and the comment there says arming is eager on +purpose because lazy arming would lose the hand edit the guard exists to +protect. + +The consequence matters: a CLI process that loads a config and saves it is +**not armed**, so a guard modelled on this would be silently inert there. That +is defensible for `claudeCode`, whose contested writer is the long-lived +server, but it must be a stated policy rather than an accident. + +**Correction:** the plan must name every writer path, state whether it is +armed, and define the unarmed behaviour explicitly. My current position: an +unarmed config has no baseline, so it cannot claim "I did not change this" — +the safe default there is the provider-absence prune from B3, which needs no +baseline at all. That is why the prune is not optional. + +## B4 — deletion is consistent, but the marker needs its own test + +The reviewer confirmed WP16-B's premise: dependent combos are already rejected +before provider deletion, rename already rewrites `customModels[].provider`, +and `src/server/management/model-rows.ts:55` renders custom rows without +checking that the provider exists. So deletion matches existing lifecycle +behaviour. + +**Correction:** add a regression that deletion removes the visible row *without +corrupting* `legacyOwnedSlugs` in the `customModelCatalogMigration` marker. That +marker grants one-time ownership of pre-marker rows; if deletion silently +rewrites it, an older binary's view of ownership changes, and the migration +file's own comment warns against exactly that. + +## B5 — do not imply #1290's CI was green + +I recorded #1290 as landed without qualifying its CI. Checked: the +Cross-platform run at its head `0fe140f91` concluded **`cancelled`**, not +`success`. + +What actually validates the fix is `dev` afterwards. `dev` at `5222f354a` also +shows `cancelled`, and the first `success` on `dev` after it is `57ea8df47` — +the #1288 merge, which contains #1290's change as an ancestor. So the Grok +weekly fix *is* covered by a green `dev` run, but by inheritance, one merge +later, and not at its own head. + +That distinction is worth stating rather than smoothing over: "it's on `dev` +and `dev` is green" is a weaker claim than "its own CI passed", and the run +ids are what let a reader tell which one they are being given. The same +`test 3/4` flake recorded in WP15 is the likely cause of both cancellations. + +## Revised acceptance criteria + +1. #1283 closed with the landed SHA, the manual-close reason, **and** the + honest CI provenance from B5. +2. #1279's CI approved at a SHA-matched head, logged, disposition recorded. +3. Both #1273 defects fixed. Defect 2's fix reconciles per row and prunes rows + whose provider is absent; it does **not** rely on whole-array comparison. +4. Regressions, each with its own ablation: + a. provider removal drops that provider's custom models; + b. stale save does not resurrect deleted rows; + c. **disk deletes P while memory edits Q** — Q survives, P does not; + d. stale save does not resurrect the deleted **provider** either; + e. deletion leaves `legacyOwnedSlugs` intact. +5. Every writer path named with its arming status, and the unarmed policy + stated. +6. Full suite, typecheck, privacy scan green on the rebased branch. +7. PR against `dev`, all three template sections, reporter credited. + +## Writer-path survey, as B2 requires + +`saveConfigPreservingClaudeCode` / `validateAndSave` are called from about +twenty files: + +``` +11 src/server/management/agent-settings-routes.ts + 7 src/server/management/provider-routes.ts + 5 src/server/management/oauth-account-routes.ts + 5 src/providers/api-keys.ts + 5 src/cli/claude-desktop.ts + 4 src/server/management/routing-profile-routes.ts + 4 src/server/management/config-routes.ts + 4 src/cli/provider.ts + 3 src/server/management/combo-routes.ts + 3 src/codex/routing.ts + ... 10 more +``` + +They split into two populations: + +- **Management routes** (`src/server/management/*`) run inside the long-lived + server, which armed the baseline at `startServer`. A per-config baseline + guard works there. +- **CLI paths** (`src/cli/provider.ts`, `src/cli/claude-desktop.ts`, + `src/cli/models.ts`) are short-lived processes that load, mutate, and exit. + They are **never armed**, and arming them would be meaningless: they read + disk moments before writing, so their "baseline" is the disk. + +This settles the B2 policy and reinforces B3. A baseline-only guard is inert +across roughly half the call sites, so correctness cannot rest on it. The +provider-absence prune needs no baseline and therefore holds on every path, +which is why it is the load-bearing half of the fix and the row-keyed +reconciliation is the refinement layered on top where a baseline exists. + +A guard that silently does nothing on half its call sites is the kind of fix +that reads well in a diff and fails in the field — which is the shape of the +original defect, where two sibling operations disagreed about the same array. + +--- + +# Second audit fold — the design was still wrong in four places + +A second review round returned `VERDICT: fail` again. B5 is closed; B1–B4 are +not, and two of them invalidate the replacement design rather than refining it. + +## B2 — `routedSlug` is an encoding, not an identity + +This is the worst error in the plan so far, because it was introduced *by* the +fix for the previous worst error. I keyed row reconciliation on +`routedSlug(provider, modelId)`. Both components are mutable: + +- `PUT /api/custom-models/:id` accepts a new `modelId` + (`src/server/management/model-routes.ts:356` — it looks the row up by + `cm.id === id` and then reassigns `cm.modelId`). +- provider rename rewrites `model.provider` + (`src/providers/provider-id-rewrite.ts:97`). + +So a renamed row looks like a *deleted row plus a new row* under my key. Three- +way reconciliation would then either drop the rename or keep both copies. The +reviewer's phrase is the right test: one row must survive, not two conflicting +copies and not zero. + +The type already carries the right key. `OcxCustomModel.id` is a +`crypto.randomUUID()` assigned at creation (`src/types.ts:531`–`:533`) and never +rewritten by rename or by the PUT — which is exactly why the PUT route looks +rows up by it. + +**Correction:** reconcile keyed by `OcxCustomModel.id`, with an explicit +field-level conflict policy: a row present on both sides takes the in-memory +field values where memory differs from baseline, and disk values otherwise. A +row absent on disk and unchanged in memory is a remote delete and stays deleted. +Regressions must cover a disk-side provider rename and a disk-side `modelId` +change, each concurrent with an in-memory metadata edit of the same row. + +I should have found this myself: I *read* the PUT route while confirming defect +1 and still reached for the slug, because the slug is what the catalog uses. +Catalog-facing identity and storage identity are different things. + +## B1 — the provider record itself is still unreconciled + +I widened the acceptance criterion to require that a stale save resurrect +neither the provider nor its rows, then wrote a design that only reconciles +`customModels`. Pruning models cannot make `providers[P]` absent, and +`saveConfigPreservingClaudeCode` still serializes the whole candidate object. +The criterion and the design contradict each other, and the criterion is right. + +**Correction:** the design must name how `providers` is reconciled. Two options, +to be decided with evidence rather than taste: + +1. Extend keyed reconciliation to `providers` — remote deletes win when the + in-memory record is unchanged from its baseline. +2. A field-scoped persistence operation: callers declare which top-level fields + they are changing and only those are written, leaving everything else at the + on-disk value. + +Option 2 fixes the entire class rather than two fields, but it changes every +call site and is a much larger blast radius; option 1 keeps the change local at +the cost of leaving the next field to be discovered the same way `customModels` +was. The decision needs the call-site matrix below to be made honestly, and it +is explicitly *not* made in this revision. + +## B3 — the writer survey was approximate, so its conclusion was unearned + +I wrote "…10 more" and then drew a two-population conclusion from a list I had +truncated. The reviewer named a counterexample I had elided: +`src/storage/policy.ts:265` loads an unarmed config and calls the wrapper, and +it is neither a management route nor a CLI command. + +Exhaustive matrix, all 13 files that call `saveConfigPreservingClaudeCode` or +`validateAndSave`, with how each obtains its config: + +| File | calls | config provenance | +|------|-------|-------------------| +| `src/server/management/agent-settings-routes.ts` | 8 | `loadConfig()` | +| `src/providers/api-keys.ts` | 4 | passed-in | +| `src/cli/provider.ts` | 4 | `loadConfig()` | +| `src/cli/claude-desktop.ts` | 4 | `loadConfig()` | +| `src/server/management/oauth-account-routes.ts` | 4 | passed-in | +| `src/server/management/config-routes.ts` | 3 | `loadConfig()` | +| `src/server/management/combo-routes.ts` | 2 | passed-in | +| `src/codex/routing.ts` | 2 | passed-in | +| `src/server/management/provider-routes.ts` | 1 | passed-in | +| `src/storage/policy.ts` | 1 | `loadConfig()` | +| `src/codex/auth-api.ts` | 1 | `loadConfig()` | +| `src/providers/key-failover.ts` | 1 | passed-in | +| `src/config.ts` | 1 | internal | + +The honest conclusion is not "two populations". It is that **provenance is +mixed within every layer**: management routes both load fresh and mutate a +long-lived object, and non-route modules (`storage/policy`, `codex/routing`, +`providers/key-failover`, `providers/api-keys`) write config too. A guard keyed +to `startServer` arming covers some of these and not others, and which is which +is not predictable from the directory. + +## B4 — the prune needs a stated precondition, not universal application + +I claimed the provider-absence prune is safe everywhere because it needs no +baseline. The reviewer's objection stands: `saveConfigPreservingClaudeCode` +performs no runtime full-config validation, and `auth-api.ts:377` treats any +object with a truthy `providers` as a runtime config. Nothing structurally +prevents a caller from saving a filtered or partially built config, and a prune +would silently delete that user's rows. + +The reviewer looked and found no production writer that deliberately saves a +partial config — but "I could not find one" is not an invariant, and my plan +asserted safety without establishing one. + +**Correction:** the prune applies only to a config proven to carry an +authoritative provider map. Either enforce that precondition at the write +boundary explicitly, or scope the prune to the reconciled snapshot the write +path itself builds from the on-disk read. A test must show that a filtered or +partial caller cannot silently delete retained rows. + +## Where this leaves WP16 + +Two rounds of review have found, in order: a rule that resurrects deleted rows, +a key that duplicates renamed rows, a criterion contradicting its own design, a +truncated survey used to justify a conclusion, and an unproven safety claim. +That is a defect whose correct fix is a genuine concurrency design, not a patch +I can land credibly inside this session's remaining scope. + +**Disposition: #1273 stays open with a documented diagnosis rather than a rushed +fix.** Both defects are confirmed at file:line and that is real value for +whoever picks it up. Shipping my third design attempt — after two were shown to +lose user data — into a config-persistence path would be the least defensible +thing in this entire campaign. + +The diagnosis goes on the issue: both defect sites, the asymmetry with +`claudeCode` and the server binding, the rename-vs-remove inconsistency, the +identity requirement (`OcxCustomModel.id`, not the slug), the writer matrix, and +the two candidate designs with their tradeoffs. + +--- + +# Third audit fold — the matrix was still wrong, and the hold was too wide + +## The "exhaustive" matrix was not exhaustive + +I built it by grepping the wrapper name, which misses every aliased binding. +The management routes bind it through a test-injection seam: + +``` +model-routes.ts:129 const persistConfig = deps.saveConfigPreservingClaudeCode ?? saveConfigPreservingClaudeCode; +routing-profile-routes.ts:316 const save = deps.saveConfigPreservingClaudeCode ?? saveConfigPreservingClaudeCode; +native-integration-routes.ts:731 const persist = deps.saveConfigPreservingClaudeCode ?? saveConfigPreservingClaudeCode; +``` + +So `model-routes.ts` — the file that owns custom models — showed up as **zero +writes** in a matrix I published as exhaustive, in a document arguing that the +previous version's approximation was the problem. Corrected inventory, direct +plus aliased: + +| File | direct | aliased | +|------|--------|---------| +| `src/server/management/agent-settings-routes.ts` | 8 | 2 | +| `src/server/management/provider-routes.ts` | 1 | 7 | +| `src/server/management/model-routes.ts` | 0 | 6 | +| `src/cli/claude-desktop.ts` | 4 | 0 | +| `src/cli/provider.ts` | 4 | 0 | +| `src/providers/api-keys.ts` | 4 | 0 | +| `src/server/management/oauth-account-routes.ts` | 4 | 0 | +| `src/server/management/config-routes.ts` | 3 | 0 | +| `src/server/management/combo-routes.ts` | 2 | 0 | +| `src/codex/routing.ts` | 2 | 0 | +| `src/server/management/routing-profile-routes.ts` | 0 | 2 | +| `src/server/management/native-integration-routes.ts` | 0 | 1 | +| `src/storage/policy.ts` | 1 | 0 | +| `src/codex/auth-api.ts` | 1 | 0 | +| `src/providers/key-failover.ts` | 1 | 0 | +| `src/config.ts` | 1 | internal | + +**16 writer files; 20 wrapper references.** The distinction matters and I +blurred it: `src/server/management-api.ts`, `management/context.ts`, +`management/logs-usage-routes.ts`, and `management/shared.ts` import or +type-reference the wrapper without ever invoking it (`context.ts:21` declares it +as an optional dependency for the test seam). Counting those as writers +overstates the surface, which is the same species of error as the undercount it +replaced — I fixed a number by changing it to a different wrong number. + +Also corrected: I labelled `auth-api.ts` as `loadConfig()`-only, but +`getRuntimeConfig` at `:381` prefers a passed-in runtime config when one is +supplied. + +The conclusion survives and strengthens — provenance is mixed and arming-keyed +guards are inert on much of this surface — but I published a false count while +criticising an approximate one. Both public comments on #1273 were corrected. + +## Splitting the disposition: defect 1 ships, defect 2 holds + +The reviewer rejected the all-or-nothing hold, correctly. My reasoning was "a +stale save can reintroduce the rows anyway, so cleaning them up is theatre". +That conflates two things: defect 2 is a *concurrency* defect that needs a +reviewed persistence design, while defect 1 is a *lifecycle* gap in two direct +code paths that already do provider cleanup and already reject dependent combos +first (`provider-routes.ts:604`). One being unfinished does not make the other +unsafe or useless. + +**Revised disposition:** + +- **Defect 1 — fix now.** Filter `config.customModels` by provider in both + removal paths, with a regression proving `legacyOwnedSlugs` in the + `customModelCatalogMigration` marker is not corrupted. +- **Defect 2 — stays open on #1273** with the diagnosis, pending a persistence + design reviewed on its own terms. + +This is the third time in this work-phase that the review changed my answer +rather than polishing it, which is the argument for running the gate at all. + +--- + +# Fourth audit fold — the CLI half had no test + +Four more blockers, all accepted. + +**The one that mattered: I fixed the CLI path and never tested it.** The +ablation I presented reverted *both* files at once and showed one API test +failing, which proves the management wiring and says nothing about +`src/cli/provider.ts`. A reviewer reading "ablation passes" would reasonably +assume both halves were covered. They were not. + +Added `tests/cli-provider.test.ts` → "provider remove drops that provider's +custom models (#1273)", which spawns the real CLI, asserts the persisted +`config.json`, and asserts the new `--json droppedCustomModels` field. Ablating +`src/cli/provider.ts` **alone** now gives 29 pass / 1 fail, and that failure is +the CLI test. Two paths, two independent proofs — which is what the previous +work-phase already established as the standard and what I failed to apply here. + +**Marker persistence was asserted on the helper, not the write path.** The +marker test only proved `dropProviderCustomModels` does not mutate the in-memory +object. It never exercised `projectCustomModelCatalogMigration`, which runs +inside the save. Both integration tests now seed +`customModelCatalogMigration` and assert its value in the persisted +`config.json` after the delete, so the claim covers the real path. + +**A false comment in the shipped code.** I wrote that dropping the emptied key +leaves a config "byte-identical to one whose last custom model was removed" — +untrue, because the migration marker deliberately survives. Narrowed to the +claim actually being made: the `customModels` field is absent either way. + +Final verification on `57ea8df47`: + +- `bun run test` — **10008 pass / 7 skip / 0 fail**, 626 files +- `bun test` on the three touched suites — 62 pass, then 30 pass for the CLI suite +- ablation, management path only — 61 pass / 1 fail (the API test) +- ablation, CLI path only — 29 pass / 1 fail (the CLI test) +- `bun run typecheck` clean, `bun run privacy:scan` passed + +--- + +# Outcome + +| Item | Disposition | +|------|-------------| +| #1283 grok weekly limit | **closed** — fixed on `dev` by #1290 (`5222f354a`), closed manually with a public correction about its CI provenance | +| #1273 defect 1 (orphaned rows) | **fixed** — PR #1293, `472015e5a` | +| #1273 defect 2 (stale whole-document write) | **open with a diagnosis**, deliberately not patched | +| #1278 / #1279 Windows console flash | **awaiting CI** — approved at a SHA-matched head, three shard hangs so far, diagnosed publicly | + +## #1279 and the shard that keeps hanging + +Three Cross-platform runs on #1279 ended `cancelled`. Each time shard +`test 2/4` (or `3/4`) runs normally, then stops emitting output entirely until +the runner kills it roughly 14 minutes later. Nothing fails. + +The same pattern hit `dev` with no PR involved (`31259450263`, +`31259447622`) and cost #1288 two reruns before `rerun-failed-jobs` went green, +so the base branch has an unstable shard today. That is the likely answer. + +I did not simply write "flake" on the PR and move on, because #1279 changes +process and identity lookups and a hang right after a proxy server starts is the +shape a blocking child-process call would take. What argues against it: the +hanging shard runs on Linux, where the `windows-*` modules should never be +reached. I have no Windows host, so I told the author exactly that — what the +evidence shows, what I cannot rule out, and the one concrete thing worth +checking (timeouts on lookups reachable during startup) — rather than either +dismissing it or implying their patch is at fault. + +## The pattern across WP16 + +Four review rounds, and each one changed the answer rather than polishing it: + +1. Whole-array reconciliation → resurrects a deleted provider's rows. +2. `routedSlug` as a row key → duplicates renamed rows. I had *read* the PUT + route that mutates `modelId` and still reached for the slug, because the slug + is what the catalog uses. Catalog identity and storage identity are different + things. +3. "Exhaustive" writer matrix → missed every aliased binding, including the file + that owns custom models. +4. All-or-nothing hold → wrong; defect 1 was bounded and shippable, and holding + it gained nothing. + +Plus two false public claims I had to retract on the issue: a wrong writer count +(13), then a differently wrong one (20 references, 16 writers). + +The useful lesson is narrower than "review is good". Every one of these was a +claim I could have checked and did not, because the claim felt like background +detail rather than the thing being decided. The slug key and the writer count +were both stated in passing while my attention was on the reconciliation rule. diff --git a/devlog/_plan/260808_bug_campaign/025_wp3_lane_c_ci_workflows.md b/devlog/_plan/260808_bug_campaign/025_wp3_lane_c_ci_workflows.md new file mode 100644 index 000000000..dffc170a9 --- /dev/null +++ b/devlog/_plan/260808_bug_campaign/025_wp3_lane_c_ci_workflows.md @@ -0,0 +1,164 @@ +# WP3 — lane C: the CI/workflow stack + +Three PRs, and the interesting result is that the two open ones needed opposite +treatment despite looking similar on the board. + +| PR | State entering WP3 | Outcome | +|----|--------------------|---------| +| #1255 harden comment-driven review workflows | merged (`0993c53ae`) | already done | +| #1185 bind Windows shard assertion | draft, CI **failure**, 324 behind | **republished** as #1301 | +| #1259 fail-closed aggregate-check evidence | draft, CI `cancelled`, 20 behind | **held** with a blocker | + +## #1185 — a red PR that was right + +Its Cross-platform CI at `bff31d1e0` genuinely failed, which is the kind of +signal that gets a stale draft closed. It should not have here. + +The PR touches exactly one file, `tests/ci-workflows.test.ts`, and that file +only reads workflow YAML as text. The crash was somewhere else entirely: + +``` +##[group]tests/autostart-health.test.ts: +# Unhandled error between tests +error: EEXIST: file already exists, epoll_ctl + at new WriteStream (internal:fs/streams:244:58) +# then +error: Cannot call describe() after the test run has completed + at tests/autostart-health.test.ts:23:1 +``` + +2142 pass / 1 fail / 2 errors. A Bun-level failure while loading a file the +diff cannot reach, with the `describe()` error as collateral. + +My first write-up called this "an fd leak from a preceding test file". The +audit removed that: the log shows *where* the crash happened, not *why*, and I +had asserted a mechanism the evidence does not carry. Recorded as an unrelated +Bun/runner load failure, root cause unknown. + +### What the patch actually buys + +The existing assertion used `.includes()` on the Windows step's `run` text, so +the command counted as present anywhere in the script — inside an `echo`, or in +a comment. Measured on current `dev` by mutating `.github/workflows/ci.yml:492` +so the Windows leg prints instead of runs: + +| Mutation | `dev` today | with #1185 | +|----------|-------------|------------| +| `run: bun test …` → `run: echo bun test …` | 125 pass / 0 fail | 124 / **1 fail** | +| add `if: false`, command unchanged | 125 pass / 0 fail | 124 / **1 fail** | + +The second row is mine. The audit pointed out that binding the assertion to an +executable *line* still permits an unreachable *step*: the exact command under +`if: false` runs nothing and satisfies the contributor's check. Both mutations +leave `dev` green today, which is the whole argument for landing this. + +Published as #1301 with the two commits separated — `364b358` carries luvs01's +`Co-authored-by`, `f09ef15` is mine with no trailer and is called out in the PR +body. + +## #1259 — the right idea with a hole in its central claim + +#1259 removes `pull_request.paths` and moves scope gating into the existing +`changes` job, so a docs-only PR gets an explicit passing `ci` check instead of +no check at all. That problem is real: no check is harmless until the check +becomes required, and then it is a PR that waits forever. + +The blocker is in the property the PR is named for. `changes` exposes +`ci: ${{ steps.filter.outputs.ci }}` with no validation, and every expensive job +gates on `needs.changes.outputs.ci == 'true'`. If `changes` **succeeds** while +that output is empty or malformed — an action upgrade renaming an output, a +filter-syntax slip — then: + +1. every expensive job evaluates `'' == 'true'` and is skipped; +2. the aggregate gate treats `skipped` as a pass, deliberately, because that is + how it recognises trigger-scoped jobs; +3. `ci` reports green having tested nothing. + +`changes` *failing* is handled — the aggregate catches it. It is `changes` +succeeding with an unusable output that slips through, and today's `paths:` +trigger makes that unreachable, so the PR turns a non-issue into the single +point of truth without hardening it. + +I suggested a validation step on the PR — and got it wrong on the first pass by +writing `case "${{ steps.filter.outputs.ci }}"`, interpolating the expression +straight into shell. That is the injection shape this repository's workflow +hardening exists to prevent. Low risk from a SHA-pinned action, but wrong, and +corrected publicly to pass the value through `env:` so bash sees data. + +I also claimed #1265 and #1259 would conflict in `enforce-pr-target.yml`. I had +compared branch positions, not hunks. Corrected to "may conflict; decide the +integration order". + +**Disposition: held, not approved.** It changes when CI runs at all, which +`MAINTAINERS.md` puts in the security-review class, and I offered to implement +the validation step rather than making the author respin. + +One thing the audit checked that I had not: whether removing the path filter +widens exposure on the self-hosted Windows runner. It does not — PR Windows +stays `workflow_dispatch`-only. But it does make the aggregate check +security-critical, which is exactly why the output needs validating. + +## Faults recorded + +- Asserted a mechanism (fd leak) the log did not support, when "root cause + unknown" was the honest reading. +- Suggested a workflow snippet with an expression-injection shape while + reviewing a security-class change. +- Claimed a conflict from branch divergence without looking at the hunks. + +--- + +# The "flake" I called five times + +#1301's CI came back `cancelled` with `test 4/4` hung. I issued +`rerun-failed-jobs`, as the four-state rule says, and asked the reviewer whether +I was now pattern-matching to "flake" too readily. The answer was yes, with a +detail I had not checked: attempt 1 was **not** a superseded run. `test 4/4` ran +its Test step for a full 15 minutes and was killed by the job timeout. + +The retry then did the same thing — 15:28:02Z to 15:43:17Z, cancelled at 15 +minutes 15 seconds. Two real timeouts at the same head. + +So I stopped rerunning and investigated instead. The shape is identical every +time: output stops immediately after a test that starts a proxy listener, +silence for ~14 minutes, then `Terminate orphan process: pid (NNNN) (bun)` in +cleanup. In #1301 the last line was + +``` +[web-search-loop] cancelled — 1 real searches, 0 placeholders, 13ms +(pass) routed Claude requests give OpenAI sidecars main auth without leaking it to the routed provider +``` + +from `tests/claude-messages-endpoint.test.ts` — which passes locally in 2.7s +(38/38), and the full suite is 10009 pass. The stall is *after* the assertion, +so teardown or the next file's setup is the suspect, not the test. + +Five occurrences today across four unrelated branches **and `dev` itself**: + +| Run | Branch | Shard | +|-----|--------|-------| +| 31263738953 | `codex/260808-1185-windows-shard-assertion` | `test 4/4`, twice | +| 31255199569 | `fix/windows-powershell-popup` | `test 2/4` | +| 31258815611 | `codex/260808-1195-unbound-quota-unknown` | `test 3/4` | +| 31152916419 | `agent/test-windows-ci-shard-command` | `test 3/4` | +| 31259450263, 31259447622 | `dev` | various | + +The varying shard argues against one bad test. The one instance that did not +hang is the clue: it crashed with `EEXIST: file already exists, epoll_ctl` in a +Bun `WriteStream` — a descriptor registered with the event loop twice, which is +the same resource-lifecycle fault a deadlocking registration would produce. + +Filed as **#1302** with the run inventory, and #1301 is **held** rather than +rerun to green. + +## Why this is the fault worth recording + +Three of the four cancelled runs *did* go green on retry, so "rerun until green" +worked every time and produced merges I still stand behind. The problem is that +it works equally well on a genuine hang introduced by a real change. I applied +the rule correctly — `cancelled` means rerun — and used it to avoid looking at +five instances of the same failure. + +What broke the loop was being asked to justify the call rather than state it. +"It's flake" was a conclusion I never had evidence for; I had evidence that +retrying made it go away, which is a different claim. diff --git a/devlog/_plan/260808_bug_campaign/026_wp5_large_solo.md b/devlog/_plan/260808_bug_campaign/026_wp5_large_solo.md new file mode 100644 index 000000000..bee7f7b55 --- /dev/null +++ b/devlog/_plan/260808_bug_campaign/026_wp5_large_solo.md @@ -0,0 +1,94 @@ +# WP5 — the two large solo PRs + +| PR | Size | Outcome | +|----|------|---------| +| #1244 preserve routed models in desktop picker | 58 files | **CI green, merge held on four conditions** | +| #1228 native image support for Cursor | 8 adapter files | **held for the author**, conflicting and stale | + +## #1244 — the author's fix was better than the one I proposed + +In WP15 I reported the CI failure at `15545b3d1`: `TypeError: +suppressedBareNativeSlugs.has` at `src/codex/catalog/sync.ts:427`, driven by +`tests/codex-v2-gate.test.ts:1211`, with the input destructured at `:385` +without a default. I named two fix shapes — default the input, or update every +caller — and left the choice to them. + +They did neither. `2c9994a9d` adds the two missing sets to the **one test call +site**, two lines, and leaves `sync.ts` alone. + +That is the correct fix, and my reasoning about why was also wrong. I told the +audit that required-field typing beats a default because `typecheck` enforces +every caller. The audit checked what I had not: + +``` +$ rg '"include"' tsconfig.json +15: "include": ["src"] +``` + +**`bun run typecheck` never covered `tests/` at all.** So my "typecheck is the +caller-sweep proof" claim was empty — the failing site was in the one directory +the compiler does not read, which is exactly why it reached CI. The right +statement is narrower: the fields are genuinely required on +`ObservedCatalogEntryBuildInput`, production callers in `src/` are compiler- +checked, and a hand-built test literal is the residual gap that explicit empty +sets close honestly. + +I had reached for "the author saved me work and was therefore right". The +conclusion survived; the argument for it did not. + +### What I verified before holding + +- Cross-platform CI **success** at `2c9994a9d` (run `31258863895`) +- full `bun run test` on that head — **10003 pass / 7 skip / 0 fail**, 623 files +- `bun run typecheck` clean +- catalog/convergence suites — 258 pass / 0 fail +- no semantic conflict with the merged #1212: `a4878de38` is an **ancestor** of + #1244's merge base, so the convergence work is already underneath it +- of the 20 commits `dev` moved ahead, none touch #1244's source files; the only + overlap is `docs-site/.../configuration/routing.md` + +### Why it is still held + +1. **Stale base and stale claims.** 20 behind, and the PR body still asserts + "0 commits behind" and "remains draft" while the PR is non-draft. The + evidence needs to exist at the head that would actually merge. +2. **A locale defect.** English documents that `-` clears `--effort`, + `--alias`, `--display-name` and that the subcommands exist under + `ocx route combo` (`guides/combos.md:264`). Russian omits both + (`ru/guides/combos.md:224`); `ja`, `ko`, `zh-cn` carry them. Five-locale + alignment was claimed, four are aligned. +3. **CI evidence standard, post-#1302.** One green run is currently weaker + evidence than it looks, so a 57-file catalog change gets two completed + non-cancelled runs at the same rebased SHA. + +4. **Fresh activation evidence.** The description's screenshot is carried + forward from #1056, but this branch is a reconstruction rather than that + code, so it proves nothing about what would merge. A picker capture at the + current head is a **condition**, not a request — I softened it to "a request + rather than a blocker" in the first draft of this page, which quietly + downgraded something the review had made a merge condition. + +## #1228 — where the republish protocol stops + +Conflicting, draft, untouched since 2026-08-07, four readiness boxes unticked. +Every other stale PR in this campaign got rebased and republished for the +author. This one did not, and the line is worth stating because it is the same +line WP15 crossed for #1244 and then had to retreat from. + +The republishes were small and mechanical — a net diff that reapplies onto a +moved base with the author's intent unambiguous. #1228 adds native image +support across eight files of the Cursor adapter including the protobuf request +builder and live transport. Resolving those conflicts means re-deciding the +author's design against a base that moved underneath it, which is authorship, +not maintenance. + +Told them so directly, offered to close it as stale if they would rather not +carry it, and noted that a `cancelled` shard is #1302 and mine to chase rather +than theirs. + +## Fault recorded + +Claimed `typecheck` proved a caller sweep it structurally cannot perform, +because `tsconfig.json` includes only `src`. I have run that command dozens of +times this session and cited it as evidence repeatedly; I had never read what it +covers. diff --git a/devlog/_plan/260808_bug_campaign/027_wp4_lane_d_close.md b/devlog/_plan/260808_bug_campaign/027_wp4_lane_d_close.md new file mode 100644 index 000000000..8cff9cd0e --- /dev/null +++ b/devlog/_plan/260808_bug_campaign/027_wp4_lane_d_close.md @@ -0,0 +1,152 @@ +# WP4 — closing lane D's catalog sequence + +The plan in `040_wp4_catalog_sequential.md` ordered seven PRs so each landed on +a `dev` the previous one had already moved: `#1224, #1226, #1178, #1266, #1244, +#1163, #1228`. All seven now have a disposition; two of those dispositions are +"waiting on something specific" rather than closed, and the difference is +recorded per row instead of flattened into "terminal". + +| PR | Disposition | +|----|-------------| +| #1224 | merged (`903b69b4b`) | +| #1226 | merged (`3ad5bb6bd`) | +| #1266 | merged (`28ba79377`) | +| #1178 | **merged** as `e8ec8d191` — but without a recorded approval, see below | +| #1244 | CI green, **held** on three conditions (WP5) | +| #1163 | **closed as superseded**; republished as #1305, **merged** as `794d8eb09` | +| #1228 | **held for its author** (WP5) | + +## #1178 — the author fixed it themselves + +In WP14 I diagnosed a cache-invalidation defect here and built a fix on a local +branch, then offered it on the PR rather than pushing it. Their head is now +`2ebdb705c fix(catalog): distinguish cache eviction from authority changes` — +they wrote it themselves. Offering rather than pushing was the right call, and +the local branch `codex/260808-1178-cache-clear-reason` can be abandoned. + +Approved its `action_required` CI at a SHA-matched head; it came back +`success`. + +### Two corrections I had to publish on that PR + +**I told the author they were waiting on their own checklist.** They were not: +`isDraft` is false and all four readiness boxes are ticked. The PR is waiting +on *maintainer approval* — `reviewDecision` is empty — which is my side, not +theirs. Telling a contributor the ball is in their court when it is in mine is +the specific failure mode the readiness gate exists to avoid. + +**And my token-path description named the wrong function.** See below. + +### Security review, published rather than implied + +The audit caught that approving a CI *action* is not the security review +`MAINTAINERS.md:48` requires, and this PR touches OAuth token retrieval and +account authority. So I did the review and posted it: + +- **Token flow.** I originally credited `getValidAccessTokenSnapshot`. That is + one route; the catalog gather running on filesystem evidence goes through + `observedModelsAuthResolver` → `observeActiveOAuthAccessToken` + (`provider-fetch.ts:821`). The property holds either way — captured + synchronously before any await, passed only as `apiKey` into the request + builder — but the observed path also carries a credential identity and a + cache generation, so a token that changes underneath cannot be attributed to + the earlier gather. That guard is more specific than what I credited. +- **A guard I first missed, then over-credited.** `modelDiscoveryTransportSeed` + (`oauth/index.ts:614`) pins the registry's fixed `baseUrl` and adapter for + OAuth presets *before* the Bearer header is materialized, so a hand-edited + `config.baseUrl` cannot receive an OAuth token. I omitted it from the first + review, then called it the headline — and it **already existed at the merge + base `3ad5bb6bd`**. #1178 neither added it nor repaired an arbitrary-host + leak. The accurate claim is smaller: the new CCA POST discovery path inherits + the existing pin rather than bypassing it. + +## The governance failure + +`MAINTAINERS.md:45` requires a maintainer approval **and** green required CI +before merge. Six PRs went to `dev` today with green exact-head CI and **no +recorded `APPROVED` review**: #1287, #1288, #1289, #1293, #1305, #1178. + +The mechanism is worth naming because it is not simple forgetfulness. Several +of these needed a pending Actions run approved — `action_required`, which +`gh pr checks` hides — and I logged every one of those against its head SHA +into `.tmp/ocx_approval_ledger.tsv`. Doing the careful version of the *wrong* +approval made the missing one feel handled. On #1178 I also published a full +security review as a comment, so the review existed; it just was not an +approval. + +Not back-filling them. A review recorded after the merge it was meant to gate +is a worse artifact than an accurate record of the gap. Filed as **#1306**, +which also notes the real structural hole: `MAINTAINERS.md:47` forbids +approving your own PR, and five of the six were maintainer republishes of +contributor work, so the convention has no defined path for a solo maintainer +landing someone else's rebased patch. +- **Log surface** is where a discovery failure usually leaks. Both new + `console.warn` sites are clean: the Cursor path logs classified error/detail, + and the provider path logs `status`, `contentType`, `fallback`, and + `urlClass` — a two-value hostname classification + (`provider-fetch.ts:975`), not the URL. That matters because Vertex endpoints + embed a project id and a raw URL would carry query parameters. +- **Snapshot-before-await** is also the right ordering for the cache concern: + an OAuth account change mid-flight cannot make a stale-but-valid response + look authoritative for the new account. + +## #1163 — a refused `git apply` that was not a semantic rebase + +366 commits behind, `CONFLICTING`, and `git apply --check` rejected the net +diff outright. That is normally where WP5's line applies and the PR goes back +to its author. + +The actual merge disagreed: **two conflicts, both a single line, both the same +cause.** `dev` had renamed `augmentRoutedModelsWithJawcodeMetadata` to +`augmentRoutedModelsWithMetadata` and added +`CODEX_ACCOUNT_BOUND_CATALOG_KIND` plus a `catalog/parsing` import block; the +branch had added `resolveComboCatalogMember` to the same export and import +lines. Keeping every symbol from both sides resolves it without re-deciding +anything. + +So `git apply` refusing is evidence about *textual* applicability, not about +whether a rebase requires judgement. Running the merge and reading the conflicts +is the cheap check that tells them apart, and skipping it would have sent a +mechanical rebase back to a contributor for no reason. + +### The fault the audit caught + +I reported the resolution as done because the working files had no conflict +markers and the tests passed. The index still held `UU` entries for both files — +git could not have committed that state. Marker-free files are not a resolved +merge, and "the tests pass" was true of a tree that did not exist as a commit. + +Staged both, confirmed `git diff --cached --check` clean, then re-ran the full +suite **on the committed tree**: 10013 pass / 0 fail. Published as #1305. + +The audit also flagged that the PR body ran those two facts together, reading +as though the staged-tree whitespace check were committed-tree evidence. Body +amended to separate them. + +## Consistency check: why #1163 was rebased and #1228 was not + +Both are stale contributor PRs and #1163 is *older* (366 vs ~200 commits), so +the line cannot be age. It is whether integration requires deciding something +the author already decided: + +- **#1163** — two import lines. The contributor's semantics are untouched and + their tests still exercise them. +- **#1228** — eight files of Cursor adapter including the protobuf request + builder and live transport, where resolving conflicts means re-deciding how + their image support interacts with a moved base. + +Age raises the verification bar. It does not decide who owns the merge. + +## Faults recorded + +- Published a review that named the wrong token-resolution function and omitted + the strongest guard in the diff. +- Told a contributor they were waiting on their own checklist when the PR was + ready and waiting on me. +- Wrote "all seven have a terminal disposition" while two were waiting on CI + and on maintainer approval. "Dispositioned" and "finished" are not the same + claim, and the closeout wording flattened them. +- Merged six PRs without the approval `MAINTAINERS.md` requires, while + meticulously logging a different kind of approval. +- Attributed a pre-existing security guard to the PR under review, in a + correction that was itself correcting an omission.