Repository navigation
fix(codex): require live credential evidence for pool quota policy - #6326
Conversation
Preserve display snapshots and legacy login observations while gating pool policy changes on the captured writer. Cover missing writers in compact and WebSocket paths.
|
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 configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (9)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughPool 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. ChangesPool quota protection
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to 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 ReviewSecurity architecture risk: 🔵 Low · up to 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 Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation 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.)
✨ Finishing Touches🧪 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 |
|
✅ Deterministic PR hygiene checks passed. |
리뷰 · 우선순위 58 / 80풀 계정 응답으로 사용량을 볼 때, 화면 숫자는 고쳐도 계정을 멈추는 일은 살아 있는 자격 증명 증거가 있을 때만 하게 막는 PR이에요. 예전에는 writer가 없거나 이미 바뀐 자격 증명에서 온 응답도, 레거시·로그인처럼 보고 pause를 걸 수 있었어요. 그러면 방금 갈아 끼운 새 계정이 옛 응답 때문에 멈출 수 있어요. 고치는 방법은 간단해요. src/codex/quota.ts src/codex/auth-api/pool-quota-probe.ts tests/codex-integration/low-quota-protection.test.ts PR 본문 Verification - 이 헤드에서 전체 메인테이너의 판단이 필요한 지점 풀 응답에서 writer가 없을 때 화면 숫자만 고치고 pause만 막는 선택이 맞는지 정해 주세요. 아예 관측을 버려야 한다는 쪽도 있어요. 너의 추천 Draft로 두고, hosted CI에서 이 댓글은 grok-bot이 작성했습니다 |
|
Maintainer integration into
Thanks @luvs01! |
Summary
d7b1e161ddf922fca94ee3042dbba5422d832e2dand378348f446c5dd8d3ad29f4e0749668341c0b987; both source commits and this PR are authored byluvs01. The missing-writer review finding is covered by a separate pool-response provenance flag and regressions.Verification
592c5cfc043cd5b69e8aea0f12b9a0644cc50612(2.75.0); validated head:02de3eb8ee1be8869daded7803194c86c8faafe9. The 96-file upstream delta fromcda8ff4was 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 upstreamdev592c5cfc043cd5b69e8aea0f12b9a0644cc50612is 0 commits ahead, without overlap in the changed paths, instructions or dependency locks.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.--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, andgit 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.(cd docs-site && bun run build)failed during Astro route-manifest construction withThe URL must be of scheme filein 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.bun run testandbun run test:changedhave 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.Additional Linux validation and publication identity
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-lockfileinstalled the pinned dependencies. The first build could not create Astro's telemetry configuration in the cloud home.ASTRO_TELEMETRY_DISABLED=1 bun run buildthen passed: 545 pages, 74,725 internal links checked, 30.21 seconds. No source, dependency lock, timeout or security/network routing change was made.bun test tests/codex-integration/low-quota-protection.test.tsproduced 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
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
devcommit592c5cfc043cd5b69e8aea0f12b9a0644cc50612.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