Skip to content

fix(codex): require live credential evidence for pool quota policy - #6326

Merged
lidge-jun merged 1 commit into
lidge-jun:devfrom
luvs01:transfer/677-live-pool-quota-evidence
Sep 30, 2026
Merged

lidge-jun merged 1 commit into
lidge-jun:devfrom
luvs01:transfer/677-live-pool-quota-evidence

Conversation

@luvs01

@luvs01 luvs01 commented Sep 30, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • Pool WHAM and response-header usage may pause an account only when its captured credential writer is still live. Missing capture fails closed for pool responses while the existing display snapshot, writer-free legacy/login behavior, and native-main policy remain separate.
  • Carry my original changes, source commits d7b1e161ddf922fca94ee3042dbba5422d832e2d and 378348f446c5dd8d3ad29f4e0749668341c0b987; both source commits and this PR are authored by luvs01. The missing-writer review finding is covered by a separate pool-response provenance flag and regressions.
  • Tag pool origins at WHAM, warm-account headers, HTTP delivery, alternate-account retry, compact outcomes, and WS metadata. Add real compact-handler and WS-observer cases, and retain the newer upstream credits publication and documentation unchanged.

Verification

  • Base: 592c5cfc043cd5b69e8aea0f12b9a0644cc50612 (2.75.0); validated head: 02de3eb8ee1be8869daded7803194c86c8faafe9. The 96-file upstream delta from cda8ff4 was integrated before applying this carry; official tip tree and commit hashes were verified. Credits overlap was reviewed in both the WHAM producer and the account contract document, then exercised by the existing credits probe regressions. Inspected upstream dev 592c5cfc043cd5b69e8aea0f12b9a0644cc50612 is 0 commits ahead, without overlap in the changed paths, instructions or dependency locks.
  • Windows, Bun 1.4.0: bun test tests/codex-integration/low-quota-protection.test.ts: 31 pass, 0 fail. Tests cover stale and missing header writers, valid/invalid WHAM usage, missing WHAM writer, current/missing/retired WS writers, and real compact dispatch with a current or deliberately omitted writer. Fetch responses and credentials are synthetic; the compact seam omits only the optional writer after the real fixture resolver runs.
  • Red evidence against the base runtime: the initial 29-case file produced 25 pass, 4 expected failures (stale header writer, missing header writer, missing WHAM writer, missing WS writer). The two final compact cases, run with --test-name-pattern 'compact pool headers', produced 1 pass, 1 expected failure for the missing writer; both pass on the patched runtime.
  • bun test tests/codex-integration/main-quota-provenance.test.ts tests/codex-integration/codex-credits-probes.test.ts tests/codex-integration/codex-quota-auto-refresh.test.ts tests/codex-integration/codex-quota-auto-refresh-generation.test.ts tests/responses/responses-compaction.test.ts tests/responses/responses-compact-handoff-admission.test.ts tests/responses/ws-upstream.test.ts tests/responses/reserve-dispatch-ws.test.ts: 236 pass, 1 skip, 0 fail. The existing older-runtime HTTP fallback case is skipped under Bun 1.4.0.
  • bun run typecheck, bun run structure:check, bun run privacy:scan, bun run scripts/file-size-ratchet.ts, and git diff --check: passed. Source-to-doc mappings were reviewed; the low-quota authority and English user guidance were updated, and existing translated threshold guidance does not contradict the change.
  • Windows documentation build limitation: (cd docs-site && bun run build) failed during Astro route-manifest construction with The URL must be of scheme file in the existing Windows dependency-junction layout. The patched attempt exited 9 after an additional Windows runtime assertion; an exact clean-base comparison reproduced the same initial URL error and exited 1. Identical frozen dependency locks and the existing installed dependency tree were reused, with no new install. No source, dependency, build budget or security setting was changed to work around the error. The matching-tree Linux documentation build subsequently passed as described below.
  • Focused-validation resource exception: the complete affected low-quota test file plus the eight relevant provenance/credits/refresh/compact/WS files exercise the changed behavior. Repeating the repository-wide import-connected suite for every independent transfer in this series is disproportionate to the available local validation budget. Full local bun run test and bun run test:changed have not been run on this head; wider integration remains for exact-head CI. No timeout, test budget or baseline was relaxed, and prior series aggregate timeout results are not treated as passing evidence here.
  • Exact-head hosted CI has passed. Local source review supports review readiness; independent maintainer security review remains required before merge. No live provider acceptance, maintainer approval or sponsorship is inferred from offline fixtures.

