Repository navigation
fix(session): mark the failed credential by row before re-resolving it on a quota failure - #5422
Conversation
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
This PR captures the failed credential row ID before session re-resolution so a Codex usage_limit_reached backoff targets the row that actually failed. The normal sticky-pointer drift scenario is addressed, but the new identity path converts that ID to a mutable array index across an asynchronous usage lookup, allowing concurrent credential inventory mutation to misattribute the backoff. In addition, the exact-head affected-test CI path did not run because the branch does not contain the required current dev base.
Findings / Required Changes
-
[P1] Re-resolve the failed row after the asynchronous usage lookup —
packages/ai/src/auth-storage.ts:4595-4618- This PR introduces a
rowIdcontract to target the failed stored credential rather than the current sticky pointer, but it resolves that durable ID to an array index before awaiting#getUsageReport()and later blocks the saved index. - A concurrent shared-store update that removes or reorders a row preceding the failed row during that await shifts the array. When the await resumes, the code backs off the healthy row now at that index while the failed row remains eligible. Credential snapshots can replace/reorder rows through
#setStoredCredentials, so this is a reachable failure of the new row-identity guarantee rather than the base behavior. - Preserve the row ID across the await, find its current index immediately before marking and computing the remaining pool, and return
falseif it no longer exists. Add a deferred-usage regression that removes or reorders a preceding credential while the lookup is pending.
- This PR introduces a
-
[P1] Restore exact-head CI eligibility for the regression proof —
packages/coding-agent/test/credential-rotation-midturn.e2e.test.ts:192-204- The reviewed head
444a096007d63836384fc45d1fd622a086086c6fis missing required base411b7f3809cec3949edeb6a60a767495c5639a96. Dev CI run34209889710failed atAffected path validation / plan(Verify PR head contains exact base) before affected-test planning, so the new regression tests were skipped. - Rebase onto the current required
devbase and provide a green exact-head affected-validation run that executes the credential regression coverage.
- The reviewed head
CI / Verification
The added unit test covers explicit row-ID selection, legacy pointer behavior, and an unknown ID. The new E2E test covers first-request and post-tool quota failures, including the cache-already-exhausted case, and observes rotation/replay. However, Dev CI run 34209889710 failed before affected-test selection because this exact head does not contain the required dev base; the affected execution matrix was skipped. State-gate and native-addon checks succeeded but do not demonstrate the new behavior.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1 — Intent / Policy / Contract | CHANGES_REQUESTED |
Finding 1 violates the PR's explicit durable-row attribution contract. |
| A2 — Architecture / Correctness / Failure | CHANGES_REQUESTED |
Finding 1 is reachable across the usage-fetch await and concurrent inventory mutation. |
| A3 — Security / Privacy / Trust | APPROVED |
The opaque in-process row ID does not cross a new trust boundary; existing pin and owner/config guards remain. |
| A4 — Verification / Tests / CI | CHANGES_REQUESTED |
Finding 2: exact-head affected-test CI was skipped after the required-base failure. |
| A5 — Context / Compatibility / Platform | APPROVED |
The optional API preserves callers that omit rowId; gateway and remote/broker credential-ID paths remain compatible. |
Limitations
The review was based on immutable commits d2f30f5c2d198e5dee6240428b5407f7d59ea989 (merge-base) and 444a096007d63836384fc45d1fd622a086086c6f (reviewed head). CI branch-protection required-context configuration was not available through the GitHub API, but the exact-head Actions run establishes that affected-test execution never began.
444a096 to
fb67367
Compare
|
Thanks for the review. Both findings are addressed on the new head 1. Row re-located after the await — 2. Exact-head base — rebased onto the current Verification on the new head: the 14 credential/rotation test files across |
|
One more commit on top, |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
I reviewed the full dev...196bffd6e30c410f924435240090685a18b9e1f5 diff, the markUsageLimitReached callers and consumers, the prior requested changes, and the credential-inventory mutation windows. The current head re-resolves both the explicit row-id target and the sticky-pointer target after the asynchronous usage lookup, so a removed or reordered preceding credential cannot redirect the backoff to a bystander. The quota path also captures the failed stored row before any re-resolution, marks that row, then verifies rotation by row identity.
The prior exact-head concerns are addressed. I found no blocking regression in the reviewed scope.
Verified in this worktree:
bun test packages/coding-agent/test/credential-rotation-midturn.e2e.test.ts packages/coding-agent/test/auth-storage-mark-row-id.test.ts packages/ai/test/auth-storage-rotation-observability.test.ts packages/coding-agent/test/auth-storage-rotation.test.ts— 18 passedbun --cwd=packages/ai run check— passedbun --cwd=packages/coding-agent run check— passed
d207cf8 to
e32f2f8
Compare
…t on a quota failure `#markFailedCredential` resolved the session's credential before marking it. That resolution re-runs the pre-request usage check, and once the cached usage report already reads exhausted — the status-bar poll refreshes it on its own clock — the check silently moves the session's sticky pointer to the next healthy row. `markUsageLimitReached` then blocked the healthy row for its default 60 s backoff, reported no remaining credential, and with default settings the turn surfaced the raw `usage_limit_reached` error with no replay while the row that actually failed stayed unmarked. The quota branch now reads the row the session used, marks it first, and names it: `markUsageLimitReached` gains an optional `rowId` that targets a stored row directly instead of the sticky pointer, so nothing that resolves in between can redirect the mark. Rotation is proven by stored row id rather than key equality. When the pointer no longer resolves to a row (a row removed earlier in the list shifts the index) the mark still goes first and falls back to the pointer. The auth branch keeps its key-based proof. Regression tests: test/credential-rotation-midturn.e2e.test.ts covers the plain first-request failure, the failure right after a tool result, and the cache-already-exhausted case that fails on the previous order (replay stays on the exhausted account, then the error surfaces); it also pins that no `retry.*` setting is configured, since a configured legacy retry would mask the defect. test/auth-storage-mark-row-id.test.ts covers the new option in both polarities and an unknown row id.
…imitReached The rowId path resolved the id to an array index before awaiting the usage report and then blocked that saved index. A credential snapshot that removes or reorders a preceding row while the lookup is pending shifts the array, so the backoff landed on whatever row now sat at the old index while the failed row stayed eligible. The id is now kept across the await and re-located right before marking and before the remaining-pool count; a row that vanished marks nothing and reports false. The pointer path keeps its captured position, since the pointer is the only identity it has. Regression: test/auth-storage-mark-row-id.test.ts parks the failed row's usage lookup, removes a preceding row meanwhile, and checks the named row is the one blocked. It fails when the captured index is reused.
…well The previous commit re-located only the rowId target after the await and kept the pointer path on its captured index, reasoning that the pointer was the only identity it had. That reasoning was wrong: #setStoredCredentials calls #resetProviderAssignments on any identity or order change, which drops the provider's session pointers, so after a mid-await removal the captured index names whichever row now sits there and the mark blocked a bystander while the failed row stayed eligible. Both paths now locate the target again right before marking; a cleared pointer marks nothing and returns false. providerKey is derived from the re-located target. Regression: the pointer-path case in test/auth-storage-mark-row-id.test.ts parks the failed row's lookup, removes a preceding row meanwhile, and checks that nothing is blocked and the row at the old index stays usable. It fails on the captured index (Expected false, Received true).
… before parking the pointer-path lookup The pointer-path regression aged the cache by 6 minutes, but the usage-report TTL carries +25% jitter and can reach 6.25 minutes, so on CI the mark's lookup hit the still-valid cache, never reached the provider, and the test waited on a handshake that could not fire until the runner's 5 s timeout. The cache is now aged by 15 minutes, the handshake fails fast with a named reason instead of the runner timeout, and releasing a lookup that was never parked is an error.
The helper indexed #getStoredCredentials twice, so the id lookup and the type read could observe different snapshots when a concurrent update replaced the provider's rows between the two calls. Reading the array once keeps id and type on the same snapshot. Lore-id: b7e2c4f1 Constraint: one logical fix, no behavior change for single-threaded reads Confidence: high Scope-risk: narrow Reversibility: trivial Tested: bun test of the credential/rotation suites, check in both packages
bd7b8e1 to
e30df4a
Compare
|
PR #5422 is verified locally but cannot be advanced to a terminal disposition from this session. Identity remains base I rebased the fix-forward tree onto current The authorized push cannot complete: The only remaining owner action is to provide a GitHub credential with — |
…t on a quota failure `#markFailedCredential` resolved the session's credential before marking it. That resolution re-runs the pre-request usage check, and once the cached usage report already reads exhausted — the status-bar poll refreshes it on its own clock — the check silently moves the session's sticky pointer to the next healthy row. `markUsageLimitReached` then blocked the healthy row for its default 60 s backoff, reported no remaining credential, and with default settings the turn surfaced the raw `usage_limit_reached` error with no replay while the row that actually failed stayed unmarked. The quota branch now reads the row the session used, marks it first, and names it: `markUsageLimitReached` gains an optional `rowId` that targets a stored row directly instead of the sticky pointer, so nothing that resolves in between can redirect the mark. Rotation is proven by stored row id rather than key equality. When the pointer no longer resolves to a row (a row removed earlier in the list shifts the index) the mark still goes first and falls back to the pointer. The auth branch keeps its key-based proof. Regression tests: test/credential-rotation-midturn.e2e.test.ts covers the plain first-request failure, the failure right after a tool result, and the cache-already-exhausted case that fails on the previous order (replay stays on the exhausted account, then the error surfaces); it also pins that no `retry.*` setting is configured, since a configured legacy retry would mask the defect. test/auth-storage-mark-row-id.test.ts covers the new option in both polarities and an unknown row id.
…imitReached The rowId path resolved the id to an array index before awaiting the usage report and then blocked that saved index. A credential snapshot that removes or reorders a preceding row while the lookup is pending shifts the array, so the backoff landed on whatever row now sat at the old index while the failed row stayed eligible. The id is now kept across the await and re-located right before marking and before the remaining-pool count; a row that vanished marks nothing and reports false. The pointer path keeps its captured position, since the pointer is the only identity it has. Regression: test/auth-storage-mark-row-id.test.ts parks the failed row's usage lookup, removes a preceding row meanwhile, and checks the named row is the one blocked. It fails when the captured index is reused.
…well The previous commit re-located only the rowId target after the await and kept the pointer path on its captured index, reasoning that the pointer was the only identity it had. That reasoning was wrong: #setStoredCredentials calls #resetProviderAssignments on any identity or order change, which drops the provider's session pointers, so after a mid-await removal the captured index names whichever row now sits there and the mark blocked a bystander while the failed row stayed eligible. Both paths now locate the target again right before marking; a cleared pointer marks nothing and returns false. providerKey is derived from the re-located target. Regression: the pointer-path case in test/auth-storage-mark-row-id.test.ts parks the failed row's lookup, removes a preceding row meanwhile, and checks that nothing is blocked and the row at the old index stays usable. It fails on the captured index (Expected false, Received true).
… before parking the pointer-path lookup The pointer-path regression aged the cache by 6 minutes, but the usage-report TTL carries +25% jitter and can reach 6.25 minutes, so on CI the mark's lookup hit the still-valid cache, never reached the provider, and the test waited on a handshake that could not fire until the runner's 5 s timeout. The cache is now aged by 15 minutes, the handshake fails fast with a named reason instead of the runner timeout, and releasing a lookup that was never parked is an error.
The helper indexed #getStoredCredentials twice, so the id lookup and the type read could observe different snapshots when a concurrent update replaced the provider's rows between the two calls. Reading the array once keeps id and type on the same snapshot. Lore-id: b7e2c4f1 Constraint: one logical fix, no behavior change for single-threaded reads Confidence: high Scope-risk: narrow Reversibility: trivial Tested: bun test of the credential/rotation suites, check in both packages
e30df4a to
bd705e7
Compare
|
Pushed the rebase myself, so the The rebase was not mechanical. The Extending the fix to the Verification on
The body's verdict digest and maintenance snapshot are rebound to base |
Resolve-before-mark could move the sticky row and penalize a healthy credential. Capture the mark target once, re-find it after usage awaits, and require two known different rows before claiming rotation. This does not create dispatch-bound attribution. Lore-id: 5422mark20260910 Confidence: high Scope-risk: bounded Reversibility: easy Tested: 43 tests, 266 assertions, package types, Biome Not-tested: request-dispatch identity attribution
Retain the original PR head as an ancestor while keeping the complete reviewed candidate tree. Stable mark-time row capture intentionally supersedes re-reading an omitted-row sticky pointer after usage; rotation requires two known, different IDs instead of unknown-to-known inference. All useful quota-recovery obligations remain, without claiming dispatch-bound attribution. Constraint: resolve exactly six files to candidate 5583174, rejecting the obsolete helper and tail Scope-risk: narrow Tested: staged tree equality e9584c3 Not-tested: no tests or gates run for this ancestry-only merge
|
Backlog follow-up prepared a current-dev narrow mark-order candidate with43 tests/266 assertions, ai/coding-agent types/Biome and independent scoped review CLEAR. It captures the initial row across usage awaits, marks before resolving, and requires two known different IDs for rotation; it explicitly does not claim dispatch-bound attribution. Before publication, the live head changed from e30df4a to bd705e7. No push was attempted. The current contributor head is being reconciled read-only so no concurrent changes are overwritten or silently discarded. |
|
Current-head reconciliation found a substantive addition in bd705e: mark-before-resolve now also covers the OAuth-only credential trigger with default backoff. That obligation is being preserved and tested, not discarded by the earlier local candidate. The local improvements additionally stabilize omitted-row targeting across usage awaits and reject unknown-before as proof of rotation. No push/force update performed; fresh combined trigger coverage and ancestry reconciliation are required. |
Incorporate the contributor update from bd705e7 without losing stable mark-time targeting or known-row rotation proof. Preserve OAuth-only eligibility and default backoff rather than response retryAfter. Lore-id: 5422oauth20260910 Confidence: high Scope-risk: bounded Reversibility: easy Tested: 49 tests, 347 assertions, types, Biome Not-tested: dispatch-bound attribution
Retain contributor bd705e7 as an ancestor with the exact reviewed b105541 candidate tree. The candidate incorporates OAuth account-model rejection mark-before-resolve, its OAuth-only guard and default backoff without response retry-after. Preserve stronger stable mark-time row targeting and two-known-ID rotation proof instead of the older pointer reread and unknown-to-known inference. Auth handling and pin protections remain intact; no dispatch-bound attribution is claimed. Constraint: exactly six candidate-version resolutions, no other tree changes Scope-risk: narrow Tested: staged tree equality dbac182 Not-tested: no tests or gates run for ancestry-only integration
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Pre-review notes (head fa194dd, auto-generated — not a verdict)
CI: PR-cause 1 + base-origin 1 — Two failures:
- "PR contract bootstrap" + "Validate exact-head PR contract": the verdict line in the PR body carries digest
1cd7ba255...(stale, from before the final rebase); the exact digest for headfa194ddisc115989f.... Gate rejects the stale digest. Author must update the verdict line in the PR body. - "Virtual integration validation": canary
sdk-broker-restart.test.tsfails withENOENT broker.lock/owner.jsoninbroker.ts:2380. That file is not in this PR's diff — this is a pre-existing base failure. Dev CI run 34433019335 also hits sdk-broker canary failures (branchfix/issue-3350-risk-canaries-virtual-integrationlisted in affected-path logs, indicating dev is already broken there).
범위: +424 / -43, 6 files — packages/ai/src/auth-storage.ts, packages/coding-agent/src/session/agent-session.ts, two new test files (auth-storage-mark-row-id.test.ts, credential-rotation-midturn.e2e.test.ts), both CHANGELOGs
규약: CHANGELOG 있음 (packages/ai and packages/coding-agent both updated), generated 파일 없음, 라벨 없음, merge-self-approved 없음. PR body marks regression-risk with extra:independent reviewer required.
주목할 곳:
packages/ai/src/auth-storage.ts:4617–4625— After the usage-report await, the target row is re-located from a fresh#getStoredCredentialssnapshot. Correct. But theremainingCredentialsfilter at the bottom (lines ~4618–4625) calls#getCredentialsForProvider, which may return a third snapshot. In the window between the first re-find and the remaining-check, another row could be removed, makingremainingCredentialsinconsistent with thetargetIndexused above. Practical impact is low (the mark already committed), but two distinct calls to the credential list after one await create a theoretical torn-read in the return value. Worth reviewer awareness.packages/coding-agent/src/session/agent-session.ts:21596–21610— Whenbeforeisundefined(no session credential row id captured,unknown-beforescenario), the code skips rotation and returnsremaining ? "unchanged" : "exhausted"without callinggetApiKey. The e2e test'sunknown-beforescenario covers this: it expectskeys.length === failOn(no replay, no TOKEN-b). This is intentional and documented in the PR body. Confirmed by test.
blocking 후보: 없음 (CI failures are PR-author's stale digest + pre-existing base breakage; code review found no blocking defects)
verdict 초안 (digest c115989fa1e7, 리뷰어가 확인 후 복붙):
gajae.pr-review-verdict.v1 needs-human sha256:c115989fa1e732b8209610c299e0f8d11b116cc3415615a437488592e8c2c5fd reviewer:critic reviewer-id:probepark evidence:ci-stale-digest;base-sdk-broker-canary;ai-types-pass;agent-types-pass;biome-pass;torn-read-remaining-noted
probepark 리뷰 시 참고용. verdict는 리뷰어가 확정.
|
Exact-head contract refresh for PR #5422:
The current-dev fix-forward candidate is locally rebased and verified but not yet pushed: — |
d215341 to
ed7b363
Compare
|
Pushed and refreshed PR #5422 at the final current-dev head:
This comment is the exact-head contract retrigger after publication. Any subsequent push requires a new digest, approval, and CI refresh. — |
Yeachan-Heo
left a comment
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Reviewed the final exact head ed7b363006da94eee40bc77d77c86e2ddf0d770e against current dev 54e8cc3f8c517b0afe71eb0cccbabcb2872197d9.
The six-file change preserves the contributor's mark-before-resolve behavior across quota, rate-limit, and OAuth account-model credential triggers. The stored row is captured before awaited usage lookup, re-located by stable row ID after the await, and rotation is reported only for two known distinct row IDs. Pinned credentials and forbidden auth remain guarded. Focused row-ID/mid-turn tests, trigger coverage, AI/coding-agent checks, and native build passed locally; exact-head CI has passed the affected credential tests and is still completing the broader Dev CI run.
No blocking code finding remains on this exact head. This approval is intentionally bound to ed7b363006da94eee40bc77d77c86e2ddf0d770e and is invalid for any later push.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
|
Final exact-head approval and verdict refresh for PR #5422:
This comment retriggers the head-bound contract validation after the approval and verdict update. Merge remains gated on the required CI checks. — |
|
Current-dev refresh completed for PR #5422:
This comment retriggers the exact-head contract after the current-dev refresh. Any later push requires another digest, review, and CI refresh. — |
Yeachan-Heo
left a comment
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Reviewed exact head b48a7ff65526a90e08f199e6fb400d4d97d1398b against current base 1e8221fcf788a7beffe3d598bdc3d99087dda7bd.
The current-dev merge preserves the six-file credential-rotation change. AuthStorage captures a stable row ID before awaited usage lookup and re-finds it before marking; session recovery marks before re-resolution and only reports rotation for two known distinct IDs across quota, rate-limit, and OAuth account-model triggers. Pinned credentials, forbidden auth, OAuth-only guards, default backoff, row deletion/reordering, and stale usage-report cases are covered by the implementation and focused tests. Local focused tests and both package checks passed; affected exact-head credential shards passed on CI.
No blocking code finding remains. This approval is bound only to b48a7ff65526a90e08f199e6fb400d4d97d1398b.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
|
Final exact-head approval/verdict refresh for PR #5422:
This comment retriggers the head-bound contract and metadata validation after the current-dev refresh and exact-head approval. Merge remains gated on green CI. — |
A failed credential can be followed by a blocked fallback when no usable row remains. Treat the mark operation result as the availability proof and stop before re-resolving when every row is blocked. Lore-id: pr5422-all-blocked Constraint: replay requires an unblocked replacement row Rejected: row-ID difference alone | all-blocked fallback can select a blocked row Confidence: high Scope-risk: regression Reversibility: straightforward Directive: keep mark-time row identity and default backoff behavior Tested: all-blocked quota/rate-limit/credential mid-turn matrix
|
Final code fix and current-dev refresh for PR #5422:
This comment retriggers exact-head contract validation for the final fix. Any later push requires a new digest, approval, and CI refresh. — |
Yeachan-Heo
left a comment
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Reviewed final exact head 5f45011171155f5c8ad4ba7e9ee9f8a6897c4d6 against dev base c50bcaffd7e252528ef7e895ee22bb1da8955943.
The P1 all-blocked fallback finding is fixed. markUsageLimitReached remains the availability proof; when it returns false, AgentSession#markFailedCredential returns exhausted before resolving any blocked fallback. Replay now requires an unblocked replacement plus two known distinct stored row IDs. The stable row-ID mark-before-await fix, quota/rate-limit/OAuth credential trigger behavior, OAuth-only/default-backoff guard, pin guard, forbidden-auth guard, and row removal/reordering handling remain intact. Added all-blocked coverage passes for quota, rate-limit, and OAuth account-model triggers. Local changed suites pass 26 tests; focused trigger coverage passes 93 tests; AI and coding-agent checks pass; current exact-head affected CI credential/type/build shards pass while virtual integration is still queued.
No blocking code finding remains. This approval is bound only to 5f45011171155f5c8ad4ba7e9ee9f8a6897c4d6.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
|
Final exact-head approval and verdict refresh for PR #5422:
This comment retriggers the head-bound contract validation after the final code fix and approval. Merge remains gated on the required CI checks. — |
# Conflicts: # packages/ai/CHANGELOG.md
|
Current-dev merge refresh for PR #5422:
This comment retriggers the exact-head contract for the final current-dev head. Any later push requires a new digest, approval, and CI refresh. — |
Yeachan-Heo
left a comment
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Reviewed final exact head fa12a000fa8e23cbcf2638ea1717ad875b7cf338 against current base fbc56ec5cec026620c444cd6080881af3cf9b349.
The all-blocked P1 safeguard remains correct after the latest current-dev merge: markUsageLimitReached returning false now stops recovery before any blocked fallback resolution, while valid rotation still requires an unblocked replacement and two known distinct stored row IDs. The stable row-ID mark-before-await logic, quota/rate-limit/OAuth trigger coverage, pin/forbidden guards, default backoff, and row reorder/removal handling are preserved. Final local suites pass 26 changed credential tests and 93 focused trigger tests; AI and coding-agent checks pass. Exact-head affected validation passed after a transient unrelated terminal-abort test timeout was rerun successfully; virtual integration passed.
No blocking code finding remains. This approval is bound only to fa12a000fa8e23cbcf2638ea1717ad875b7cf338.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
|
Final exact-head approval/verdict refresh for PR #5422:
This comment retriggers the head-bound contract after final approval. Merge remains gated on the updated metadata check and final PR state. — |
|
Final contract retry after all review/body events settled: Base The exact-head owner approval is present, the P1 all-blocked fix is included, and affected CI plus virtual integration are green. This retry must validate the current — |
What
On a Codex
usage_limit_reachedfailure,AgentSession#markFailedCredentialresolved the session's credential before marking it. That resolution re-runs the pre-request usage check, and once the cached usage report already reads exhausted (the status-bar poll refreshes it on its own clock), the check blocks the exhausted row and silently moves the session's sticky pointer to the next healthy row.markUsageLimitReachedthen blocked the healthy row for its default 60 s backoff, reported no remaining credential, and with default settings (noretry.*key) the turn surfaced the rawCodex error event: The usage limit has been reached (code=usage_limit_reached)with no replay, while the row that actually failed stayed unmarked.AuthStorage.markUsageLimitReachedgains an optionalrowIdthat targets a stored row directly instead of the sticky pointer, and both paths locate the target again after the usage-report await (a snapshot that removes or reorders a preceding row meanwhile shifts indexes and drops the provider's session pointers; a vanished row marks nothing and returnsfalse).#markFailedCredentialreads the row the session used, marks it first and by id, and only then resolves; rotation is proven by stored row id rather than key equality. Theauthbranch is unchanged and keeps its own resolve-then-compare proof. Rebasing onto currentdevmoved this onto the rewritten function, which added acredentialtrigger class (facts.credentialModelUnavailable) that also blocked a row through the sticky pointer, so that class now takes the same marked-by-id path. It keeps itsgetSessionCredentialTypeprecondition, which like the row id is a synchronous read of the pointer, and it keeps the default backoff instead of the response'sretry-after, exactly as before. The pre-resolve that used to run for every class now runs only in theauthbranch, which is the one that needs the active key.Why
Observed on 0.16.4 in a
--prefer-credentialsession with two OAuth accounts: the failing message was content-free withtransportFailure.providerCode = usage_limit_reached, nothing replayed, and the session sat on the error although the other account had its full quota. Instrumented, the mark step readactive=TOKEN-B remaining=false after=TOKEN-B.Testing
d2153419694b905c714f32b1503077b68f22fba8passed the row-ID regression, credential-rotation mid-turn test, AI package check, native build, CLI smoke, state gates, and the other selected test shards. The coding-agent package-check job ended after a 45-minute runner setup timeout with no test step executed; this is runner infrastructure evidence, not a code diagnostic.b48a7ff65526a90e08f199e6fb400d4d97d1398bincludes currentorigin/dev54e8cc3f8c517b0afe71eb0cccbabcb2872197d9as a parent and preserves the six-file PR diff.bun test packages/coding-agent/test/auth-storage-mark-row-id.test.ts packages/coding-agent/test/credential-rotation-midturn.e2e.test.tspassed 23 tests; focused trigger coverage passed 93 tests;bun --cwd=packages/ai run checkandbun --cwd=packages/coding-agent run checkpassed.needs-humanbecause no authenticated independent approval exists on the exact head. The priorprobeparkapproval is stale at196bffd6e30c410f924435240090685a18b9e1f5; the latestprobeparkreview is commentary atfa194dd08a880af0a3a4db2fbce2ec26c8c330c6.Known limits, out of scope here: the failed row is captured at mark time, not request time, so a subagent sharing
credentialSessionIdcan move the pointer during a retry backoff; a sticky index that dangles after an earlier row is removed still marks by pointer (existing index-keyed pointer maintenance).Risk classification
low-risk— ordinary fix/maintenance; the repository owner may use the explicitmerge-self-approvedsolo verdict (no independent human review; the verdict name itself records this) with a risk-record comment bound to the exact head.regression-risk— fix with material regression risk; requires one assigned independent domain reviewer whose authenticated exact-headAPPROVEDreview the gate verifies (extra:independent:<login>; the token alone never suffices).high-risk— large refactor, feature, or materially high-risk change (security/auth/install/remove/public API/destructive lifecycle/architecture); requires one assigned independent domain reviewer with an authenticated exact-headAPPROVEDreview (extra:independent:<login>).GJC verdict
devbun checkpasses🤖 Generated with Claude Code
Current maintenance snapshot: base
fbc56ec5cec026620c444cd6080881af3cf9b349, headfa12a000fa8e23cbcf2638ea1717ad875b7cf338. Earlier narrative/test evidence is historical unless explicitly rebound to this head. Rebase/diff verification does not replace required CI or independent approval.Current exact-head reconciliation
The final current-dev head
fa12a000fa8e23cbcf2638ea1717ad875b7cf338preserves all six changed paths after honest conflict resolution and includes the all-blocked replay safeguard. The exact digest, owner approval, affected CI, and virtual integration are all bound to this head; metadata validation is being retriggered after the final approval.