Skip to content

fix(codex): settle reset-credit retry aliases with canonical identity - #3965

Merged
lidge-jun merged 2 commits into
devfrom
codex/248-b-alias-8456
Sep 8, 2026
Merged

fix(codex): settle reset-credit retry aliases with canonical identity#3965
lidge-jun merged 2 commits into
devfrom
codex/248-b-alias-8456

Conversation

@lidge-jun

@lidge-jun lidge-jun commented Sep 7, 2026

Copy link
Copy Markdown
Owner

Summary

A reset-credit retry may join an unfinished operation with a different caller ID. The request already uses the canonical upstream ID, but recording its settlement or ambiguous failure with the alias can be rejected by the ledger. Adopt the ledger-returned operation ID for both outcomes after existing authentication, ownership and execute admission.

Carries #3919 from 6f20c3d08cd202e43b8679de33674133bdc7f34c, preserving the contributor's two production hunks and management API explanation. Added regressions start with a real pending canonical operation and exercise an alias fetch error, non-2xx response and unknown code; the durable row must become ambiguous without changing account key, canonical ID or terminal code. Existing settled-alias replay remains covered. No automatic-redemption worker or identity policy change.

Verification

  • git diff --check passed. Only the original three-file scope changes.
  • Independent source-plan audit passed; final independent source/security review passed on 6c1477d19c7d1a77a1866cabfd2b4411f1a210d7 with no blocking findings. Hosted CI pending.
  • Current-head PR CI and full ci.yml workflow dispatch with lane=all will be inspected before landing. Skipped or cancelled work is not passing evidence.
  • Local product tests/suites NOT RUN per owner instruction. No local runtime validation is claimed. An initial baseline-sync post-merge hook accidentally ran install/typecheck/build earlier in this session; it was disclosed and excluded from evidence. Subsequent Git writes disable hooks per invocation, with --no-verify pushes.
  • Fixtures use synthetic credentials and mocked upstream responses. No real account or reset credit is used. The fix prevents unnecessary consume requests; it does not claim an externally verified upstream exactly-once guarantee.

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.

Co-authored-by: luvs01 27862058+luvs01@users.noreply.github.com

Integration refresh: base 9c8f66b9df4cdf133a16c95a95ee07ff5171a46d; both commits unchanged by range-diff. Independent interdiff/security review PASS, with no alias dependency impact from Go translation/release-note changes. New-head PR CI 34169890598 and full dispatch 34169887448 pending. Earlier-head CI is historical evidence only.

Summary by CodeRabbit

  • Bug Fixes

    • Improved reset-credit consumption retries when different operation IDs refer to the same in-progress action.
    • Retries now consistently use the original operation identity, preventing duplicate upstream consumption.
    • Improved handling and status reporting for upstream failures and uncertain outcomes.
  • Documentation

    • Clarified reset-credit operation identity, retry behavior, and when a new operation begins in the Management API reference.

@coderabbitai

coderabbitai Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: bda0ef1a-3151-496c-b2f3-94271aa8a51e

📥 Commits

Reviewing files that changed from the base of the PR and between abb46a1 and 62412d3.

📒 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

📝 Walkthrough

Walkthrough

The reset-credit consume handler now adopts the ledger’s canonical operation ID for unfinished operations. Integration tests cover alias retries and ambiguous upstream failures. The Management API documentation describes these idempotency semantics.

Changes

Reset-credit operation identity

Layer / File(s) Summary
Canonical operation identity
src/codex/auth-api.ts (lines 2344, 2380)
The handler makes identity mutable and replaces its operationId with the canonical ID returned by openManualResetCreditOperation before downstream processing.
Alias behavior validation and documentation
tests/codex-integration/codex-auth-api.test.ts (lines 32, 3284–3337), docs-site/src/content/docs/reference/management-api.md (lines 430–436)
Integration tests verify canonical IDs for pending aliases and ambiguous ledger states for upstream failures. The Management API reference documents alias retries and new redemption behavior.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant ConsumeHandler
  participant ResetCreditLedger
  participant UpstreamConsume
  Client->>ConsumeHandler: submit operationId
  ConsumeHandler->>ResetCreditLedger: openManualResetCreditOperation
  ResetCreditLedger-->>ConsumeHandler: canonical operationId
  ConsumeHandler->>UpstreamConsume: consume with canonical operationId
  UpstreamConsume-->>ConsumeHandler: result or failure
  ConsumeHandler->>ResetCreditLedger: record settlement state
