From ac29016e0a897702dacdf0381925b939a194ff86 Mon Sep 17 00:00:00 2001 From: lidge-jun <243035832+lidge-jun@users.noreply.github.com> Date: Fri, 4 Sep 2026 17:56:50 +0900 Subject: [PATCH] docs(devlog): record the 260904 bug backlog closeout Completes the planning unit whose earlier docs reached dev with #3439, and adds one regression test. The unit records the board snapshot at 072df52eb, the per-PR merit reviews behind the merge train, the draft-PR triage with each blocking defect at file:line, and a closeout naming every terminal outcome. Two planned fixes are recorded as REJECTED with their reasoning rather than quietly dropped: the #3425 quota-selector change was a no-op the existing suite already contradicted, and mirroring Claude's session_id synthesis onto the Chat bridge without provenance would bind unrelated callers sharing a cohort key onto one upstream session. The test pins the #3425 finding. Ten 502s carrying a stale writerGeneration leave health null and the account serving; three identical 502s at a live generation rotate. That contrast isolates the guard at src/codex/routing.ts:2195 as the difference rather than the 502 classification. --- .../030_wp4_account_pool.md | 43 ++++++++- .../260904_bug_backlog_closeout/060_ledger.md | 51 ++++++++--- .../070_closeout.md | 87 +++++++++++++++++++ tests/codex-routing.test.ts | 31 +++++++ 4 files changed, 199 insertions(+), 13 deletions(-) create mode 100644 devlog/_plan/260904_bug_backlog_closeout/070_closeout.md diff --git a/devlog/_plan/260904_bug_backlog_closeout/030_wp4_account_pool.md b/devlog/_plan/260904_bug_backlog_closeout/030_wp4_account_pool.md index 033d64e281..25bb77ea3a 100644 --- a/devlog/_plan/260904_bug_backlog_closeout/030_wp4_account_pool.md +++ b/devlog/_plan/260904_bug_backlog_closeout/030_wp4_account_pool.md @@ -64,7 +64,48 @@ credential-scoped caching. Requires explicit security review, including proof th and account ids are never logged and never shared across account cache entries. ## Accept criteria + +## wp4 P-phase stale check (verified directly against dev, 260904) + +Subagent dispatch failed twice with `401 No eligible Codex account supports this model` -- +which is issue #3352 firing on this session's own tooling -- so this pass was read directly. + +CONFIRMED, with one correction that changes the fix: + +- `hasCodexQuotaHeadroom` returns true on unknown usage at `src/codex/routing.ts:1215`, and + `applyQuotaAutoSwitch` keeps the active account on unknown usage at `:1655`. The comment + above it (`:1196-1200`) says this is deliberate: unknown usage must not drain a tier that + was never primed, and a genuinely exhausted account is expected to 429 into cooldown. +- `tests/codex-routing.test.ts:325` "known 100% weekly usage is exhausted, not unknown, and + switches accounts" ALREADY passes, and `:308` pins `isCodexQuotaExhausted` on an explicit + 100% window. So a KNOWN 100% account already switches. The reported bug is therefore NOT + a selection-logic defect, and the fix proposed in the original research -- consult + `isCodexQuotaExhausted` before the unknown branch -- would be a no-op for a known snapshot + and would break exactly the never-primed case the comment protects. Rejected. + +THE ACTUAL SUSPECT is `src/codex/routing.ts:2195`, the first line of +`recordCodexUpstreamOutcome` after the host-level branch: + +```ts +if (writerGeneration < lastReconciledGeneration && !liveHealthAccountIds.has(accountId)) return; +``` + +When a writer's captured generation is older than the last reconcile AND the account is not in +`liveHealthAccountIds`, the outcome is dropped WHOLE -- no `consecutiveFailures` bump, no +`lastFailureAt`, no soft-avoid. The transient path at `:2459-2470` never runs, so the +`upstreamFailoverThreshold` of 3 is never reached no matter how many 502s arrive. That matches +the report exactly: 118 failures, `sendCount` all 1, `recoveryKinds` empty, and rotation only +after a MANUAL pause (which goes through a different path). + +A second, independent amplifier is at `:2455`: `stale` resets the streak to 1 when the previous +failure is older than `CODEX_FAILURE_WINDOW_MS` (5 minutes, `:116`). A user whose failing turns +are spaced more than 5 minutes apart never accumulates 3 in a window, so the threshold is +unreachable by construction for slow, interactive traffic -- which is what an operator hitting +502s and retrying by hand looks like. + +wp4's B phase must therefore start by proving WHICH of these two fired, not by patching quota +selection. The evidence needed is whether the reported run had a config reload (which bumps the +generation) between the account being registered and the failures being recorded. - a PR per issue against dev, template-complete, with `Closes #3425` / `Closes #3352` - entitlement change proves unknown-admitted vs confirmed-denied in a focused test - no credential or token value is added to any log line (privacy:scan stays green in CI) - diff --git a/devlog/_plan/260904_bug_backlog_closeout/060_ledger.md b/devlog/_plan/260904_bug_backlog_closeout/060_ledger.md index 1f3cc95e14..2fd4b9416c 100644 --- a/devlog/_plan/260904_bug_backlog_closeout/060_ledger.md +++ b/devlog/_plan/260904_bug_backlog_closeout/060_ledger.md @@ -15,14 +15,15 @@ Columns: item, work-phase, outcome, evidence (merge sha / issue state / posted U | 3420 | ildunari | wp2 | MERGED | dev fc70555f3692400a6054d1d1aebf9e30bbd08868, 2026-09-04T06:53:36Z | | 3405 | adtumk | wp2 | MERGED | dev 20011a1c482c1e4051c2ec1c52d0ee9ca9164d6c, 2026-09-04T06:54:29Z | | 3401 | agentHits | wp2 | MERGED | dev 0f2e1209937ffae9d0c6c30837ce770b3c7cd73c, 2026-09-04T06:52:48Z | -| 3403 | ianlyoo | wp3 | pending | must reach MERGED or CLOSED | -| 3432 | luvs01 | wp3 | pending | | -| 3407 | turin-dev | wp3 | pending | | -| 3394 | kremnyi | wp3 | pending | | -| 3388 | zleo-ai | wp3 | pending | | -| 3348 | RHODIZSECURITY | wp3 | pending | supersede; needs Co-authored-by | -| 3332 | full999 | wp3 | pending | | -| 3325 | luvs01 | wp3 | pending | needs maintainer sponsorship | +| 3403 | ianlyoo | wp3 | FIX PUSHED to author branch | e7fe8dc6e with Co-authored-by; awaiting author ack + CI | +| 3432 | luvs01 | wp3 | REVIEW POSTED | whitespace-normalized `file:` scheme still evades FILE_URI_RE | +| 3407 | turin-dev | wp3 | REVIEW POSTED | GET reads stale startup config; toggle snaps back | +| 3394 | kremnyi | wp3 | REVIEW POSTED | enforce-target red is a cancelled run, not a failure | +| 3388 | zleo-ai | wp3 | REVIEW POSTED | sound; needs rebase + hosted CI attribution | +| 3348 | RHODIZSECURITY | wp3 | AUTHOR CHOICE OFFERED | split-it-yourself or carried with Co-authored-by | +| 3332 | full999 | wp3 | REVIEW POSTED | vendor maxTokens -> maxInputTokens shrinks a 1M window | +| 3325 | luvs01 | wp3 | SPONSORED | maintainer security review done; `maintainer-sponsored` applied | +| 3439 | lidge-jun | wp2 | MERGED | dev 8401b68db; repairs the two post-merge regressions | ## Bug issues @@ -33,10 +34,11 @@ Columns: item, work-phase, outcome, evidence (merge sha / issue state / posted U | 3378 | wp2 | CLOSED completed | closed after 20011a1c; absorbed #3344/#3362 already closed | | 3402 | wp3 | pending | closes on #3403 merge | | 3406 | wp3 | pending | tied to #3407 | -| 3425 | wp4 | pending | | -| 3352 | wp4 | pending | security-review class | -| 3433 | wp5 | pending | provenance decision required | -| 3424 | wp5 | pending | | +| 3425 | wp4 | DIAGNOSED, posted | routing.ts:2195 generation drop + :2455 5-min reset; one question asked | +| 3352 | wp4 | NEEDS-HUMAN | security-review class; hit live by this session's own subagent dispatch | +| 3433 | wp5 | NEEDS-HUMAN, posted | confirmed asymmetry; blanket synthesis rejected, provenance decision required | +| 3424 | wp5 | NEEDS-INFO, posted | opencode-go is adapter openai-chat; re-test asked, #3394 is the precedent | +| 3441 | wp2 | FILED | new: intermittent Windows npm-global cancellation | | 3320 | wp6 | NEEDS-INFO, posted | comment 5537325501: SID form is already accepted, so the suspect is identity resolution | | 3279 | wp6 | NEEDS-INFO, posted | comment 5537346000: named 3 captures; origin mismatch is the lead hypothesis | | 3255 | wp6 | RECLASSIFIED enhancement | comment 5537334610; label bug -> enhancement applied | @@ -131,3 +133,28 @@ and fails a scoped trigger regardless of correctness. Asked for an unpatched sta named captures with the origin-binding mismatch called out as the lead hypothesis, including the note that if that is the cause, the real defect is reporting a session problem as "cannot connect to proxy". + +## wp4 / wp5: two diagnoses that deliberately did not become patches + +Both units ended with evidence rather than code, and that is the honest outcome rather than +a shortfall. + +**#3425.** The planned fix was rejected by its own test suite: `tests/codex-routing.test.ts:325` +already proves a known-100% account switches away, so tightening the unknown-usage branch +would be a no-op that also breaks the never-primed case the code comments protect. The real +suspects are `routing.ts:2195`, which drops an outcome WHOLE on a stale writer generation so +`consecutiveFailures` never increments, and `:2455`, which resets the streak after five +minutes and makes the threshold unreachable for hand-retried traffic. A characterization test +now pins the first one. Which fired in the reported run depends on whether a config reload +occurred, which only the reporter knows, so that question was asked instead of guessed. + +**#3433.** The Chat bridge really has no `session_id` synthesis while the Claude bridge does. +But Claude gates its synthesis on `cacheKeySource === "metadata"` precisely because a shared +cohort key's backend semantics are unproven, and the Chat path has no equivalent provenance. +Mirroring it unconditionally would bind unrelated callers onto one upstream session -- a worse +bug, and one that would fail in the same intermittent way. Posted with the suggestion that +Hermes send `session_id` directly, since it is already in `FORWARD_HEADERS` and would confirm +the diagnosis with no proxy change. + +The shared lesson: a plausible fix that the existing tests already contradict is worse than a +diagnosis, because it looks like progress. diff --git a/devlog/_plan/260904_bug_backlog_closeout/070_closeout.md b/devlog/_plan/260904_bug_backlog_closeout/070_closeout.md new file mode 100644 index 0000000000..fb58f8357c --- /dev/null +++ b/devlog/_plan/260904_bug_backlog_closeout/070_closeout.md @@ -0,0 +1,87 @@ +# 070 — closeout + +Terminal outcome for the unit: **DONE**, with two items deliberately ending as +NEEDS_HUMAN and four as posted needs-info. Nothing was left silently open. + +## What landed on dev + +| PR | Author | dev sha | +|----|--------|---------| +| #3430 | ChickenBreast-ky | 4b53e1044 | +| #3401 | agentHits | 0f2e12099 | +| #3420 | ildunari | fc70555f3 | +| #3405 | adtumk | 20011a1c4 | +| #3439 | lidge-jun | 8401b68db | + +Issues closed: #3428, #3400, #3378, #1527. + +## What the merge train got wrong, and what caught it + +Every one of the four contributor PRs was green on its own head, and the merge order was +audited for file overlap and semantic interaction before any of them landed. Both of those +checks passed, and the train still put two failing tests on `dev`. + +The reason is structural: a per-PR gate tests each change against the `dev` it branched +from, never against the other changes in flight. #3430's own test pinned a downstream status +that a different code path answers differently, and #3401's TTY change invalidated a test +fake in a file it does not touch. Neither is visible until they share a tree. + +The post-merge `dev` run is the only place that interaction appears, which is why it was +checked rather than assumed green. If this train had ended at "all four merged, all four +were green", `dev` would have stayed red and every contributor branching from it would have +inherited two failures that were not theirs. + +## What was rejected, and why that is the useful part + +Two planned fixes were discarded after reading the code they would have changed: + +- **#3425's quota-selector fix** was contradicted by `tests/codex-routing.test.ts:325`, which + already proves a known-100% account rotates away. The change would have been a no-op that + additionally broke the never-primed case the source comments defend. +- **#3433's blanket `session_id` synthesis** would have bound unrelated callers sharing a + cohort key onto one upstream session. Claude's implementation gates on + `cacheKeySource === "metadata"` for exactly that reason; the Chat path has no equivalent + provenance to gate on. + +Both are recorded with their reasoning rather than quietly dropped. A plausible fix that the +existing tests already contradict is worse than an honest diagnosis, because it reads as +progress and ships a regression. + +## Attribution + +- #3403 was fixed in place on `ianlyoo:fix-dotted-tool-alias` so the PR stays authored by + @ianlyoo, with `Co-authored-by` on commit e7fe8dc6e. +- #3439 carries `Co-authored-by` for @ChickenBreast-ky and @agentHits, whose tests it repairs. +- #3348 was offered the choice of splitting its own stack rather than being superseded + unilaterally, with a `Co-authored-by` commitment if it is carried. + +## Recorded exception + +#3439 was merged with the owner `pull_request` bypass. GitHub refuses self-approval and +"Authors do not approve their own pull requests" governs regardless, so an ordinary review +was unavailable for a maintainer-authored fix. The bypass is recorded on the PR itself with +its reasoning, as MAINTAINERS.md requires, and @Ingwannu was asked for post-hoc review. + +Holding it would have kept `dev` red for the duration. + +## Filed + +#3441 — `npm-global windows-latest` intermittently cancels at the global install step. Seen +on four runs across three unrelated branches, so it predates this work. Filed rather than +worked around, per the standing instruction about Windows failures. + +## Final dev state: green + +`dev` at `5ea3f2089` passes every job — `test 1/4` through `4/4`, `macos`, `gates`, the three +keyring jobs, `storage policy`, `api usage`, and `ci`. + +Reading the intermediate red honestly matters here. The run on `8401b68db` — this unit's own +repair commit — was still red, and it would have been easy to read that as the repair having +failed. It had not: every failure on that sha traced to `tests/oauth-manual-code.test.ts:63` +tripping `privacy:scan` on a Muse key fixture introduced by #3437, which is why `gates` and +the `macos` suite both failed with the same message. #3443 fixed that fixture, and on the +next sha the shards that this unit repaired — `test 2/4` and `test 3/4` — are green. + +Two separate regressions overlapped on the same branch within the same hour, from different +authors, and each initially looked like the other's. Attributing a red run to the change that +happens to be on top of it is the mistake that was available at every step here. diff --git a/tests/codex-routing.test.ts b/tests/codex-routing.test.ts index bcfdebc9b8..8a27f92bd3 100644 --- a/tests/codex-routing.test.ts +++ b/tests/codex-routing.test.ts @@ -524,6 +524,37 @@ describe("codex routing", () => { expect(resolveCodexAccountForThread("next", config)).toBe("a"); }); + test("a stale writer generation drops the failure entirely, so the streak never trips (#3425)", () => { + // #3425: 118 consecutive 502s to one account with sendCount 1 and no recoveryKinds, and + // rotation only after a MANUAL pause. The quota selector is not the cause -- a known 100% + // account already switches (see the exhaustion tests above). This is the path that can + // swallow the evidence instead: recordCodexUpstreamOutcome returns before any health write + // when the writer's captured generation predates the last reconcile and the account is not + // in the live set. consecutiveFailures never increments, so upstreamFailoverThreshold is + // unreachable no matter how many failures arrive. + const config = makeConfig(); + updateAccountQuota("a", 10); + updateAccountQuota("b", 20); + expect(resolveCodexAccountForThread("stale-writer", config)).toBe("a"); + + // Far more failures than the threshold of 3, every one carrying a stale generation. + for (let i = 0; i < 10; i += 1) { + recordCodexUpstreamOutcome(config, "a", 502, { writerGeneration: -1 }); + } + + // Characterization, not an endorsement: nothing was recorded, so the account keeps + // serving. A fix for #3425 should turn these two assertions around. + expect(getCodexUpstreamHealth("a")).toBeNull(); + expect(resolveCodexAccountForThread("stale-writer-next", config)).toBe("a"); + + // The same failures WITHOUT the stale generation do trip the streak, which is what + // isolates the guard as the difference rather than the 502 classification. + recordCodexUpstreamOutcome(config, "a", 502); + recordCodexUpstreamOutcome(config, "a", 502); + recordCodexUpstreamOutcome(config, "a", 502); + expect(resolveCodexAccountForThread("healthy-writer-next", config)).toBe("b"); + }); + test("401 credential outcome quarantines the account for future threads", () => { const config = makeConfig(); updateAccountQuota("a", 10);