fix(codex): reconcile prior cooldown after manual credit reset - #3995
fix(codex): reconcile prior cooldown after manual credit reset#3995luvs01 wants to merge 1 commit into
Conversation
github-actions
Bot
added
the
intake: hygiene-blocked
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughManual reset-credit consumption now clears a matching reset-derived quota cooldown only after a fresh, complete usage read confirms the same account identity. Identity changes, newer failures, incomplete reads, busy reconciliation, and replayed operations preserve cooldown state. Tests and documentation cover these cases. ChangesReset-credit recovery
Priority: ➖ Normal — Impact reflects medium issue severity. Estimated code review effort: 4 (Complex) | ~45 minutes Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to Reset-credit recovery now clears only matching ordinary cooldowns after fresh same-account usage confirmation, while preserving newer failures and unrelated limits. No actionable merge-blocking risk is identified. Sequence Diagram(s)sequenceDiagram
participant Operator
participant ResetCreditsAPI
participant QuotaRefresh
participant CooldownState
Operator->>ResetCreditsAPI: consume reset credit
ResetCreditsAPI->>CooldownState: capture existing cooldown
ResetCreditsAPI->>QuotaRefresh: drain flights and request fresh usage
QuotaRefresh-->>ResetCreditsAPI: return usage and account identity
ResetCreditsAPI->>CooldownState: clear matching reset-derived cooldown
ResetCreditsAPI-->>Operator: return success and fresh remaining credits when available
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 3 files. (3 skipped: 3 unsupported.)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
⏳ DRAFT
What to do
Review readiness checklist
3/4 boxes ticked. This pull request was already a draft. Its draft status will be preserved after every issue above is resolved. Hygiene
|
|
@coderabbitai full review Please review head |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== PR metadata and checks ==='
gh pr view 3995 --repo lidge-jun/opencodex \
--json number,title,headRefOid,baseRefOid,files,reviewDecision,statusCheckRollup \
--jq '{number,title,headRefOid,baseRefOid,files: [.files[].path], checks: [.statusCheckRollup[] | {name,status,conclusion,detailsUrl}]}'
echo '=== Changed file summary ==='
git diff --stat HEAD^ HEAD
git diff --name-status HEAD^ HEAD
echo '=== Full PR diff ==='
git diff --find-renames --find-copies --unified=80 HEAD^ HEAD
echo '=== Relevant source outlines ==='
fd -a '^(auth-api|.*ledger.*|.*auth.*)\.(ts|tsx)$' . -E node_modules -E dist | while IFS= read -r file; do
echo "--- $file"
ast-grep outline "$file" --items all --type function,class,interface,type --view compact || true
done
echo '=== Focused identifiers with context ==='
rg -n -i -C 8 \
'reset-derived|already_redeemed|retry-after|cooldown|redeem|identityGeneration|identity.*generation|quota' \
--glob '*.{ts,tsx,md}' \
-g '!node_modules/**' -g '!dist/**' .
echo '=== Test names and relevant assertions ==='
fd -a -E node_modules -E dist | rg '(^|/)(test|tests|__tests__)/|(\.|-)(test|spec)\.(ts|tsx)$' | while IFS= read -r file; do
if rg -q -i 'reset-derived|already_redeemed|cooldown|redeem|quota' "$file"; then
echo "--- $file"
rg -n -i -C 6 'reset-derived|already_redeemed|cooldown|redeem|quota' "$file"
fi
doneLength of output: 201943 🏁 Script executed: #!/bin/bash
set -euo pipefail
base="$(git merge-base HEAD origin/dev 2>/dev/null || git merge-base HEAD HEAD^)"
echo "=== merge base: $base ==="
echo '=== PR files ==='
git diff --name-status "$base" HEAD
echo '=== PR patch ==='
git diff --find-renames --unified=35 "$base" HEAD
echo '=== CI state ==='
gh pr view 3995 --repo lidge-jun/opencodex --json headRefOid,statusCheckRollup \
--jq '{headRefOid,checks:[.statusCheckRollup[] | {name,status,conclusion,detailsUrl}]}'
echo '=== Definitions and usages of reconciliation APIs ==='
rg -n -C 12 \
'function (capture|settle|reconcile).*Cooldown|const (capture|settle|reconcile).*Cooldown|capture.*cooldown|settle.*cooldown|reconcile.*cooldown|cooldownSource|identityGeneration|generation.*identity' \
src/codex tests \
-g '*.{ts,tsx}'
echo '=== Changed-test names ==='
git diff --unified=3 "$base" HEAD -- tests | rg -n -C 4 '^[+-].*(test|describe)\(' || true
echo '=== Changed documentation sections ==='
git diff --unified=20 "$base" HEAD -- '*.md'Length of output: 209692 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Reset endpoint admission and native-main claim ==='
sed -n '2280,2495p' src/codex/auth-api.ts
echo '=== Main usage read and freshness result construction ==='
sed -n '760,1020p' src/codex/auth-api.ts
echo '=== Pool usage result types, commit path, and flight handling ==='
sed -n '1180,1455p' src/codex/auth-api.ts
echo '=== Cooldown stores and lifecycle clearing paths ==='
sed -n '1,120p' src/codex/routing.ts
sed -n '620,710p' src/codex/routing.ts
sed -n '2380,2490p' src/codex/routing.ts
rg -n -C 8 'clearCodexUpstreamHealth|deleteScopedHealth|setScopedHealth|clear.*Health' src/codex/routing.ts src/codex/account-lifecycle.ts
echo '=== Failed check annotations and logs ==='
for run in 34184176148 34184176157; do
echo "--- workflow $run"
gh run view "$run" --repo lidge-jun/opencodex --json status,conclusion,event,headSha,workflowName,jobs \
--jq '{status,conclusion,event,headSha,workflowName,jobs:[.jobs[]|{name,conclusion,startedAt,completedAt,databaseId,url}]}'
done
echo '--- hygiene log'
gh run view 34184176148 --repo lidge-jun/opencodex --job 101929084539 --log-failed || true
echo '--- enforce-target log'
gh run view 34184176157 --repo lidge-jun/opencodex --job 101929124312 --log-failed || true
echo '=== PR base and reviewer state ==='
gh pr view 3995 --repo lidge-jun/opencodex --json baseRefName,headRefName,reviewRequests,reviews \
--jq '{baseRefName,headRefName,reviewRequests,reviews:[.reviews[]|{author:.author.login,state,body}]}'Length of output: 212627 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Reset authentication wrapper ==='
rg -n -C 20 'async function withResetCreditAuth|function withResetCreditAuth|withResetCreditAuth\(' src/codex/auth-api.ts
echo '=== Main claim fields in the reset authentication result ==='
rg -n -C 14 'nativeMainLease|nativeMainSharedClaimHeld|isMain:' src/codex/auth-api.ts
echo '=== Auth-surface ownership requirements ==='
if [ -f MAINTAINERS.md ]; then
sed -n '1,240p' MAINTAINERS.md
fi
echo '=== PR labels and gate comment ==='
gh pr view 3995 --repo lidge-jun/opencodex --json labels,comments \
--jq '{labels:[.labels[].name],gateComments:[.comments[]|select(.author.login=="github-actions[bot]")|.body]}'Length of output: 44143
I found no functional blocker in the cooldown reconciliation implementation. Reviewed behavior
Required follow-up before review-readyThe current PR checks are not green.
Add the required maintainer sponsorship and attribution credit. Then obtain the requested explicit security review. These requirements remain separate from this code review. ✅ Action performedFull review finished. |
리뷰 · 우선순위 71 / 80이 PR은 이슈 #3973을 고칩니다. 수동 reset-credit이 하는 일은 두 축입니다. 현재 라인 src/codex/routing.ts 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
|
Follow-up to the client-contract check in #3995 (comment), on unchanged head
Contributor CI now passes all 26 jobs on this head after one retry of the failed Windows job and aggregate. The description records both the initial failure and the successful retry. The PR remains Draft pending maintainer sponsorship and explicit security review. |
lidge-jun
added
the
landed-via-maintainer
2 participants
Summary
Closes #3973.
A confirmed manual reset can refresh the displayed usage while the same account remains blocked by its older local quota cooldown. Reconcile only the captured ordinary
reset-derivedcooldown aftercode: "reset"and a complete post-reset usage observation for the same credential identity.Capture the cooldown object before consuming the credit, then settle it once without changing pins, pauses or account selection. Preserve explicit Retry-After, Spark/Reserve scopes, newer failures, replaced identities and deleted/recreated cooldown state. Drain pre-existing Pool quota requests before obtaining recovery evidence; the main path retains its native claim and identity-generation checks.
already_redeemedand durable replay do not clear cooldowns.Once the credit has been consumed, a failed or busy usage refresh retains the confirmed success code and the cooldown. It omits
remainingwhen no fresh same-identity credit count is available, so a read failure does not invite spending another credit. The management API reference in English/Korean and the CLI reference describe this contract.Scope is six files. Strict-quota waiting (#3738) and background recovery scheduling are separate. Canonical reset-operation alias settlement (#3965) is already present in this PR's base branch.
Verification
Head
e172453052bf7bbc4a0ae5aa24592982c0c64b15, based ondev402be7c1f88283eb8465c3aec8437ccecd2542ec.already_redeemedresponse could lose its fresh remaining count. Both new cases failed before the correction. Reconcile main identity under the existing native claim before capturing the baseline; the corrected authentication API suite passed 271 tests / 1,060 assertions before the final combined run. Read-only re-review confirmed the fix and found no further blocker.disconnect-after-clearcase separately (1 passed / 5 assertions, 12.66 seconds within its unchanged 15-second internal deadline, on the pre-review-correction head5ee4d5910). A prior single-case control using the parent runtime source also passed at 14.52 seconds. This is evidence of timing sensitivity, not proof that all larger-suite failures were environmental.bun run typecheck,bun run privacy:scan, andgit diff --check: passed. Documentation build: 425 pages, with the changed generated text checked without opening a preview.The larger import-connected local test run was incomplete and is not reported as green. The full contributor CI is now successful: 26/26 jobs on this exact head, attempt 2. The first attempt had 24 successful jobs and one config-lock fixture failure (plus the aggregate): cleanup expected child exit 0 but received 143. The unchanged failing case passed locally with its original deadline; one retry of the failed Windows job and aggregate then passed. This preserves the first failure as timing/lifecycle evidence rather than treating it as an initial clean run.
The existing reset-credit handler was also invoked with mocked responses on this head for
reset/already_redeemed, with/withoutremaining, andload(true)returningfalse: all four cases returned success and closed the modal contract after exactly one consume and one refresh call. The controller converts read failures tofalse; the component closes the reset/confirmation state fromclose: true. No automatic credit re-consumption occurs in that path. This is bounded handler/source evidence, not an interactive UI test or a claim that a later manual action reuses the same operation ID.CodeRabbit's exact-head review found no functional blocker. Maintainer sponsorship for the protected authentication/routing surface and explicit security review remain pending; agent review does not satisfy that requirement. The earlier attribution notice referred to #3965 in the base branch; the clarified description and commit text contain no declared attribution references in the repository checker.
Checklist
Review readiness checklist
Summary by CodeRabbit
New Features
Documentation