docs(devlog): record the 260904 bug backlog closeout #3446
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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. | ||
|
Comment on lines
+3
to
+4
There was a problem hiding this comment.
The closeout declares the unit terminal even though AGENTS.md reference: AGENTS.md:L83-L86 Useful? React with 👍 / 👎. |
||
|
|
||
| ## 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 | ||
|
Contributor
There was a problem hiding this comment. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Use a lint-safe form for issue IDs at paragraph starts.
Also applies to: 69-69 🧰 Tools🪛 markdownlint-cli2 (0.23.2)[warning] 60-60: No space after hash on atx style heading (MD018, no-missing-space-atx) 🤖 Prompt for AI AgentsSource: Linters/SAST tools |
||
| "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. | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 }); | ||
|
There was a problem hiding this comment.
This test passes Useful? React with 👍 / 👎. |
||
| } | ||
|
|
||
| // 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"); | ||
| }); | ||
|
|
||
|
Comment on lines
+527
to
+557
Contributor
There was a problem hiding this comment. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: #!/bin/bash
set -euo pipefail
rg -n -C 8 \
'beforeEach|afterEach|lastReconciledGeneration|liveHealthAccountIds|reset.*Codex|clear.*Health' \
tests/codex-routing.test.ts src/codex/routing.ts || true
sed -n '2180,2210p' src/codex/routing.ts
sed -n '1,90p' tests/codex-routing.test.tsRepository: lidge-jun/opencodex Length of output: 43779 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- target test ---'
sed -n '500,565p' tests/codex-routing.test.ts
printf '%s\n' '--- reconciliation import and calls ---'
rg -n -C 5 'reconcileCodexRoutingHealth|captureConfigGeneration' tests/codex-routing.test.ts src/codex/routing.tsRepository: lidge-jun/opencodex Length of output: 7686 🏁 Script executed: #!/bin/bash
set -euo pipefail
rg -n -C 4 'reconcileCodexRoutingHealth' --glob '!node_modules' --glob '!dist' --glob '!build' .Repository: lidge-jun/opencodex Length of output: 2757 Set both stale-generation preconditions explicitly. At 🤖 Prompt for AI Agents |
||
| test("401 credential outcome quarantines the account for future threads", () => { | ||
| const config = makeConfig(); | ||
| updateAccountQuota("a", 10); | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Add the required comment URLs for the
NEEDS-HUMANrows.Rows
#3352and#3433useNEEDS-HUMAN, but their Evidence cells contain descriptions only. Lines 54-55 require a posted comment URL for this outcome. Add the exact URL for each row so the disposition is auditable.🤖 Prompt for AI Agents