feat(codex): coordinate reset-credit recovery attempts - #1410
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ChangesReset-credit recovery
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant RecoveryCoordinator
participant RecoveryDependencies
participant ConsumeAPI
Caller->>RecoveryCoordinator: recover(turn, generation, authorization)
RecoveryCoordinator->>RecoveryDependencies: revalidate generation and credit
RecoveryDependencies-->>RecoveryCoordinator: validated recovery state
RecoveryCoordinator->>ConsumeAPI: dispatch operation-ID-bound consume request
ConsumeAPI-->>RecoveryCoordinator: consume outcome
RecoveryCoordinator-->>Caller: normalized recovery result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
|
✅ Deterministic PR hygiene checks passed. |
⏳ DRAFT
What to do
Review readiness checklist
3/4 boxes ticked. This PR stays in draft until every box above is ticked. |
d6da400 to
9ce9b26
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/codex/reset-credit-recovery.ts`:
- Around line 439-447: Gate or make internal the resetProcessStateForTests
helper and its process registries so production code cannot clear
terminalByAccount; update src/codex/reset-credit-recovery.ts lines 439-447
accordingly. In tests/codex-reset-credit-recovery.test.ts lines 880-916, split
the post-reset confirmed scenario into a separate test and perform the reset in
beforeEach instead of mid-test.
- Around line 403-421: Update the static active-flight coordination around
CodexResetCreditRecoveryCoordinator so flights are not shared across
coordinators with different dependency sets or limits; include a
per-dependency-set identity in the flight key, or validate identity and limits
when joining and return a dedicated not-dispatched reason on mismatch. In
tests/codex-reset-credit-recovery.test.ts lines 315-359, give each coordinator
distinct consume implementations and assert which one executes to verify the
selected contract.
- Around line 320-322: Update hasFlightCapacity to emit an operational signal
whenever capacity is rejected, such as a log line or counter containing only the
current saturation count. Ensure the signal is triggered on the false path for
the MAX_TRACKED_RECOVERY_ACCOUNTS limit and never includes accountId. Preserve
the existing fail-closed not-dispatched/recovery-state-capacity behavior.
- Around line 145-148: Import CodexResetEligibleExhaustionCode from
quota-rejection.ts, type RESET_ELIGIBLE_CODES as
ReadonlySet<CodexResetEligibleExhaustionCode>, and cast value.semanticCode to
that type in authorizedResetRejection when calling has. Preserve the existing
eligible code values while making changes to the shared union produce
compile-time drift errors.
- Around line 546-572: Add a deadline timer field to RecoveryFlight, assign the
setTimeout handle from runFlightWithDeadline to that field, and clear it during
resetProcessStateForTests alongside the aborts and registry cleanup. Preserve
the existing finally cleanup and ensure the reset handles flights with pending
timers.
In `@tests/codex-reset-credit-recovery.test.ts`:
- Around line 405-410: Export MAX_TRACKED_RECOVERY_ACCOUNTS and
MAX_TRACKED_RECOVERY_FLIGHTS from the reset-credit recovery module, then update
both affected tests to derive loop bounds, expected consume-call counts,
terminal-generation counts, flight counts, and exhaustionGeneration from the
appropriate exported constants instead of literal 128 values. Preserve the
overflow assertions by using the constants consistently for capacity and the
subsequent over-capacity case.
- Around line 315-359: Update the concurrent-flight test around makeCoordinator
and consume so firstCoordinator and secondCoordinator use distinguishable
consume adapters that record which coordinator executed. Keep shared revalidate
behavior, then assert the recorded executedBy value explicitly alongside the
existing single-flight assertions, documenting the implementation’s intended
adapter ownership contract.
- Around line 749-752: Replace the single Promise.resolve microtask drain in the
late transport rejection test with the coordinator’s idle synchronization, such
as waitForIdleForTests, so execution has reached the retry decision before
asserting consumeCalls is 1. Ensure the idle signal actually tracks the captured
flight or otherwise resolves only after the consume loop exits, since the flight
may be removed from allFlights by its finally handler.
- Around line 774-784: Update the exact consume-outcome test.each block so each
case uses a unique suffixed accountId, following the pattern used by the other
multi-case tests, and pass that accountId when creating the logical turn or
coordinator input as appropriate. Confirm the generated identifiers satisfy
CODEX_ACCOUNT_ID_RE, while preserving the existing expected outcome assertions.
- Around line 176-201: Add focused negative recovery cases near the existing
`generic` and `wrongStatus` cases: one must preserve an otherwise authorized
rejection while setting `resetCreditEligible: false`, and another must preserve
valid kind/status and eligibility while using an unrecognized `semanticCode`.
Assert both results equal `generic` alongside the existing recovery assertions,
isolating each gate in `authorizedResetRejection`.
- Around line 133-146: Update the test to import and use MAIN_CODEX_ACCOUNT_ID
instead of hardcoding "__main__" in the accepted account-id case, ensuring it
follows the production contract. Extend the rejected account-id cases with
"__PROTO__" to verify case-insensitive handling of reserved identifiers while
retaining the existing canonical-format rejection coverage.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4e8adcb2-4dd6-48d5-a1ef-4cd319e0a8e5
📒 Files selected for processing (2)
src/codex/reset-credit-recovery.tstests/codex-reset-credit-recovery.test.ts
|
Tip For best results, initiate chat on the files or code changes.
You are interacting with an AI system. |
|
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ce9b2685d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
9ce9b26 to
bf189c5
Compare
|
|
|
Tip For best results, initiate chat on the files or code changes.
You are interacting with an AI system. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== PR diff summary ==='
git diff --stat HEAD^ HEAD || true
printf '%s\n' '=== Candidate files ==='
fd -t f '^(reset-credit-recovery\.ts|codex-reset-credit-recovery\.test\.ts)$' . || true
printf '%s\n' '=== Source outline ==='
ast-grep outline src/codex/reset-credit-recovery.ts --items all --view expanded 2>/dev/null || true
printf '%s\n' '=== Test outline ==='
ast-grep outline tests/codex-reset-credit-recovery.test.ts --items all --view expanded 2>/dev/null || true
printf '%s\n' '=== Changed source ==='
git diff --no-ext-diff -U80 HEAD^ HEAD -- src/codex/reset-credit-recovery.ts || true
printf '%s\n' '=== Changed tests ==='
git diff --no-ext-diff -U25 HEAD^ HEAD -- tests/codex-reset-credit-recovery.test.ts || trueLength of output: 83522 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Coordinator state-management sections ==='
nl -ba src/codex/reset-credit-recovery.ts | sed -n '360,870p'
printf '%s\n' '=== Imported account and rejection contracts ==='
ast-grep outline src/codex/account-id.ts --items all --view expanded 2>/dev/null || true
ast-grep outline src/codex/quota-rejection.ts --items all --view expanded 2>/dev/null || true
nl -ba src/codex/account-id.ts | sed -n '1,220p'
nl -ba src/codex/quota-rejection.ts | sed -n '1,300p'
printf '%s\n' '=== Module and coordination scope references ==='
rg -n -C 4 --glob '!src/codex/reset-credit-recovery.ts' \
'(reset-credit-recovery|CodexResetCreditRecoveryCoordinator|coordinationScope|resetCodexResetCreditRecoveryProcessStateForTests)' \
. || true
printf '%s\n' '=== Relevant terminal-state test references ==='
rg -n -C 8 \
'(terminal|coordination-mismatch|scope|process-wide|same generation)' \
tests/codex-reset-credit-recovery.test.tsLength of output: 1364 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Coordinator state-management sections ==='
cat -n src/codex/reset-credit-recovery.ts | sed -n '360,870p'
printf '%s\n' '=== Imported account and rejection contracts ==='
ast-grep outline src/codex/account-id.ts --items all --view expanded 2>/dev/null || true
ast-grep outline src/codex/quota-rejection.ts --items all --view expanded 2>/dev/null || true
cat -n src/codex/account-id.ts | sed -n '1,220p'
cat -n src/codex/quota-rejection.ts | sed -n '1,300p'
printf '%s\n' '=== Module and coordination scope references ==='
rg -n -C 4 --glob '!src/codex/reset-credit-recovery.ts' \
'(reset-credit-recovery|CodexResetCreditRecoveryCoordinator|coordinationScope|resetCodexResetCreditRecoveryProcessStateForTests)' \
. || true
printf '%s\n' '=== Relevant terminal-state test references ==='
rg -n -C 8 \
'(terminal|coordination-mismatch|scope|process-wide|same generation)' \
tests/codex-reset-credit-recovery.test.tsLength of output: 52056
As a result, coordinator A can complete a recovery for Include You are interacting with an AI system. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf189c5bec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/codex/reset-credit-recovery.ts`:
- Around line 552-574: Define and document an explicit retention policy for
terminalByAccount before the runtime adapter is introduced. Update
hasFlightCapacity and the terminal-outcome recording path to evict fences based
on their recorded credentialGeneration and exhaustionGeneration after the chosen
replay-protection lifetime, while preserving active-flight capacity checks and
allowing capacity for accounts whose fences have expired.
In `@tests/codex-reset-credit-recovery.test.ts`:
- Around line 46-73: Add a test using two coordinators with distinct
coordinationScope objects while keeping revalidate, consume, lifecycleSignal,
operationTimeoutMs, and maxConsumeAttempts identical, and assert they do not
share the irreversible consume path. Use makeCoordinator overrides and the
existing recovery test helpers to isolate and verify the coordinationScope
comparison in recoveryContractsMatch.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 97e446f1-dbe0-4b78-b578-011008e2bb5c
📒 Files selected for processing (2)
src/codex/reset-credit-recovery.tstests/codex-reset-credit-recovery.test.ts
bf189c5 to
1996e79
Compare
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Current commit ==='
git rev-parse HEAD
git status --short
printf '%s\n' '=== Candidate files ==='
fd -t f '^(reset-credit-recovery\.ts|codex-reset-credit-recovery\.test\.ts)$' .
printf '%s\n' '=== Source structure ==='
ast-grep outline src/codex/reset-credit-recovery.ts --items all --view expanded
printf '%s\n' '=== Terminal-state and coordination references ==='
rg -n -C 8 \
'(terminalByAccount|terminal.*Key|coordinationScope|coordination-mismatch|recoveryContract|contract)' \
src/codex/reset-credit-recovery.ts
printf '%s\n' '=== Relevant source region ==='
nl -ba src/codex/reset-credit-recovery.ts | sed -n '330,560p'
printf '%s\n' '=== Regression-test references ==='
rg -n -C 10 \
'(different scope|coordinationScope|terminal.*scope|terminal.*generation|coordination-mismatch)' \
tests/codex-reset-credit-recovery.test.tsLength of output: 20476 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Terminal-state contract regression tests ==='
rg -n -i -C 14 \
'(coordination-mismatch|coordination scope|different scope|terminal.*contract|terminal.*generation|same generation)' \
tests/codex-reset-credit-recovery.test.ts || true
printf '%s\n' '=== Test structure ==='
ast-grep outline tests/codex-reset-credit-recovery.test.ts --items all --view expanded
printf '%s\n' '=== Test file: relevant numbered excerpts ==='
cat -n tests/codex-reset-credit-recovery.test.ts | sed -n '1,420p'Length of output: 30434
In The comparison includes
You are interacting with an AI system. |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/codex/reset-credit-recovery.ts`:
- Around line 674-681: Update the class documentation paragraph describing
timed-out adapter work to match trackedCapacity and hasFlightCapacity: state
that orphaned executions are removed from the public flight registry but
continue counting toward both flight and account admission limits until they
settle. Do not change the runtime logic or tests.
In `@tests/codex-reset-credit-recovery.test.ts`:
- Around line 272-285: Extend the existing test.each block for
CodexResetCreditRecoveryCoordinator construction to include invalid
operationTimeoutMs and maxConsumeAttempts cases. Assert the exact validation
messages for values outside 1..MAX_OPERATION_TIMEOUT_MS and 1..3, using the
source constant for the timeout upper bound so the tests pin both numeric ranges
alongside the adapter callability checks.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a53fb6dc-b5c0-4b9e-a3f3-ae7877f4e1ca
📒 Files selected for processing (2)
src/codex/reset-credit-recovery.tstests/codex-reset-credit-recovery.test.ts
93124bf to
583fb84
Compare
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/codex/reset-credit-recovery.ts`:
- Around line 428-435: Validate lifecycleSignal in the constructor alongside
revalidate and consume, rejecting values that are not AbortSignal-compatible
with a TypeError that names lifecycleSignal. Apply this validation before
freezing it into this.contract, then continue using the validated local
lifecycleSignal when constructing the contract.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b4a0c71a-d386-4172-91fe-da7fbbbd99eb
📒 Files selected for processing (2)
src/codex/reset-credit-recovery.tstests/codex-reset-credit-recovery.test.ts
|
Latest head |
583fb84 to
bc23a3a
Compare
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc23a3ae8e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
bc23a3a to
63c9feb
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63c9feb1a4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Wibias
left a comment
Wibias
left a comment
There was a problem hiding this comment.
Requesting changes on exact head 63c9feb1a4b4faa9ee7339c8c5ec5eb439e8e35e.
Two fail-closed invariants are still not proven for this irreversible-operation coordinator:
-
High: malformed per-request
options.signalcan reject the caller while leaving an authorizing waiter behind.lifecycleSignalis runtime-validated, butoptions.signalis not. A non-AbortSignalvalue can pass the initialsignal.abortedcheck, create/register a flight and waiter, then throw duringsignal.addEventListener(...)injoinFlight. That rejection does not roll backactiveWaitersor the output-guard registration. The scheduled flight can then revalidate and dispatchconsumein the background even though the caller already received a rejection. Validate the request signal before flight creation and make listener registration rollback-safe. Add a regression proving malformed/listener-throwing signals cause no revalidation, no consume, no terminal fence, and no retained flight. -
Major: a fully cancelled flight can still invoke
revalidate(). The flight starts on the next microtask. If the last waiter cancels afterrecover()returns but before that microtask runs,preDispatchAbortis already aborted andactiveWaiters === 0, butrunFlight()still calls the revalidation adapter before checking those conditions. This violates the pre-dispatch cancellation contract and can start network work or retain an abort-ignoring orphan for a request with no live waiter. Check cancellation/waiter count at the very start ofrunFlight()before invoking any adapter, and add the immediate-post-recover()cancellation regression.
The rest of the coordinator has substantially improved and the previously resolved idempotency, single-flight, re-entrancy, capacity, output-guard, and deadline findings appear addressed. These two issues should block wiring or merge because this PR is specifically the safety foundation for a scarce, irreversible reset-credit consume.
63c9feb to
201e5d3
Compare
|
I rechecked the current head I am not approving this head yet. It is 33 commits behind current The direction remains potentially valuable, but this risk surface needs current-base validation rather than an approval based only on the focused 68-test suite. |
201e5d3 to
4e761a4
Compare
4e761a4 to
c693cb9
Compare
3 participants
Summary
This PR intentionally does not wire automatic redemption or request replay into Responses, change the manual consume route, add account selection, or expose configuration/UI. It is a review-ready foundation for independently proving the irreversible-operation invariants requested in #657.
The coordinator is process-local. A future runtime adapter must persist and reuse the same operation identity across uncertain transport outcomes, echo that identity from consume results, bind it to the account generation, advance the main-account identity epoch, and keep the output-exposure guard monotonic.
Refs #657.
Verification
bun test --isolate tests/codex-reset-credit-recovery.test.ts— 68 passed, 0 failedbun test --isolate tests/codex-reset-credit-recovery.test.ts— 68 passed, 0 failedbun x --package typescript@7.0.2 tsc --noEmit— passedbun x --package typescript@7.0.2 tsc --noEmit— passedbun run privacy:scan— passedgit diff HEAD^ --check— passedChecklist
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit
New Features
Bug Fixes