feat(codex): persist reset-credit operation identity - #1526
Conversation
github-actions
Bot
added
enhancement
|
⏳ DRAFT
What to do
Review readiness checklist
✅ 4/4 boxes ticked. This pull request was already a draft. Its draft status will be preserved after every issue above is resolved. |
|
@CodeRabbit review |
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughThe reset-credit flow now uses a dedicated authenticated consumption client, a durable SQLite operation ledger, and operation-specific recovery. The auth API maps cancellation and upstream failures to structured responses. Tests cover validation, persistence, recovery, and request behavior. ChangesReset-credit consumption and recovery
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AuthAPI
participant consumeCodexResetCredit
participant CodexUpstream
AuthAPI->>consumeCodexResetCredit: credentials, operation ID, request signal
consumeCodexResetCredit->>CodexUpstream: authenticated reset-credit POST
CodexUpstream-->>consumeCodexResetCredit: response code and operation ID
consumeCodexResetCredit-->>AuthAPI: result or typed error
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🤖 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/auth-api.ts`:
- Around line 1775-1780: In src/codex/auth-api.ts:1775-1780, integrate
CodexResetCreditRecoveryGeneration with openResetCreditOperation before
consumeCodexResetCredit, reuse the reserved operationId, return stored terminal
outcomes without dispatching upstream, settle terminal results, and mark
ambiguous transport failures while preserving fail-closed state. In
tests/codex-auth-api.test.ts:2198-2201, add an ambiguous-transport retry case
asserting only one upstream consume and reuse of the same redeem_request_id.
In `@src/codex/reset-credit-operation-ledger.ts`:
- Around line 389-391: In src/codex/reset-credit-operation-ledger.ts lines
389-391, update the catch handler to capture the error and warn for every cause
except SQLITE_BUSY, using only the ledger’s own error strings; apply the same
change to the second handler at lines 425-427. In src/config.ts lines 2414-2422,
enforce the documented precondition in the config-mutation flow by throwing when
configMutationLockDepth > 0, so nested mutations surface as errors.
- Around line 430-464: Document the public throw-versus-return contract for
markResetCreditOperationAmbiguous, openResetCreditOperation, and
settleResetCreditOperation. State that invalid now values and invalid
generations throw TypeError, while invalid operation IDs or non-terminal settle
codes return the existing mismatch result, keeping the current behavior
unchanged.
- Around line 15-20: Replace TERMINAL_CODES with an exhaustive
Record<CodexResetCreditConsumeCode, "confirmed" | "stopped">, assigning each
consume code its terminal state so future enum members require compiler updates.
Update terminal membership checks and state validation in
settleResetCreditOperation, including the mappings around lines 165-166 and 459,
to derive behavior from this record rather than set membership.
- Around line 282-293: Reuse the existing compareGenerationOrder comparator in
compareGeneration instead of maintaining duplicate lexicographic ordering logic.
Import or otherwise expose compareGenerationOrder as needed, and preserve the
current credentialGeneration-then-exhaustionGeneration ordering contract.
- Around line 197-208: Update the table_xinfo query in the reset-credit
operation ledger validation to interpolate or otherwise use the existing
TABLE_NAME constant instead of the hardcoded reset_credit_operations identifier,
while preserving the current column retrieval and validation flow.
- Around line 305-306: Update the withLedger callback type to reject inferred
PromiseLike results at compile time, while preserving synchronous return values
and the existing runtime guard. Ensure async callbacks and direct promise
returns fail type checking rather than reaching the surrounding catch path.
- Around line 57-67: Add a regression test in the reset credit operation ledger
test suite that creates a fresh table and asserts its main.sqlite_schema.sql
equals the EXPECTED_SCHEMA_SQL value derived from CREATE_TABLE.replace("main.",
""). Reuse the existing table-opening/setup path and verify the schema contract
before exercising openResetCreditOperation.
In `@src/codex/reset-credit-recovery.ts`:
- Around line 487-493: Make createLogicalTurnForOperation require a branded
reserved-operation-id type rather than a plain string, while retaining its
existing UUID validation and restoration behavior. Define the brand near the
recovery types and expose a controlled conversion usable only by
openResetCreditOperation; return the branded value from that function’s execute
and terminal results so arbitrary UUIDs cannot reach the restoration seam
without a ledger-issued token.
In `@src/config.ts`:
- Around line 2414-2422: Enforce the documented non-nesting precondition in
prepareConfigMutationDatabasePathForWrite by checking configMutationLockDepth
and throwing a clear error when called while withConfigMutationLockSync is
active; otherwise preserve the existing database-path return. Ensure the related
withLedger error-reporting path surfaces this specific failure rather than
leaving it indistinguishable from ordinary SQLITE_BUSY contention.
In `@tests/codex-reset-credit-operation-ledger.test.ts`:
- Around line 234-242: Move the holder.exec call that sets busy_timeout and
begins the transaction inside the existing try block so holder.close() is always
reached via finally. Because ROLLBACK can fail when BEGIN IMMEDIATE did not open
a transaction, guard or otherwise suppress rollback errors while ensuring
holder.close() still executes.
- Around line 37-63: Add a focused regression test near the existing
duplicate-ledger tests that uses the canonical schema, creates a reservation for
one account, inserts the same operationId under a different account, and
verifies subsequent openResetCreditOperation calls fail closed with unavailable.
Keep createLaxDuplicateLedger for schema-validation coverage and exercise
initializeTable’s cross-account operationIds rejection path.
- Around line 105-112: Add an assertion to the recovery test around
coordinator.recover and settleResetCreditOperation that explicitly verifies the
durable ledger remains pending after recovery while terminalByAccount contains
the terminal fence, documenting this intentional boundary without changing
production behavior. Use the existing ledger lookup and coordinator state
symbols rather than adding new flows.
- Around line 289-304: Update the overflow-row setup in the test to derive its
operation_id from MAX_RESET_CREDIT_OPERATION_ACCOUNTS using a value beyond the
loop’s generated index range, rather than hardcoding a potentially colliding
suffix. Add a concise comment documenting that this over-capacity test relies on
SELECT_ALL fetching MAX_RESET_CREDIT_OPERATION_ACCOUNTS + 1 rows so the capacity
check is exercised.
- Around line 24-35: Update corruptFirstRecord to remove the LIMIT 1 clause from
its UPDATE statement, leaving the existing single-row test setup and
databasePath implementation unchanged.
🪄 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: 203e3b01-da12-407a-8b20-1221513b02b8
📒 Files selected for processing (8)
src/codex/auth-api.tssrc/codex/reset-credit-consume.tssrc/codex/reset-credit-operation-ledger.tssrc/codex/reset-credit-recovery.tssrc/config.tstests/codex-auth-api.test.tstests/codex-reset-credit-consume.test.tstests/codex-reset-credit-operation-ledger.test.ts
|
This is not ready for maintainer sponsorship or integration review on the current head. The PR describes a durable operation ledger, but the production manual route in That mismatch matters: after an ambiguous consume, a retry can still mint a new redemption ID, so the central idempotency property this foundation claims is not exercised end to end. CodeRabbit's first blocker identifies the same missing integration and should be treated as valid. The remaining ledger schema/transaction findings also need individual resolution before a security review. Please keep this draft and do not request |
d089c8a to
aefc06f
Compare
|
Implemented the requested durable manual-route integration on exact head
Focused verification passed on Bun 1.3.14 and 1.4.0-canary.1 (45 affected regressions on each), TypeScript passed on both, privacy scan and diff check passed, and two independent read-only reviews found no remaining actionable P0-P3 findings. I did not duplicate the full GitHub CI workload locally. This branch is based on current |
a1911d1 to
a89e331
Compare
2 participants
Summary
The manual ledger is keyed by a domain-separated hash of the authoritative physical ChatGPT account ID. It supports both the main login and pool aliases without inventing a recovery generation. The GUI creates one browser-compatible UUID per opened redemption intent and retains it across a failed retry; the CLI creates one UUID per explicit
--consume --yesinvocation. The API rejects missing or malformed IDs instead of manufacturing a new intent after an ambiguous response.Before dispatch, the ledger commits the reservation with an independent top-level
BEGIN IMMEDIATEtransaction andsynchronous=FULL. A pending or ambiguous record restores the same ID, a terminal record short-circuits the upstream call, and a post-dispatch error is marked ambiguous. Terminal results are persisted before quota refresh or the HTTP success response. A process-local physical-account flight prevents duplicate concurrent dispatches; separate processes still converge on the same durable upstream idempotency ID.The persistence authority validates an exact
STRICT, WITHOUT ROWIDschema, rejects triggers, duplicate operation IDs, malformed or over-capacity rows, confirms every write by exact in-transaction readback, and atomically migrates only the exact prior Draft recovery-only schema. Stored account references are hashes; credentials and raw physical account IDs are not persisted.This remains a focused foundation for the remaining contract in #657. It does not wire Responses failures to redemption, replay a request, add automatic-spend policy/configuration, or activate the recovery coordinator. Automatic recovery still requires a separately proven authoritative exhaustion-generation claim.
Refs #657.
Test plan
tsc --noEmitpassed on both Bun runtimesbun run privacy:scanpassed on both Bun runtimesgit diff --checkpasseddevrebase preserved all 17 changed-file blobs byte-for-byteThe full local suite was intentionally not duplicated because GitHub CI owns that workload. An unrelated Windows ACL teardown failure observed in an earlier shared config-generation diagnostic is not attributed to this patch.
UI evidence
The GUI change is behavioral only: the existing reset-credit modal now keeps one opaque operation ID across a failed retry. It does not change layout, copy, or visual state. A maintainer
gui-screenshot-waivedreview is requested instead of publishing a local account screenshot.Checklist
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