Additional Linux validation and publication identity

  • Published HEAD: 082c8aa358e8f857affa577f194c186959b1585b; tree: 8d95217e41794dbc0e50ff82dc6f9e55063fbfd6. The cloud-applied patch, all nine published blob hashes and the parent/tree were verified against the Windows-tested tree. The GitHub commit API supplied new commit metadata; the validated Windows commit above remains the evidence identity.
  • cd docs-site && bun install --frozen-lockfile installed the pinned dependencies. The first build could not create Astro's telemetry configuration in the cloud home. ASTRO_TELEMETRY_DISABLED=1 bun run build then passed: 545 pages, 74,725 internal links checked, 30.21 seconds. No source, dependency lock, timeout or security/network routing change was made.
  • The additional Linux raw command bun test tests/codex-integration/low-quota-protection.test.ts produced 29 pass / 2 fail: both real compact-handler cases received 502 instead of 200. These same cases passed in the recorded Windows run. Their cloud-specific cause remains unclassified; this failure is not presented as a pass. Exact-head hosted CI must exercise these cases before readiness.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed locally for secrets, auth, and unsafe defaults (synthetic fixtures and privacy scan; independent maintainer review remains pending).

Privacy validation passed; explicit upstream review of credential-bound policy evidence remains pending. Maintainer review/sponsorship requirements before merge, where applicable, remain separate. No merge, auto-merge, runtime installation or user configuration cleanup is requested.

Review readiness

  • Windows focused validation, static checks, Linux docs build and exact-head hosted regression coverage passed; the separate Linux failures remain documented.
  • Branch is based on the latest inspected dev commit 592c5cfc043cd5b69e8aea0f12b9a0644cc50612.
  • The known missing-writer finding is covered; no unresolved inline findings exist on this PR before the Ready-event review.
  • Ready for review.

Exact-head CI checkpoint

CI 36721022453 passed at 082c8aa358e8f857affa577f194c186959b1585b, including four test shards, aggregate, documentation build and applicable gates/smokes. Shard 3 logs explicitly show 31 passing low-quota tests, including both real compact-handler cases that failed in the separate cloud run. Those cloud failures remain unclassified and are not retroactively passing. Optional skipped jobs are not verification evidence. Source review confirmed pool provenance at WHAM/header/compact/WS producers and fail-closed behavior when the writer is missing; display/legacy/main behavior remains separately covered. Automatic review will follow the normal Ready event. No human security approval, merge or deployment is implied.

Summary by CodeRabbit

  • Bug Fixes
    • Low-quota protection now pauses a pool account only when the quota data is valid and tied to a currently active credential.
    • Quota information can still refresh for display when that verification is unavailable, without triggering an account pause.
  • Documentation
    • Updated guidance to clarify when pool quota information can trigger low-quota protection.

Preserve display snapshots and legacy login observations while gating pool policy changes on the captured writer. Cover missing writers in compact and WebSocket paths.
@coderabbitai

coderabbitai Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 2dc6f1f7-3603-40ac-b90a-5799a54141d4

📥 Commits

Reviewing files that changed from the base of the PR and between 592c5cf and 082c8aa.

📒 Files selected for processing (9)
  • docs-site/src/content/docs/guides/codex-integration.md
  • src/codex/auth-api/pool-quota-probe.ts
  • src/codex/quota-auto-refresh.ts
  • src/codex/quota.ts
  • src/server/responses/compact.ts
  • src/server/responses/core-codex-account.ts
  • src/server/responses/passthrough-delivery.ts
  • structure/providers/openai-accounts.md
  • tests/codex-integration/low-quota-protection.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.


📝 Walkthrough

Walkthrough

Pool quota updates now require a captured, current credential writer before they can trigger low-quota protection. Responses without that evidence can still update displayed usage. The change marks pool responses across quota update paths and adds integration coverage and documentation.

Changes

Pool quota protection

Layer / File(s) Summary
Quota evidence and policy
src/codex/quota.ts
setAccountQuotaFromParsed distinguishes pool requests from other observations. Pool history and low-quota observations require matching, live writer evidence.
Pool response call paths
src/codex/auth-api/pool-quota-probe.ts, src/codex/quota-auto-refresh.ts, src/server/responses/compact.ts, src/server/responses/core-codex-account.ts, src/server/responses/passthrough-delivery.ts
Quota updates mark pool responses at the probe, warmup, compact, WebSocket, and passthrough entry points.
Integration tests and guidance
tests/codex-integration/low-quota-protection.test.ts, structure/providers/openai-accounts.md, docs-site/src/content/docs/guides/codex-integration.md
Tests cover missing and stale writers across response paths. The guidance describes when pool responses can trigger a pause and when they can update displayed usage.

Priority: ➖ Normal

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

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 082c8

The change prevents pool quota responses without live credential evidence from pausing accounts while preserving displayed usage. No concrete merge-blocking issue is established; the separately reported Linux compact-test failures still warrant owner follow-up.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 082c8

The inspected changes strengthen protection against delayed or unproven responses pausing replacement credentials. No introduced security regression was established. Confidence remains bounded because broader deployment and security coverage is incomplete.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The inspected sensitive outcome is automatic pause state and its persistence for the named selectable account in enabled registered configurations. The observer fan-out means scope follows registered configuration ownership; the supplied evidence does not establish tenant or deployment-wide isolation.

