From 73b8257b2e4f6895d5f0636db62a55aaa78d13c5 Mon Sep 17 00:00:00 2001 From: olddonkey Date: Tue, 1 Sep 2026 12:10:34 -0700 Subject: [PATCH] test(quota): address the eleven review findings on the contract move MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #3155 (PR #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 #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 --- gui/tests/provider-capacity-shell.test.tsx | 44 ++++++++++++++++++++++ tests/provider-quota.test.ts | 36 +++++++++++++----- 2 files changed, 70 insertions(+), 10 deletions(-) diff --git a/gui/tests/provider-capacity-shell.test.tsx b/gui/tests/provider-capacity-shell.test.tsx index 7531833f13..fdf9c5c09d 100644 --- a/gui/tests/provider-capacity-shell.test.tsx +++ b/gui/tests/provider-capacity-shell.test.tsx @@ -328,6 +328,50 @@ test("all-stale response renders coverage only without a numeric fallback", asyn expect(text).toContain("Incomplete coverage: 2 account(s) excluded"); }); +test("a fully included pool still surfaces the uncalibrated-plan notice", async () => { + // The #3155 reporter's own shape: every seat included, complete coverage, one seat counted + // at the baseline weight. The uncalibrated notice is the ONLY remaining uncertainty signal + // here, so it must render independently of the incomplete gate — folding it under the + // incomplete branch would pass every other fixture in this file and silently hide it. + quotaPayload = { + reports: [{ + provider: "openai", + label: "OpenAI (Codex login)", + source: "chatgpt:wham", + updatedAt: Date.now(), + quota: { weeklyPercent: 44, updatedAt: Date.now() }, + aggregation: { + kind: "capacity-weighted-v1", + scope: "routable-known", + presentation: "aggregate", + includedAccounts: 2, + excludedAccounts: 0, + unknownPlanAccounts: 1, + missingQuotaAccounts: 0, + pausedAccounts: 0, + reauthAccounts: 0, + staleQuotaAccounts: 0, + incomplete: false, + weekly: { + usedPercent: 44, + includedAccounts: 2, + excludedAccounts: 0, + incomplete: false, + updatedAt: Date.now(), + }, + currentAccount: { isMain: false, quota: { weeklyPercent: 77, updatedAt: Date.now() } }, + }, + }], + }; + + await mountShell(); + + const text = host.textContent ?? ""; + expect(text).toContain("1 account(s) on an uncalibrated plan are counted at the baseline seat weight"); + expect(text).not.toContain("Incomplete coverage"); + expect(text).toContain("44% used"); +}); + test("coverage-only API report remains visible in the rate-limit overview", async () => { quotaPayload = { reports: [{ diff --git a/tests/provider-quota.test.ts b/tests/provider-quota.test.ts index 2768ead96f..4c659df5b8 100644 --- a/tests/provider-quota.test.ts +++ b/tests/provider-quota.test.ts @@ -1859,13 +1859,16 @@ describe("fetchProviderQuotaReports", () => { expect(JSON.stringify(openai?.aggregation)).not.toMatch(/(?:total|consumed|remaining)Weight|projectedUsedPercent/i); }); - // #3198 changed what "tolerate" means here: an uncalibrated plan — a name the weight map - // does not list, or a malformed non-string value like the `{ tier: "pro" }` below (both - // normalize to undefined via codexPlanKey) — is now counted at the baseline seat weight - // instead of being excluded from the aggregate. Exclusion silently overstated coverage; - // baseline counting is the visibly conservative estimate. The account still shows up in - // `unknownPlanAccounts` so the operator can see the estimate is conservative for that seat. - test("pool reports tolerate a malformed persisted plan through cache and aggregation", async () => { + // #3155 (PR #3198) changed what "tolerate" means here: an uncalibrated plan is counted at + // the baseline seat weight instead of being excluded (rationale on + // CODEX_DEFAULT_CAPACITY_WEIGHT in src/providers/codex-capacity.ts). Two distinct inputs + // converge on that weight by different routes: an unlisted plan NAME normalizes to a defined + // key that fails the weight-map lookup, while a malformed non-string like the + // `{ tier: "pro" }` below never reaches the aggregation at all — poolAccountDto strips it via + // codexPlanValue, so the aggregate sees an ABSENT plan. This test pins the second route end + // to end: baseline weight, `unknownPlanAccounts` flagged, and the malformed value kept out of + // the public report shape. + test("pool reports count a malformed persisted plan at baseline and keep it out of the public shape", async () => { saveCodexAccountCredential("added", { accessToken: "added-access", refreshToken: "added-refresh", @@ -1886,6 +1889,9 @@ describe("fetchProviderQuotaReports", () => { calls += 1; const added = (init?.headers as Record | undefined)?.["ChatGPT-Account-Id"] === "added-chatgpt-id"; return new Response(JSON.stringify({ + // Load-bearing: a fetched plan_type outranks the persisted plan (auth-api.ts freshPlan), + // so this mock must stay non-string too — a string here would recalibrate the weight and + // change every aggregate value asserted below. plan_type: added ? { tier: "pro" } : "plus", rate_limit: { secondary_window: { used_percent: added ? 77 : 11, reset_at: 1_999_000_000 } }, }), { status: 200, headers: { "content-type": "application/json" } }); @@ -1893,21 +1899,31 @@ describe("fetchProviderQuotaReports", () => { const refreshed = await fetchProviderQuotaReports(config, true); const openai = refreshed.reports.find(row => row.provider === "openai"); - // Both seats weigh the same (malformed -> baseline, "plus" -> calibrated baseline), so the - // blend of 77 and 11 lands at 44 — not the 11 the old exclusion contract produced. + // The blend leans on CODEX_DEFAULT_CAPACITY_WEIGHT equalling the calibrated "plus" weight + // (both 1 today), so 77 and 11 land at 44 — not the 11 the old exclusion contract produced. expect(openai?.quota.weeklyPercent).toBe(44); expect(openai?.aggregation).toMatchObject({ includedAccounts: 2, excludedAccounts: 0, unknownPlanAccounts: 1, incomplete: false, + // The per-window flags are what the dashboard renders per bar; #3155 flipped them + // together with the top-level flag, so pin both. + weekly: { includedAccounts: 2, excludedAccounts: 0, incomplete: false }, currentAccount: { quota: { weeklyPercent: 77 } }, }); expect(openai?.aggregation?.currentAccount).not.toHaveProperty("plan"); + // The malformed value must not escape into the public report shape anywhere, and the + // internal weight fields must not either (same guard as the sibling test above). + expect(JSON.stringify(refreshed)).not.toContain("tier"); + expect(JSON.stringify(openai?.aggregation)).not.toMatch(/(?:total|consumed|remaining)Weight|projectedUsedPercent/i); expect(calls).toBe(2); + // The unforced path returns the cached response BY IDENTITY (no clone, no recompute), so + // re-asserting its fields would only re-read the object checked above. Pin the identity + // contract itself plus the call count; content is covered once, honestly, up there. const cached = await fetchProviderQuotaReports(config); - expect(cached.reports[0]?.aggregation?.unknownPlanAccounts).toBe(1); + expect(cached.reports[0]?.aggregation).toBe(openai?.aggregation); expect(calls).toBe(2); });