stack 6/6: triage the overnight PRs and fix the #955 defects they found - #973
stack 6/6: triage the overnight PRs and fix the #955 defects they found#973lidge-jun wants to merge 12 commits into
Conversation
#967 found two real defects in my own #955 code and both verify at runtime: a Team account with a monthly window could never recover because the predicate picked its window by plan name while the parser picks by window duration, and the probe's own token refresh was mistaken for an external credential replacement. #963 and #965 both claim #962; #965 wins because it inherits from the row it actually replaces rather than recomputing config hints, and because #963 rewrites an existing regression contract to justify a broader change. #966 is a fifth design for #914 that survives two of the four prior falsifications but not all: mixed 5xx-then-rejection still loses the attributable failure, and five newly-classified sidecar paths keep default redirects, so a credential-visible 307 to a dead host still reads as neutral.
…ned refresh generations Addresses the two unresolved Codex review threads on #955: - isCompleteCodexQuotaRecoverySnapshot() required weeklyPercent for every non-Go/Free plan by plan name, but the parser classifies windows by duration: a Team response with an explicitly monthly primary window parses to monthlyPercent only, so those accounts could never recover early and stayed cooled until their predicted expiry. - settleCodexQuotaRecoveryProbe() required the claim-time credential generation to match exactly. A probe-owned token refresh inside getValidCodexToken() advances the generation by one before WHAM completes, so a successful fresh reading was rejected and the account waited another probe interval. replacedAt is preserved by refresh and stamped by external replacement, so it fences the +1 transition. (cherry picked from commit 79d2164)
(cherry picked from commit c5565d0)
(cherry picked from commit 3f7e4cf)
(cherry picked from commit 46756f5)
(cherry picked from commit a366934)
(cherry picked from commit 9ba66a1)
… recovers a weekly plan #967 correctly found that requiring weeklyPercent by plan name stranded Team accounts whose primary window is explicitly monthly. Its remedy — accept any window the parser wrote — was too permissive in the other direction: a tertiary-only response also writes monthlyPercent, describes a different period, and says nothing about the weekly quota that actually gates the account, so it could clear a cooldown on a reading of the wrong window. parseUsageQuota() now records monthlyIsPrimaryWindow when the monthly value came from an explicitly-monthly PRIMARY window, and recovery requires that provenance before accepting monthly-only evidence for a weekly-quota plan. Go/Free are unaffected: the monthly window governs them either way. The two shapes were previously indistinguishable — both parsed to {monthlyPercent} with no way to tell which window produced it.
Recovery reads freshQuota directly, so this is not on its path today — but setAccountQuotaFromParsed() copies fields one by one, and a cached snapshot that kept monthlyPercent while dropping monthlyIsPrimaryWindow would look exactly like tertiary-only data to any future reader. A flag that silently fails to persist makes the guard decorative, and that failure would be invisible rather than loud.
…er parser too The audit noted the cache round trip was untested — so the guard could have been silently reduced to decoration by a later refactor. The test now asserts the flag survives setAccountQuotaFromParsed(), and ablating that copy fails it. parseUpstreamQuotaHeaders() recognizes the same explicitly-monthly primary window and now records the same provenance. It is not on the recovery path today, but two parsers disagreeing about what a bare monthlyPercent means is the kind of divergence that surfaces later as an unexplainable bug.
…enance Nine tests asserted parseUsageQuota()/getAccountQuota() output with toEqual, so the new monthlyIsPrimaryWindow field failed them on shape while every value was unchanged. Each expectation now states which side of the distinction it is on, which is the thing those tests were already about: - explicit-monthly PRIMARY windows carry the flag - the Go/Free thirtyDayOnly branch does not (recovery never consults it there) - a tertiary-sourced monthly value does not, which is the case that made the guard necessary - a credits-only refresh preserving prior usage does not The cached monthly-A snapshot now carries it too, proving propagation through setAccountQuotaFromParsed() rather than only asserting the parse.
The last copy site of the same class: an unrelated weekly update rebuilt the record and carried monthlyPercent forward without its provenance, silently downgrading a proven explicit-primary reading to unproven. The mirror case matters as much — a caller-supplied monthly value arrives with no window information, so it must REPLACE the proof rather than inherit it. Both directions are now pinned, and ablating the carry fails the test.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 74c4f765e6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| **Falsification 3 survives on five expanded surfaces.** Manual redirects were | ||
| added only to Responses and Compact; the five sidecar paths #966 newly | ||
| classifies still use default-follow fetch, so a credential-bearing sidecar that | ||
| receives a 307 to a dead host is misclassified as neutral — after the origin | ||
| already read the `Authorization` header: |
There was a problem hiding this comment.
This tracked _plan note publishes the exact credential-bearing redirect counterexample and live reproduction while the document later states that #914 remains open and #966 is not mergeable. Pushing this commit would disclose bypass reasoning and reproduction details before the fix ships, and deleting them later would not remove them from Git history; keep this material in .tmp/ or mktemp scratch space and publish only the fixed outcome.
AGENTS.md reference: AGENTS.md:L61-L69
Useful? React with 👍 / 👎.
| const enrichedByName = new Map(activeProviders); | ||
| // Provider-derived rows keyed by their Codex-facing slug: a custom override replaces the row | ||
| // with the same slug below, so that row's provider capability metadata is the inheritance source. | ||
| const replacedByRoutedSlug = new Map(all.map(model => [routedSlug(model.provider, model.id), model])); |
There was a problem hiding this comment.
When a provider exposes both a slash-containing ID such as foo/bar and the literal ID foo-bar, routedSlug() maps both to the same key. This map therefore retains whichever row appears last, so a custom override for the exact foo-bar model can inherit the context, modalities, or reasoning metadata of foo/bar, even though routing gives the literal native ID precedence. Index the inheritance source by provider plus raw model ID, or explicitly reject ambiguous encoded-slug collisions.
Useful? React with 👍 / 👎.
3 participants
Stack
6/6 — overnight PR triage
Base:
codex/915-cooldown-recovery-probe(#955)Summary
Nine PRs arrived overnight while this stack was in review. This layer carries the three that belong here, and records an evidence-backed disposition for the other six.
All carried commits keep their authors (
cherry-pick -x, patch-id verified identical).#967 found two real bugs in my #955, and both verify
Monthly-classified snapshots were rejected. My predicate picked the required window from the plan name; the parser picks it from the window duration. A Team account whose primary window is explicitly monthly parses to
monthlyPercentonly, so every successful fresh read was thrown away:That is the same failure #915 exists to fix, reintroduced for monthly-window plans — the third time this predicate has been wrong in the same direction.
The probe's own token refresh looked like a replacement.
getValidCodexToken()refreshes a near-expiry token mid-probe and advances the credential generation by one; my settle required an exact match, so a valid fresh reading was discarded and recovery waited another interval. Fenced onreplacedAt, which is preserved by a CAS refresh and stamped fresh by a real replacement.Where I disagreed with #967, and why
Its remedy for the first defect — accept whatever window the parser wrote — is too permissive in the other direction. A tertiary-only response also writes
monthlyPercent, describes a different period, and says nothing about the weekly quota that actually gates the account. The two were literally indistinguishable:So this adds provenance at the source instead of guessing at the sink:
parseUsageQuota()recordsmonthlyIsPrimaryWindowwhen the value came from an explicitly-monthly primary window, and recovery requires it before accepting monthly-only evidence for a weekly-quota plan. Go/Free are unaffected — the monthly window governs them either way.The flag is propagated through every copy site (
setAccountQuotaFromParsed,updateAccountQuota, the credits-only and weekly-preserving branches,parseUpstreamQuotaHeaders) and pinned by tests, because a flag that silently fails to persist makes the guard decorative.Why #963 loses to #965
Both claim #962. #962 is about a custom row replacing a same-slug provider row, and #965 models exactly that — it inherits from the row actually being replaced, so it also retains live
/modelsmetadata. #963 recomputes config hints for every custom row including unmatched ones, cannot retain discovered metadata, and rewritestests/catalog-vision-sidecar-modalities.test.tsfrom "no registry reasoning leaks onto an unmatched override" to expecting that leak while dropping threefetch should not be calledguards. Changing a test that encodes a deliberate prior decision, to make a broader change pass, is what decided it.Why #966 stays open
It is a genuine advance on the four previously-falsified #914 designs — real Bun labels, no hostname probing, manual redirects on the pool paths. But two falsifications survive, both reproduced live:
fetchWithTransientRetry()discards a prior 503 when a later attempt rejects, so a real account failure is recorded as neutral; and the five sidecar paths it newly classifies still use default-follow fetch, so a credential-visible 307 to a dead host reads as neutral after the origin has already seen theAuthorizationheader. It does supersede #922.Verification
bun x tsc --noEmit— exit 0bun run test— 7740 pass, 8 skip, 0 fail, 508 filesbun run privacy:scan— passedNine pre-existing
toEqualassertions needed updating for the new field — values unchanged, and each now states which side of the provenance distinction it is on.Audit
Four rounds, three FAIL. The reviewer caught the tertiary-only over-permissiveness, then two further copy sites of the same class. Every finding was reproduced at runtime before fixing.