diff --git a/devlog/_plan/260828_quota_reset_detection/000_plan.md b/devlog/_plan/260828_quota_reset_detection/000_plan.md new file mode 100644 index 0000000000..ce77bd0420 --- /dev/null +++ b/devlog/_plan/260828_quota_reset_detection/000_plan.md @@ -0,0 +1,160 @@ +# Quota reset detection and notification + +Unit: `260828_quota_reset_detection` +Branch: `codex/quota-reset-detection` (target `dev`) +Class: C4 (new subsystem, config surface, background timer, outbound network sink) + +## Objective + +When a usage window resets, opencodex should notice and say so exactly once. + +Two reset shapes matter and they are not the same event: + +- **scheduled** — the window's own clock ran out. The previous snapshot carried a + `resetAt` in the future, wall-clock passed it, and the next snapshot reports a + lower used-percent. This is the weekly/5-hour rollover an operator can already predict. +- **surprise** — used-percent drops while the previous `resetAt` is *still in the + future*, or `resetAt` jumps forward before its own deadline. Upstream moved the window + out of band. Nobody can predict this one, which is exactly why it needs a signal. + +The deliverable is detection plus a default-OFF notification sink, not a routing change. + +## Constraints + +- Bun-native TypeScript, strict `tsc`. No Node-only APIs. +- `src/router.ts`, `src/server/lifecycle.ts`, `src/server/responses/core.ts` must not + gain a transitive `src/lab/` import (`tests/core-lab-boundary.test.ts`). The new + subsystem is itself optional and must not become a second core-path passenger. +- Notification default OFF. A user with no reset config runs no timer and invokes no sink. +- Event payloads carry closed-union labels and numbers only. No account ids, no emails, + no tokens, no paths. `bun run privacy:scan` scans *repository files*, not runtime + output, so payload privacy is a design obligation the scanner cannot enforce. +- `src/codex/reset-credit-recovery.ts` owns credit *consumption* and stays untouched. + A deliberate credit redemption is not a surprise reset. + +## Current state (verified 260828) + +Detection is absent. `rg -ni 'resetdetect|quotareset|reset-event|resetEvent' src` returns +three hits, all inside `function quotaResetAt(...)` in `src/providers/quota.ts:1646` — a +DTO field reader. Notification is absent: `rg -n 'webhook' src scripts docs-site/src` +returns zero matches. + +What does exist, and what the design leans on: + +| Fact | Location | +|---|---| +| Codex per-account windows (`weeklyResetAt`, `shortResetAt`, `monthlyResetAt`, `resetCredits`) | `src/codex/quota.ts:7` | +| The one writer holding both prev and next in scope | `src/codex/quota.ts:274` (`const existing = accountQuota.get(accountId)`) | +| Commit points that snapshot becomes durable through | `src/codex/quota.ts:289`, `:336` | +| Disk snapshot, version 1, 6-hour read-side age limit | `src/codex/quota.ts:40`, `:41`, `:485` | +| Provider-side windows (`fiveHourResetAt` etc.) | `src/providers/quota.ts:93` | +| Provider-level snapshot commit — the ONLY place a newer report displaces an older one | `src/providers/quota.ts:2343`, with `previous` in scope at `:2290` | +| Provider per-account cache replacement sites | `src/providers/quota.ts:1585`, `:1592`, `:1603` | +| Provider quota has NO background refresh — one caller, request-driven | `src/server/management/provider-routes.ts:421` | +| Reset-sentinel normalization (`0`/negative are not clocks) | `src/providers/quota.ts:279` | +| Opt-in background job pattern (unref'd timer, gate in the callee) | `src/storage/policy-scheduler.ts:13`, `src/storage/policy-job.ts:445` | +| Bounded ring + snapshot accessor for a read route | `src/server/memory-watchdog.ts:48` | +| Optional-subsystem teardown registry | `src/lib/optional-shutdown-hooks.ts:32` | +| Strict optional config section template | `src/config.ts:843`, `:898`, `:2058`, `:2179` | +| SSRF policy for an operator-supplied URL | `src/lib/destination-policy.ts:377` | + +## Four traps the design has to survive + +These are the reasons a naive "percent went down, fire" detector is wrong here. + +1. **Credits-only writes rewrite `updatedAt` with byte-identical windows.** + `src/codex/quota.ts:276` (`creditsOnly`) copies every window field from `existing` + and changes only `resetCredits`. Keying on `updatedAt` fires on nothing. +2. **Writers never hydrate from disk.** `hydrateAccountQuotasFromDisk` is called by the + three readers only (`src/codex/quota.ts:511`, `:516`, `:542`). A cold-start write can + see `existing === undefined` while a valid snapshot sits on disk. Treating absent-prev + as a reset invents an event on every restart. +3. **Rows get deleted for reasons that are not resets.** Reauth clears the row on purpose + (`src/codex/auth-api.ts:2019`), reconciliation drops non-live accounts + (`src/codex/quota.ts:540`), and account purge clears it + (`src/codex/account-lifecycle.ts:39`). Delete-then-readd looks like 0% arriving fresh. +4. **Header writes are partial snapshots.** `src/server/responses/core.ts:3793` writes on + every pooled response and may omit the burst tuple entirely; the merge at + `src/codex/quota.ts:323` carries forward what the payload lacks. A detector must diff + the *committed* snapshot, not the incoming payload. + +5. **Provider reports are keyed by provider, not by account.** `clearProviderQuotaCache()` plus + an account switch makes the next `anthropic` report a *different account's* usage — lower + percent, different `resetAt`. That is an identity change, not a reset. Events must be keyed + by `(provider, account, window)`. +6. **Provider quota is never refreshed on its own.** `fetchProviderQuotaReports` has exactly + one caller — the `/api/provider-quotas` route. With no dashboard open and no CLI call, no + two consecutive snapshots exist, so a reset passes unobserved indefinitely. The opt-in + poller in wp3 is therefore load-bearing, not a nicety. +7. **Two `normalizeResetAt` implementations disagree.** `src/providers/quota.ts:279` treats + `<= 0` as a sentinel and scales seconds to ms; `src/codex/quota.ts:192` admits `0` and + does no scaling. The detector normalizes at its own boundary rather than trusting either. + +Consequence: absent-prev is never a reset, identity is `(scope, account, window)`, and window +values — not the write timestamp — decide whether anything happened. + +Observation cadence is also bounded by design: the provider cache TTL is 5 minutes +(`src/providers/quota.ts:37`) and the per-account TTL is 10 (`:1425`), so a reset instant can +only ever be bracketed between two observations, never timestamped exactly. Events carry +`detectedAt` and the observed `resetAt`, and never claim to know when the reset occurred. + +## Detection contract + +``` +observe(scope, windowLabel, prev, next, now) -> ResetEvent | null +``` + +`kind: "scheduled"` requires `prev.resetAt !== undefined && now >= prev.resetAt` and +a percent drop. `kind: "surprise"` requires a material percent drop (>= 5 points, so +rounding noise cannot trip it) while `prev.resetAt` is still ahead of `now`, or +`next.resetAt` advancing past `prev.resetAt` before that deadline. Every other +transition, including any missing `prev`, returns `null`. + +Idempotence key: `scope | windowLabel | resetAtBucket`. Persisted, because "exactly once" +has to hold across a restart, and the whole point of a surprise reset is that it happens +while nobody is watching. + +## Work-phase map (dependency-ordered) + +Locked at the close of the wp1 docs cycle. Files named here are the authoritative +deliverable list; a later cycle amends its own doc rather than reinterpreting this table. + +| Phase | Doc | Delivers | New files | Depends on | +|---|---|---|---|---| +| wp1 | `000`, `001`, `010`–`040` | roadmap, contract, 7 traps, audit response | 6 docs | — | +| wp2 amendment | `002_wp2_audit_response.md` | the 9-blocker A-gate response | 1 doc | wp1 | +| wp2 | `010_phase2_detection_core.md` | pure detector + durable claim store | `src/quota/reset-detector.ts`, `src/quota/reset-seen-store.ts`, 2 test files | wp1 | +| wp3 | `020_phase3_observation_wiring.md` | codex + provider seams, opt-in poller | `src/quota/reset-observer.ts`, `src/quota/reset-poller.ts`, 1 test file; edits `src/codex/quota.ts`, `src/providers/quota.ts`, `src/server/background-lifecycle.ts` | wp2 | +| wp4 | `030_phase4_sinks_and_surface.md` | config section, sinks, event ring, API + CLI | `src/quota/reset-notify-config.ts`, `src/quota/reset-sinks.ts`, `src/server/management/quota-reset-routes.ts`, 1 test file; edits `src/types/config.ts`, `src/config.ts` (schema, register, write-validate, warn ×3, `validFileConfigDiagnostics`), `src/cli/config-command.ts` (redact `webhookUrl`), `src/server/management-api.ts`, `src/cli/provider-runtime.ts`, `src/cli/registry.ts` | wp3 | +| wp5 | `040_phase5_hardening_delivery.md` | boundary guard, full gates, docs, evidence, PR | `tests/quota-reset-core-boundary.test.ts`, `050_activation_evidence.md`, `060_closeout.md`; edits 3 docs-site pages | wp4 | + +Ordering is structural: nothing can be wired before the contract exists, no sink can fire +before something detects, and delivery proves the whole chain. Each phase closes with +something independently verifiable. + +## Out of scope + +Routing/failover reaction to a reset; automatic credit consumption; GUI work beyond what +an operator needs to read the event log; any credential or OAuth change; `src/lab/`. + +## Verifiers (run, not assumed) + +| Command | Exit | Observes this change? | +|---|---|---| +| `bun x tsc --noEmit` | 0 on baseline 295860825 | Yes — `tsconfig.json` includes `src/**/*.ts` | +| `bun test tests/.test.ts` | 0 (8 pass on `codex-quota-parser-parity`) | Yes — names the new test file directly | +| `bun run test` | full suite | Yes | +| `bun run privacy:scan` | 0 | Repository text only — NOT runtime payloads | +| `bun test tests/core-lab-boundary.test.ts` | 0 | Yes — walks the runtime import graph | +| `bun test tests/quota-reset-core-boundary.test.ts` | added in wp5 | Yes — the existing Lab guard hardcodes `/src/lab/` (`tests/core-lab-boundary.test.ts:63`) and cannot see `src/quota/` | + +`bun install` was required first: a fresh worktree fails with +`Cannot find module 'zod/v4'` and every focused run reports a spurious single error. + +## Bypass ledger + +The default-OFF guarantee is enforced by a test (E7-class), not by anything unbypassable. +Executing surface: `bun run test`. Known bypass: a contributor who wires the sink into a +path the test does not observe. Residual risk: a future caller invoking the sink directly +rather than through the gate. Final enforcement layer: none — the boundary is the test plus +review. Wording is deliberately "early warning", not "enforcement". diff --git a/devlog/_plan/260828_quota_reset_detection/001_audit_response.md b/devlog/_plan/260828_quota_reset_detection/001_audit_response.md new file mode 100644 index 0000000000..d050774ddf --- /dev/null +++ b/devlog/_plan/260828_quota_reset_detection/001_audit_response.md @@ -0,0 +1,93 @@ +# A-phase audit response + +Two dispatched grok-4.6 auditors did not return: the first errored with +`Selected model is at capacity`, the second went silent through three bounded wait cycles +and was retired under DISPATCH-RETIRE-01. The audit below was performed directly against +the tree at `c752929d7`. Stating that plainly because a claimed-but-absent reviewer is the +one failure mode the A gate exists to catch. + +## Citation audit — PASS + +All 33 cited `path:line` claims were read back and match. Sample: +`src/codex/quota.ts:274` is `const existing = accountQuota.get(accountId);`; +`src/providers/quota.ts:2290` is the `previous` binding; `:2343` is the `cache = {...}` +commit; `src/config.ts:3161` is `SALVAGEABLE_CONFIG_SECTIONS`. + +## Verifier reality — PASS + +`bun install` then `bun x tsc --noEmit` exits 0 with no output; +`bun test tests/codex-quota-parser-parity.test.ts` reports 8 pass / 0 fail. +`bunfig.toml` pins `[test] root = "tests"` and preloads `./tests/preload.ts`, which is why +a bare `bun test` in a fresh worktree reports one spurious error until `bun install` runs. +That belongs in the plan and is now recorded there. + +## Field chain — PASS + +`rg -n "agentTaskRecovery" src/ gui/src` outside `src/config.ts` returns nothing, and +`tokenGuardian` has only its type declaration plus one comment. There is no config DTO, +no sanitize path, and no docs generator enumerating sections, so `config.ts` + +`types/config.ts` really is the whole chain for an optional section. No missed consumer. + +## Reachability — PASS with one correction + +- A percent DROP does land: `snapshotHasWeekly` (`src/codex/quota.ts:246`) tests + `weeklyPercent !== undefined`, so a lower value takes the `:294` branch and is written. + The merge only carries values FORWARD when the incoming snapshot omits a window. +- The poller keeps its own commit authority. `invalidationEpoch += 1` happens at `:2285`, + then `const epoch = invalidationEpoch;` at `:2286` captures the bumped value, so the + `epoch === invalidationEpoch` check at `:2338` passes for the forced probe itself. My + concern that a forced refresh would lose its own commit was wrong. +- `previous` is non-empty on a poller refresh as long as the cache key is unchanged; the + `:2309` comment only resets it when the provider SET changes, which is a config edit. + +## Blockers folded into the plan + +### 1. HIGH — the boundary claim was unverifiable + +`tests/core-lab-boundary.test.ts:63` tests `next.includes("/src/lab/")`. The guard is +hardcoded to Lab and says nothing about `src/quota/`, so wp5's "verify by hand" was the +only thing standing behind the claim — exactly the situation AGENTS.md describes as "this +paragraph was the only thing holding the guarantee". + +Fix, folded into `040`: wp5 adds a real guard asserting no static runtime edge reaches +`src/quota/reset-` from the four protected entrypoints, reusing the same walker. + +### 2. MEDIUM — `src/server/management-api.ts` is itself protected + +It is the fourth entry in `PROTECTED` (`tests/core-lab-boundary.test.ts:25`), added because +eagerly importing handlers put ~70 modules on every dashboard request. The wp4 route must +therefore be lazy for a second, independently sufficient reason. Recorded in `030`. + +Worth noting the walker deliberately does NOT propagate through `import()` +(`tests/core-lab-boundary.test.ts:76`: "a deferred edge, not a load-time one"), which is +what makes the wp3 lazy-import approach the sanctioned remedy rather than a loophole. + +### 3. MEDIUM — check-and-set was not atomic + +`hasSeenQuotaReset` followed by `markQuotaResetSeen` is two steps. Two observers racing +the same key — a poller tick and a live pooled response — can both read false and both +notify, defeating criterion c-4 under exactly the load that makes detection interesting. + +Fix, folded into `010`: replace both with one synchronous claim. + +### 4. MEDIUM — 30-day pruning could evict a live key + +A monthly window's key can legitimately be older than 30 days while still current, so +pruning by age alone can drop it and permit a duplicate notification. + +Fix, folded into `010`: never prune a key whose `resetAt` is still in the future, and +raise the age floor to 90 days. + +## Residuals accepted, not fixed + +- `sweepExpiredProviderAccountQuotaRows` (`src/providers/quota.ts:1485`) has no caller and + no registration. Wiring it would add a fourth silent row-removal path with the same + misread-as-reset hazard. Out of scope; noted for a separate unit. +- The two divergent `normalizeResetAt` implementations stay divergent. Unifying them + touches every provider parser and belongs in its own unit; the detector normalizes at its + own boundary instead, which is already in the plan. +- `LOCAL_MANAGEMENT_READ_PATHS` (`src/lib/local-management-capability.ts:10`) is an + allowlist for bound local reads used by `doctor`/`health`. The new route does not need + to join it; not adding it is a deliberate choice, not an oversight. + +VERDICT: GO-WITH-FIXES (blockers=4) — all four folded above. diff --git a/devlog/_plan/260828_quota_reset_detection/002_wp2_audit_response.md b/devlog/_plan/260828_quota_reset_detection/002_wp2_audit_response.md new file mode 100644 index 0000000000..9a6027164f --- /dev/null +++ b/devlog/_plan/260828_quota_reset_detection/002_wp2_audit_response.md @@ -0,0 +1,91 @@ +# wp2 A-gate audit response + +The retired plan auditor (grok-4.6, `audit-quota-reset-plan-2`) returned after its third +wait cycle with `GO-WITH-FIXES (blockers=9)` — after I had already audited directly. Both +audits are recorded; this one found things mine did not. I re-verified every blocker I acted +on rather than taking the verdict on trust. + +## Blocker 1 — Critical, and correct. The provider seam could never have fired. + +`src/providers/quota.ts:2290` binds `previous` only when `cache.key === key`. I had read +the `:2309` comment saying the key encodes the provider SET and stopped there. The key is +actually built by `cacheKeyWithAggregationState` (`:193`), which folds +`quotaSignatureValue` (`:155`) — `weeklyPercent`, `weeklyResetAt`, `monthlyResetAt`, +`customWindows`, and `updatedAt` — into a sha256 digest appended to the key. + +A reset changes exactly those values, so the key rotates, so `previous` is `[]`, so the +detector's no-prev rule returns null. On a pooled install the key rotates on every quota +write, since `updatedAt` is in the digest. + +Verified by reading `:2273-2290` and `:193-217`. The wp3 seam claim was wrong: `:2343` is +indeed the only place a newer report displaces an older one, but it displaces under a +DIFFERENT key, which makes the displacement invisible to a cache-key-equality diff. + +Fix folded into `020`: provider observation no longer reads `previous` at all. The detector +owns its own last-seen map keyed by `(provider, accountTag, window)`, which is immune to +cache-key rotation by construction. That map is the same store wp2 already persists. + +## Blocker 2 — High, and correct. Fixed in this B. + +`src/codex/quota.ts:323-329` carries the previous burst tuple forward verbatim when a +header write omits it. So a partial write reproduces the old deadline AND the old percent; +once wall-clock passes that copied deadline, my "an expired clock is sufficient evidence" +rule fired on a snapshot where upstream said nothing — on the once-per-pooled-response path. + +My reasoning for dropping the drop-requirement (catching low-usage rollovers) was sound; the +conclusion was too broad. `scheduled` now requires the expired deadline PLUS corroboration: +either usage fell, or upstream issued a new deadline. A byte-identical carried-forward window +supplies neither. Regression test: "a carried-forward window past its deadline is NOT a +reset". + +## Blocker 4 — High, and correct. Three missed consumers. + +My field-chain audit searched for `agentTaskRecovery` and concluded `config.ts` plus +`types/config.ts` was the whole chain. It missed: + +- `validFileConfigDiagnostics` (`src/config.ts:1957`) — a diagnostics warning surface + SEPARATE from the three `loadConfig` branches, feeding `ocx config show --source`. +- `SECRET_KEYS` (`src/cli/config-command.ts:18`) matches + `apiKey|key|accessToken|refreshToken|idToken|token|password|clientSecret`. `webhookUrl` + matches none of them, so a Slack or Discord webhook — whose secret IS the URL — would be + echoed in plaintext by `ocx config show` and written by `config export`. That is a real + credential-disclosure defect, not a style nit. +- `safeConfigDTO` (`src/server/auth-cors.ts:695`) is an explicit whitelist, so the section + is correctly invisible to the GUI. Right outcome, undocumented. + +All three added to the wp4 file map in `030`, with `webhookUrl` redaction as a named +requirement. + +## Blockers 3, 5, 7, 8 — accepted, folded into their phases + +- **3:** `loadConfig` (`src/config.ts:1805`) is a `readFileSync` plus a full + `safeParse` with no memoization. Calling it per pooled response to ask "is this feature + off" is absurd. The gate becomes generation-cached via `captureConfigGeneration`. +- **5:** `PROTECTED` has FOUR entries and all four reach `src/codex/quota.ts` statically, + so the lazy-import requirement is load-bearing and nothing enforced it. wp5's guard is + parameterized over a target set and gets a synthetic attack case. +- **7:** `Bun.spawn` rejects a string `stdin`; encoded bytes it is. +- **8:** two concurrent forced refreshes make the loser skip both the commit and the notify. + Once observation moves off `cache.key` (blocker 1) the loser still observes, so this + largely dissolves — but the residual window is stated in `020` rather than hidden. + +## Blocker 9 — Low, correct + +`QUOTA_PERSIST_DEBOUNCE_MS` is at `src/codex/quota.ts:43`, not `:493` (that line is the +function). And `000_plan.md` promised docs `010`–`050` for wp1 while `050` is a wp5 +deliverable. Both corrected. + +## Blocker 6 — already fixed before the verdict arrived + +The racy has/mark pair became one atomic `claimQuotaReset` during my own audit. The +reviewer noticed the shipped code already says "claim". + +## Found by me, not the reviewer + +`quotaResetKey` used `resetAt ?? "none"`. For the several provider parsers that never emit +a reset clock, every reset of one window collapsed onto a single key, so the first claim +would have permanently suppressed all later ones. Now falls back to the expired deadline +before "none", and a window with no deadline on either side is not evaluated at all. + +VERDICT ACCEPTED: GO-WITH-FIXES (blockers=9). Two fixed in wp2, seven folded forward, none +rebutted. diff --git a/devlog/_plan/260828_quota_reset_detection/003_wp3_audit_response.md b/devlog/_plan/260828_quota_reset_detection/003_wp3_audit_response.md new file mode 100644 index 0000000000..8b2ce8a4ab --- /dev/null +++ b/devlog/_plan/260828_quota_reset_detection/003_wp3_audit_response.md @@ -0,0 +1,88 @@ +# wp3 A-gate audit response + +A third grok-4.6 reviewer (`review-wp3-observation-wiring`) went silent through four bounded +wait cycles and was retired under DISPATCH-RETIRE-01. Two of three dispatched reviewers have +now failed this way — one on provider capacity, two on silence — so the audits below were run +directly. Recording that rather than implying a reviewer signed off. + +## 1. Does the provider seam actually fire? — PROVEN YES + +This is the question that killed the original design, so it gets a live probe rather than a +reading. Two consecutive committed reports for one anthropic account, driven through the same +calls `notifyProviderQuotaSnapshot` makes: + +``` +after report1 hits: 0 +after report2 hits: 1 kinds: scheduled:5h +payload: {"kind":"scheduled","scope":"anthropic","accountTag":"1aw4hwbh","window":"5h", + "percentBefore":94,"percentAfter":3,"previousResetAt":...,"resetAt":..., + "detectedAt":...,"key":"anthropic|1aw4hwbh|5h|..."} +after report3 hits (idempotent): 1 +``` + +First report is a baseline and fires nothing. The second is detected. A third identical +observation does not re-notify. The cache-key rotation that made the old design dead is now +irrelevant, because the baseline comes from the persisted swap map rather than +`cache.key === key`. + +The payload contains only closed-union labels and numbers — no account id, no email, no path. + +## 2. Lazy-import contract — VERIFIED + +`rg` for a static `import ... from ".../quota/reset-"` in `src/codex/quota.ts` and +`src/providers/quota.ts` returns nothing; only the two dynamic `import()` calls exist +(`src/codex/quota.ts:359`, `:362`; `src/providers/quota.ts:2281`, `:2284`). None of the +four protected entrypoints names `quota/reset-` at all. + +Residual, stated rather than fixed: because the seams do not await, observation order for two +writes in quick succession is promise-resolution order. Both compute the same idempotence key +for the same new deadline, so the claim ledger collapses them to one notification; the only +consequence is which one defines the baseline. wp5's guard will make the no-static-edge half +of this enforceable instead of grep-verified. + +## 3. Found by me: the generation-cached enable gate was stale by construction — FIXED + +The wp2 audit told me to cache the enable check against `captureConfigGeneration()`, and I +did. That was wrong, and I caught it while verifying the reviewer's fourth question myself. + +`configGeneration` is only assigned at `src/lib/state-store-sweeper.ts:149`, inside +`reconcileStateGeneration`, which runs from `reconcileLiveStateStores` on account and +provider changes. Editing `quotaResetNotify` alone never bumps it. So enabling the feature +would have had NO effect until some unrelated account edit happened to reconcile — the exact +"toggling enabled takes effect on the next tick" property the doc claimed. + +Now keyed on the config file's mtime and size, with a 5-second TTL bounding how often the hot +path stats. A config edit is picked up within 5 seconds; a quiet install pays one `statSync` +per 5 seconds rather than a full `safeParse` per request. + +Worth naming the pattern: a cache key that does not actually change when the cached input +changes is worse than no cache, because it converts a performance concern into a correctness +bug that only shows up as "the feature does nothing". + +## 4. Detector regressions since the last review — checked for missed REAL resets + +The tightened rules could in principle suppress a genuine reset. Cases checked: + +- rolling 5h window that genuinely resets: usage falls, so the drop carries it. Fires. +- weekly window at 0% on both sides past its deadline: no drop, but upstream issues a new + deadline, so the corroboration branch fires. +- account that resets while completely unused with NO new deadline: returns null. This is a + deliberate false negative — the snapshot is byte-identical to a carried-forward one, and + there is no way to tell them apart. Recorded as a known limitation. +- rollover immediately followed by heavy use (3% -> 24% past the deadline): returns null via + the rise check. Also deliberate; also recorded. + +## 5. Test honesty + +`settle()` in `tests/quota-reset-observation.test.ts` drains microtasks then waits 5 ms, +which is a race in principle. It is load-bearing only for the two seam tests, and the +observer-contract tests call `observeQuotaSnapshot` synchronously and assert its return +value, so the same behavior is covered without any timing dependency. If CI ever flakes here, +the fix is to assert the synchronous return rather than to raise the sleep. + +Two assertions were weak and are now real: the account-tag test asserts the salt actually +changes the tag across installs (it previously only checked length and the absence of "@", +which any digest satisfies), and the claim-durability test now spawns a real second process +instead of calling a test-only flush. + +VERDICT (direct audit): GO-WITH-FIXES (blockers=1) — the stale enable gate, fixed above. diff --git a/devlog/_plan/260828_quota_reset_detection/004_wp3_review_response.md b/devlog/_plan/260828_quota_reset_detection/004_wp3_review_response.md new file mode 100644 index 0000000000..2ab415ab6a --- /dev/null +++ b/devlog/_plan/260828_quota_reset_detection/004_wp3_review_response.md @@ -0,0 +1,144 @@ +# wp3 adversarial review — response + +Reviewer: independent subagent, dispatched against `f4fcbb547` (HEAD moved to `2e4b3be3e` +mid-review; the reviewer noted this and verified both). Verdict: GO-WITH-FIXES, 4 blockers. + +Every blocker was reproduced here before being accepted, and every fix was driven red +against the pre-fix code before being committed green. Two of the reviewer's proposed +remedies were rejected on evidence and replaced; both are recorded below, because a review +response that only records agreement is not evidence of independent judgement. + +## Blocker 1 (Critical) — out-of-order observations manufacture false resets + +Accepted, reproduced, fixed. + +The seam awaited two `import()` calls before swapping the baseline. Bun does not resolve +concurrent dynamic imports in call order, so a burst arrives reordered. Reproduced through +the real writer with 21 monotonically RISING writes (10% -> 90%, no reset anywhere): + +``` +write order: 10,14,18,...,90 +events: [{"k":"surprise","w":"5h","pb":82,"pa":10}] # 4/4 isolated runs +``` + +The compounding harm is the durable claim: the false event takes the idempotence key, so +the genuine reset on that window is then suppressed permanently. That is what makes this a +correctness defect rather than noise. + +**Fix.** Both seams now serialize observations through a module-level promise chain +reassigned SYNCHRONOUSLY at call time (`pendingObservation = pendingObservation.then(...)`), +so each link starts only after the previous one committed its baseline. The snapshot is also +copied before the boundary, because `next` is the live map value and the following write +mutates it. + +The reviewer's alternative — statically import `window-mapping` and observe synchronously — +was rejected: it adds a static edge from a file that `src/server/responses/core.ts` reaches, +and `tests/quota-reset-core-boundary.test.ts` (added this phase) forbids exactly that. The +promise chain achieves the same ordering guarantee without spending the boundary. + +Evidence, pre-fix vs post-fix, isolated `OPENCODEX_HOME` per run: + +``` +pre-fix: FALSE_EVENT_COUNT: 1 1 1 1 +post-fix: EVENTS: [] [] [] [] +``` + +## Blockers 2 and 3 (High) — the account key was wrong in two ways + +Accepted, fixed together, because both are the same mistake: identity was resolved +asynchronously from mutable global state, after the commit it describes. + +- **Key-auth pool collapse.** `getAccountSet()` reads the OAuth store, so every key in a + key-auth provider's `apiKeyPool` fell through to `"default"`. Rotating from a spent key to + a fresh one inherited the spent key's history and read as a reset. +- **Mid-flight failover.** `promoteAnthropicActiveAccount` rewrites `activeAccountId` during + request routing, so a 429 between the commit and a later async read attributes this + report to a different account. `fetchAnthropicQuota` already captures `probedAccountId` + before awaiting for precisely this reason. + +**Fix.** `providerObservationAccountKey` resolves identity synchronously at the commit site, +and mirrors the discriminator the report cache already uses (`apiKeyPoolEntryId`) instead of +inventing a second notion of identity. + +## Blocker 4 — the trap-3 regression test was vacuous + +Accepted; this was the most useful finding, because the test was green and wrong. + +`tests/quota-reset-observation.test.ts` called `resetQuotaResetStoreForTests()` between the +row clear and the fresh write. No production path does that: real reauth clears the quota +row only, and the observer's baseline lives in a separate file. Removing the line: + +``` +REAUTH_EVENTS: [{"k":"surprise","w":"5h","pb":91,"pa":0}] +``` + +So reauth of a used account fired a false reset on every occurrence, and the test that +existed to prevent it was simulating a state that never happens. + +**Fix.** `forgetLastObservedWindows` in the store, `forgetQuotaBaseline` in the observer +(which owns the salted tag), called from `clearAccountQuota` on the same serialized chain so +it cannot be overtaken by an in-flight observation. The claim ledger is deliberately NOT +released — a cleared row must not re-notify a reset it already reported. + +## Finding 6 (Medium) — fixed, but NOT by the proposed remedy + +The finding is correct: a rolling window's percent decays naturally, and the surprise branch +accepted a bare drop. Confirmed at 88% -> 61% one hour into a 5h window, no reset. + +The proposed remedy — bound the drop by `elapsed/windowLength * previousPercent` — was +implemented, measured, and **rejected**. Decay magnitude cannot be bounded from elapsed time: +the percent that ages out depends on WHEN the usage occurred, so an hour of idling can retire +a burst that all landed in one minute. Measured against the proportional bound, 88% -> 5% +one hour in (a 83-point drop) was suppressed as "explainable decay" while the genuine +27-point decay case it was written for still fired. It was wrong in both directions. + +**What shipped instead:** deadline MOVEMENT against elapsed time. While a window is merely +rolling, its deadline advances by roughly the elapsed gap; a genuine out-of-band reset issues +a deadline a full window into the future, hours beyond a gap measured in minutes. A deadline +that stands still while usage falls is the clearest surprise signature there is, and is +explicitly allowed through. Fails OPEN whenever the evidence is missing. + +## Finding 5 (Medium) — accepted + +The eviction comment described behavior the code did not have: re-setting a key does not move +it in a Map, so the EARLIEST-INSERTED row was evicted — on a real install the long-lived +codex account, while 63 transient rows survived. Fixed with delete-then-set, making it a true +LRU and making the existing comment true. The regression test fails against the old code. + +## Findings 4 and 7 — deferred to wp5, with reasons + +- **Finding 4 (debounce starvation).** Real: a write cadence under 250 ms defers the baseline + write indefinitely, so a SIGKILL loses the baseline. Not a correctness defect in the + detection contract (the trailing write lands once traffic quiesces, and a lost baseline + re-baselines rather than misfires), and the maximum-staleness cap belongs with the other + persistence hardening in wp5. Recorded in `040_phase5_hardening_delivery.md`. +- **Finding 7 (`updateAccountQuota` does not notify).** Has no in-repo caller, but is public + API through `src/codex/auth-api.ts`. wp5 will either notify or state why not. + +## Reviewer claim NOT accepted + +`settle()` flakiness: the reviewer measured it and concluded it is sound (0.51 ms against a +5 ms budget, 14 runs clean including under CPU load). Agreed, and the earlier plan to +rewrite it is dropped. The burst test does not rely on it — it spawns a child process, +because an in-process burst test PASSED against the unfixed seam: earlier tests in the file +leave the observer module cached, and a cached import resolves in call order. Only a cold +module registry reproduces the defect. A test that cannot fail is worth less than no test, +so this one was driven red 3/3 in a child process before being trusted. + +## Boundary guard (the wp3 deliverable itself) + +`tests/quota-reset-core-boundary.test.ts`. `tests/core-lab-boundary.test.ts:63` hardcodes +`/src/lab/`, so nothing enforced the same obligation for `src/quota/`. The walker was +EXTRACTED to `tests/helpers/import-graph.ts` and shared rather than copied, because the Lab +guard already records what a duplicated predicate costs: its own self-test re-declared a +private copy of the matcher and so proved a local literal behaved, not that the guard did. + +Guards: no load-time edge from the 4 protected entrypoints into `src/quota/` (whole +directory, not a `reset-` prefix — a prefix would let a future sibling through); both seams +reach the observer and reach it ONLY dynamically; the composition-root exemption is pinned to +an exact chain and the poller is asserted to pull in nothing at load time. + +Driven red three ways: a static import in `src/router.ts` (4 assertions fail, including the +two files that transitively reach it), a seam converted to a static import (1 fails), and the +observer wiring deleted entirely (the reachability assertion fails, proving the +dynamic-only check is not vacuously satisfiable by absent wiring). diff --git a/devlog/_plan/260828_quota_reset_detection/010_phase2_detection_core.md b/devlog/_plan/260828_quota_reset_detection/010_phase2_detection_core.md new file mode 100644 index 0000000000..423bffb377 --- /dev/null +++ b/devlog/_plan/260828_quota_reset_detection/010_phase2_detection_core.md @@ -0,0 +1,155 @@ +# wp2 — Detection core + +Pure detection plus the durable store that makes "exactly once" true across restarts. +Nothing in this phase touches an existing call path; it closes with its own tests green. + +## NEW `src/quota/reset-detector.ts` + +Pure functions only: no imports from `config`, no clock of its own, no I/O. `now` is a +parameter so tests drive time instead of waiting for it. + +```ts +/** One observed usage window, normalized away from provider-specific field names. */ +export type QuotaWindowObservation = { + /** Closed-union window identity. Custom provider windows arrive as "custom: