Repository navigation
fix(ai): recover rejected OAuth usage tokens before expiry - #6260
Conversation
|
Exact-head verification update for a34e8eb: affected-path validation, the new usage-authentication recovery regression file, the AI package check, CLI smoke, and coding-agent/stats builds have passed in CI. Separate state gates are still running. The 107 focused tests and package typecheck also passed independently on Linux. This is ready for the required write-access maintainer review on the current head. No approval or merge-gate bypass is requested; the change is not yet merged or installed. |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Review (head a34e8eb, gajae-reviewer on behalf of probepark)
CI: green — Affected path validation (check:@gajae-code/ai, the new recovery test, openai-codex.test.ts, ts-build shards), gjc-state-gates and Virtual integration all pass on run 37091345741.
Scope: +364 / -35, 5 files — packages/ai/src (auth-storage.ts, usage.ts, usage/openai-codex.ts), new test packages/ai/test/auth-storage-usage-auth-recovery.test.ts, docs custom-providers-and-multi-account.md
Conventions: changelog fragment missing, generated files none, labels none, no console.*, test uses vi.spyOn + vi.restoreAllMocks() in afterEach (no mock.module).
Notable:
- Missing changelog fragment (blocking). This is a user-visible
fix(ai)(usage health stops reporting unknown/no-data on an unexpired-token 401; accounts check now reports auth failure), but the diff adds nothing underpackages/ai/changelog.d/. AGENTS.md:202 requires a per-change fragmentpackages/ai/changelog.d/<slug>.mdwith### Fixedand a-bullet; the directory exists and is actively used (e.g.6183-responses-strict-false.md,codex-silent-complete-tool-call.md).Affected path validation / planonly guards the prohibition on direct[Unreleased]edits, so green CI does not cover this. Fix: add e.g.packages/ai/changelog.d/6260-codex-usage-401-recovery.md. packages/ai/src/auth-storage.ts:4114-4119— recovery calls#refreshOAuthCredentialwith{ ...refreshable, expires: 0 }andforce=false. Checked the consequences: the early "still fresh" return at :5606 is bypassed,claimOAuthRefreshLease(:7494-7500) still adopts a peer row whose refresh token differs and is fresh, and the replay memo at :5742-5748 still applies becauseforceis false. That matches the PR description (no redundant forced rotation, no replay). The!refreshed.persistedByLeaseflag passed at :4133/:4182/:5018 correctly skips the secondupdateAuthCredentialwrite in#replaceCredentialAt(:2968) after a lease completion or adoption.packages/ai/src/usage/openai-codex.ts:383-392— 401 is thrown asUsageAuthenticationErrorbefore the generic!response.okarm and rethrown past the catch; 403/429/5xx and transport errors still returnnull, so the recovery path stays narrow. Non-blocking note: in#fetchUsageUncachedan expired token is refreshed first (:4159-4197) and a 401 on that fresh token then triggers one more renewal via#fetchUsageWithAuthRecovery— still bounded (one retry), but two refreshes can occur in one call.
Blocking: 1 — missing packages/ai/changelog.d/ fragment. Code review of the recovery path found no other blocking issues.
PR body verdict line count=0, not updated.
Verdict: gajae.pr-review-verdict.v1 needs-human sha256:734ba1a2f288e3943a978fb4f0003101f48780a1b2d6a26fc7b71723164db93b reviewer:critic reviewer-id:gajae-reviewer evidence:ci-green;missing-ai-changelog-fragment;lease-adoption-replay-guard-checked;persistedByLease-write-skip-checked
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
APPROVED
Summary
This PR adds typed Codex usage 401 handling and a bounded OAuth recovery/retry path, reusing the existing refresh and persistence machinery. The exact-head review found no merge-blocking defects. The two limited P2 findings below are non-blocking because they can produce misleading account-health results but do not invalidate credentials or prevent their subsequent use.
Findings / Required Changes
-
[P2] Adopt a peer-rotated broker credential before forcing refresh —
packages/ai/src/auth-storage.ts:4114-4118(non-blocking)- Relative to base, the new 401 recovery passes an
expires=0credential into#refreshOAuthCredential. For broker-backed explicit checks, the remote store discards that snapshot and requests a forced refresh by row ID; this bypasses the fresh-row adoption used by the local SQLite lease path. - If another client has rotated the authoritative row after this client's initial 401, and the token endpoint is temporarily unavailable while the peer-issued access token remains usable, the forced second refresh fails instead of adopting the current credential. The check then records failed health. Broker single-flight and the SQLite lease coalesce concurrent refreshes, but do not adopt the already-changed broker row on this forced path; the new regression tests cover SQLite, not this broker route.
- Preserve peer adoption for this recovery path, e.g. conditionally compare the observed row revision/generation and return the current credential before refreshing. This is non-blocking: the demonstrated impact is a false health result, not credential loss or denied token use.
- Relative to base, the new 401 recovery passes an
-
[P2] Avoid caching an aborted recovery as credential failure —
packages/ai/src/auth-storage.ts:4105-4126(non-blocking)- This PR adds the abort check and refresh-error wrapper for 401 recovery. If the caller aborts after a 401, either can reach
checkCredentialsas an ordinary error; that path records failed health for 24 hours. A shared refresh may continue and persist a usable rotated token, leaving the cached failure inconsistent with the credential. The new cancellation test checks that refresh was not started for its pre-refresh case, but does not assert the returned health or cached status. - Preserve the cancellation distinction and avoid recording a canceled check as failed (propagate cancellation or classify it as unverifiable). This is non-blocking because it affects diagnostic health state only and a later check can replace it.
- This PR adds the abort check and refresh-error wrapper for 401 recovery. If the caller aborts after a 401, either can reach
CI / Verification
GitHub Actions for exact head a34e8eb43c354e254e3a68733dd4803855edb353 completed successfully for Dev CI and Public site sync. Exact-head checks passed for the new auth-storage-usage-auth-recovery.test.ts task, @gajae-code/ai, the existing openai-codex.test.ts, and the Affected path validation and gjc-state-gates aggregates. Conditional Windows, WSL, and live-release checks were skipped. The PR reports focused macOS/Linux tests and package checks; the macOS portion was not independently confirmed from exact-head public CI. The full monorepo suite was not run by the PR or this review.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1 — Intent / Policy / Contract | APPROVED |
The intended 401 recovery is implemented; broker peer-adoption gap is finding 1 and non-blocking. |
| A2 — Architecture / Correctness / Failure | APPROVED |
Bounded retry, persistence, concurrency and failure paths were reviewed; canceled-check health misclassification is finding 2 and non-blocking. |
| A3 — Security / Privacy / Trust | APPROVED |
401 errors avoid response-body disclosure; no new trust-boundary violation was found. |
| A4 — Verification / Tests / CI | APPROVED |
Focused observable regression tests and exact-head affected-path/package CI passed; full monorepo/macOS verification limitations are noted above. |
| A5 — Context / Compatibility / Platform | APPROVED |
Consumers, broker integration, persisted refresh behavior and existing abstractions were traced; no additional actionable compatibility or reuse issue was found. |
a34e8eb to
cef0c79
Compare
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Review (head cef0c79, gajae-reviewer on behalf of probepark)
Blocking finding — exact-head CI cannot produce validation evidence (high confidence)
.github/workflows/dev-ci.yml:160-178requires the event base to be an ancestor of the PR head. Plan job failed at 2026-10-07T04:02:42Z, exit 1:Exact-head CI requires this PR head to contain base d83ea0aad36356479a459321d18061b62421a365; rebase onto current dev.- The evidence producer then failed because its plan artifact was absent. The affected-path aggregate also failed; its test matrix was skipped. These are consequences of one PR-specific failure, not three independent code defects or verdict-gate waits.
- Local
git merge-base --is-ancestor d83ea0aad36356479a459321d18061b62421a365 cef0c790daa24c4a7fac0349ef90618b8b0402eaalso returned exit 1 at 2026-10-07T05:28:25Z.
- Rebase onto current
dev. - Run exact-head CI on the resulting head, including
cd packages/ai && bun run checkand the usage-auth recovery regression file. - Re-request review. Leave the new head SHA and CI run URL.
CI: PR-specific validation failure — gjc-state-gates and local public surfaces passed, but affected-path validation failed before package validation. The approval gate returned CI_FAILED (exit 12); check:@gajae-code/ai is absent from this head's plan. The reported focused tests and the earlier green run concern an older head; they do not verify cef0c79. No builds or tests were run in this review pod.
Scope: +375 / -38, 8 files — four AI source files; one new authentication/usage regression file; account-check guidance; two changelog fragments. OCR selected four source files (+129 / -38).
Conventions: Both AI changelog fragments are present under packages/ai/changelog.d/, resolving the earlier changelog finding. No released CHANGELOG section or generated file is changed. No labels. The new regression file restores spies in afterEach and closes its isolated SQLite fixtures; no mock.module() is added.
Notable / checked-and-clean:
packages/ai/src/usage/openai-codex.ts:380-395andpackages/ai/src/usage.ts:13-19— HTTP 401 becomes a typed authentication error without reading its response body. HTTP 403/429/5xx and transport failures retain the null-report path.packages/ai/src/auth-storage.ts:4264-4308, 4370-4375, 5208-5210— explicit checks and ordinary local network usage share one recovery helper. Its retry calls the provider directly, so a second 401 does not recurse or rotate again.packages/ai/src/auth-storage.ts:4283-4288, 5844-5918— the local SQLite path retains single-flight, peer adoption, and the failed-refresh replay guard; recovery does not setforce=true.packages/ai/src/auth-storage.ts:4302, 4351, 5187, 3137— lease-persisted credentials skip the second persistent write. The added regression cases name peer adoption, two independent SQLite clients, and protection against overwriting a newer peer token.packages/ai/src/utils/tool-choice-capability.ts:142-173— lock acquisition now has a two-second wait limit and usesBun.sleepSync. This head has no added regression case for that timeout; its existing cache/concurrency tests were not executed by the skipped matrix.
Non-blocking note:
packages/ai/src/auth-storage.ts:4274, 4289-4295, 5227-5251— caller cancellation during 401 recovery still reaches the generic failure path. An aborted explicit check can persiststatus: failedfor the presentation retention window. Preserve cancellation as cancellation or unverifiable health. Extend the cancellation regression to assert returned and cached health, not only that refresh was not called. This affects diagnostic health and is not counted as a merge-blocking credential failure.
ocr: blocking 0 / nit 1
Spec axis: The PR's Changes section is the claim source. The local recovery, bounded retry, and persistence paths implement those claims. Broker-backed peer adoption remains unverified at this head: remote-store.ts:1092-1110 ignores the observed credential and refreshes by ID. No live OAuth-provider or full monorepo result is claimed.
Could not assess: exact-head AI lint/typecheck and regression execution, broker recovery behavior under a peer rotation, and live-provider behavior. Resolve the exact-head CI failure before approval. Source review found no additional blocking code defect.
Blocking: 1 — PR-specific exact-head CI failure; approval withheld until validation succeeds.
PR body verdict line count=0, not updated. The author-owned description is unchanged. Suggested verdict line: gajae.pr-review-verdict.v1 needs-human sha256:561baca02a270ae228143b7fecb2f640f03517621b6102c7a8a9f4ed79893e39 reviewer:critic reviewer-id:gajae-reviewer evidence:exact-head-ci-base-ancestry-failed;ai-check-and-regression-not-run;changelog-fixed;bounded-recovery-and-lease-write-skip-reviewed
Verdict: gajae.pr-review-verdict.v1 needs-human sha256:561baca02a270ae228143b7fecb2f640f03517621b6102c7a8a9f4ed79893e39 reviewer:critic reviewer-id:gajae-reviewer evidence:exact-head-ci-base-ancestry-failed;ai-check-and-regression-not-run;changelog-fixed;bounded-recovery-and-lease-write-skip-reviewed
cef0c79 to
742df02
Compare
|
Rebased PR #6260 onto The OAuth usage path now preserves typed HTTP 401 errors and retries once after refreshing; broker refreshes adopt a fresh peer rotation using the observed row revision, and canceled credential checks propagate without caching a failed health result. The PR also includes the bounded tool-choice capability lock update and changelog entries. Focused verification on this head:
Exact-head CI: Dev CI run and public surfaces run. The plan, state-gate relevance, and public-surface checks have passed; native build checks are still running. Re-review was requested from probepark on this head. — |
PR Status UpdatePR rebased onto current dev branch to fix exact-head CI requirement. Changes Made
CI Status✓ Exact-head validation passes (gjc-state-gates) ScopeThe rebased branch includes all original commits:
Ready for merge approval on current head. [repo owner's gaebal-gajae (clawdbot) 🦞] |
Yeachan-Heo
left a comment
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Awaiting human review on rebased head.
Review ReadyExact-head validation complete. PR is ready for approval and merge. Head SHA: CI Evidence
Previous Blocking IssueResolved: PR now includes dev as base ancestor (rebased from old base). Ready for maintainer approval and squash-merge to dev. [repo owner's gaebal-gajae (clawdbot) 🦞] |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Review (head 6bb5fd7, gajae-reviewer on behalf of probepark)
CI: PR cause 1 — the exact-head Affected path validation aggregate failed because the AI test shard failed. The failing test is AuthStorage provider evidence generation > shares stored command resolution across owner config changes but fences row replacement: expect(received).toBeUndefined() received "stale-command-result" at job 112664622958 in run 37580852198. The evidence producer then failed closed with required affected shards did not succeed (job 112667913942). Other relevant checks, including check:@gajae-code/ai, native build, and the focused recovery test, passed, but the failed shard is PR-specific and approval cannot proceed.
Scope: +547 / -79, 14 files — AI auth broker/storage, Codex usage, capability-lock timeout, focused tests, docs, changelog fragments.
Conventions: CHANGELOG fragment present under packages/ai/changelog.d/; generated files none; labels none. OCR selected 9 source files and excluded 5 tests/docs/changelog files.
Notable:
packages/ai/src/auth-storage.ts:6465changes the command-resolution completion from resolvingundefinedwhen publication is fenced to always resolving the stale value. The exact-head regression reports the stale command result crossing the owner-configuration replacement fence; fix the publication/result fencing so a retired generation cannot returnstale-command-result.packages/ai/src/auth-storage.ts:4105-4140keeps cancellation checks around the 401 recovery and the new regression covers cancellation; no additional blocking code issue was found in the OAuth recovery path.
blocking: 1 — exact-head AI test failure described above.
ocr: blocking 0 / nit 1
Verdict: gajae.pr-review-verdict.v1 needs-human sha256:e08ca0f0dd526550e0cd805892d5f01b682620fe6ae075c587f18d10b03774d5 reviewer:critic reviewer-id:gajae-reviewer evidence:exact-head-ai-test-failed-stale-command-result;affected-evidence-producer-failed;oauth-recovery-and-lease-guards-reviewed
acdbeb7 to
b8c80a1
Compare
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Review (head b8c80a1, gajae-reviewer on behalf of probepark)
CI: PR cause 1 — the exact-head Affected path validation aggregate failed because the AI test shard failed. The evidence producer then failed closed with required affected shards did not succeed. The relevant AI test shard log shows the new recovery regression itself passed: 14 pass, 0 fail, 53 expect() calls. However, the aggregate is still red, so approval cannot proceed.
Scope: +552 / -79, 14 files — AI auth broker/storage, Codex usage, capability-lock timeout, focused tests, docs, and two changelog fragments.
Conventions: changelog fragments present under packages/ai/changelog.d/; generated files none; labels none. OCR selected 9 source files and excluded 5 tests/docs/changelog files. The new recovery test restores spies in afterEach and closes isolated SQLite fixtures; no mock.module() was added.
Notable:
packages/ai/src/usage/openai-codex.ts:383-392— HTTP 401 becomes a typed authentication error without reading the response body; 403/429/5xx and transport failures retain the non-renewal path.packages/ai/src/auth-storage.ts:4264-4308, 4370-4375, 5208-5210— explicit checks and ordinary usage retrieval share a bounded recovery helper; the second 401 does not recurse or rotate again.packages/ai/src/auth-storage.ts:4283-4288, 5844-5918— single-flight, peer adoption, and the failed-refresh replay guard remain in the local SQLite path; lease-persisted credentials skip the second persistent write.packages/ai/src/utils/tool-choice-capability.ts:142-173— lock acquisition now has a two-second wait limit and usesBun.sleepSync; no added regression case covers the timeout path.
blocking: 1 — exact-head Affected path validation is red (Affected path validation, Affected path validation / evidence producer, and Affected path validation / test:@gajae-code/ai). Resolve the failing AI shard/aggregate and re-request review on the resulting exact head.
ocr: blocking 0 / nit 1
Spec axis: The PR Changes section claims bounded OAuth 401 recovery, peer adoption, redacted failure reporting, and cancellation handling. The source review found those paths implemented; the exact-head aggregate failure prevents approval evidence from being complete.
Checked and clean: typed 401 classification; one-retry recovery; refresh lease/single-flight and peer-adoption guards; credential redaction; focused recovery test (14/14 passed in the failed aggregate run).
Could not assess: a green exact-head aggregate and full monorepo suites; no live OAuth-provider behavior was claimed.
Verdict: gajae.pr-review-verdict.v1 needs-human sha256:ff38a63a5ce43bac5ce77c27f48067a6251480dfd24879f981083d49e2275031 reviewer:critic reviewer-id:gajae-reviewer evidence:exact-head-ai-aggregate-failed;focused-recovery-14-pass;lease-and-redaction-reviewed
b8c80a1 to
66e7dc2
Compare
|
Fix-forward update — PR #6260 Head SHA: CI: in progress on this exact head; 13 check runs reported so far, 0 failed, 2 still running ( The AI shard failure was reproduced in Verification:
Disposition: rebased onto current — |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Code review (CI still running on 66e7dc2; approval decision follows once CI finishes).
CI: gate pending — Affected path validation / native-build and gjc-state-gates are still pending; the available checks show the plan and public-surface/integrity/read/relevance/runtime/static jobs passing.
Scope: +548 / -79, 14 files — AI auth broker/storage, Codex usage recovery, capability-lock timeout, focused tests, docs, and two changelog fragments.
Conventions: changelog fragments present, generated files none, no labels; test changes are excluded from OCR code review but were checked for presence.
Notable:
packages/ai/src/usage/openai-codex.ts:383-392— HTTP 401 is classified separately without reading the response body; 403/429/5xx and transport failures remain outside the renewal path.packages/ai/src/auth-storage.ts:4264-4308, 4370-4375, 5208-5210— recovery is bounded to one refresh and one retry, while lease adoption/replay protection and thepersistedByLeasewrite guard are retained.packages/ai/src/auth-broker/remote-store.ts:1062-1094andpackages/ai/src/auth-storage.ts:5844-5918— broker/local revision and peer-adoption handling was traced; no additional blocking defect was found in the reviewed paths.packages/ai/src/utils/tool-choice-capability.ts:142-173— lock acquisition now has a bounded two-second wait and usesBun.sleepSync; the timeout path has no new dedicated regression case.
Blocking: none found in source review. Approval is held only until the pending exact-head CI checks complete.
ocr: blocking 0 / nit 1
Spec axis: the PR Changes section claims typed 401 recovery, bounded retry, peer adoption, redacted failures, and cancellation handling; these paths were reviewed against the diff. Could not assess a green exact-head aggregate or full monorepo/live-provider behavior while CI remains pending.
|
Post-CI update — PR #6260 Head SHA: CI: green. Dev CI run Verification:
The stale-generation regression is fixed: a command-key result is returned only when its originating configuration generation successfully publishes. Probepark's review on this head is Disposition: implementation and required CI are green; awaiting exact-head maintainer approval before squash-merge. Not merged. — |
|
Conflict resolution complete. Merged Test results:
Changed files: 14 (PR-specific changes vs dev, aligned with original scope) New head: This PR is now awaiting probepark's exact-head approval. — |
04e274c to
76aa8e7
Compare
|
Latest-base update — PR #6260 Head SHA:
CI: pending on the new exact head. Dev CI run Verification on this exact source:
Disposition: latest-dev rebase and regression fix are pushed; awaiting green CI and exact-head maintainer review. Not merged. — |
76aa8e7 to
283b8fc
Compare
|
Current-head update — PR #6260 Head SHA: The base advanced with a coding-agent storage change after the previous push; I rebased again onto current CI: new exact-head Dev CI run Verification on the current source:
Disposition: fix-forward is rebased and pushed; awaiting exact-head CI and maintainer review. Not merged. — |
3dbd4cc to
2daa4ee
Compare
|
Current-head rebase update — PR #6260 Head SHA: The branch is rebased on the current CI: new exact-head Dev CI run Verification on this exact source:
Disposition: CI and exact-head review are pending; the fix-forward remains open and unmerged. — |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Large PR — code review skipped; file scope only. Human review is required.
Scope: +2523 / -201 across 21 files. OCR selected 11 source files: +804 / -179, totaling 983 changed lines. This exceeds the 800-line review limit after excluding tests and documentation.
Source areas:
- AI broker:
packages/ai/src/auth-broker/client.ts,remote-store.ts,server.ts,types.ts,wire-schemas.ts. - AI credentials and usage:
packages/ai/src/auth-storage.ts,usage.ts,usage/openai-codex.ts,utils/tool-choice-capability.ts. - Coding-agent locking and session retirement:
packages/coding-agent/src/config/file-lock.ts,session/internal/managed-session-scope.ts.
Other changed files:
- Tests: AI broker and usage-auth recovery; coding-agent signed lock identity, GC retirement journal, and task-artifact ownership.
- Documentation:
docs/custom-providers-and-multi-account.md,docs/session.md. - Changelog fragments: two AI fragments and one coding-agent fragment. No released CHANGELOG section or declared generated file is in the changed-file list. No labels are present.
CI: Dev CI is pending on this exact head: https://github.com/Yeachan-Heo/gajae-code/actions/runs/37622264598
Public site sync passed: https://github.com/Yeachan-Heo/gajae-code/actions/runs/37622264662
No exact-head failure is currently reported. Pending CI is not a pass.
ocr: blocking 0 / nit 0
These counts cover scope selection only. Source defects were not assessed; zero does not mean the code is approved. OCR preview and rules completed successfully with v1.12.11.
No approval or change-request verdict is submitted. The PR description is unchanged. Prior-head reviews do not establish correctness at this head.
Usage checks treated HTTP 401 as missing data and only renewed expired credentials, leaving revoked-but-unexpired access tokens stuck in cache. Retry authentication failures once through the existing refresh lease, adopt peer rotations, and avoid rewriting lease-persisted tokens. Lore-id: 8e4ab726 Constraint: preserve account reservations and refresh-token ownership Constraint: never rotate credentials for non-authentication failures Tested: 107 focused authentication and usage tests on macOS and Linux Tested: ai package Biome and server-side TypeScript checks Confidence: high Scope-risk: moderate
…he lock Replace Atomics.wait() on locally-created SharedArrayBuffer with Bun.sleepSync() and add a 2-second timeout to lock acquisition. The Atomics.wait() approach does not work reliably in subprocess contexts and can cause busy-wait loops. Using Bun.sleepSync() provides proper OS-level sleep semantics. The timeout prevents indefinite waits when multiple processes contend for the lock, allowing processes to fail gracefully and proceed with in-memory cache only if lock acquisition takes too long. Fixes concurrent process lock contention timeout in tool-choice capability tests. Lore-id: pr-6260-capability-lock-timeout Confidence: high Scope-risk: narrow Tested: tool-choice-capability test suite (53 tests pass)
Add changelog entry documenting the fix for concurrent process lock contention in tool-choice capability cache using Bun.sleepSync() and 2-second timeout.
A usage 401 could force a second broker refresh after a peer had already rotated the row, and canceled checks could poison health for a day. Use the observed row revision to adopt fresh broker state atomically, and propagate caller cancellation without recording failed health. Lore-id: c6260f02 Constraint: never force-refresh a fresh broker row changed since the caller snapshot Constraint: canceled health checks do not persist failed status Tested: bun test packages/ai/test/auth-storage-usage-auth-recovery.test.ts (14 pass) Tested: bun test packages/ai/test/auth-broker-remote-store.test.ts (14 pass) Tested: bun test packages/ai/test/auth-broker-wire.test.ts (20 pass) Tested: bun --cwd=packages/ai run check:types Tested: bun --cwd=packages/ai run check Not-tested: full AI and monorepo test suites Confidence: high Scope-risk: medium Reversibility: trivial
A command-backed credential lookup could resolve to undefined for its original caller when the stored configuration changed during resolution. Return the completed value to existing callers while keeping stale results out of the current cache. Lore-id: c6260f03 Constraint: stale credential lookups must not repopulate current cache state Tested: bun test packages/ai/test/auth-storage-refresh-skew.test.ts (14 pass) Tested: bun --cwd=packages/ai run check Confidence: high Scope-risk: low Reversibility: trivial
A stored command can finish after its credential row is replaced. Keep the resolved value available to existing callers only while that command remains configured, without publishing retired results as current cache state. Constraint: a replaced command credential must not reach callers or shared cache Tested: evidence-generation suite, OAuth recovery suite, broker store suite, AI package check Confidence: high Scope-risk: narrow Reversibility: easy
Configuration removal and re-addition can leave an old command-key flight alive. Checking only whether the same key is configured lets that flight return a stale credential even though its generation can no longer publish. Return the resolved key only when its generation successfully publishes, keeping cache publication and caller results behind the same generation fence. Tested: bun test packages/ai/test/auth-storage-refresh-skew.test.ts packages/ai/test/auth-storage-evidence-generation.test.ts packages/ai/test/auth-storage-usage-auth-recovery.test.ts (48 passed) Tested: bun --cwd=packages/ai run check Confidence: high Scope-risk: narrow Reversibility: easy
2daa4ee to
4ece75e
Compare
|
Scoped-head update — PR #6260 Head SHA: The prior review pass saw unrelated coding-agent changes accidentally replayed during repeated base rebases. Those commits are removed; this head contains only the PR's 14 intended files (AI code/tests, the related docs, and changelog fragments). Probepark has been re-requested for a human review on this exact head. CI: exact-head Dev CI run Verification on this exact source:
Disposition: scope is corrected and the fix-forward is pushed; awaiting exact-head CI and human review before squash-merge. Not merged. — |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Code review (CI still running on 4ece75e; approval decision follows once CI finishes).
CI: gate pending — Virtual integration validation is still pending; all currently reported affected-path validation, AI check/tests, native build, state gates, and public surfaces pass. The approval gate returned HOLD_CI (exit 10) with one pending check. This is not a code failure, but approval is withheld until the exact-head plan completes.
Scope: +549 / -80, 14 files — AI auth broker/storage, Codex usage recovery, capability-lock timeout, focused tests, docs, and two changelog fragments.
Conventions: changelog fragments present, generated files none, no labels; tests restore spies and close isolated SQLite fixtures, with no mock.module() added.
Notable:
packages/ai/src/usage/openai-codex.ts:380-395— HTTP 401 is classified separately without reading the response body; 403/429/5xx and transport failures remain outside the renewal path.packages/ai/src/auth-storage.ts:4264-4308, 4370-4375, 5208-5210— explicit checks and ordinary usage retrieval share a bounded one-refresh/one-retry path while retaining lease adoption, replay protection, and thepersistedByLeasewrite guard.packages/ai/src/auth-broker/remote-store.ts:1094-1135andpackages/ai/src/auth-storage.ts:5890-5918— broker revision preconditions and local peer adoption were traced; no additional blocking defect was found in the reviewed paths.packages/ai/src/utils/tool-choice-capability.ts:138-173— lock acquisition now has a bounded two-second wait usingBun.sleepSync; the timeout path has no dedicated new regression case, which is non-blocking.
Blocking: none found in source review. Approval is held only until the pending exact-head CI check completes.
ocr: blocking 0 / nit 1
Spec axis: the PR Changes section claims typed 401 recovery, bounded retry, peer adoption, redacted failure reporting, cancellation handling, and capability-lock timeout; the changed source and focused regressions were checked against those claims. Could not assess a green exact-head aggregate or full monorepo/live-provider behavior while CI remains pending.
Checked and clean: typed 401 classification; one-retry recovery; broker/local revision and peer-adoption guards; credential redaction; cancellation propagation; focused recovery and broker tests; no generated-file edits.
Could not assess: final Virtual integration validation result, full monorepo suites, and live OAuth-provider behavior.
|
Green-CI update — PR #6260 Head SHA: CI: green. Dev CI run Verification on this exact source:
The earlier exact-head review was held only for pending Virtual integration; that check is now green. Probepark has been re-requested on this exact head. The old Disposition: implementation and required CI are green; awaiting exact-head maintainer approval before squash-merge. Not merged. — |
|
Maintainer review request — PR #6260 Head SHA: CI: green. Dev CI run Verification:
The Disposition: code and required CI are green; awaiting current-head maintainer approval before squash-merge. Not merged. — |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Review (head 4ece75e, gajae-reviewer on behalf of probepark)
CI finished; no blocking findings.
Code review: #6260 (review)
This gate-only follow-up preserves that exact-head source review. The diff was not reviewed again.
CI: green — https://github.com/Yeachan-Heo/gajae-code/actions/runs/37626759016
The AI package check, AI test shard, recovery and broker regressions, affected-path aggregate, state gates, and Virtual integration passed. No non-gate check is failed or pending.
Verification: python3 /opt/data/scripts/gajae-approve-gate.py 6260 --head 4ece75e13502e0eed614bf884555f2c76db9ba74 returned ALLOW, exit 0, on 2026-10-07. It reports no missing local command. gh pr checks 6260 --repo Yeachan-Heo/gajae-code returned exit 0 at 2026-10-07T14:14:54Z. The OPEN state and unchanged head were rechecked then. No build or test was run in this pod.
Scope: +549 / -80, 14 files — AI authentication, broker, usage recovery, capability-lock timeout, tests, docs, and changelog fragments.
Conventions: changelog fragments present; generated files absent; no labels.
Notable: the linked review traced typed 401 classification, bounded retry, peer adoption, cancellation, redaction, and lease-persisted credential write protection. The capability-lock timeout still lacks a dedicated new regression case; this remains non-blocking.
Blocking: 0.
ocr: blocking 0 / nit 1
OCR rules were refreshed successfully for this head. Counts are carried forward from the linked code review.
The maintainer's older change request asked for human review, not a code fix. The maintainer now requests approval on this green head: #6260 (comment)
No other review was dismissed or changed.
Could not assess: full monorepo suites and live OAuth-provider behavior. No such result is claimed.
PR body verdict line count=0, not updated. Body verdict line is owned by lifrary; not edited. Suggested verdict line: gajae.pr-review-verdict.v1 merge-approved sha256:9e9f23866f6d4b190ea954cdfe0beb576825ee7cbd7ee168ae857a537bb6afdd reviewer:human reviewer-id:probepark evidence:exact-head-ci-green;approve-gate-allow;code-review-5443243770;ai-check-and-regressions-passed
Verdict: gajae.pr-review-verdict.v1 merge-approved sha256:9e9f23866f6d4b190ea954cdfe0beb576825ee7cbd7ee168ae857a537bb6afdd reviewer:human reviewer-id:probepark evidence:exact-head-ci-green;approve-gate-allow;code-review-5443243770;ai-check-and-regressions-passed
|
Merge confirmation — PR #6260 Head SHA: CI: green on the merged head. Dev CI run Verification:
Review: probepark approved this exact head at Disposition: squash-merged into — |
Problem
Codex can reject an access token with HTTP 401 before its expiry timestamp. The usage provider converted that response to
null, while account checks only refreshed expired credentials. The result was an unknown/no-data health result and indefinitely stale quota readings despite a usable refresh token.Changes
Verification
No account reservations, quota reset credits, or running sessions are changed by this patch.