feat(codex): coordinate reset-credit recovery attempts - #1410
Conversation
📝 WalkthroughWalkthroughAdds ChangesReset-credit recovery
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
| const RESET_ELIGIBLE_CODES: ReadonlySet<string> = new Set([ | ||
| "usage_limit_exceeded", | ||
| "insufficient_quota", | ||
| ]); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm the CodexResetEligibleExhaustionCode union members and export status.
set -euo pipefail
fd -t f 'quota-rejection.ts' src | while IFS= read -r file; do
echo "== $file =="
rg -n -C 4 'CodexResetEligibleExhaustionCode' "$file"
done
# Find every other place that hardcodes the same semantic codes.
rg -n 'usage_limit_exceeded|insufficient_quota' --type=tsRepository: lidge-jun/opencodex
Length of output: 2419
🏁 Script executed:
#!/bin/bash
set -u
echo "== candidate files =="
git ls-files 'src/codex/reset-credit-recovery.ts' 'src/codex/quota-rejection.ts'
echo
echo "== reset-credit-recovery relevant declarations/usages =="
sed -n '1,270p' src/codex/reset-credit-recovery.ts
echo
echo "== quota-rejection exact section =="
sed -n '1,25p' src/codex/quota-rejection.tsRepository: lidge-jun/opencodex
Length of output: 10324
Import and enforce CodexResetEligibleExhaustionCode for RESET_ELIGIBLE_CODES.
src/codex/reset-credit-recovery.ts:145-148 defines an invariant set against "usage_limit_exceeded" | "insufficient_quota" by copying values instead of using the shared src/codex/quota-rejection.ts:3-9 union. If CodexResetEligibleExhaustionCode changes, authorizedResetRejection at src/codex/reset-credit-recovery.ts:221 can silently exclude the new code and return not-dispatched / ineligible-rejection. Import the type from quota-rejection.ts, make RESET_ELIGIBLE_CODES typed as ReadonlySet<CodexResetEligibleExhaustionCode>, and cast value.semanticCode at the has call so drift is a compile error.
🤖 Prompt for 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.
In `@src/codex/reset-credit-recovery.ts` around lines 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.
| private static readonly activeFlights = new Map<string, RecoveryFlight>(); | ||
| private static readonly allFlights = new Set<RecoveryFlight>(); | ||
| private static readonly terminalByAccount = new Map<string, TerminalGeneration>(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial
terminalByAccount never evicts, so the 128-account cap is permanent for the process lifetime.
terminalByAccount is only cleared by resetProcessStateForTests. hasFlightCapacity at Lines 453-456 counts terminal accounts plus in-flight accounts against MAX_TRACKED_RECOVERY_ACCOUNTS. In a long-lived server with more than 128 distinct pool accounts, the registry fills permanently. Every subsequent new account then receives not-dispatched / recovery-state-capacity forever, with no log line and no metric.
The behavior fails closed, which is correct for an irreversible operation. The operational gap is that it is silent and unrecoverable without a process restart.
Before this coordinator is wired to the runtime adapter, add one of the following:
- Time-based or LRU eviction for
terminalByAccountentries, keyed on the recorded generation. - A counter or log line when
hasFlightCapacityreturnsfalse, so operators can detect saturation.
Do not log the accountId value itself in that signal; emit a count only.
Also applies to: 449-457
🤖 Prompt for 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.
In `@src/codex/reset-credit-recovery.ts` around lines 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.
| let flight = CodexResetCreditRecoveryCoordinator.activeFlights.get(key); | ||
| if (flight | ||
| && !flight.dispatchStarted | ||
| && (flight.preDispatchAbort.signal.aborted || flight.activeWaiters === 0)) { | ||
| turnState.attempt = Promise.resolve(CANCELLED_BEFORE_DISPATCH); | ||
| return turnState.attempt; | ||
| } | ||
| if (!flight) { | ||
| if (!CodexResetCreditRecoveryCoordinator.hasFlightCapacity( | ||
| generationSnapshot.accountId, | ||
| )) { | ||
| turnState.attempt = Promise.resolve(notDispatched("recovery-state-capacity")); | ||
| return turnState.attempt; | ||
| } | ||
| flight = this.createFlight(key, generationSnapshot, turn.operationId); | ||
| CodexResetCreditRecoveryCoordinator.activeFlights.set(key, flight); | ||
| } | ||
|
|
||
| turnState.attempt = this.joinFlight(flight, signals, authorization.isOutputExposed); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
The static flight key omits coordinator identity, so a joining instance silently runs another instance's adapters and limits. activeFlights is static and the key at Line 383 contains only accountId, credentialGeneration, and exhaustionGeneration. A flight created by one instance closes over that instance's dependencies, operationTimeoutMs, and maxConsumeAttempts. A second instance that joins discards its own configuration for that flight, including its consume adapter for an irreversible credit operation.
src/codex/reset-credit-recovery.ts#L403-L421: either include a per-dependency-set identity in the flight key so instances with different adapters never share a flight, or validate the joining instance's dependency identity and limits against the flight and return a dedicatednot-dispatchedreason on mismatch.tests/codex-reset-credit-recovery.test.ts#L315-L359: both coordinators receive the samerevalidateandconsumereferences at Lines 333-340, so the assertions cannot distinguish which instance executed. Give each coordinator a distinguishableconsumeand assert which one ran, so the chosen contract is pinned.
📍 Affects 2 files
src/codex/reset-credit-recovery.ts#L403-L421(this comment)tests/codex-reset-credit-recovery.test.ts#L315-L359
🤖 Prompt for 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.
In `@src/codex/reset-credit-recovery.ts` around lines 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.
| static resetProcessStateForTests(): void { | ||
| for (const flight of this.allFlights) { | ||
| flight.preDispatchAbort.abort(); | ||
| flight.operationAbort.abort(); | ||
| } | ||
| this.activeFlights.clear(); | ||
| this.allFlights.clear(); | ||
| this.terminalByAccount.clear(); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
A public, ungated resetProcessStateForTests erases the only fence against a duplicate irreversible consume. terminalByAccount is the sole record that a given (accountId, credentialGeneration, exhaustionGeneration) tuple already dispatched a consume. resetProcessStateForTests is public static on an exported class, clears that map, and carries no runtime gate. The test body demonstrates the exact sequence.
src/codex/reset-credit-recovery.ts#L439-L447: gate the method so production code cannot reach it, or move the process registries and this helper into an internal module that the production entry point does not re-export.tests/codex-reset-credit-recovery.test.ts#L880-L916: after Line 904 clears the fence, the assertion at Line 915 confirms a second consume runs for the sameBASE_GENERATIONthat already recorded anambiguousterminal outcome. Split the post-reset "confirmed" scenario into its owntestso the reset happens inbeforeEach, and the suite stops relying on a mid-test call to the fence eraser.
📍 Affects 2 files
src/codex/reset-credit-recovery.ts#L439-L447(this comment)tests/codex-reset-credit-recovery.test.ts#L880-L916
🤖 Prompt for 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.
In `@src/codex/reset-credit-recovery.ts` around lines 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.
| private async runFlightWithDeadline( | ||
| flight: RecoveryFlight, | ||
| ): Promise<Awaited<RecoveryFlight["promise"]>> { | ||
| let timer: ReturnType<typeof setTimeout> | undefined; | ||
| const deadline = new Promise<Awaited<RecoveryFlight["promise"]>>(resolve => { | ||
| timer = setTimeout(() => { | ||
| flight.expired = true; | ||
| if (!flight.dispatchStarted) { | ||
| flight.preDispatchAbort.abort(); | ||
| resolve(notDispatched("operation-expired-before-dispatch")); | ||
| return; | ||
| } | ||
| flight.operationAbort.abort(); | ||
| resolve(freezeResult({ | ||
| kind: "ambiguous", | ||
| reason: "consume-timeout", | ||
| operationId: flight.operationId, | ||
| })); | ||
| }, this.operationTimeoutMs); | ||
| }); | ||
|
|
||
| try { | ||
| return await Promise.race([this.runFlight(flight), deadline]); | ||
| } finally { | ||
| if (timer !== undefined) clearTimeout(timer); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Look for existing timer-lifetime handling patterns in src.
set -euo pipefail
rg -n -C 3 'setTimeout\(|clearTimeout\(|\.unref\(\)' --type=ts -g 'src/**' | head -60Repository: lidge-jun/opencodex
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the relevant target file and surrounding code without executing repository code.
if [ -f src/codex/reset-credit-recovery.ts ]; then
echo "== file exists =="
wc -l src/codex/reset-credit-recovery.ts
echo "---- lines 410-585 ----"
sed -n '410,585p' src/codex/reset-credit-recovery.ts | nl -ba -v410
echo "---- lines 640-715 ----"
sed -n '640,715p' src/codex/reset-credit-recovery.ts | nl -ba -v640
else
echo "missing src/codex/reset-credit-recovery.ts"
fd -a reset-credit-recovery.ts .
fi
echo "---- RecoveryFlight declarations/usages ----"
rg -n "interface RecoveryFlight|type RecoveryFlight|RecoveryFlight|operationTimeoutMs|MAX_OPERATION_TIMEOUT_MS|resetProcessStateForTests|runFlightWithDeadline|runFlight\\(" --type=ts src/codex/reset-credit-recovery.tsRepository: lidge-jun/opencodex
Length of output: 278
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path("src/codex/reset-credit-recovery.ts")
if not p.exists():
print("missing")
raise SystemExit
text = p.read_text()
checks = {
"runFlightWithDeadline_has_timer_local": "let timer: ReturnType<typeof setTimeout> | undefined" in text,
"runFlightWithDeadline_clears_in_finally": "if (timer !== undefined) clearTimeout(timer)" in text,
"resetProcessState_aborts_flights": "flight.preDispatchAbort.abort()" in text and "flight.operationAbort.abort()" in text,
"resetProcessState_clears_timers": "clearTimeout" in text and "flight.preDispatchAbort.abort()" in text and text.find("clearTimeout") <= text.find("flight.preDispatchAbort.abort()"),
"has_unref_pattern": ".unref()" in text or "unref()" in text,
}
for k, v in checks.items():
print(f"{k}={v}")
# Extract a deterministic structural model based only on text occurrences and line numbers.
for name in ["RecoveryFlight", "resetProcessStateForTests", "runFlightWithDeadline"]:
idx = text.find(f" {name} ")
print(f"\n--- first occurrence of {name} at char {idx} ---")
# Print nearby lines around first occurrence.
lines = text.splitlines()
line = next((i+1 for i,l in enumerate(text.splitlines()) if name in l), None)
if line:
lo, hi = max(1,line-15), min(len(lines),line+60)
for i in range(lo,hi+1):
if "RecoveryFlight" in lines[i-1] or "resetProcessStateForTests" in lines[i-1] or "runFlightWithDeadline" in lines[i-1] or "clearTimeout" in lines[i-1] or "operationTimeoutMs" in lines[i-1]:
print(f"{i:4}:{lines[i-1]}")
PYRepository: lidge-jun/opencodex
Length of output: 1161
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Probe Bun timer event-loop semantics and abort-signal promise behavior if Bun is available.
if command -v bun >/dev/null 2>&1; then
echo "== bun version =="
bun --version
echo "== setTimeout keeps loop alive behavior =="
cat > /tmp/bun_timers_probe.ts <<'JS'
let settled = false;
setTimeout(() => {
console.log("timer fired");
settled = true;
}, 10);
setTimeout(() => {
console.log("second timer", settled);
}, 2;
JS
(timeout 1s bun /tmp/bun_timers_probe.ts 2>&1 || true)
echo "== Bun AbortSignal abort behavior =="
cat > /tmp/bun_abort_probe.ts <<'JS'
const controller = new AbortController();
const signal = controller.signal;
async function ignored() { await new Promise(() => {}); }
const p = ignored();
setTimeout(() => { controller.abort(); }, 10);
setTimeout(() => {
console.log("aborted", signal.aborted);
}, 30);
setTimeout(() => console.log("settled"), 50);
// No await or catch here; emulate flight promise settling without observing abort.
JS
(timeout 1s bun /tmp/bun_abort_probe.ts 2>&1 || true)
else
echo "bun not available"
fiRepository: lidge-jun/opencodex
Length of output: 175
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path("src/codex/reset-credit-recovery.ts")
if not p.exists():
print("missing")
raise SystemExit
text = p.read_text()
checks = {
"runFlightWithDeadline_has_timer_local": "let timer: ReturnType<typeof setTimeout> | undefined" in text,
"runFlightWithDeadline_clears_in_finally": "if (timer !== undefined) clearTimeout(timer)" in text,
"resetProcessState_aborts_flights": "flight.preDispatchAbort.abort()" in text and "flight.operationAbort.abort()" in text,
"resetProcessState_clears_timers": "clearTimeout" in text and "flight.preDispatchAbort.abort()" in text and text.find("clearTimeout") <= text.find("flight.preDispatchAbort.abort()"),
"has_unref_pattern": ".unref()" in text or "unref()" in text,
}
for k, v in checks.items():
print(f"{k}={v}")
for name in ["RecoveryFlight", "resetProcessStateForTests", "runFlightWithDeadline"]:
print(f"\n--- matches for {name} ---")
lines = text.splitlines()
for i, line in enumerate(lines, 1):
if name in line:
print(f"{i}:{line}")
PYRepository: lidge-jun/opencodex
Length of output: 1318
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,130p' src/codex/reset-credit-recovery.ts | cat
sed -n '420,585p' src/codex/reset-credit-recovery.ts | cat
sed -n '574,625p' src/codex/reset-credit-recovery.ts | cat
sed -n '640,710p' src/codex/reset-credit-recovery.ts | catRepository: lidge-jun/opencodex
Length of output: 14275
Clear flight timers during test state reset.
In src/codex/reset-credit-recovery.ts, runFlightWithDeadline stores the deadline handle in a local variable and clears it only in the finally block after Promise.race settles. resetProcessStateForTests aborts preDispatchAbort/operationAbort and clears registries but does not clear any pending timers. If revalidate or consume ignore the abort signal, the race can remain pending and keep the Bun event loop alive for up to MAX_OPERATION_TIMEOUT_MS (60s) until the timeout fires.
Add a timer field to RecoveryFlight, assign it in runFlightWithDeadline, and clear it in resetProcessStateForTests.
🤖 Prompt for 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.
In `@src/codex/reset-credit-recovery.ts` around lines 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.
Source: Path instructions
| const generic = await recover( | ||
| coordinator, | ||
| coordinator.createLogicalTurn(), | ||
| BASE_GENERATION, | ||
| {}, | ||
| { | ||
| enabled: true, | ||
| isOutputExposed: () => false, | ||
| rejection: { | ||
| kind: "generic-rate-limit", | ||
| status: 429, | ||
| alternateRetryEligible: true, | ||
| resetCreditEligible: false, | ||
| }, | ||
| }, | ||
| ); | ||
| const wrongStatus = await recover( | ||
| coordinator, | ||
| coordinator.createLogicalTurn(), | ||
| BASE_GENERATION, | ||
| {}, | ||
| { | ||
| ...AUTHORIZATION, | ||
| rejection: { ...AUTHORIZATION.rejection, status: 500 }, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add negative cases that isolate the resetCreditEligible and semanticCode gates.
authorizedResetRejection in src/codex/reset-credit-recovery.ts applies five independent conditions at Lines 216-221. The current suite does not isolate two of them.
- Line 219 checks
resetCreditEligible === true. Thegenericcase at Lines 176-191 setsresetCreditEligible: false, but it also changeskindto"generic-rate-limit"and dropssemanticCode. Three conditions fail at once. If Line 219 were deleted, this test would still pass. - Line 221 checks membership in
RESET_ELIGIBLE_CODES. No case supplies a validkindandstatuswith an unrecognizedsemanticCode. If Line 221 were deleted, the whole suite would still pass.
Both gates authorize an irreversible credit consume. Each needs a case that fails exactly one condition.
As per path instructions for tests/**: "A behavior change in src/ should come with a focused regression test near the existing tests for that subsystem."
♻️ Proposed additional cases
const wrongStatus = await recover(
coordinator,
coordinator.createLogicalTurn(),
BASE_GENERATION,
{},
{
...AUTHORIZATION,
rejection: { ...AUTHORIZATION.rejection, status: 500 },
},
);
+ const notResetCreditEligible = await recover(
+ coordinator,
+ coordinator.createLogicalTurn(),
+ BASE_GENERATION,
+ {},
+ {
+ ...AUTHORIZATION,
+ rejection: { ...AUTHORIZATION.rejection, resetCreditEligible: false },
+ },
+ );
+ const unknownSemanticCode = await recover(
+ coordinator,
+ coordinator.createLogicalTurn(),
+ BASE_GENERATION,
+ {},
+ {
+ ...AUTHORIZATION,
+ rejection: {
+ ...AUTHORIZATION.rejection,
+ semanticCode: "some_other_code" as never,
+ },
+ },
+ );Then assert both equal generic alongside Lines 222-223.
🤖 Prompt for 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.
In `@tests/codex-reset-credit-recovery.test.ts` around lines 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`.
Source: Path instructions
| test("single-flights concurrent turns process-wide for the same generation", async () => { | ||
| const gate = deferred<CodexResetCreditRevalidationResult>(); | ||
| const started = deferred<void>(); | ||
| let revalidationCalls = 0; | ||
| let consumeCalls = 0; | ||
| const seenOperationIds: string[] = []; | ||
| const revalidate = async () => { | ||
| revalidationCalls += 1; | ||
| started.resolve(); | ||
| return await gate.promise; | ||
| }; | ||
| const consume: CodexResetCreditRecoveryDependencies["consume"] = async ({ | ||
| operationId, | ||
| }) => { | ||
| consumeCalls += 1; | ||
| seenOperationIds.push(operationId); | ||
| return { code: "reset", operationId }; | ||
| }; | ||
| const firstCoordinator = makeCoordinator({ | ||
| revalidate, | ||
| consume, | ||
| }); | ||
| const secondCoordinator = makeCoordinator({ | ||
| revalidate, | ||
| consume, | ||
| }); | ||
|
|
||
| const firstTurn = firstCoordinator.createLogicalTurn(); | ||
| const secondTurn = secondCoordinator.createLogicalTurn(); | ||
| const first = recover(firstCoordinator, firstTurn); | ||
| const second = recover(secondCoordinator, secondTurn); | ||
| await started.promise; | ||
| gate.resolve(eligible(BASE_GENERATION)); | ||
|
|
||
| expect(await Promise.all([first, second])).toEqual([ | ||
| { kind: "refresh-required", code: "reset" }, | ||
| { kind: "refresh-required", code: "reset" }, | ||
| ]); | ||
| expect(revalidationCalls).toBe(1); | ||
| expect(consumeCalls).toBe(1); | ||
| expect(secondTurn.operationId).not.toBe(firstTurn.operationId); | ||
| expect(seenOperationIds).toEqual([firstTurn.operationId]); | ||
| expect(firstCoordinator.activeFlightCountForTests()).toBe(0); | ||
| expect(firstCoordinator.terminalGenerationCountForTests()).toBe(1); | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
This test cannot detect which instance's dependencies the shared flight uses.
The test constructs firstCoordinator and secondCoordinator at Lines 333-340, but passes the identical revalidate and consume function references to both. Both also use the default operationTimeoutMs and maxConsumeAttempts.
The assertions at Lines 353-356 therefore hold no matter which instance owns the flight. The test proves that a flight is shared. It does not prove which coordinator's adapters ran. That distinction is the substance of the finding on src/codex/reset-credit-recovery.ts Lines 403-421.
Give each coordinator a distinguishable consume and assert which one executed.
As per path instructions for tests/**: "Flag PRs that change shared routing, adapters, config, or server behavior without touching tests."
♻️ Proposed discriminating assertion
+ const executedBy: string[] = [];
const firstCoordinator = makeCoordinator({
revalidate,
- consume,
+ consume: async input => {
+ executedBy.push("first");
+ return await consume(input);
+ },
});
const secondCoordinator = makeCoordinator({
revalidate,
- consume,
+ consume: async input => {
+ executedBy.push("second");
+ return await consume(input);
+ },
});Then assert executedBy explicitly. Whichever value the implementation produces, record it as the documented contract.
🤖 Prompt for 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.
In `@tests/codex-reset-credit-recovery.test.ts` around lines 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.
Source: Path instructions
| for (let index = 0; index < 128; index += 1) { | ||
| expect(await recover(coordinator, coordinator.createLogicalTurn(), { | ||
| ...BASE_GENERATION, | ||
| accountId: `capacity-${index}`, | ||
| })).toEqual({ kind: "refresh-required", code: "reset" }); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Derive the capacity bounds from exported constants instead of the literal 128.
Line 405 loops to 128. Line 419 asserts consumeCalls === 128. Line 420 asserts terminalGenerationCountForTests() === 128. Line 439 builds 128 flights and Line 446 uses exhaustionGeneration: 128.
These literals mirror MAX_TRACKED_RECOVERY_ACCOUNTS and MAX_TRACKED_RECOVERY_FLIGHTS in src/codex/reset-credit-recovery.ts Lines 125-126. Neither constant is exported, so the compiler cannot link them. If a maintainer lowers either limit, these tests fail with an off-by-N mismatch that does not name the constant that moved. If a maintainer raises either limit, the overflow assertions at Lines 412-418 and 444-450 stop testing overflow and instead assert a successful dispatch, so the tests fail for the opposite reason.
Export the two constants and compute the loop bounds from them.
♻️ Proposed fix
In src/codex/reset-credit-recovery.ts:
-const MAX_TRACKED_RECOVERY_ACCOUNTS = 128;
-const MAX_TRACKED_RECOVERY_FLIGHTS = 128;
+export const MAX_TRACKED_RECOVERY_ACCOUNTS = 128;
+export const MAX_TRACKED_RECOVERY_FLIGHTS = 128;In this test file:
- for (let index = 0; index < 128; index += 1) {
+ for (let index = 0; index < MAX_TRACKED_RECOVERY_ACCOUNTS; index += 1) {- const pending = Array.from({ length: 128 }, (_, exhaustionGeneration) => recover(
+ const pending = Array.from(
+ { length: MAX_TRACKED_RECOVERY_FLIGHTS },
+ (_, exhaustionGeneration) => recover(Update the remaining 128 assertions in both tests to reference the same constants.
Also applies to: 439-450
🤖 Prompt for 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.
In `@tests/codex-reset-credit-recovery.test.ts` around lines 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.
| firstAttempt.reject(new Error("late transport rejection")); | ||
| await firstAttemptSettled.promise; | ||
| await Promise.resolve(); | ||
| expect(consumeCalls).toBe(1); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
The microtask drain is too weak to prove that no late retry was launched.
Line 750 awaits firstAttemptSettled.promise. The test's own finally at Lines 736-738 resolves that deferred before the rejection propagates to the coordinator. Line 751 then yields exactly one microtask.
In src/codex/reset-credit-recovery.ts, the rejection must travel from the await at Line 616 into the catch at Line 629, reach the flight.expired check at Line 630, and return at Line 631. That is more than one microtask hop.
The assertion at Line 752 is consumeCalls === 1, meaning "no second call yet". If the drain is too short, the assertion passes without ever reaching the retry decision point. The test cannot fail from an insufficient drain, so it silently under-verifies the guard it is named for.
Await the coordinator's own idle signal instead of counting microtasks.
♻️ Proposed fix
firstAttempt.reject(new Error("late transport rejection"));
await firstAttemptSettled.promise;
- await Promise.resolve();
+ await coordinator.waitForIdleForTests();
expect(consumeCalls).toBe(1);waitForIdleForTests uses Promise.allSettled over allFlights. The flight is already removed from allFlights by the finally at Line 538, so also consider capturing the flight promise before the deadline settles, or add an explicit drain helper that resolves after the consume loop exits.
🤖 Prompt for 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.
In `@tests/codex-reset-credit-recovery.test.ts` around lines 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.
| test.each([ | ||
| ["reset", { kind: "refresh-required", code: "reset" }], | ||
| ["already_redeemed", { kind: "refresh-required", code: "already_redeemed" }], | ||
| ["nothing_to_reset", { kind: "stopped", code: "nothing_to_reset" }], | ||
| ["no_credit", { kind: "stopped", code: "no_credit" }], | ||
| ] as const)("maps the exact %s consume outcome", async (code, expected) => { | ||
| const coordinator = makeCoordinator({ | ||
| consume: async ({ operationId }) => ({ code, operationId }), | ||
| }); | ||
| expect(await recover(coordinator, coordinator.createLogicalTurn())).toEqual(expected); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Use a per-case accountId so this test.each block does not depend on per-entry beforeEach.
All four test.each entries dispatch against the default BASE_GENERATION, which is a single (accountId, credentialGeneration, exhaustionGeneration) tuple. The first entry writes a terminal fence for "account-a" through src/codex/reset-credit-recovery.ts Lines 522-532. Entries two through four depend on the beforeEach at Lines 79-81 running before every generated entry to clear that fence.
If Bun ever scoped beforeEach per test.each block rather than per generated entry, entries two through four would replay the cached refresh-required / reset outcome and fail. The failure would be loud, not silent, so this is a robustness point rather than a defect.
Every other multi-case test in this file already avoids the dependency by suffixing the account id: Lines 802, 830, and 859. Apply the same pattern here for consistency.
♻️ Proposed fix
- ] as const)("maps the exact %s consume outcome", async (code, expected) => {
+ ] as const)("maps the exact %s consume outcome", async (code, expected) => {
const coordinator = makeCoordinator({
consume: async ({ operationId }) => ({ code, operationId }),
});
- expect(await recover(coordinator, coordinator.createLogicalTurn())).toEqual(expected);
+ expect(await recover(coordinator, coordinator.createLogicalTurn(), {
+ ...BASE_GENERATION,
+ accountId: `outcome-${code.replace(/_/g, "-")}`,
+ })).toEqual(expected);
});Confirm that the generated account ids satisfy CODEX_ACCOUNT_ID_RE before adopting this exact naming.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test.each([ | |
| ["reset", { kind: "refresh-required", code: "reset" }], | |
| ["already_redeemed", { kind: "refresh-required", code: "already_redeemed" }], | |
| ["nothing_to_reset", { kind: "stopped", code: "nothing_to_reset" }], | |
| ["no_credit", { kind: "stopped", code: "no_credit" }], | |
| ] as const)("maps the exact %s consume outcome", async (code, expected) => { | |
| const coordinator = makeCoordinator({ | |
| consume: async ({ operationId }) => ({ code, operationId }), | |
| }); | |
| expect(await recover(coordinator, coordinator.createLogicalTurn())).toEqual(expected); | |
| }); | |
| test.each([ | |
| ["reset", { kind: "refresh-required", code: "reset" }], | |
| ["already_redeemed", { kind: "refresh-required", code: "already_redeemed" }], | |
| ["nothing_to_reset", { kind: "stopped", code: "nothing_to_reset" }], | |
| ["no_credit", { kind: "stopped", code: "no_credit" }], | |
| ] as const)("maps the exact %s consume outcome", async (code, expected) => { | |
| const coordinator = makeCoordinator({ | |
| consume: async ({ operationId }) => ({ code, operationId }), | |
| }); | |
| expect(await recover(coordinator, coordinator.createLogicalTurn(), { | |
| ...BASE_GENERATION, | |
| accountId: `outcome-${code.replace(/_/g, "-")}`, | |
| })).toEqual(expected); | |
| }); |
🤖 Prompt for 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.
In `@tests/codex-reset-credit-recovery.test.ts` around lines 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.
|
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".
| if (kind === "no-credit") return notDispatched("no-credit-after-revalidate"); | ||
| if (kind !== "eligible") return notDispatched("invalid-revalidation"); | ||
|
|
||
| if (![...flight.outputGuards.keys()].some(guard => !outputIsExposed(guard))) { |
There was a problem hiding this comment.
When two turns share a flight, a guard can synchronously abort its own waiter during this .some() check, as the re-entrant-guard test establishes. The snapshot still contains that guard, so its returned false short-circuits the search even though onAbort removed it; if another active waiter's output is already exposed, activeWaiters remains nonzero and the coordinator dispatches the irreversible consume with no eligible waiter. After invoking a guard, confirm that it is still registered before treating it as authorization, and cover this concurrent case.
AGENTS.md reference: AGENTS.md:L106-L113
Useful? React with 👍 / 👎.
| const finish = (result: Awaited<RecoveryFlight["promise"]>) => { | ||
| if (settled) return; | ||
| settled = true; | ||
| removeAbortListeners(); |
There was a problem hiding this comment.
At flight completion, abort listeners are removed before outputGuard runs. If that guard re-entrantly aborts the request on its final invocation and returns false, the abort cannot set detachedAfterDispatch, so the raw refresh-required outcome is returned and can authorize replay of a canceled turn. Evaluate the guard while abort observation remains active or explicitly recheck the signals afterward; resolveTerminalOutcome needs the same post-guard check for cached outcomes.
AGENTS.md reference: src/AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
1 participant
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 Draft 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— 34 passed, 0 failedbun test --isolate tests/codex-reset-credit-recovery.test.ts— 34 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 --cached --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:
Summary by CodeRabbit
New Features
Bug Fixes