Loading
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/248-b-alias-8456

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 commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Sep 7, 2026
@lidge-jun lidge-jun added the maintainer-sponsored Maintainer sponsors this change to an auth, workflow, release, or dependency surface label Sep 7, 2026
@lidge-jun

Copy link
Copy Markdown
Owner Author

Explicit owner-authorized agent-assisted maintainer security review for head 7c7b9081abb309209961e7dd82e1b855b640e60b, base eb4188a9f2e127f5ee2980b62d6e5bb213c43c70.

Independent Astra high source/security audit and main-agent verification found no blocking issue. Canonical ID adoption occurs after authentication, physical-account ownership and execute admission. Both account fields survive; legacy no-ID and fail-closed unavailable/identity-mismatch branches remain intact. Three real-ledger pending-to-ambiguous regressions distinguish the original defect without already-ambiguous setup. Source change remains the contributor's exact two hunks, with original authorship retained.

This is explicit security acceptance/sponsorship for this revision under the owner's delegated maintainer delivery, not a separate human approval or runtime test result. Current-head hosted PR and full dispatch CI remain required. Synthetic fixtures only; no real credits consumed. The known Windows routing-fixture failure from another candidate is not treated as passing evidence here.

@lidge-jun

Copy link
Copy Markdown
Owner Author

리뷰 · 우선순위 73 / 80

이 PR은 Codex reset-credit 수동 소모에서, 호출자가 보낸 operationId가 이미 열린 원본 작업의 별칭(alias) 으로 붙었을 때, 업스트림 요청은 원본(canonical) id로 나가는데 장부 정산·모호 처리는 여전히 별칭 id로 남는 구멍을 막습니다. 증상은 “같은 물리 계정에 미완료 작업이 있는데 다른 operationId로 재시도하면, 소모 자체는 원본 id로 가지만 settle / ambiguous 기록이 별칭이라 장부가 거절하거나 상태가 어긋난다”는 형태입니다. 기여 원본은 #3919이고, 이 PR은 그 두 production hunk를 현재 dev에 다시 올린 maintainer carry 입니다.

지금 dev HEAD 60bcb9050src/codex/auth-api.ts reset-credits/consume 경로를 보면, openManualResetCreditOperation(identity)execute를 돌려줄 때 opened.operationId가 canonical id입니다. 주석도 “Canonical id, which an alias join may map to an earlier caller id”라고 적혀 있고, idempotencyKey = opened.operationId로 업스트림 redeem_request_id는 이미 맞게 바꿉니다. 그런데 identity는 여전히 operationId: requestedOperationId(별칭)인 const 객체라서, 아래에서 markManualResetCreditOperationAmbiguous(identity) / settleManualResetCreditOperation(identity, …)가 별칭으로 장부를 칩니다. alias join 직후 실패·비-2xx·unknown code면 원본 pending 행이 안 바뀌거나, 성공 정산이 identity-mismatch로 깨질 수 있습니다.

변경은 한 줄에 가깝습니다. identitylet으로 바꾸고, open 직후 identity = { ...identity, operationId: opened.operationId }로 canonical id를 입힙니다. 인증·소유권·execute 판정은 그대로 두고, 정산/모호 기록 identity만 원본에 맞춥니다. docs-site/.../management-api.md에 “다른 operationId는 alias로 join하고, 결과는 원본 id로 기록·재현된다”는 설명을 추가했습니다. 테스트는 진짜 pending canonical 행을 openManualResetCreditOperation으로 심은 뒤, alias로 throw / non-2xx / unknown-code를 넣어 행이 ambiguous가 되는지 확인합니다. 기존 settled-alias replay 경로도 그대로 둡니다. types/config 분할과 안 겹칩니다.

