test(quota): correct the contract comment and close the coverage holes from #3200 - #3327
test(quota): correct the contract comment and close the coverage holes from #3200#3327olddonkey wants to merge 1 commit into
Conversation
The review's headline finding was that the new comment stated a wrong mechanism at a wrong layer: on this test's integration path the malformed plan never reaches codexPlanKey — poolAccountDto strips it via codexPlanValue, so the aggregation sees an ABSENT plan — and an unlisted string name normalizes to a defined key that fails the weight-map lookup, not to undefined. The comment now states both routes correctly, cites lidge-jun#3155 (PR lidge-jun#3198) the way the ten sibling references do, and replaces its fourth copy of the policy rationale with a pointer to CODEX_DEFAULT_CAPACITY_WEIGHT's own doc. The assertions grew where the review showed regressions could ship green: - weekly: { includedAccounts, excludedAccounts, incomplete } — the per-window flags are what the dashboard renders per bar, and nothing pinned them. - JSON.stringify(refreshed) must not contain "tier" — with exclusion gone, nothing else proved the malformed value cannot escape into the public shape. - the sibling's anti-leak regex over the aggregation, closing that asymmetry. - the cached half now asserts the identity contract itself (toBe on the aggregation object) instead of re-reading a field of the object checked one line up, with a comment saying why content is only covered once. - the load-bearing mock is documented: a fetched plan_type outranks the persisted plan, so it must stay non-string for every value below. The test is renamed to what it now pins — counted at baseline, kept out of the public shape — since "through cache and aggregation" was false on both nouns. gui/tests/provider-capacity-shell.test.tsx gains the one missing pairing: incomplete:false with unknownPlanAccounts>0, the lidge-jun#3155 reporter's own fully-included pool. Mutation-checked: gating the uncalibrated notice under the incomplete branch fails only the new test; every prior fixture passes, which is exactly the silent-hide the review predicted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
github-actions
Bot
added
the
chore
|
✅ Deterministic PR hygiene checks passed. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
⏳ DRAFT
What to do
Review readiness checklist
✅ 4/4 boxes ticked. This pull request was already a draft. Its draft status will be preserved after every issue above is resolved. |
|
@lidge-jun @Ingwannu — requesting the GUI-screenshot waiver for this PR.
It adds one fixture-driven test ( The waiver regex looks for a maintainer comment, so a reply along the lines of "this does not change the GUI" is enough to clear it. Happy to answer anything about the test itself first — it is mutation-checked: gating the uncalibrated-plan notice under the |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe tests expand provider quota coverage for malformed persisted plans and complete aggregates with uncalibrated plans. They validate baseline weighting, sanitized aggregation metadata, cached identity, notice rendering, and preserved aggregate usage. ChangesProvider quota behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change strengthens quota aggregation and provider-capacity UI regression coverage without changing production behavior. The covered malformed-plan and uncalibrated-plan scenarios are ready to merge. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2 files. ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
리뷰 · 우선순위 68 / 80이 PR은 이미 설명부터 하면, 추가 assertion들도 역할이 분명합니다. GUI 쪽은 라인 문제:
메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
2 participants
Follow-up to #3200, which landed the contract move itself (
fcf0da257).An adversarial multi-agent review of that change produced eleven findings seventeen minutes after it merged, so none of them made that train. No wrong assertion values — three reviewers independently re-derived all four numbers by executing the aggregator — but one factual error in the comment I wrote, now sitting on
dev, plus several coverage holes the contract move opened.The comment on
devis wrong, in two waysIt claims an unlisted plan name and a malformed non-string "both normalize to undefined via
codexPlanKey". Neither half survives contact with the code:codexPlanKey("edu_plus")returns"edu_plus"— a defined key that then fails theObject.hasOwnlookup inconfiguredWeight. Only a non-string normalizes toundefined.codexPlanKeyat all:poolAccountDto(src/codex/auth-api.ts:265) strips it viacodexPlanValue, so the aggregation sees an absent plan.The second point has a corollary the test's own name got wrong: it cannot distinguish "malformed plan" from "no plan at all". Rewritten to state both routes, and renamed to what it actually pins — counted at baseline, kept out of the public shape. Reference normalized to
#3155 (PR #3198), matching the ten sibling citations; the fourth prose copy of the policy rationale replaced with a pointer toCODEX_DEFAULT_CAPACITY_WEIGHT's own doc comment.Assertions added where a regression would now ship green
weekly: { includedAccounts, excludedAccounts, incomplete }ProviderCapacityQuota.tsxrenders per bar — afinalizeWindowtotalAccountsregression leaves the weekly bar struck through while the top-level flag stays cleanJSON.stringify(refreshed)free of"tier"nonEmptyPlancorruptsconfig.jsonwhile every prior assertion passespublicCapacityWindowdropping its destructure leakstotalWeight/consumedWeighthere silently; the sibling catches it, this test did nottoBeon the aggregation objectThe load-bearing mocked
plan_typeis now documented at the mock: a fetched plan outranks the persisted one, so tidying it to a plain string silently recalibrates the weight and breaks four assertions at once with only a numeric mismatch to explain it.GUI: the one missing pairing
gui/tests/provider-capacity-shell.test.tsxhad no fixture withincomplete: falseandunknownPlanAccounts > 0— both uncalibrated-notice tests rode on aggregations also incomplete for another reason. That pairing is the #3155 reporter's own fully-included pool, where the notice is the only remaining uncertainty signal.Mutation-checked rather than asserted: gating the notice under the
aggregation.incompletebranch — a plausible tidy-up of three similar warning blocks sharing a CSS class — fails only the new test; all eleven prior fixtures pass. That is exactly the silent hide the review predicted.Verification
bun run test— 17419 pass, 0 fail, exit 0; all six serial lanes green.tests/provider-quota.test.ts110/0,gui/tests/provider-capacity-shell.test.tsx12/0.bun run typecheck/bun run privacy:scan— passed.Checklist
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
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