Security Findings and Attack Paths

  • observed — The inspected regression cases exercise delayed replacement-credential responses and deliberately omitted writers. Their assertions require display updates without a pause or pending pause save; the real compact-handler case permits those policy effects only with a captured writer. These are source-level test assertions, not independently executed verification results.

Trust Boundaries and Controls

  • observed — Upstream usage values do not alone acquire pool pause authority. Header validity, explicit pool classification and live captured identity gate policy observation. Native-main handling remains separate, and the WebSocket observer additionally rejects retired pool generations before applying headers.

Resilience and Maintainability Implications

  • observed — Low-quota policy consumes newly accepted observations rather than display snapshots. Display/reset publication uses a separate serialized chain. The policy tracks pause episodes, handles persistence failure with retries, and fences owner closure and notification completion, supporting failure containment without promoting rejected display evidence into pause authority.

Hardening Proposals

  • proposed — Consider a distinct typed pool-observation API so future callers cannot accidentally select legacy policy semantics by omitting the origin discriminator. This is a control-drift prevention proposal, not an observed regression.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 55.56% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 7 files. (2 skipped: 2… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely identifies the main change: requiring live credential evidence before applying pool quota policy pauses.
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.
Full details: Docstring Coverage

Explanation

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

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

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

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

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

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 58 / 80

풀 계정 응답으로 사용량을 볼 때, 화면 숫자는 고쳐도 계정을 멈추는 일은 살아 있는 자격 증명 증거가 있을 때만 하게 막는 PR이에요. 예전에는 writer가 없거나 이미 바뀐 자격 증명에서 온 응답도, 레거시·로그인처럼 보고 pause를 걸 수 있었어요. 그러면 방금 갈아 끼운 새 계정이 옛 응답 때문에 멈출 수 있어요.

고치는 방법은 간단해요. poolResponse / poolRequest 깃발을 풀 경로에 붙이고, writer가 없거나 죽은 writer면 표시 스냅샷만 남기고 observeCodexLowQuota는 건너뛰어요. WHAM, warm 헤더, HTTP 전달, 대체 계정 재시도, compact, WS 관찰기까지 같은 깃발을 넣었어요. 테스트도 낡은 writer, 빠진 writer, WHAM, WS, compact 실핸들러를 새로 넣었어요. base는 dev예요. Draft이고, 작성자도 Linux에서 compact 두 케이스가 502로 깨진 걸 통과로 치지 않았어요.

src/codex/quota.ts setAccountQuotaFromParsed poolRequest - 기본값이 false예요. writer도 없고 깃발도 빼먹으면 livePoolEvidence가 다시 참이 되어 pause가 열립니다. 안전이 기본이 아니라, 호출부가 깃발을 기억해야 하는 구조예요.

src/codex/auth-api/pool-quota-probe.ts commitPoolQuotaResponse - 일곱 번째 인자로 그냥 true를 넣어요. 이름 있는 옵션이 아니라 자리 인자라서, 나중에 인자 순서가 바뀌면 조용히 잘못 붙을 수 있어요.

tests/codex-integration/low-quota-protection.test.ts compact pool headers - 작성자 Linux 재실행에서 같은 케이스가 200이 아니라 502였어요. Windows에서는 통과했다고 했고, 원인은 아직 모름으로 적혀 있어요. 이 헤드의 exact CI가 이 케이스를 통과하기 전에는 머지 증거가 부족해요.

PR 본문 Verification - 이 헤드에서 전체 bun run test / test:changed는 안 돌렸다고 적혀 있어요. Review readiness 체크도 비어 있고 Security 항목도 비어 있어요.

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

풀 응답에서 writer가 없을 때 화면 숫자만 고치고 pause만 막는 선택이 맞는지 정해 주세요. 아예 관측을 버려야 한다는 쪽도 있어요. poolResponse를 빼먹으면 다시 fail-open이 되는 기본값을 유지할지, 풀 계정에서는 writer 없으면 항상 닫히게 바꿀지도 정해 주세요. Linux compact 502를 환경 문제로 두고 CI만 볼지, 아니면 이 PR에서 재현·수정까지 할지도 정해 주세요. 비슷한 열린 중복 PR은 안 보여요.

너의 추천

Draft로 두고, hosted CI에서 low-quota-protection compact/WS 케이스가 초록이 된 뒤에만 Ready로 올리세요. poolRequest는 자리 true 대신 이름 있는 옵션으로 바꾸거나, 풀 경로 기본을 fail-closed로 뒤집는 쪽을 검토하세요. 표시와 pause를 나누는 정책·문서·회귀 테스트 방향은 유지해도 좋아요. CI와 깃발 기본값이 정리되기 전에는 합치지 마세요.

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

@luvs01
luvs01 marked this pull request as ready for review September 30, 2026 13:32
@lidge-jun

Copy link
Copy Markdown
Owner

Maintainer integration into dev (MAINTAINERS.md, dev-only exception) by @lidge-jun.

Thanks @luvs01!

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