라인 src/codex/auth-api.ts · identity 재할당 - open 이후 identity.operationId를 opened.operationId로 덮는 것이 핵심입니다. 업스트림 키와 장부 키가 갈라지던 지점을 한곳에서 맞춥니다.

라인 tests/codex-integration/codex-auth-api.test.ts - pending canonical + alias 실패 3종(throw/non-2xx/unknown-code)이 없으면 이 회귀가 다시 열립니다. account key·canonical id·terminal code가 안 바뀌는지도 같이 보므로 유지하세요.

경로 #3919 - 같은 수정의 contributor 원본(draft)입니다. 이 carry가 먼저 들어가면 #3919는 landed-via-maintainer로 닫는 편이 맞습니다. 자동 환급 worker나 identity 정책은 건드리지 않습니다.

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

  • 지금 draft입니다. hosted CI(특히 test lane)가 이 head에서 그린인지 확인한 뒤 ready로 올릴지
  • 머지 직후 #3919에 Landed via #3965 at <sha> + landed-via-maintainer로 닫을지
  • 로컬 product 테스트는 owner 지시로 스킵했다고 적혀 있으니, 머지 전 증거는 hosted CI만으로 충분한지

너의 추천
CI 그린이면 draft 해제 후 머지하세요. reset-credit 이중 소모/장부 불일치를 직접 막는 작은 패치이고, #3919 leftover는 머지 직후 정리하면 됩니다.

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

@Ingwannu

Ingwannu commented Sep 8, 2026

Copy link
Copy Markdown
Owner

@lidge-jun Current verification status for 6c1477d: full lane=all dispatch 34169887448 has completed with failure, including windows 4/6 (job 101888425573). The PR check list also reports a failed test 1/4, so the historical pending note should not be read as a green gate. I could verify the failing job identities but did not obtain a useful failure excerpt from the log command; I am not attributing either result to the patch or calling it a baseline flake without that evidence. Keep this Draft while the exact-head failures are diagnosed. No rerun, skip, timeout change, local product execution or real reset-credit action was performed.

luvs01 and others added 2 commits September 8, 2026 11:56
Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
@lidge-jun
lidge-jun force-pushed the codex/248-b-alias-8456 branch from 6c1477d to 62412d3 Compare September 8, 2026 02:56
@lidge-jun

Copy link
Copy Markdown
Owner Author

Final candidate source/security acceptance: 62412d38606851f7cace76360f3c5737db9cae20, base abb46a1599ec0d0bbfbe03905114178df92e67f5. Both original carry/followup commits remain equal in range-diff. Independent Astra high review confirms no manual/automatic journal identity confusion, canonical adoption after ownership admission, bounded lock lifetime and preserved pending-to-ambiguous oracles. No blocking findings.

New PRCI34181771859 and full dispatch34181768953 pending. This final integration includes separately landed routing/probe/storage/PID fixture repairs; previous failed attempts are historical, not current passes. No local product tests or live credits/accounts used. Original3919 stays open until verified actual landing.

@lidge-jun
lidge-jun marked this pull request as ready for review September 8, 2026 03:16
@lidge-jun
lidge-jun requested a review from Ingwannu as a code owner September 8, 2026 03:16
@lidge-jun

Copy link
Copy Markdown
Owner Author

Owner-authorized dev-only maintainer integration for 62412d3 into abb46a1. Independent source review PASS; exact-head hosted verification: 34181771859 pull_request: 19 success / 2 skip; 34181768953 workflow_dispatch: 26 success / 0 skip. Skips are not passing tests. Original failure history retained; no local product tests/build/typecheck/install. One fixture repair only, independently revertible squash. Candidate and combined tree are identical. Actual landing is checked against precomputed merge tree, unchanged unrelated paths and fetched dev ancestry. This is agent-assisted maintainer integration, not a separate human approval. No unresolved threads or maintainer objections; ready-triggered optional review checks are not claimed as passed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working maintainer-sponsored Maintainer sponsors this change to an auth, workflow, release, or dependency surface

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants