Skip to content

fix(codex): settle reset-credit retry aliases canonically - #3919

Closed
luvs01 wants to merge 1 commit into
lidge-jun:devfrom
luvs01:agent/reset-credit-canonical-alias-20260907
Closed

fix(codex): settle reset-credit retry aliases canonically#3919
luvs01 wants to merge 1 commit into
lidge-jun:devfrom
luvs01:agent/reset-credit-canonical-alias-20260907

Conversation

@luvs01

@luvs01 luvs01 commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Summary

A manual reset-credit retry can join an unfinished operation under a different caller ID. The upstream request already uses the original canonical ID, but the API was recording settlement with the alias ID. The ledger rejects that mismatch, so a later retry dispatches another consume request instead of replaying the stored result.

Use the ledger-returned canonical identity for settlement and ambiguous outcomes. Keep account ownership checks, failure handling, and the legacy path without an operation ID unchanged. The API reference clarifies alias replay and when a new ID represents a new redemption.

This carries the focused source commit 45ff81c25ed7636382fb0144409918067d44f616 onto current dev; unrelated release ancestry is excluded and the original authorship is retained. Scope is three files with 16 added lines and one removed line.

Verification

Head 6f20c3d08cd202e43b8679de33674133bdc7f34c, based on dev 273a3ab865cdd95f3ce08f5583c3e1661978301d. Tested tree: d7ca5f7f17e0acbdfd9d5b4fcf26171c7540834b.

  • bun test tests/codex-integration/codex-auth-api.test.ts tests/codex-integration/codex-reset-credit-operation-ledger.test.ts --timeout 60000: 299 passed with the project-pinned Bun 1.4.0 on Windows (and previously with Bun 1.4.2).
  • The alias replay regression passed with the fix and failed when only the production fix was removed. The original candidate was restored and its SHA-256 verified.
  • Test credentials, configuration homes, and upstream responses are fixtures; no real reset credit was consumed.
  • bun run typecheck, bun run privacy:scan, and git diff --check: passed.
  • Documentation build: 425 pages passed. Dependency declarations and frozen lockfiles match the reused installation.
  • Independent read-only security review found no blocking issue: canonicalization happens after account ownership and execute admission, preserves both account identifiers, and does not change fail-closed or legacy behavior.

bun run test:changed --timeout 60000 reached the repository runner's 900-second limit (exit 124), which discarded the captured output before emitting a selection summary. This is not a passing suite. Its owned runner and worker processes were confirmed absent afterward. Full cross-platform contributor CI passed all 26 jobs on this exact head after one failed-job rerun. Contributor validation is complete; the PR remains in draft pending the required maintainer security review / maintainer-sponsored label for src/codex/auth-api.ts; the independent review above does not satisfy that maintainer gate.

The first CI attempt completed the macOS control with 21,405 passed, 16 skipped, and two Cursor stream-health timing assertion failures. Both cases passed in the other macOS lane on the same head, and the alias replay regression passed in the failed control. The single failed-job rerun passed on the same head without a source change; the initial attempt is retained here and is not counted as green.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

Review readiness checklist

  • 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

  • Documentation

    • Clarified reset-credit redemption behavior when requests use different operation IDs.
    • Requests with a new ID during an unfinished redemption now follow the existing operation and reuse its result after completion.
    • Previously recognized aliases can replay the settled result, while new IDs submitted after settlement begin separate redemptions.
  • Bug Fixes

    • Improved consistency for reset-credit redemption retries by preserving the original operation’s outcome across supported aliases.

@coderabbitai

coderabbitai Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Team

Run ID: 6cf5df2e-740a-4c00-8fce-701b2344c12e

📥 Commits

Reviewing files that changed from the base of the PR and between 273a3ab and 6f20c3d.

📒 Files selected for processing (3)
  • docs-site/src/content/docs/reference/management-api.md
  • src/codex/auth-api.ts
  • tests/codex-integration/codex-auth-api.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.


📝 Walkthrough

Walkthrough

The reset-credit consume flow now adopts the ledger’s canonical operation ID after alias resolution. Settled results replay for known aliases without another upstream request. The integration test and management API documentation cover this behavior.

Changes

Reset-credit operation aliases

Layer / File(s) Summary
Canonical identity and settled alias replay
src/codex/auth-api.ts:2344, src/codex/auth-api.ts:2380, tests/codex-integration/codex-auth-api.test.ts:3283-3289, docs-site/src/content/docs/reference/management-api.md:430-436
The consume flow makes the operation identity mutable and replaces it with the ledger’s canonical ID before the upstream request. The integration test verifies that a settled alias returns { code: "reset", replayed: true } and does not create another upstream request. The management API documentation describes alias reuse and new redemption behavior.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 6f20c

Reset-credit retries using known alias IDs now reuse the canonical operation and replay settled results without triggering another redemption. The covered behavior is ready to merge with no identified current-head risk.

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant Ledger
  participant Upstream
  Caller->>Ledger: Submit operationId
  Ledger-->>Caller: Resolve canonical operation ID
  Caller->>Upstream: Consume with canonical ID
  Upstream-->>Ledger: Settle result
  Caller->>Ledger: Retry with known alias
  Ledger-->>Caller: Replay settled result
Loading

Suggested reviewers: lidge-j

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. (1 skipped: 1 … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: canonical settlement for reset-credit retry aliases in Codex.
Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the intake: hygiene-blocked Deterministic PR hygiene checks failed label Sep 7, 2026
@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

⚠️ Deterministic hygiene checks failed.

  • unsponsored_surface — This changes an authentication, workflow, release-automation, or dependency surface. MAINTAINERS.md requires security review for these; ask a maintainer to apply maintainer-sponsored once they have reviewed it. Paths: src/codex/auth-api.ts.

@github-actions github-actions Bot added the bug Something isn't working label Sep 7, 2026
@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

⏳ DRAFT

  • hygiene: unsponsored_surface.

What to do

  • Fix unsponsored_surface — This changes an authentication, workflow, release-automation, or dependency surface. MAINTAINERS.md requires security review for these; ask a maintainer to apply maintainer-sponsored once they have reviewed it. Paths: src/codex/auth-api.ts.
  • Tick all four boxes in the PR description once you're done (currently 3/4).

Review readiness checklist

  • ✅ 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.

3/4 boxes ticked.

This pull request was already a draft. Its draft status will be preserved after every issue above is resolved.
@luvs01 Tick the boxes once your local CI is green, your branch is on the latest dev commit, and every correct Codex and CodeRabbit finding is resolved.

Hygiene

⚠️ Deterministic hygiene checks failed.

  • unsponsored_surface — This changes an authentication, workflow, release-automation, or dependency surface. MAINTAINERS.md requires security review for these; ask a maintainer to apply maintainer-sponsored once they have reviewed it. Paths: src/codex/auth-api.ts.

@luvs01

luvs01 commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 62 / 80

이 PR은 수동 reset-credit 소비 API에서 별칭(alias) operationId로 재시도할 때 정산 키가 어긋나는 작은 버그를 고칩니다. 같은 물리 계정에 아직 끝나지 않은 소비가 있는데 클라이언트가 다른 operationId를내면, 원장(src/codex/reset-credit-operation-ledger.ts)은 그걸 별칭으로 붙이고 upstream에는 원래 canonical idredeem_request_id로 보냅니다. 그런데 지금 devsrc/codex/auth-api.ts는 정산·모호(ambiguous) 표시에 쓸 identity.operationId를 요청에 들어온 별칭 그대로 둡니다. settleManualResetCreditOperation / updateOperation은 현재 행의 operationId(canonical)와 인자 id가 같아야 해서, 별칭으로 정산하면 mismatch가 나고 행은 pending으로 남습니다. 사용자는 이미 성공 응답을 받았는데 원장은 안 끝났으니, 나중에 같은 별칭으로 다시 치면 또 execute 경로로 가서 upstream consume이 한 번 더 나갈 수 있습니다.

지금 dev HEAD는 aeefb3ab5(#3918 스폰서 문서), 패키지 2.47.0 open-dev입니다. 이 PR head는 6f20c3d08, base는 273a3ab86입니다. tip과의 차이는 docs-only라 코드 충돌 위험은 없습니다. types/config 대형 분리와 무관하고, preview deploy도 필요 없습니다. 고치는 파일은 세 개뿐입니다. auth-api.ts에서 identitylet으로 바꾼 뒤 opened.operationId(canonical)로 덮어쓰고, 통합 테스트에 “정산 후 별칭으로 다시 치면 { code: "reset", replayed: true }이고 upstream id 목록은 그대로”를 추가하며, docs-site/.../management-api.md에 별칭 재시도·정산 후 새 id는 새 소비라는 문단을 넣습니다.

근처 PR과의 관계만 짚으면, #3912(fix(codex): serialize reset-credit auto-redeem journal updates)는 자동 환급 저널을 설정 변이 락으로 직렬화하는 다른 표면입니다. 같은 reset-credit 제품군이지만 수동 consume 원장 별칭 정산(#3919)과 자동 redeem 다중 프로세스 경합(#3912)은 서로 독립입니다. 병합 순서는 강제되지 않습니다. 다만 둘 다 크레딧을 두 번 쓰는 방향을 막는 축이라, 리뷰·스폰서 게이트만 각자의 규칙으로 통과하면 됩니다.

변경 자체는 한 줄 의미의 수정이라 읽기 쉽습니다. upstream 호출 전에 이미 idempotencyKey = opened.operationId로 canonical을 쓰고 있었고, 정산 쪽만 그걸 따라가지 않았던 구멍입니다. 계정 소유 검사·실패 시 ambiguous·operationId 없는 legacy 경로는 그대로입니다. 본문은 별칭 회귀 테스트가 수정 없이 실패하고 수정과 함께 통과한다고 적었고, auth-api/reset-credit-operation-ledger 관련 테스트 299개를 돌렸다고 합니다. 전체 test:changed는 900초 한도에 걸려 통과로 주장하지 않았습니다.

다만 프로세스 게이트가 막혀 있습니다. hygiene가 unsponsored_surface로 실패했고 intake: hygiene-blocked 라벨이 붙어 있습니다. 경로가 src/codex/auth-api.tsMAINTAINERS.md 보안 리뷰 후 maintainer-sponsored가 필요합니다. draft이고 readiness는 1/4(최신 dev 맞춤만 체크)입니다. enforce-target도 hygiene 실패와 함께 빨간 상태입니다. CodeRabbit 최근 패스는 actionable 없음이었습니다.

라인 2344 - src/codex/auth-api.ts - const identitylet identity로 바꾸고, execute 분기에서 identity = { ...identity, operationId: opened.operationId }를 넣는다. 별칭 join 후 settle/ambiguous가 canonical을 쓰게 하는 핵심이다. accountId·chatgptAccountId는 유지된다.

라인 2380 근처 - 기존 주석 “Canonical id, which an alias join may map to an earlier caller id”는 이미 있었는데, 정산 identity 갱신이 빠져 있던 상태다. 이번 한 줄이 그 주석과 실제 동작을 맞춘다.

tests/codex-integration/codex-auth-api.test.ts - 기존 “pending 중 다른 id는 원래 durable id 유지” 테스트 뒤에, 정산 완료 후 OTHER_OP_ID로 다시 호출해 replayed: true를 확인한다. 구멍이었던 “정산 실패 → 별칭 재실행”을 직접 잠근다. upstream redeemRequestIds[OP_ID, OP_ID]인 기대는 그대로다.

docs-site/src/content/docs/reference/management-api.md - 표 아래 문단으로 별칭 join·정산 후 새 id 의미를 적는다. API 계약 문서라 코드와 같이 가는 편이 맞다.

메인테이너의 판단이 필요한 지점

  • auth-api.ts 한 줄 수준이라도 maintainer-sponsored를 찍을지(보안 표면 규칙 그대로 적용)
  • fix(codex): serialize reset-credit auto-redeem journal updates #3912(자동 redeem 저널 직렬화)와 같은 배치로 묶을지, 독립 작은 픽스로 먼저 받을지
  • draft를 유지한 채 포크 Cross-platform CI·hygiene 해제를 기여자에게 맡길지, 스폰서 라벨 후 메인테이너가 바로 랜딩할지

너의 추천

코드 방향은 dev에 맞고, 별칭 정산 구멍은 크레딧 이중 소비로 이어질 수 있어 고칠 가치가 있습니다. types/config 분리에 걸려 닫을 대상이 아니고 #3912와 중복도 아닙니다. 다만 지금은 hygiene/unsponsored_surface 때문에 병합 불가입니다. 메인테이너가 auth-api.ts 변경을 짧게 확인한 뒤 maintainer-sponsored를 붙이고, tip에 맞춘 뒤 draft를 풀면 됩니다. CI가 초록이면 독립 픽스로 받아도 됩니다. 지금은 스폰서·hygiene 대기로 보류.

이 댓글은 grok-bot이 작성했습니다

@lidge-jun

Copy link
Copy Markdown
Owner

Delivered independently via #3965 as 402be7c1f88283eb8465c3aec8437ccecd2542ec on dev. Original source 6f20c3d08cd202e43b8679de33674133bdc7f34c preserved, with actual landing Co-authored-by attribution to luvs01 verified.

The carry retains the two production hunks and adds three real-ledger pending-to-ambiguous oracles for alias throw/non-2xx/unknown-code outcomes. Final candidate 62412d38606851f7cace76360f3c5737db9cae20 passed PR CI34181771859 (19 success,2 skip) and full dispatch34181768953 (26 success,0 skip). This final baseline includes independently landed routing, prompt-probe, storage-overlap and stale-temp-PID fixture repairs. Earlier failed runs remain historical evidence, not passing results.

Actual landing tree equals the fully tested candidate tree, fetched dev contains the landing, and original author credit survives. No real credits/accounts or local product suites used. Closing this original as carried, not directly merged. Thank you for the original correction.

@lidge-jun lidge-jun closed this Sep 8, 2026
@lidge-jun lidge-jun added the landed-via-maintainer Original PR closed after landing via a maintainer merge train label Sep 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working intake: hygiene-blocked Deterministic PR hygiene checks failed landed-via-maintainer Original PR closed after landing via a maintainer merge train

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants