Skip to content

fix(codex): bound the reset-credit consume response like every other read - #4571

Draft
luvs01 wants to merge 3 commits into
lidge-jun:devfrom
luvs01:agent/reset-credit-consume-bound-20260914
Draft

luvs01 wants to merge 3 commits into
lidge-jun:devfrom
luvs01:agent/reset-credit-consume-bound-20260914

Conversation

@luvs01

@luvs01 luvs01 commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Summary

Apply the shared bounded-body reader to reset-credit consume responses, including the extracted service module. Unreadable consume outcomes remain ambiguous and cannot be treated as a fresh redeem or invented local credit change.

Current author verification

  • Published head: b5c313edac2fa1d6eb55027bf4a94a8ab5a88c69.
  • Full ordinary CI run 34959895394 completed successfully on this exact published head; the run head SHA was verified. This is hosted execution, not a claim that the full matrix was repeated locally.
  • The branch remains within the repository's current allowed dev-drift window. No tip-only rebase was performed.
  • All known applicable inline and review-body findings have been addressed. Explicit security review remains outstanding for this credential/reset-credit boundary. A successful hygiene job does not grant security-review approval.

Review readiness checklist

The validation checkbox refers to the explicit scope above. Historical run IDs and prior local results are not represented as new-head full-suite execution.

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

@coderabbitai

coderabbitai Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

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 14, 2026
@github-actions

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 14, 2026
@github-actions

github-actions Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

⏳ DRAFT

  • review readiness checklist open (3/4 boxes ticked).

What to do

  • 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 PR stays in draft until every box above is ticked.

@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 70 / 80

이 PR은 reset-credit consume 응답이 아직 resp.json()으로 전체 바디를 버퍼링하던 마지막 구멍을 막는다. 대상은 src/codex/auth-api.ts 두 곳이다. 배경 auto-redeemer(createResetCreditWhamClient)와 수동 POST /api/codex-auth/reset-credits/consume 핸들러다. 같은 파일의 가용성(availability) 읽기는 이미 readResetCreditJson을 쓰고, 이 헬퍼는 선언된 content-length가 크면 바로 거절하고, 공유 64 KiB 바운디드 리더와 fatal UTF-8 디코드로 읽은 뒤 잘리거나 빈 답을 거절한다. consume만 예외였던 비대칭을 없애는 패치다.

현재 dev HEAD ae3cb2311에서도 consume 두 경로는 여전히 await resp.json()이다. PR은 두 경로를 readResetCreditJson으로 돌린 뒤, 배경 경로에서는 unreadable이면 throw(기존 availability와 같은 실패 형태), 수동 핸들러에서는 markManualResetCreditOperationAmbiguous 후 502를 돌려 “upstream에서 spend는 됐을 수 있는데 결과 코드를 못 읽음”을 표현한다. 이미 non-OK 분기와 같은 ambiguous 모양이라 의미론이 일관된다. redeem 수치·cooldown 화해·remaining 계약은 바꾸지 않는다고 본문에 명시돼 있다. structure 문서(structure/providers/openai-tiers.md)에도 “모든 reset-credit 읽기가 바운디드 리더를 통과한다”를 보강했다.

테스트 tests/codex-integration/codex-auth-api.test.ts에 200이지만 바디가 BOUNDED_BODY_MAX_BYTES * 2인 consume을 넣고, 502 + unconfirmed 에러·usage refresh 0회를 단언한다. 소스만 되돌리면 335/1로 깨진다는 증빙도 있다. 보안·auth 표면이라 hygiene가 unsponsored_surface로 막혀 draft다. 라벨에도 intake: hygiene-blocked가 있다. types/config 분할과 무관하고, 범위가 auth-api + 테스트 + 문서라 충돌면도 좁다. 현재 dev의 Codex forward identity(#4563) 축과는 다른 파일이다.

라인 수동 핸들러 502 분기 - spend가 이미 올라갔을 수 있어 ambiguous 처리가 맞다. 클라이언트가 502를 “재시도 가능한 일시 오류”로만 보이면 idempotency 키 재사용 압력이 생긴다. GUI/CLI가 이 에러를 “확인 불가, 재클릭 주의”로 보여 주는지는 이 PR 밖이지만, 메인테이너가 UX 카피까지 볼지는 선택이다.
라인 배경 redeemer throw - availability와 맞춰 올린 선택은 일관적이다. 다만 throw 메시지가 기존 upstream status 오류와 섞이면 관측에서 구분이 어렵다. 코드 경로상 이미 별도 문자열이므로 로그 메트릭만 있으면 충분해 보인다.
경로 MAINTAINERS.md unsponsored_surface - auth 변경이라 maintainer-sponsored 없이 ready로 올리면 게이트가 계속 막는다. 내용이 좋아도 스폰서 라벨이 merge 전제다.
경로 다른 resp.json() 잔존 - 같은 파일에 WHAM usage 등 다른 resp.json()이 남아 있다. 이번 PR 범위 밖이 맞지만, “reset-credit 읽기만 바운드”라는 문서 문장이 usage 경로까지 포함한 것처럼 읽히지 않게 문서 문장을 reset-credit으로 한정한 현재 서술이 좋다. 그 톤을 유지하자.

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

  • 보안 리뷰 후 maintainer-sponsored를 붙일지(사실상 merge 조건)
  • 502 ambiguous UX를 이 PR에서 손볼지, 후속으로 둘지
  • usage 등 다른 unbounded read를 같은 열차에 태울지(비추천: 범위 팽창)

너의 추천
메인테이너가 짧게 읽고 maintainer-sponsored를 붙인 뒤, checklist·ready만 채우면 merge한다. 동작 변화는 “너무 큰 바디를 성공으로 오인하지 않음”뿐이라 회귀 위험이 낮고, auth 표면 위생으로 지금 dev에 넣기 좋다. 우선순위 70 — 실피해 방지와 계약 일관성이 분명하고, 스폰서만 막혀 있는 상태다.

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

…read

Both consume call sites parsed the upstream answer with resp.json(), which buffers
the whole body before anything checks its size. Every neighbouring reset-credit read
already goes through readResetCreditJson, which short-circuits an oversized declared
length, reads through the shared 64 KiB bounded reader with fatal UTF-8, and rejects
a truncated or empty answer. Only these two were left unbounded.

The background auto-redeemer now treats an unreadable answer the same way its sibling
availability read does and raises. The manual handler marks the operation ambiguous
and answers 502, because the spend may already have landed upstream while its outcome
code is unreadable, and a replay of that id must never be admitted as new work.
@luvs01
luvs01 force-pushed the agent/reset-credit-consume-bound-20260914 branch from 5ba458c to b157f78 Compare September 14, 2026 05:01
@github-actions github-actions Bot removed the intake: hygiene-blocked Deterministic PR hygiene checks failed label Sep 15, 2026
Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants