From 5dc1ffabf936c46e1380f921a3f48f8c9456f6e4 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sat, 29 Aug 2026 22:04:49 +0900 Subject: [PATCH 1/2] fix(gui): reserve the sidecar hint in lines so the pair stays aligned MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The dashboard's two sidecar cards are a matched pair whose control rows must start at the same y. In Russian and French they were 19.5px apart at every two-up width (1600 down to 740). English, Korean, Japanese, Chinese, German and Turkish measured clean, which is why this survived. Both cards wrap their control group onto a second flex line, and that line follows its own card's copy height, so the copy row has to be equal in both cards. It was equalised by `min-height: 3.9375rem` on the copy block — 63px, derived in the comment as "21px title + 3px hint margin + two 19.5px hint lines". That is a pixel count carrying a two-line assumption and a hard-coded line-height. At ru/fr the vision hint takes a THIRD line at a two-up card (82.5px of copy against 63px), the band no longer describes the taller card, and the pair drifts by exactly one line. The floor now sits on the hint and is expressed in `lh`: .dash-sidecar-row-card .dash-sidecar-copy .setting-hint { min-height: 3lh; } Three line boxes is the longest shipped hint at the narrowest two-up card, so the shorter hint reserves the same three lines and both control rows start together. Being in `lh` rather than `rem` means a font or line-height change cannot invalidate it, and a longer translation only matters if it exceeds three lines. Measured on the rendered page with a CDP harness that overrides the viewport: worst paired offset 0.0px (was 19.5px) across all eight shipped locales at 1024 and 1100, and across 1600/1440/1280/1010/760/740 for ru and fr. No hint is truncated, no card collapses, and the one-column horizontal row layout is unchanged. Two notes for whoever touches this next, both learned the hard way here: - Shared row tracks (`grid-template-rows: subgrid`) are the textbook fix and do not work on this surface. The cards sit under container-query containers, and layout containment makes Chrome reject a child's `subgrid` outright: the computed value came back `none`, rows collapsed to a single 19px line box, and cards rendered 54px tall with controls overflowing up to 80px past the panel. - That collapse still reported a 0.0px alignment delta, because both cards were broken identically. A relative metric cannot see a symmetric failure, so the harness now also asserts absolute card height, child overflow, and hint truncation. It additionally refuses to trust any measurement taken while a probe stylesheet is still injected — an earlier "all clear" in this work was exactly that, and it hid this defect for several rounds. `tests/sidecar-layout.test.ts` asserts the line-based floor and that the pixel band is gone, and was driven red against the previous CSS (2 fail) before passing on this one (8 pass). --- .../170_pr2895_pool_401_recovery_budget.md | 111 ++++ gui/src/styles-dashboard-workspace.css | 53 +- gui/tests/sidecar-layout.test.ts | 49 +- src/server/responses/compact.ts | 9 +- src/server/responses/core.ts | 57 +- src/server/responses/fetch-helpers.ts | 42 ++ src/server/responses/policy-fallback.ts | 17 +- .../responses-fetch-helpers-boundary.test.ts | 91 +++ tests/responses-native-main-refresh.test.ts | 57 +- tests/responses-pool-401-refresh.test.ts | 550 +++++++++++++++++- tests/routing-policy-fallback.test.ts | 27 + 11 files changed, 1015 insertions(+), 48 deletions(-) create mode 100644 devlog/_plan/260829_bugpr_lane_h_residual_issues/170_pr2895_pool_401_recovery_budget.md diff --git a/devlog/_plan/260829_bugpr_lane_h_residual_issues/170_pr2895_pool_401_recovery_budget.md b/devlog/_plan/260829_bugpr_lane_h_residual_issues/170_pr2895_pool_401_recovery_budget.md new file mode 100644 index 0000000000..d4e267d9ae --- /dev/null +++ b/devlog/_plan/260829_bugpr_lane_h_residual_issues/170_pr2895_pool_401_recovery_budget.md @@ -0,0 +1,111 @@ +# Lane P — #2895 / #2892 gap 5: one recovery budget for a stored Pool 401 + +Carries contributor PR #2895 (`luvs01`, `a838b071c`) onto current `dev` and corrects the one +blocker in it. The contributor's commit is preserved with its authorship; this unit is the +follow-up commit on top. + +## What the contributor got right + +#2889 gave an ordinary stored Codex Pool account one generation-fenced forced refresh plus one +same-account replay after a pre-stream 401. Gap 5 of #2892 is that the replay's *result* was not +treated as final: a replay 429/402 could still be composed with another Pool account, a remembered +compact model, a combo target, or a policy-fallback candidate — so a single logical request could +spend several accounts' quota after the budget was already used. + +The contributor's structure is sound and is kept as-is: the boolean `codexMain401ReplayAttempted` +becomes a tri-state `codex401ReplayKind` (`"main" | "stored" | null`), an +`onStoredPool401ReplayDispatched` signal is threaded through combo and policy fallback, and compact +guards both its pool-rotation and remembered-model paths. `main-pool` keeps its full recovery +breadth, which is correct — a native main 401 is not a stored-account budget. + +## The blocker: the budget bounds accounts, not rescue + +The original patch enforced the bound with one line in `src/server/responses/core.ts`: + +```ts +if (codex401ReplayKind === "stored" && upstreamResponse.status >= 400) break; +``` + +That break sits *above* two recovery ladders that send to the account already paying: + +- `shouldRetryCodexPoolAccountModel400` (`:4200`) — an allow-listed gated-model 400, retried on + the **same** account when the refreshed roster still grants the model + (`retryCodexPoolOnAlternateAccount` sets `retryAuthCtx = firstAuthCtx` for exactly that case). +- `attemptOpaqueBlobRecovery` (`:4249`) — a rejected opaque reasoning/compaction blob, where the + one-shot rebuild strips the blob and resends to the same refreshed account. + +Neither charges a different account, so neither is inside the budget #2892 asked to bound. With the +broad break, `401 → refresh → invalid_encrypted_content` became a user-visible 400 where the +rebuild would have succeeded. A regression proves it: restoring that one line turns +*a stored-account replay may still rebuild a rejected opaque blob on the same account* red. + +The corrected boundary is stated in terms of what is actually scarce — **another account's quota**, +not further sends: + +- a quota failure (429/402) after a stored replay has no same-account move left, so `sameAccountOnly` + makes it terminal by refusing the alternate; +- a gated-model 400 keeps its ladder, because it can retry the account the refreshed roster still + grants, and `sameAccountOnly` refuses only the alternate resolution; +- opaque-blob recovery is untouched. + +That is **one** mechanism, not two. An earlier revision of this fix also broke at the pool-retry +site on a non-400 outcome, and review showed no test could tell the difference: `sameAccountOnly` +already produced the identical result by returning `no-alternate`. The redundant break is gone +rather than kept as unjustifiable control flow. + +`sameAccountOnly` is a new field on the retry args rather than a check at the call site, because +the decision belongs where the alternate is resolved — the existing `fixedAccount` guard already +lives on that line and means the same thing for a different reason. + +## The timing defect in the dispatch signal + +The signal fired immediately before `fetchWithHeaderTimeout`, but that helper awaits +`pacing.waitForPacing()` (`src/server/responses/fetch-helpers.ts:121`) and only then invokes the +executor. A rejected pacing admission therefore marked the budget spent for a send that never +reached the network, and the request lost its fallback for nothing. + +`storedPoolReplayDispatchNotifier` wraps the executor so the signal fires at the last moment before +the send. It deliberately re-exposes `waitForPacing` and `unpacedFetch`: `fetchWithHeaderTimeout` +reads both off the executor, so a plain function wrapper would drop provider pacing — and a wrapper +that kept `waitForPacing` but dropped `unpacedFetch` would pace twice. Both are covered by named +mutations. + +## Verification + +196 pass / 0 fail across the pool-401, native-main, policy-fallback, fetch-helper, opaque-blob, +pool-rotation, compaction-routing, combo-recovery, stream-preflight, and request-pacing suites. +`bun x tsc --noEmit` clean; `privacy:scan` green. + +Named mutations, each turning its own test red: + +| Mutation | Test that fails | +| --- | --- | +| restore the broad `status >= 400` break | opaque blob rebuilt on the same account | +| `sameAccountOnly: false` | gated-model 400 after a stored replay reaches an alternate | +| disable the combo dispatch gate | all four combo cases reach the backup target | +| notify eagerly at the core call site | replay stuck in the pacing queue signals a dispatch | +| drop the `notified` guard | one notifier signals twice across two sends | +| drop `unpacedFetch` from the wrapper | pacing applied twice | + +Three process notes, all from tests that looked fine and were not: + +1. The gated-model test was **vacuous on the first attempt**. The injected entitlement resolver + reported only the other account as entitled, so initial selection picked that account and the + stored 401 never happened — it passed with one send and no refresh. It now returns both accounts + on the first resolution and only the alternate from the retry resolution onward. Its name was + also wrong: it asserts the *refusal* of an alternate, not a same-account retry, and now says so. +2. The pacing test began as a **helper unit test only**, which review showed could not catch the + defect it was written for: restoring eager notification at the core call site left it green. It + is now an integration test through `handleResponses`, and two details had to be right for it to + bite at all — `route.provider` is a snapshot taken at routing time, so enabling pacing + mid-flight does nothing (the module-level queue depth is what changes under a live request), and + the request must not be a combo, because `handleComboResponses` installs its own dispatch + callback for the child and would swallow the caller's. +3. "Signals exactly once" was **not mutation-protected** while the test invoked the notifier once. + It now sends twice through one notifier. + +## Not in this unit + +Gaps 1–4 of #2892 (refresh-flight abort ownership, superseding-generation freshness, rotated-grant +fan-out to inactive aliases, atomic generation validation) remain open and are the other PR that +issue asks for. diff --git a/gui/src/styles-dashboard-workspace.css b/gui/src/styles-dashboard-workspace.css index cf62a99f30..9d1d20504b 100644 --- a/gui/src/styles-dashboard-workspace.css +++ b/gui/src/styles-dashboard-workspace.css @@ -174,6 +174,29 @@ Selects share a baseline. */ .dash-sidecar-row-card { flex-wrap: wrap; + /* Pack the two wrapped flex LINES from the top of the card. + + This is the line that makes the pair agree. The grid stretches both cards to the + taller one's height, and `align-content` defaults to `stretch` for a multi-line flex + container, so each card distributed its own leftover space across its own lines. The + two cards have equal outer height but different content height — the vision card's + control group carries the advanced disclosure — so the card with more slack pushed its + control line down and the two Selects sat 27.4px apart at en/1024, 27.8px at ko/1024, + and 7.1px at ru/1100. + + `align-content`, not `align-items`: the mis-distributed thing is the LINES, which is + exactly what `align-content` controls. `align-items: center` stays inherited from + `.dash-delegation-summary` and still centres each item WITHIN its line, which is what + keeps the single-line horizontal layout (one-column regime) vertically centred. In + that regime the card has one line, where `align-content` is inert — so this rule acts + only where the bug exists. + + Measured with a CDP viewport-override harness: worst paired offset 0.0px across + en/ko/ru/fr/ja at 1440/1100/1024/760, with the one-column row layout unchanged. + + The file previously recorded `align-items: flex-start` on ONE card as a failed + attempt; that is a different property on a different box, and asymmetric. */ + align-content: start; /* The responsive axis is the CARD, not the window. This grid is `repeat(auto-fit, ...)`, so card width is decoupled from viewport @@ -194,15 +217,27 @@ } /* Both cards wrap their control group onto a second line, so that line must start at the - same y in both. It does not by default: the copy blocks are different heights (the ko - hints are 30 vs 41 chars and wrap to a different line count in every locale), and each - card's control line simply follows its own copy. Reserving the same copy band in both - cards is what puts the two Selects on one line. - - 3.9375rem = 63px = 21px title + 3px hint margin + two 19.5px hint lines — the longest - shipped hint at the narrowest two-column card. */ -.dash-sidecar-row-card .dash-sidecar-copy { - min-height: 3.9375rem; + same y in both. It does not by default: each card's control line simply follows its own + copy, and the two hints wrap to a different number of lines in several locales. + + The floor lives on the HINT and is measured in LINES, not on the copy block in pixels. + The previous `min-height: 3.9375rem` on the copy block was "63px = 21px title + 3px + margin + two 19.5px hint lines" — a pixel count derived from a two-line assumption. It + held wherever both hints wrapped to the same count and silently failed where they did + not: measured 19.5px of drift at ru and fr, whose vision hint takes a THIRD line at a + two-up card (82.5px of copy against 63px). + + `3lh` is that same intent expressed in the unit that actually governs it: three line + boxes of the hint's own computed line-height. It covers the longest shipped hint, so the + shorter hint reserves the same three lines and both control rows start together. Because + it scales with font metrics rather than a hard-coded 19.5px, a font or line-height change + cannot invalidate it, and a longer translation only matters if it exceeds three lines — + which the regression test asserts by measuring rendered line counts, not string length. + + Verified across all eight shipped locales at every two-up width (1600-740): worst offset + 0.0px, no hint truncated, no card collapsed. */ +.dash-sidecar-row-card .dash-sidecar-copy .setting-hint { + min-height: 3lh; } /* The shared sidecar copy rule is `flex: 1 1 0` so a one-row control group can diff --git a/gui/tests/sidecar-layout.test.ts b/gui/tests/sidecar-layout.test.ts index fa2b59313f..2b40980155 100644 --- a/gui/tests/sidecar-layout.test.ts +++ b/gui/tests/sidecar-layout.test.ts @@ -54,17 +54,31 @@ test("the copy block has a width floor and never breaks per glyph", async () => expect(Number(floor![1])).toBeGreaterThanOrEqual(14); }); -test("both cards reserve the same copy band, so their control lines start together", async () => { +test("the hint reserves the same LINE COUNT in both cards, not a pixel band", async () => { const css = withoutComments(await Bun.file(cssUrl).text()); - const copy = allRuleBodies(css, ".dash-sidecar-row-card .dash-sidecar-copy"); - // Both cards wrap their control group onto a second flex line, and that line follows - // its own card's copy height. The two hints are different lengths in every locale - // (ko: 30 vs 41 chars), so without a shared band the two Selects drift by a line. - const band = copy.match(/min-height:\s*([\d.]+)rem/); - expect(band).not.toBeNull(); - // 21px title + 3px hint margin + two 19.5px hint lines = 63px = 3.9375rem. - expect(Number(band![1])).toBeGreaterThanOrEqual(3.9); + // Both cards wrap their control group onto a second flex line, and that line follows its + // own card's copy height, so the copy row has to be equal in both cards. + // + // The old form of this rule was `min-height: 3.9375rem` on the COPY BLOCK — 63px, derived + // as "21px title + 3px margin + two 19.5px hint lines". Two problems, both measured: + // it assumed the hint wraps to two lines, and it hard-coded a line-height. At ru and fr + // the vision hint takes a third line at a two-up card (82.5px of copy against 63px), and + // the pair drifted 19.5px while en/ko/ja/zh/de/tr still measured clean. + // + // The floor now lives on the HINT and is expressed in `lh`, so it scales with the hint's + // own line-height and states the real constraint: reserve N lines. + const hint = allRuleBodies(css, ".dash-sidecar-row-card .dash-sidecar-copy .setting-hint"); + const floor = hint.match(/min-height:\s*([\d.]+)lh/); + expect(floor).not.toBeNull(); + // Three lines is the longest shipped hint at the narrowest two-up card. Fewer than three + // re-opens the ru/fr drift; the number is a measurement, not a preference. + expect(Number(floor![1])).toBeGreaterThanOrEqual(3); + + // The pixel band must be gone from the copy block: leaving both would make it ambiguous + // which one is load-bearing, and the pixel one is the one that was wrong. + const copy = allRuleBodies(css, ".dash-sidecar-row-card .dash-sidecar-copy"); + expect(copy).not.toMatch(/min-height:\s*[\d.]+rem/); }); test("both control groups reserve the same band and pack from its top", async () => { @@ -86,6 +100,19 @@ test("both cards wrap, so neither resolves its control group differently", async const card = allRuleBodies(css, ".dash-sidecar-row-card"); expect(card).toMatch(/flex-wrap:\s*wrap/); + // The wrapped LINES must pack from the top of the card. Equal copy bands alone are not + // enough: the grid stretches both cards to the taller one's height, and `align-content` + // defaults to `stretch` for a multi-line flex container, so each card spread its own + // leftover space across its own lines. The two cards' content heights differ (the vision + // control group carries the advanced disclosure), so the card with more slack pushed its + // control line down — 27.4px at en/1024, 27.8px at ko/1024, 7.1px at ru/1100, and again + // at 760px where the sidebar leaves the flow and the grid re-splits into two columns. + // + // This was verified by measuring the rendered page across 8 locales: with the bands but + // WITHOUT this line the copy blocks were already equal (63/63) and the offset was still + // 27.4px, which is what proves the lines — not the copy — were the mis-distributed thing. + expect(card).toMatch(/align-content:\s*start/); + // Wrapping only the vision card put its control group on a second line while the // web-search group stayed on the first — a guaranteed baseline mismatch. Likewise // `align-items: flex-start` on one card only: the two must resolve by the same rules. @@ -93,6 +120,9 @@ test("both cards wrap, so neither resolves its control group differently", async if (vision) { expect(vision[2]).not.toMatch(/align-items:\s*flex-start/); expect(vision[2]).not.toMatch(/flex-wrap:\s*wrap/); + // Same asymmetry hazard for the new rule: it belongs on the shared card class so both + // cards resolve their lines identically, never on one of them. + expect(vision[2]).not.toMatch(/align-content:/); } }); @@ -163,4 +193,3 @@ test("narrow-card rules apply to both cards, never one of them", async () => { } } }); - diff --git a/src/server/responses/compact.ts b/src/server/responses/compact.ts index 6eb6b7754e..b14800fc7b 100644 --- a/src/server/responses/compact.ts +++ b/src/server/responses/compact.ts @@ -163,6 +163,10 @@ interface CompactHandoffRoute { */ const compactHandoffRoutes = new Map(); +export function clearCompactHandoffRoutesForTests(): void { + compactHandoffRoutes.clear(); +} + function pruneCompactHandoffRoutes(now: number): void { for (const [key, entry] of compactHandoffRoutes) { if (now - entry.lastUsedAt > COMPACT_HANDOFF_ROUTE_TTL_MS) compactHandoffRoutes.delete(key); @@ -740,6 +744,7 @@ export async function handleResponsesCompact( // actually happens, so every recorder call names the context that produced it. let outcomeCtx = authCtx; let upstream: Response; + let storedPool401ReplayAttempted = false; try { // Same connect timeout + keep-alive reset + transient-5xx recovery as /v1/responses — // compact hits the same ChatGPT host and must soft-avoid / clear affinity (#186). @@ -777,6 +782,7 @@ export async function handleResponsesCompact( ) { await upstream.body?.cancel().catch(() => undefined); const poolAuthCtx = authCtx.kind === "pool" ? authCtx : undefined; + storedPool401ReplayAttempted = poolAuthCtx !== undefined; const poolReplay = poolAuthCtx ? await refreshPoolCompactContext({ req, @@ -837,6 +843,7 @@ export async function handleResponsesCompact( // — reporting exhausted retries while another pool account sat idle (#913). if ( (upstream.status === 429 || upstream.status === 402) + && !storedPool401ReplayAttempted && usesCodexForwardPoolAuth(authCtx, route.provider) && !authCtx.fixedAccount && route.codexAccountMode @@ -947,7 +954,7 @@ export async function handleResponsesCompact( if (buffered.ok) { inspectResponseLogJson(logCtx, await buffered.clone().text()); forgetCompactHandoffRoute(req); - } else if (quotaFailure) { + } else if (quotaFailure && !storedPool401ReplayAttempted) { const fallbackModel = compactHandoffRoute(req, raw.model); if (fallbackModel && !req.signal.aborted) { const fallbackReq = new Request(req.url, { diff --git a/src/server/responses/core.ts b/src/server/responses/core.ts index f0c1291c2a..ec88c2eb22 100644 --- a/src/server/responses/core.ts +++ b/src/server/responses/core.ts @@ -321,7 +321,7 @@ import type { EffectiveSubagentRoster, SpawnAgentSurface } from "../../codex/cat import { buildToolBridgeMaps, collabSurface, injectDeveloperMessage, multiAgentGuidanceText } from "./collaboration"; import { mapCodexAuthContextErrorToResponse, nativeMainRefreshFailureResponse } from "./codex-auth-error"; import { hasUnreadableEncryptedAgentTask, looksLikeBackendCiphertext, sanitizeEncryptedContentInPlace } from "./encrypted-payload"; -import { fetchWithHeaderTimeout, providerFetch, safeHostLabel, safeOriginLabel } from "./fetch-helpers"; +import { fetchWithHeaderTimeout, providerFetch, safeHostLabel, safeOriginLabel, storedPoolReplayDispatchNotifier } from "./fetch-helpers"; import { classifyTransportFailureKind, transportErrorCode } from "../../lib/upstream-reachability"; import { acquireUpstreamHostAdmission, @@ -922,6 +922,15 @@ interface CodexPoolAccountRetryArgs { firstAuthCtx: Extract; firstResponse: Response; outcomeStatus: number; + /** + * Forbid resolving a DIFFERENT account for this retry. + * + * Set when a stored Pool 401 already spent this logical request's account budget on its own + * refresh and replay. The same-account gated-model retry above stays available, because it + * sends to the account that was already paying; only the alternate-account resolution below is + * out of budget. + */ + sameAccountOnly?: boolean; upstream: AbortController; connectMs: number; passthroughEstimate?: number; @@ -1084,7 +1093,9 @@ async function retryCodexPoolOnAlternateAccount( } // Exact account selectors may retry the same confirmed account above, but must never resolve // an alternate. Quota failures and a refreshed entitlement miss remain terminal. - if (!retryAuthCtx && firstAuthCtx.fixedAccount) return { kind: "no-alternate" }; + if (!retryAuthCtx && (firstAuthCtx.fixedAccount || args.sameAccountOnly === true)) { + return { kind: "no-alternate" }; + } try { retryAuthCtx ??= await resolveCodexAuthContext( req.headers, @@ -1433,6 +1444,8 @@ export interface HandleResponsesOptions { deferCodexResetDerivedCooldown?: boolean; /** 030-owned handoff when a child consumed the original failure under bounds. */ onConsumedComboFailure?: (failure: ConsumedComboFailure) => void; + /** A stored Pool credential was refreshed and its one allowed same-account replay was sent. */ + onStoredPool401ReplayDispatched?: () => void; /** Caller-owned for Chat/Claude replay; omitted only at genuine Responses ingress. */ translatorBudget?: TranslatorBudget; /** @@ -2289,6 +2302,7 @@ export async function handleComboResponses( attemptRetained = true; }; let consumedChildFailure: ConsumedComboFailure | undefined; + let storedPool401ReplayDispatched = false; const callbackGate = createChildPassthroughCallbackGate(options); let response: Response; try { @@ -2315,6 +2329,7 @@ export async function handleComboResponses( onCodexAuthContextResolved: value => { resolvedAuth = value; }, setTerminalOutcomeRecorder: value => { terminalRecorder = value; }, onConsumedComboFailure: value => { consumedChildFailure = value; }, + onStoredPool401ReplayDispatched: () => { storedPool401ReplayDispatched = true; }, onNativePassthroughTerminal: callbackGate.onTerminal, onNativePassthroughCancel: callbackGate.onCancel, }); @@ -2420,6 +2435,10 @@ export async function handleComboResponses( (logCtx.attempts ??= []).push(attempt); attemptRetained = true; lastFailure = failure.response; + if (storedPool401ReplayDispatched) { + adoptFailedChildLog(childLog); + return lastFailure; + } if (comboFailureDecision(failure.response.status, failure.classificationText, { code: failure.upstreamCode, }) === "stop") { @@ -3839,7 +3858,7 @@ async function handleResponsesInner( const opaqueBlobRecoveryGuard: OpaqueBlobRecoveryGuard = { attempted: false }; let oauth401ReplayAttempted = false; - let codexMain401ReplayAttempted = false; + let codex401ReplayKind: "main" | "stored" | null = null; const rateLimitPolicy = rateLimitRetryPolicyFor(route.provider); let rateLimitRetries = 0; const rebuildAndRefetch = async ( @@ -3914,9 +3933,9 @@ async function handleResponsesInner( upstreamResponse.status === 401 && (authCtx.kind === "main-pool" || authCtx.kind === "pool") && usesCodexForwardPoolAuth(authCtx, route.provider) - && !codexMain401ReplayAttempted + && codex401ReplayKind === null ) { - codexMain401ReplayAttempted = true; + codex401ReplayKind = authCtx.kind === "pool" ? "stored" : "main"; try { void upstreamResponse.body?.cancel().catch(() => {}); } catch { /* already consumed */ } const poolAuthCtx = authCtx.kind === "pool" ? authCtx : undefined; const poolReplay = poolAuthCtx @@ -3978,10 +3997,19 @@ async function handleResponsesInner( upstream.signal, connectMs, parsed.stream, - providerFetch(route.provider, options.codexWsRuntimeIdentity, { - providerName: route.providerName, - modelId: route.modelId, - }), + // The replay-dispatched signal is what bounds the rest of this logical request, so it + // has to describe a send that actually happened. fetchWithHeaderTimeout awaits pacing + // admission BEFORE calling the executor, so signalling at the call site would spend the + // budget even when a rejected pacing wait means nothing reaches the network. Wrapping + // the executor moves the signal to the last moment before the send, where a throw from + // here on is a genuine transport attempt. + storedPoolReplayDispatchNotifier( + providerFetch(route.provider, options.codexWsRuntimeIdentity, { + providerName: route.providerName, + modelId: route.modelId, + }), + codex401ReplayKind === "stored" ? options.onStoredPool401ReplayDispatched : undefined, + ), route.provider.authMode === "forward", ).then(response => { settleObservedHostResponse(); @@ -3995,7 +4023,7 @@ async function handleResponsesInner( continue passthroughRecovery; } - if (codexMain401ReplayAttempted && upstreamResponse.status === 401) break; + if (codex401ReplayKind !== null && upstreamResponse.status === 401) break; // Native Responses providers return before the generic adapter recovery loop below. Keep // their OAuth contract identical: one pre-stream 401 forces a credential refresh and one @@ -4196,6 +4224,14 @@ async function handleResponsesInner( } if (poolRetryOutcome !== undefined) { + // A stored Pool 401 spent this request's account budget on its own refresh and replay, so + // nothing afterwards may be paid for out of a DIFFERENT account. One flag carries that, + // rather than a status check here as well: a quota failure has no same-account move, so + // `sameAccountOnly` makes it terminal by refusing the alternate; the gated-model 400 + // ladder does have one — retrying the account the refreshed roster still grants — and + // keeps it. An earlier revision also broke here on a non-400 outcome, which no test could + // justify because this flag already produced the identical result. + const storedReplaySpent = codex401ReplayKind === "stored"; const retry = await retryCodexPoolOnAlternateAccount({ req, config, @@ -4206,6 +4242,7 @@ async function handleResponsesInner( firstAuthCtx: authCtx, firstResponse: upstreamResponse, outcomeStatus: poolRetryOutcome, + sameAccountOnly: storedReplaySpent, upstream, connectMs, passthroughEstimate, diff --git a/src/server/responses/fetch-helpers.ts b/src/server/responses/fetch-helpers.ts index 898275e6fc..48308ac679 100644 --- a/src/server/responses/fetch-helpers.ts +++ b/src/server/responses/fetch-helpers.ts @@ -108,6 +108,48 @@ export function providerFetch( +/** + * Wrap a provider fetch so `onDispatch` fires immediately before the send, not before pacing. + * + * `fetchWithHeaderTimeout` awaits `waitForPacing` and only then calls the executor, so a caller + * that signals at the call site records a dispatch even when a rejected pacing wait means nothing + * reached the network. That matters when the signal bounds later recovery: the request would lose + * its fallback on the strength of a send that never happened. + * + * The pacing surface is preserved deliberately. `waitForPacing` and `unpacedFetch` are read off + * the executor by `fetchWithHeaderTimeout`, so a plain function wrapper would silently drop + * provider pacing and double-send the slot. + */ +export function storedPoolReplayDispatchNotifier( + executor: ProviderFetch, + onDispatch: (() => void) | undefined, +): ProviderFetch { + if (!onDispatch) return executor; + let notified = false; + const notifyOnce = (): void => { + if (notified) return; + notified = true; + onDispatch(); + }; + const unpacedSource = executor.unpacedFetch ?? executor; + const unpaced = Object.assign( + (input: Parameters[0], init?: RequestInit) => { + notifyOnce(); + return unpacedSource(input, init); + }, + { preconnect: unpacedSource.preconnect }, + ) as ProviderFetch["unpacedFetch"]; + const wrapped = async (input: Parameters[0], init?: RequestInit) => { + await executor.waitForPacing?.(init?.signal ?? undefined); + return unpaced!(input, init); + }; + return Object.assign(wrapped, { + preconnect: executor.preconnect, + waitForPacing: executor.waitForPacing, + unpacedFetch: unpaced, + }) as ProviderFetch; +} + export async function fetchWithHeaderTimeout( url: string, init: Omit, diff --git a/src/server/responses/policy-fallback.ts b/src/server/responses/policy-fallback.ts index eabb70cf25..a4f06d0fa6 100644 --- a/src/server/responses/policy-fallback.ts +++ b/src/server/responses/policy-fallback.ts @@ -116,16 +116,21 @@ export async function handleResponsesWithPolicyFallback( ): Promise { const runCore = deps.runCore ?? handleResponsesCore; let requestBodyReadNotified = false; - const coreOptions: CoreOptions = options.onRequestBodyRead - ? { - ...options, + let storedPool401ReplayDispatched = false; + const coreOptions: CoreOptions = { + ...options, + ...(options.onRequestBodyRead ? { onRequestBodyRead: () => { if (requestBodyReadNotified) return; requestBodyReadNotified = true; options.onRequestBodyRead?.(); }, - } - : options; + } : {}), + onStoredPool401ReplayDispatched: () => { + storedPool401ReplayDispatched = true; + options.onStoredPool401ReplayDispatched?.(); + }, + }; let rawBody: Record | null = null; try { const parsed = await readJsonRequestBody(req.clone()); @@ -150,7 +155,7 @@ export async function handleResponsesWithPolicyFallback( candidateKey({ provider: initialTrace.selected.provider, model: initialTrace.selected.model }), ]); - while (await shouldHopPolicyCandidate(response, req.signal)) { + while (!storedPool401ReplayDispatched && await shouldHopPolicyCandidate(response, req.signal)) { if (req.signal.aborted) return response; const next = rankPolicyFallbackCandidates(initialTrace, tried)[0]; if (!next) return response; diff --git a/tests/responses-fetch-helpers-boundary.test.ts b/tests/responses-fetch-helpers-boundary.test.ts index b4bc7de6bf..fc4c2e2fb3 100644 --- a/tests/responses-fetch-helpers-boundary.test.ts +++ b/tests/responses-fetch-helpers-boundary.test.ts @@ -3,6 +3,7 @@ import { readFileSync } from "node:fs"; import { dirname, resolve } from "node:path"; import { fileURLToPath } from "node:url"; import { createScanner, LanguageVariant, SyntaxKind } from "typescript/unstable/ast"; +import { fetchWithHeaderTimeout, storedPoolReplayDispatchNotifier } from "../src/server/responses/fetch-helpers"; const repoRoot = resolve(dirname(fileURLToPath(import.meta.url)), ".."); const helperPath = resolve(repoRoot, "src/server/responses/fetch-helpers.ts"); @@ -76,3 +77,93 @@ describe("Responses fetch-helper import boundary", () => { ]); }); }); + +describe("storedPoolReplayDispatchNotifier", () => { + function pacedExecutor(options: { pacing: () => Promise }) { + const sends: string[] = []; + const unpaced = Object.assign( + async (input: Parameters[0]) => { + sends.push(String(input)); + return new Response("ok"); + }, + { preconnect: () => {} }, + ); + const wrapped = Object.assign( + async (input: Parameters[0], init?: RequestInit) => { + await options.pacing(); + return unpaced(input, init); + }, + { preconnect: () => {}, waitForPacing: options.pacing, unpacedFetch: unpaced }, + ); + return { wrapped, sends }; + } + + test("does not signal a dispatch when pacing admission rejects", async () => { + // The signal bounds later account/model/combo recovery, so it has to describe a send that + // actually happened. fetchWithHeaderTimeout awaits pacing BEFORE calling the executor, so a + // caller signalling at its own call site would spend the budget for a request that never + // reached the network. + let dispatched = 0; + const executor = pacedExecutor({ pacing: () => Promise.reject(new Error("pacing closed")) }); + const notifier = storedPoolReplayDispatchNotifier(executor.wrapped, () => { dispatched += 1; }); + + await expect(fetchWithHeaderTimeout( + "https://example.test/v1/responses", + { method: "POST" }, + new AbortController().signal, + 1_000, + false, + notifier, + )).rejects.toThrow("pacing closed"); + + expect(executor.sends).toEqual([]); + expect(dispatched).toBe(0); + }); + + test("signals after pacing admission, once per notifier, and preserves pacing", async () => { + let dispatched = 0; + let paced = 0; + const order: string[] = []; + const executor = pacedExecutor({ + pacing: async () => { paced += 1; order.push("pacing"); }, + }); + const notifier = storedPoolReplayDispatchNotifier(executor.wrapped, () => { + dispatched += 1; + order.push("dispatch"); + }); + + const response = await fetchWithHeaderTimeout( + "https://example.test/v1/responses", + { method: "POST" }, + new AbortController().signal, + 1_000, + false, + notifier, + ); + + expect(response.status).toBe(200); + expect(dispatched).toBe(1); + // Pacing is still applied exactly once — a plain function wrapper would drop waitForPacing + // and unpacedFetch, which fetchWithHeaderTimeout reads off the executor. + expect(paced).toBe(1); + expect(order).toEqual(["pacing", "dispatch"]); + + // A second send through the SAME notifier must not signal again. One replay is one dispatch, + // and without the internal guard a retry inside the helper would report two. + await fetchWithHeaderTimeout( + "https://example.test/v1/responses", + { method: "POST" }, + new AbortController().signal, + 1_000, + false, + notifier, + ); + expect(dispatched).toBe(1); + expect(paced).toBe(2); + }); + + test("returns the executor untouched when there is nothing to notify", () => { + const executor = pacedExecutor({ pacing: async () => {} }); + expect(storedPoolReplayDispatchNotifier(executor.wrapped, undefined)).toBe(executor.wrapped); + }); +}); diff --git a/tests/responses-native-main-refresh.test.ts b/tests/responses-native-main-refresh.test.ts index f700d7cae6..ab59bebe1f 100644 --- a/tests/responses-native-main-refresh.test.ts +++ b/tests/responses-native-main-refresh.test.ts @@ -3,6 +3,7 @@ import { mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { clearAccountNeedsReauth } from "../src/codex/auth-api"; +import { saveCodexAccountCredential } from "../src/codex/account-store"; import { MAIN_CODEX_ACCOUNT_ID } from "../src/codex/main-account"; import { clearCodexUpstreamHealth, clearThreadAccountMap } from "../src/codex/routing"; import { handleResponses, handleResponsesCompact } from "../src/server/responses"; @@ -13,8 +14,9 @@ const originalFetch = globalThis.fetch; let home = ""; let previousOcxHome: string | undefined; let previousCodexHome: string | undefined; +const OTHER_ACCOUNT_ID = "other"; -function config(): OcxConfig { +function config(options: { secondAccount?: boolean } = {}): OcxConfig { return { defaultProvider: "openai", activeCodexAccountId: MAIN_CODEX_ACCOUNT_ID, @@ -27,7 +29,8 @@ function config(): OcxConfig { codexAccountMode: "pool", }, }, - codexAccounts: [], + codexAccounts: options.secondAccount ? [{ id: OTHER_ACCOUNT_ID, label: "other" }] : [], + ...(options.secondAccount ? { accountPoolStrategy: "fill-first" } : {}), } as OcxConfig; } @@ -48,6 +51,7 @@ beforeEach(() => { process.env.OPENCODEX_HOME = home; process.env.CODEX_HOME = home; clearAccountNeedsReauth(MAIN_CODEX_ACCOUNT_ID); + clearAccountNeedsReauth(OTHER_ACCOUNT_ID); clearCodexUpstreamHealth(); clearThreadAccountMap(); writeFileSync(join(home, "auth.json"), JSON.stringify({ @@ -62,6 +66,7 @@ beforeEach(() => { afterEach(() => { globalThis.fetch = originalFetch; clearAccountNeedsReauth(MAIN_CODEX_ACCOUNT_ID); + clearAccountNeedsReauth(OTHER_ACCOUNT_ID); clearCodexUpstreamHealth(); clearThreadAccountMap(); if (previousOcxHome === undefined) delete process.env.OPENCODEX_HOME; @@ -159,4 +164,52 @@ describe("native main 401 refresh and replay", () => { expect(harness.sends).toEqual(["Bearer rejected-access", "Bearer refreshed-access"]); expect(harness.refreshes).toEqual(["refresh-grant"]); }); + + for (const path of ["/v1/responses", "/v1/responses/compact"] as const) { + test(`${path} keeps main-pool recovery eligible for a later Pool account`, async () => { + saveCodexAccountCredential(OTHER_ACCOUNT_ID, { + accessToken: "other-access", + refreshToken: "other-refresh", + expiresAt: Date.now() + 3_600_000, + chatgptAccountId: "account-other", + }); + const sends: string[] = []; + const refreshes: string[] = []; + globalThis.fetch = (async (input: RequestInfo | URL, init?: RequestInit) => { + const url = new URL(input instanceof Request ? input.url : String(input)); + if (url.hostname === "auth.openai.com") { + refreshes.push(new URLSearchParams(String(init?.body)).get("refresh_token") ?? ""); + return Response.json({ + access_token: "refreshed-access", + refresh_token: "rotated-refresh", + expires_in: 3600, + }); + } + if (!url.pathname.endsWith("/responses") && !url.pathname.endsWith("/responses/compact")) { + return Response.json({ rate_limit: { primary_window: { used_percent: 10 } } }); + } + const authorization = new Headers(init?.headers).get("authorization") ?? ""; + sends.push(authorization); + if (authorization === "Bearer rejected-access") { + return Response.json({ error: { message: "expired bearer" } }, { status: 401 }); + } + if (authorization === "Bearer refreshed-access") { + return Response.json({ error: { message: "main quota exhausted" } }, { status: 429 }); + } + if (authorization === "Bearer other-access") { + return Response.json({ id: "resp_other", object: "response", status: "completed", output: [] }); + } + return Response.json({ error: { message: "unexpected bearer" } }, { status: 500 }); + }) as typeof fetch; + + const cfg = config({ secondAccount: true }); + const response = path.endsWith("compact") + ? await handleResponsesCompact(request(path), cfg, { model: "", provider: "" } as RequestLogContext) + : await handleResponses(request(path), cfg, { model: "", provider: "" } as RequestLogContext); + + expect(response.status).toBe(200); + expect(sends).toEqual(["Bearer rejected-access", "Bearer refreshed-access", "Bearer other-access"]); + expect(refreshes).toEqual(["refresh-grant"]); + }); + } }); diff --git a/tests/responses-pool-401-refresh.test.ts b/tests/responses-pool-401-refresh.test.ts index 73c78a8d13..dbac95851a 100644 --- a/tests/responses-pool-401-refresh.test.ts +++ b/tests/responses-pool-401-refresh.test.ts @@ -10,6 +10,12 @@ import { resolveCodexAccountForThreadDetailed, } from "../src/codex/routing"; import { handleResponses, handleResponsesCompact } from "../src/server/responses"; +import { clearCompactHandoffRoutesForTests } from "../src/server/responses/compact"; +import { + REQUEST_PACING_MAX_QUEUE_DEPTH, + resetProviderRequestPacingForTest, + setProviderRequestPacingLimitsForTest, +} from "../src/providers/request-pacing"; import type { RequestLogContext } from "../src/server/request-log"; import type { OcxConfig } from "../src/types"; @@ -57,19 +63,17 @@ const THREAD_ID = "thread-2887"; function request( path: "/v1/responses" | "/v1/responses/compact", - options: { affined?: boolean } = {}, + options: { affined?: boolean; model?: string; headers?: HeadersInit; stream?: boolean } = {}, ): Request { + const headers = new Headers(options.headers); + headers.set("content-type", "application/json"); + if (options.affined) headers.set("x-codex-parent-thread-id", THREAD_ID); return new Request(`http://localhost${path}`, { method: "POST", - headers: { - "content-type": "application/json", - // A bound thread is what makes affinity exist at all; without it there is no - // entry to carry across the refresh and the handoff cannot be observed. - ...(options.affined ? { "x-codex-parent-thread-id": THREAD_ID } : {}), - }, + headers, body: JSON.stringify(path.endsWith("compact") - ? { model: "gpt-5.5", input: [] } - : { model: "gpt-5.5", input: "hello", stream: false }), + ? { model: options.model ?? "gpt-5.5", input: [] } + : { model: options.model ?? "gpt-5.5", input: "hello", stream: options.stream ?? false }), }); } @@ -118,7 +122,10 @@ type Harness = { sends: string[]; refreshes: string[] }; * Upstream rejects the old bearer once, the token endpoint rotates, and the replay with the * new bearer succeeds — the reporter's deterministic harness. */ -function installHarness(options: { refresh?: () => Response } = {}): Harness { +function installHarness(options: { + refresh?: () => Response; + responseForSend?: (authorization: string, sendNumber: number, url: URL) => Response | undefined; +} = {}): Harness { const sends: string[] = []; const refreshes: string[] = []; globalThis.fetch = (async (input: RequestInfo | URL, init?: RequestInit) => { @@ -137,6 +144,8 @@ function installHarness(options: { refresh?: () => Response } = {}): Harness { } const authorization = new Headers(init?.headers).get("authorization") ?? ""; sends.push(authorization); + const customResponse = options.responseForSend?.(authorization, sends.length, url); + if (customResponse) return customResponse; if (authorization === "Bearer rejected-access") { return Response.json({ error: { message: "expired bearer" } }, { status: 401 }); } @@ -145,6 +154,26 @@ function installHarness(options: { refresh?: () => Response } = {}): Harness { return { sends, refreshes }; } +function recoveryComboConfig(): OcxConfig { + const cfg = config(); + cfg.providers.backup = { + adapter: "openai-responses", + baseUrl: "https://backup.example/v1", + authMode: "key", + apiKey: "backup-test-key", + }; + cfg.combos = { + recovery: { + strategy: "failover", + targets: [ + { provider: "openai", model: "gpt-5.5" }, + { provider: "backup", model: "m2" }, + ], + }, + }; + return cfg; +} + beforeEach(() => { home = mkdtempSync(join(tmpdir(), "ocx-responses-pool-401-")); previousOcxHome = process.env.OPENCODEX_HOME; @@ -152,6 +181,7 @@ beforeEach(() => { process.env.OPENCODEX_HOME = home; process.env.CODEX_HOME = home; clearAccountNeedsReauth(ACCOUNT_ID); + clearAccountNeedsReauth(OTHER_ACCOUNT_ID); clearCodexUpstreamHealth(); clearThreadAccountMap(); writeStoredAccount(); @@ -159,7 +189,9 @@ beforeEach(() => { afterEach(() => { globalThis.fetch = originalFetch; + clearCompactHandoffRoutesForTests(); clearAccountNeedsReauth(ACCOUNT_ID); + clearAccountNeedsReauth(OTHER_ACCOUNT_ID); clearCodexUpstreamHealth(); clearThreadAccountMap(); if (previousOcxHome === undefined) delete process.env.OPENCODEX_HOME; @@ -201,6 +233,215 @@ describe("ordinary pool 401 refresh and replay (#2887)", () => { expect(isAccountNeedsReauth(ACCOUNT_ID)).toBe(false); }); + test("Responses does not compose a stored-account replay 429 with another Pool account", async () => { + writeStoredAccount({ + [OTHER_ACCOUNT_ID]: storedRecord({ + accessToken: "other-access", + refreshToken: "other-grant", + generation: 1, + chatgptAccountId: "acc-other", + }), + }); + const harness = installHarness({ + responseForSend: authorization => { + if (authorization === "Bearer rejected-access") { + return Response.json({ error: { message: "rejected bearer" } }, { status: 401 }); + } + if (authorization === "Bearer refreshed-access") { + return Response.json({ error: { message: "pool exhausted" } }, { status: 429 }); + } + if (authorization === "Bearer other-access") { + return Response.json({ id: "must-not-run", object: "response", status: "completed", output: [] }); + } + return undefined; + }, + }); + + const cfg = config({ secondAccount: true }); + // This test needs one eligible alternate but must not advance the process-wide + // round-robin cursor used by the existing next-request affinity regression. + cfg.accountPoolStrategy = "fill-first"; + const response = await handleResponses( + request("/v1/responses"), + cfg, + { model: "", provider: "" } as RequestLogContext, + ); + + expect(response.status).toBe(429); + expect(harness.sends).toEqual(["Bearer rejected-access", "Bearer refreshed-access"]); + expect(harness.refreshes).toEqual(["refresh-grant"]); + }); + + test("a combo stops after a stored-account replay consumes the recovery budget", async () => { + const cfg = recoveryComboConfig(); + const harness = installHarness({ + responseForSend: (authorization, _sendNumber, url) => { + if (url.hostname === "backup.example") { + return Response.json({ id: "must-not-run", object: "response", status: "completed", output: [] }); + } + if (authorization === "Bearer rejected-access") { + return Response.json({ error: { message: "rejected bearer" } }, { status: 401 }); + } + if (authorization === "Bearer refreshed-access") { + return Response.json({ error: { message: "pool exhausted" } }, { status: 429 }); + } + return undefined; + }, + }); + + const response = await handleResponses( + request("/v1/responses", { model: "combo/recovery" }), + cfg, + { model: "", provider: "" } as RequestLogContext, + ); + + expect(response.status).toBe(429); + expect(harness.sends).toEqual(["Bearer rejected-access", "Bearer refreshed-access"]); + expect(harness.refreshes).toEqual(["refresh-grant"]); + }); + + test("a combo stops when the stored-account replay hits a transport error", async () => { + const cfg = recoveryComboConfig(); + const harness = installHarness({ + responseForSend: (authorization, _sendNumber, url) => { + if (url.hostname === "backup.example") { + return Response.json({ id: "must-not-run", object: "response", status: "completed", output: [] }); + } + if (authorization === "Bearer rejected-access") { + return Response.json({ error: { message: "rejected bearer" } }, { status: 401 }); + } + if (authorization === "Bearer refreshed-access") { + throw new TypeError("stored replay transport failure"); + } + return undefined; + }, + }); + + const response = await handleResponses( + request("/v1/responses", { model: "combo/recovery" }), + cfg, + { model: "", provider: "" } as RequestLogContext, + ); + + expect(response.status).toBe(502); + expect(harness.sends).toEqual(["Bearer rejected-access", "Bearer refreshed-access"]); + expect(harness.refreshes).toEqual(["refresh-grant"]); + }); + + test("a combo stops on a zero-output failure from the stored-account replay stream", async () => { + const cfg = recoveryComboConfig(); + const harness = installHarness({ + responseForSend: (authorization, _sendNumber, url) => { + if (url.hostname === "backup.example") { + return Response.json({ id: "must-not-run", object: "response", status: "completed", output: [] }); + } + if (authorization === "Bearer rejected-access") { + return Response.json({ error: { message: "rejected bearer" } }, { status: 401 }); + } + if (authorization === "Bearer refreshed-access") { + const events = [ + { type: "response.created", response: { id: "replay", status: "in_progress" } }, + { + type: "response.failed", + response: { + id: "replay", + status: "failed", + error: { type: "server_error", code: "upstream_server_error", message: "busy" }, + }, + }, + ]; + return new Response( + events.map(event => `data: ${JSON.stringify(event)}\n\n`).join(""), + { headers: { "content-type": "text/event-stream" } }, + ); + } + return undefined; + }, + }); + + const response = await handleResponses( + request("/v1/responses", { model: "combo/recovery", stream: true }), + cfg, + { model: "", provider: "" } as RequestLogContext, + ); + + expect(response.status).toBe(502); + const failure = await response.clone().json() as { + error?: { code?: string; message?: string }; + }; + expect(failure.error?.code).toBe("upstream_server_error"); + expect(failure.error?.message).toContain("busy"); + expect(harness.sends).toEqual(["Bearer rejected-access", "Bearer refreshed-access"]); + expect(harness.refreshes).toEqual(["refresh-grant"]); + }); + + test("compact does not compose a stored-account replay 429 with a remembered model", async () => { + const headers = { "x-codex-parent-thread-id": "compact-refresh-budget" }; + writeStoredAccount({ + [OTHER_ACCOUNT_ID]: storedRecord({ + accessToken: "other-access", + refreshToken: "other-grant", + generation: 1, + chatgptAccountId: "acc-other", + }), + }); + const cfg = config({ secondAccount: true }); + cfg.accountPoolStrategy = "fill-first"; + cfg.providers.seed = { + adapter: "openai-responses", + baseUrl: "https://seed.example/v1", + authMode: "key", + apiKey: "seed-test-key", + }; + const harness = installHarness({ + responseForSend: (authorization, _sendNumber, url) => { + if (url.hostname === "seed.example") { + return Response.json({ + id: "seed", + object: "response", + status: "completed", + output: [{ + type: "message", + role: "assistant", + content: [{ type: "output_text", text: "seed summary", annotations: [] }], + }], + }); + } + if (authorization === "Bearer rejected-access") { + return Response.json({ error: { message: "rejected bearer" } }, { status: 401 }); + } + if (authorization === "Bearer refreshed-access") { + return Response.json({ error: { message: "pool exhausted" } }, { status: 429 }); + } + if (authorization === "Bearer other-access") { + return Response.json({ id: "must-not-run", object: "response", status: "completed", output: [] }); + } + return undefined; + }, + }); + + const seed = await handleResponsesCompact( + request("/v1/responses/compact", { model: "seed/seed-model", headers }), + cfg, + { model: "", provider: "" } as RequestLogContext, + ); + expect(seed.status).toBe(200); + + const response = await handleResponsesCompact( + request("/v1/responses/compact", { headers }), + cfg, + { model: "", provider: "" } as RequestLogContext, + ); + + expect(response.status).toBe(429); + expect(harness.sends).toEqual([ + "Bearer seed-test-key", + "Bearer rejected-access", + "Bearer refreshed-access", + ]); + expect(harness.refreshes).toEqual(["refresh-grant"]); + }); + test("the replayed account is still selectable on the NEXT request, not just this one", async () => { // The affinity entry is bound under generation G; the forced refresh CAS-writes G+1 and // isThreadAffinityGenerationLive demands exact equality. Without the same-lineage handoff @@ -352,4 +593,293 @@ describe("ordinary pool 401 refresh and replay (#2887)", () => { expect(harness.sends).toEqual(["Bearer externally-replaced"]); expect(isAccountNeedsReauth(ACCOUNT_ID)).toBe(false); }); + + // The recovery budget bounds which ACCOUNT may be charged, not whether the request may be + // rescued at all. Two ladders send to the account that was already paying, so a stored replay + // must not cut them: the one-shot opaque-blob rebuild, and the allow-listed gated-model 400 + // retry against a still-entitled account. A blanket "no sends after the replay" rule passes + // every test above and silently converts both into a user-visible 400. + test("a stored-account replay may still rebuild a rejected opaque blob on the same account", async () => { + const harness = installHarness({ + responseForSend: (authorization, sendNumber) => { + if (authorization === "Bearer rejected-access") { + return Response.json({ error: { message: "rejected bearer" } }, { status: 401 }); + } + if (authorization !== "Bearer refreshed-access") return undefined; + // First refreshed send still carries the stale blob; upstream names the exact code. + if (sendNumber === 2) { + return Response.json({ + error: { type: "invalid_request_error", code: "invalid_encrypted_content" }, + }, { status: 400 }); + } + // The rebuild stripped it, so the same refreshed account now succeeds. + return Response.json({ id: "resp_rebuilt", object: "response", status: "completed", output: [] }); + }, + }); + + const req = new Request("http://localhost/v1/responses", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + model: "gpt-5.5", + input: [{ type: "reasoning", encrypted_content: "stale-blob", summary: [] }], + }), + }); + const response = await handleResponses( + req, + config(), + { model: "", provider: "" } as RequestLogContext, + ); + + expect(response.status).toBe(200); + // Three sends, all on the SAME account: rejected bearer, refreshed replay, rebuilt resend. + expect(harness.sends).toEqual([ + "Bearer rejected-access", + "Bearer refreshed-access", + "Bearer refreshed-access", + ]); + expect(harness.refreshes).toEqual(["refresh-grant"]); + }); + + test("a stored-account replay 429 cannot reach another account even when one is eligible", async () => { + // The mirror of the case above: a quota failure has no same-account move left, so it is + // terminal. Asserted with a healthy alternate present, so passing means the budget stopped + // it rather than there being nowhere to go. + writeStoredAccount({ + [OTHER_ACCOUNT_ID]: storedRecord({ + accessToken: "other-access", + refreshToken: "other-grant", + generation: 1, + chatgptAccountId: "acc-other", + }), + }); + const harness = installHarness({ + responseForSend: authorization => { + if (authorization === "Bearer rejected-access") { + return Response.json({ error: { message: "rejected bearer" } }, { status: 401 }); + } + if (authorization === "Bearer refreshed-access") { + return Response.json({ error: { message: "quota exhausted" } }, { status: 402 }); + } + if (authorization === "Bearer other-access") { + return Response.json({ id: "must-not-run", object: "response", status: "completed", output: [] }); + } + return undefined; + }, + }); + + const cfg = config({ secondAccount: true }); + cfg.accountPoolStrategy = "fill-first"; + const response = await handleResponses( + request("/v1/responses"), + cfg, + { model: "", provider: "" } as RequestLogContext, + ); + + expect(response.status).toBe(402); + expect(harness.sends).toEqual(["Bearer rejected-access", "Bearer refreshed-access"]); + expect(harness.refreshes).toEqual(["refresh-grant"]); + }); + + test("a gated-model 400 after a stored replay refuses an alternate account", async () => { + // The narrow seam the budget leaves open, tested on the branch where it could leak. When the + // refreshed roster no longer grants the model, retryCodexPoolOnAlternateAccount would + // ordinarily resolve a DIFFERENT account; after a stored replay it must decline instead, or + // the 400 ladder becomes a way to spend the account budget twice. + // + // This covers the REFUSAL only. The same-account rescue that the ladder still allows is a + // different branch (retryAuthCtx = firstAuthCtx, taken when the refreshed roster still grants + // the model) and is covered by the opaque-blob case above, which is the ladder this fix was + // actually reported to have broken. + writeStoredAccount({ + [OTHER_ACCOUNT_ID]: storedRecord({ + accessToken: "other-access", + refreshToken: "other-grant", + generation: 1, + chatgptAccountId: "acc-other", + }), + }); + const gatedModel = "gpt-5.6-sol"; + const harness = installHarness({ + responseForSend: authorization => { + if (authorization === "Bearer rejected-access") { + return Response.json({ error: { message: "rejected bearer" } }, { status: 401 }); + } + if (authorization === "Bearer refreshed-access") { + // Exactly the allow-listed unsupported-model detail the 400 ladder recognises. + return Response.json({ + detail: `The '${gatedModel}' model is not supported when using Codex with a ChatGPT account.`, + }, { status: 400 }); + } + if (authorization === "Bearer other-access") { + return Response.json({ id: "must-not-run", object: "response", status: "completed", output: [] }); + } + return undefined; + }, + }); + + const cfg = config({ secondAccount: true }); + cfg.accountPoolStrategy = "fill-first"; + const response = await handleResponses( + request("/v1/responses", { model: gatedModel }), + cfg, + { model: "", provider: "" } as RequestLogContext, + { + // Both accounts are entitled on the FIRST resolution, so ordinary selection still picks + // the affined work account and the stored 401 happens. From the retry resolution onward + // only the other account is entitled, which declines the same-account retry and leaves + // the alternate-account branch as the one under test. + resolveCodexModelEntitlements: (() => { + let call = 0; + return async () => { + call += 1; + const accounts = call === 1 ? [ACCOUNT_ID, OTHER_ACCOUNT_ID] : [OTHER_ACCOUNT_ID]; + return { + modelsByAccount: new Map(accounts.map(id => [id, new Set([gatedModel])])), + confirmedAccountIds: new Set(accounts), + credentialIdentities: new Map(), + }; + }; + })(), + }, + ); + + // The 400 is surfaced rather than paid for out of the other account. + expect(response.status).toBe(400); + expect(harness.sends).toEqual(["Bearer rejected-access", "Bearer refreshed-access"]); + }); + + test("a combo stops after a stored replay 4xx that is neither quota nor a gated-model 400", async () => { + // The contributor's original bound was a single `status >= 400` break in the passthrough loop, + // which stopped combo fallback for EVERY stored replay 4xx. Removing it to keep same-account + // rescue alive means the outer layers now rely on the dispatch signal instead. This pins that + // substitution on the case the break used to cover and the pool-retry site does not: a plain + // 403 is not a quota status, so it never reaches the quota bound at all. + const cfg = recoveryComboConfig(); + const harness = installHarness({ + responseForSend: (authorization, _sendNumber, url) => { + if (url.hostname === "backup.example") { + return Response.json({ id: "must-not-run", object: "response", status: "completed", output: [] }); + } + if (authorization === "Bearer rejected-access") { + return Response.json({ error: { message: "rejected bearer" } }, { status: 401 }); + } + if (authorization === "Bearer refreshed-access") { + return Response.json({ error: { message: "forbidden" } }, { status: 403 }); + } + return undefined; + }, + }); + + const response = await handleResponses( + request("/v1/responses", { model: "combo/recovery" }), + cfg, + { model: "", provider: "" } as RequestLogContext, + ); + + expect(response.status).toBe(403); + // The backup target is never sent to, and the account is charged exactly twice. + expect(harness.sends).toEqual(["Bearer rejected-access", "Bearer refreshed-access"]); + expect(harness.refreshes).toEqual(["refresh-grant"]); + }); + + test("a replay that never leaves the pacing queue does not spend the recovery budget", async () => { + // The dispatch signal is what bounds combo and policy fallback, so it has to describe a send + // that actually happened. fetchWithHeaderTimeout awaits pacing admission BEFORE calling the + // executor, so signalling at the call site would spend the budget for a replay that never + // reached the network — the request would lose its fallback for nothing. This drives the real + // path: pacing is enabled with a zero-depth queue, so the replay's admission is rejected. + // + // A plain request, deliberately NOT a combo: handleComboResponses installs its own + // onStoredPool401ReplayDispatched for the child, which replaces the caller's and would make + // the signal unobservable from here. + const cfg = config(); + // Pacing must be enabled BEFORE routing: `route.provider` is a snapshot taken at routing + // time, so enabling it mid-flight cannot affect the replay. The queue DEPTH limit, by + // contrast, is a module-level value read on every admission, which is what lets the first + // send through and rejects only the replay. + cfg.providers.openai!.requestPacing = { enabled: true, minIntervalMs: 60_000 }; + const harness = installHarness({ + responseForSend: (authorization, _sendNumber, url) => { + if (authorization === "Bearer rejected-access") { + // Close the admission queue only once the original send is through, so the rejection + // lands on the replay rather than on the request that produces the 401. + setProviderRequestPacingLimitsForTest({ maxQueueDepth: 0 }); + return Response.json({ error: { message: "rejected bearer" } }, { status: 401 }); + } + return undefined; + }, + }); + + let dispatchSignals = 0; + let response: Response; + try { + response = await handleResponses( + request("/v1/responses"), + cfg, + { model: "", provider: "" } as RequestLogContext, + { onStoredPool401ReplayDispatched: () => { dispatchSignals += 1; } }, + ); + } finally { + setProviderRequestPacingLimitsForTest({ maxQueueDepth: REQUEST_PACING_MAX_QUEUE_DEPTH }); + resetProviderRequestPacingForTest(); + } + + // The replay never reached the network, so the budget must not be reported as spent. Asserted + // on the signal itself rather than on a fallback outcome: a pacing overload is deliberately + // terminal (it propagates as an error and becomes a 429 above the combo layer), so the + // downstream fallback is unreachable here for a reason that has nothing to do with this fix. + expect(dispatchSignals).toBe(0); + expect(harness.sends.filter(send => send === "Bearer refreshed-access")).toEqual([]); + // The refresh did happen — this is the post-refresh replay being rejected, not an earlier stop. + expect(harness.refreshes).toEqual(["refresh-grant"]); + expect(response.status).toBe(429); + }); + + test("a gated-model 400 after a stored replay still retries the SAME account when entitled", async () => { + // The branch the budget deliberately leaves open, asserted directly rather than by implication + // from the opaque-blob case. When the refreshed roster still grants the model, + // retryCodexPoolOnAlternateAccount sets retryAuthCtx = firstAuthCtx and sends again to the + // account already paying — no other account is charged, so it is outside the budget. + const gatedModel = "gpt-5.6-sol"; + const harness = installHarness({ + responseForSend: (authorization, sendNumber) => { + if (authorization === "Bearer rejected-access") { + return Response.json({ error: { message: "rejected bearer" } }, { status: 401 }); + } + if (authorization !== "Bearer refreshed-access") return undefined; + if (sendNumber === 2) { + return Response.json({ + detail: `The '${gatedModel}' model is not supported when using Codex with a ChatGPT account.`, + }, { status: 400 }); + } + // Upstream shards can briefly disagree during a gated-model rollout, so the same account + // succeeds on the retry. + return Response.json({ id: "resp_same_account", object: "response", status: "completed", output: [] }); + }, + }); + + const response = await handleResponses( + request("/v1/responses", { model: gatedModel }), + config(), + { model: "", provider: "" } as RequestLogContext, + { + // The single configured account stays entitled across both resolutions. + resolveCodexModelEntitlements: async () => ({ + modelsByAccount: new Map([[ACCOUNT_ID, new Set([gatedModel])]]), + confirmedAccountIds: new Set([ACCOUNT_ID]), + credentialIdentities: new Map(), + }), + }, + ); + + expect(response.status).toBe(200); + // Three sends, every one of them on the same account. + expect(harness.sends).toEqual([ + "Bearer rejected-access", + "Bearer refreshed-access", + "Bearer refreshed-access", + ]); + expect(harness.refreshes).toEqual(["refresh-grant"]); + }); }); diff --git a/tests/routing-policy-fallback.test.ts b/tests/routing-policy-fallback.test.ts index ac2fc34f28..acfadb3744 100644 --- a/tests/routing-policy-fallback.test.ts +++ b/tests/routing-policy-fallback.test.ts @@ -179,6 +179,33 @@ describe("policy candidate fallback", () => { expect(logCtx.activeAttempt).toBe(logCtx.attempts?.[1]); }); + test("a stored Pool 401 replay dispatch stops policy candidate fallback", async () => { + const trace = policyTrace(); + const logCtx = { requestedModel: "policy/daily", routeDecision: trace, attempts: [] } as unknown as RequestLogContext; + const seenModels: string[] = []; + let replaySignals = 0; + + const response = await handleResponsesWithPolicyFallback(request(), {} as OcxConfig, logCtx, { + onStoredPool401ReplayDispatched: () => { replaySignals += 1; }, + }, { + runCore: async (req, _config, childLog, options) => { + const body = await req.json() as { model: string }; + seenModels.push(body.model); + childLog.routeDecision = trace; + seedAttempt(childLog, "provider-a", "model-a"); + options.onStoredPool401ReplayDispatched?.(); + return Response.json( + { error: { message: "stored replay exhausted", type: "rate_limit_error" } }, + { status: 429 }, + ); + }, + }); + + expect(response.status).toBe(429); + expect(seenModels).toEqual(["policy/daily"]); + expect(replaySignals).toBe(1); + }); + test("returns local pacing overload without switching policy candidates", async () => { const trace = policyTrace(); const logCtx = { requestedModel: "policy/daily", routeDecision: trace, attempts: [] } as unknown as RequestLogContext; From f39b1970353044ce730107048d8d74775e8fafe5 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sat, 29 Aug 2026 22:07:43 +0900 Subject: [PATCH 2/2] docs(devlog): record the sidecar pair alignment measurement and its dead ends The unit that produced the sidecar hint floor. Kept because two of the recorded attempts produced confident wrong answers, and the guards against them are the transferable part: - a symmetric layout break passes a relative alignment gate (the subgrid collapse measured 0.0px while cards rendered 54px instead of 215px) - a leftover injected probe stylesheet makes a measurement describe a page that is not the shipped page 010/011/012 propose mechanisms that were then disproved by measurement; 013 is what shipped. They are all kept in sequence rather than rewritten, so the reasoning that led to the one-line fix stays auditable. --- .../000_baseline_and_roadmap.md | 124 ++++++++++++++++++ .../010_sidecar_pair_alignment.md | 80 +++++++++++ .../011_audit_correction_align_content.md | 115 ++++++++++++++++ .../012_shipped_fix_and_subgrid_postmortem.md | 72 ++++++++++ ...3_final_shipped_and_measurement_lessons.md | 92 +++++++++++++ .../020_phantom_grid_track.md | 44 +++++++ .../030_dynamic_viewport_units.md | 41 ++++++ 7 files changed, 568 insertions(+) create mode 100644 devlog/_plan/260829_gui_dashboard_slop/000_baseline_and_roadmap.md create mode 100644 devlog/_plan/260829_gui_dashboard_slop/010_sidecar_pair_alignment.md create mode 100644 devlog/_plan/260829_gui_dashboard_slop/011_audit_correction_align_content.md create mode 100644 devlog/_plan/260829_gui_dashboard_slop/012_shipped_fix_and_subgrid_postmortem.md create mode 100644 devlog/_plan/260829_gui_dashboard_slop/013_final_shipped_and_measurement_lessons.md create mode 100644 devlog/_plan/260829_gui_dashboard_slop/020_phantom_grid_track.md create mode 100644 devlog/_plan/260829_gui_dashboard_slop/030_dynamic_viewport_units.md diff --git a/devlog/_plan/260829_gui_dashboard_slop/000_baseline_and_roadmap.md b/devlog/_plan/260829_gui_dashboard_slop/000_baseline_and_roadmap.md new file mode 100644 index 0000000000..0d58135639 --- /dev/null +++ b/devlog/_plan/260829_gui_dashboard_slop/000_baseline_and_roadmap.md @@ -0,0 +1,124 @@ +# 000 — Dashboard sidecar pair alignment and GUI polish + +Reported: the dashboard at `#dashboard` "슬롭이 났다" — components misaligned +horizontally and vertically, and wrong under dynamic viewports. + +This unit fixes what is *measured*, not what is asserted. Every defect below has +a numeric baseline captured with a CDP geometry harness that overrides the +viewport (`Emulation.setDeviceMetricsOverride`, dpr 2) and reads live +`getBoundingClientRect` values, so a claim can be re-checked rather than +re-argued. + +## Harness and why it is trustworthy + +Two harness bugs were found and fixed *before* any defect was accepted, because +each one silently manufactured agreement: + +1. **Viewport lag.** The first sweep measured a cell before the emulated + viewport had actually resized, so rows reported the *previous* width + (`ru/w1100` measured `vw=1440`). Fixed with a settle loop that requires + `innerWidth === target` before measuring, and the verdict tool now fails on + any unsettled cell. +2. **Locale never applied.** `Page.navigate` to a URL differing only in its + hash does not reload the document, so all four locales measured byte-identical + geometry — an invalid multi-locale claim that *looked* like passing evidence. + Fixed with an explicit `Page.reload` plus a settle condition on + `document.documentElement.lang`. The probe now records the rendered hint + length per card, and the verdict tool **fails** when every locale reports the + same signature. Post-fix signatures: `en=66,81 ko=30,41 ru=82,114 fr=88,111`. + +The second bug is the important one: without it, this unit would have "verified" +the locale dimension while never rendering a non-English string. + +## Defect 1 — the sidecar pair loses its shared control row (TOP PRIORITY) + +`.dash-sidecar-row-card` (web search) and `.dash-vision-sidecar-card` are +documented in the stylesheet as a *matched pair* whose first `Select` must land +on one line. Measured `ctrlYDelta` (vertical offset between the two control +rows): + +| locale | 1440 | 1100 | 1024 | +|--------|------|------|------| +| en | 0.1 | 0.1 | **27.4** | +| ko | 0.4 | 0.4 | **27.8** | +| ru | 0.1 | **7.1** | **22.6** | +| fr | 0.1 | **2.3** | **22.6** | + +### Root cause + +Both cards are `flex-wrap: wrap` with `align-items: center`, and the grid +stretches them to equal height. Below ~`36rem` of *card* width the container +query gives copy and controls `flex-basis: 100%`, so each card becomes two +wrapped flex lines. The two cards then have **equal outer height but different +content height** — vision's control column is taller (select row + advanced +disclosure). Flexbox distributes the leftover space of each card independently +and `align-items: center` centres each line within its own leftover, so the +shorter card's control row sinks by half the difference. Nothing ties one card's +second line to the other's. + +The shipped mitigation is a hard-coded reserved band: + +```css +.dash-sidecar-row-card .dash-sidecar-copy { min-height: 3.9375rem; } +``` + +`3.9375rem` = 63px = "21px title + 3px hint margin + two 19.5px hint lines". +That number only holds while *both* hints wrap to at most two lines. It is the +third attempt at this alignment recorded in the file — after wrapping only one +card, then `align-items: flex-start` on one card — each adding a magic number +instead of removing the cause. The ru/fr breakage at 1100px is the band failing +exactly as predicted: longer hints take a third line, overflow the band, and the +pair desynchronises at a width where English still looks fine. + +### Fix direction + +Align the rows *structurally* so no number has to be maintained: the pair shares +one row grid, and each card's copy row and control row are placed into shared +tracks. Then alignment holds for any hint length in any locale, and the band can +be deleted rather than re-tuned. The existing comment correctly warns that +`container-type` layout containment blocks `subgrid` from reading parent +tracks, so the container query must not sit on a subgrid participant. + +## Defect 2 — phantom zero-width grid track + +At vw ≥ 1440, `.dash-sidecar-grid` and `.dash-overview-tools` compute +`grid-template-columns: 555px 555px 0px`. `repeat(auto-fit, minmax(min(100%, 21rem), 1fr))` +emits a third, zero-width track. Trailing gap measures 0 today, so nothing +visibly shifts — but the track is real, and it becomes a phantom gap the moment a +third card is added to either grid. + +## Defect 3 — static viewport units in scroll surfaces + +`gui/src/styles.css:2003` caps `.logs-table-wrap` with +`max-height: calc(100vh - 260px)`. Static `vh` resolves against the *large* +viewport, ignoring mobile browser chrome, while the rest of the shell already +uses `100dvh` (styles.css:244, 247, 411, 412, 2198). The log table is therefore +sized for a viewport the user cannot see. `styles.css:755` and `1222` cap toast +width with `calc(100vw - Npx)`, which ignores classic scrollbar width. + +The probe measures this behaviourally — comparing each scroll container's +computed cap against `visualViewport.height` — rather than grepping for the +unit, so the assertion survives a refactor. + +## Work phases + +| phase | doc | deliverable | +|-------|-----|-------------| +| wp0 | this unit | measured baseline + roadmap | +| wp1 | `010` | sidecar pair structural alignment (top priority) | +| wp2 | `020` | phantom auto-fit track | +| wp3 | `030` | dynamic viewport units | + +Acceptance for every implementation phase: `ctrlYDelta ≤ 1px` and +`heightDelta ≤ 1px` while paired, no horizontal overflow, no zero-width track, +no scroll cap exceeding the visual viewport, across +`1440/1100/1024/900/430` × `en/ko/ru/fr`, with locale signatures proven +distinct. + +## Constraints + +- The local suite is not run here (user instruction). Gates run remotely via + `ssh lidge` + `ocx-run`; pushes use `--no-verify` only after those gates. +- Delivery is a stacked PR chain onto `dev`, each PR carrying screenshots + (`enforce-target` requires a screenshot for GUI PRs). + diff --git a/devlog/_plan/260829_gui_dashboard_slop/010_sidecar_pair_alignment.md b/devlog/_plan/260829_gui_dashboard_slop/010_sidecar_pair_alignment.md new file mode 100644 index 0000000000..437ffbbef5 --- /dev/null +++ b/devlog/_plan/260829_gui_dashboard_slop/010_sidecar_pair_alignment.md @@ -0,0 +1,80 @@ +# 010 — Sidecar pair: structural row alignment (wp1, TOP PRIORITY) + +Removes the magic reserved band and makes the two sidecar cards share real row +tracks, so their control rows align for any hint length in any locale. + +## Current shape + +``` +.dash-sidecar-grid grid, auto-fit 2 tracks, align-items: stretch + └─ .panel.dash-delegation-summary.dash-sidecar-row-card + ├─ .dash-sidecar-copy (title + hint) + └─ .dash-delegation-controls (selects / switch / disclosure) +``` + +Each card is its own flex container (`flex-wrap: wrap`, `align-items: center`) +and, below `36rem` of card width, both children take `flex-basis: 100%` — two +wrapped lines whose position depends only on that card's own leftover space. + +## Change + +Make each card a two-row grid and let both cards inherit the *same* two rows from +the pair grid: + +1. `.dash-sidecar-grid` gains `grid-template-rows: auto auto` so there are named + parent rows to inherit. +2. `.dash-sidecar-row-card` becomes `display: grid` with + `grid-template-rows: subgrid` spanning both rows, so copy lands in row 1 and + controls in row 2 **in both cards**. Row 1 is sized by the taller of the two + copy blocks, automatically — which is exactly what the 63px band was + hand-computing. +3. Delete `.dash-sidecar-row-card .dash-sidecar-copy { min-height: 3.9375rem }` + and the `min-height: 3.6875rem` band on `.dash-delegation-controls`. They are + the numbers being replaced. +4. Move `container-type: inline-size` **off** the subgrid participant. Layout + containment blocks a subgrid from reading parent tracks (the stylesheet already + warns about this). The container is re-established on a wrapper so the existing + `@container sidecar-card` rules keep working unchanged. + +## Wrapper + +Subgrid requires the card to be a grid *item* of the pair grid, but the card also +has to be the container query root's child. Structure becomes: + +``` +.dash-sidecar-grid (grid, 2 rows) + └─ .dash-sidecar-cell (container-type: inline-size, display: grid, rows: subgrid, span 2) + └─ .dash-sidecar-row-card (display: grid, rows: subgrid, span 2) +``` + +The cell carries the container query; the card carries the visible panel styling. +Both pass the rows through, so row 1 and row 2 are shared across the pair. + +Requires one JSX change in `dashboard-overview-sections.tsx`: wrap each of the +two existing card `div`s in `
`. + +## Stacked state + +When the container query stacks a card (card narrower than `22rem`), the two +cards are in *different* grid columns of a single-column grid — i.e. different +rows of the pair — so cross-card alignment is meaningless and must not be +asserted. The verdict tool already treats `sameRow: false` as `STACKED` and +skips the delta check. + +## Fallback + +`grid-template-rows: subgrid` is supported in Chrome 117+, Safari 16+, Firefox +71+. Guard with `@supports (grid-template-rows: subgrid)`; without support the +cards keep the current flex row behaviour, which is the shipped status quo rather +than a regression. The deleted bands are restored inside the negative branch so +unsupported browsers keep today's approximation. + +## Acceptance + +- `ctrlYDelta ≤ 1px` and `heightDelta ≤ 1px` at every PAIRED cell across + `1440/1100/1024` × `en/ko/ru/fr` (baseline: up to 27.8px). +- No `min-height` band remains on `.dash-sidecar-copy`. +- Locale hint signatures distinct, no unsettled cells. +- A focused GUI test asserts the subgrid contract so a future edit that reverts + to the band fails. + diff --git a/devlog/_plan/260829_gui_dashboard_slop/011_audit_correction_align_content.md b/devlog/_plan/260829_gui_dashboard_slop/011_audit_correction_align_content.md new file mode 100644 index 0000000000..198ff217da --- /dev/null +++ b/devlog/_plan/260829_gui_dashboard_slop/011_audit_correction_align_content.md @@ -0,0 +1,115 @@ +# 011 — Audit correction: subgrid was the wrong fix + +The `010` plan was written before the regime sweep existed. The sweep contradicts +its central assumption, so `010` is superseded by this document. Recorded rather +than silently edited, because the wrong assumption is the interesting part. + +## What `010` assumed + +That the cards render as a horizontal row at desktop width (copy LEFT, controls +RIGHT) and only stack when narrow — so a two-row subgrid was needed to align the +"second line" of each card. + +## What the measurement shows + +Per-card internal layout, at full reload per width, sidebar state recorded: + +| vw | mainInner | cardW | columns | inside the card | ctrlYDelta | +|------|-----------|-------|---------|-----------------|------------| +| 1600 | 1128 | 556 | 2 | STACKED | 0 | +| 1440 | 1126 | 555 | 2 | STACKED | 0 | +| 1280 | 966 | 475 | 2 | STACKED | 0 | +| 1100 | 786 | 385 | 2 | STACKED | -2.3 | +| 1024 | 782 | 347 | 2 | STACKED | **22.5** | +| 1010 | 768 | 340 | 2 | STACKED | **22.5** | +| 1000 | 758 | 686 | 1 | ROW | n/a (single column) | +| 992 | 750 | 678 | 1 | ROW | n/a | +| 980 | 738 | 666 | 1 | STACKED | n/a | +| 768 | 526 | 454 | 1 | STACKED | n/a | +| **760** | **750** | **349** | **2** | STACKED | **22.5** | +| 740 | 730 | 339 | 2 | STACKED | **22.5** | +| 720 | 710 | 674 | 1 | ROW | n/a | + +Two facts kill `010`: + +1. **The cards are ALREADY stacked internally at every two-column width.** The + `36rem` container query fires whenever the pair is side by side, because a + two-up card is at most ~556px = 34.75rem < 36rem. Copy and controls are already + on separate lines; there is no row to preserve and nothing for a two-row + subgrid to add. The horizontal row only appears when the grid collapses to ONE + column (cardW ≈ 674-686px > 36rem), and in that state the cards are stacked + vertically as a pair, so cross-card alignment is meaningless. +2. **A JSX wrapper would have been added for nothing**, and moving + `container-type` off the card would have silently killed the existing + `@container sidecar-card` rules — the exact "reads correct in review but does + nothing" failure the stylesheet already warns about. + +## The real cause of the 22.5px offset + +Both cards are stretched to equal height by the grid, and each is +`flex-wrap: wrap` + `align-items: center`. Two wrapped lines, equal outer +height, **different content height** (vision's control column is taller: select +row + 12px gap + the advanced disclosure). Flexbox gives each card its own +leftover space, and `align-items: center` centres each line inside its own +leftover. The card with less content has more leftover, so its control row sinks +by roughly half the difference. Nothing couples the two cards. + +The 63px copy band mitigates this only while both hints wrap to the same number of +lines. At `ru`/`fr`, the vision hint takes a third line at 1100px, which is why +ru/fr break at a width where en/ko still measure clean. + +## The fix + +Pack the wrapped lines from the top of each card instead of centring them in +leftover space: + +```css +.dash-sidecar-row-card { align-content: start; } +``` + +`align-content` is the correct property for a **multi-line** flex container — it +distributes the *lines*, which is exactly what is misdistributed here. +`align-items` (already `center` from `.dash-delegation-summary`) aligns items +*within* a line and must stay, so the single-line desktop row keeps its vertical +centring. + +The stylesheet notes that `align-content` "has no effect on one line" — true, and +it is why `align-content` alone was rejected for the *control group*. But the +target here is the CARD, which genuinely has two lines in exactly the regime that +misaligns. In the one-column regime the card is a single line, where +`align-content: start` is inert and the row is unaffected. That is the property +doing precisely one job in precisely one regime. + +With lines packed from the top, both control rows sit at +`padding-top + copyRowHeight`. Equal copy row height across the pair is then the +only remaining requirement, and it is what the `min-height` band already +provides — but now the band only needs to cover the *tallest actual* copy, and +alignment no longer depends on the two hints matching. The band is therefore +replaced by a locale-proof mechanism: the copy row's height is equalised by the +same `align-content` packing plus a shared floor expressed in line units +(`3lh`), not a pixel count derived from one locale's wrap count. + +## Consequences for the existing test + +`gui/tests/sidecar-layout.test.ts` currently asserts the magic band *as the +contract*: + +- "both cards reserve the same copy band" requires `min-height >= 3.9rem` on the + copy block; +- "both control groups reserve the same band and pack from its top" requires + `min-height` and `align-items: flex-start` on the control group. + +Those assertions encode the mitigation, not the requirement, so they must be +rewritten to assert the *cause* being removed (lines pack from the start; no +pixel-derived band is load-bearing). This is the file's stated purpose — "make the +specific CSS shape that caused the bug impossible to reintroduce" — applied to the +actual cause. + +## Additional defect found by the sweep (new) + +**The 760px two-column regression.** At `max-width: 760px` the sidebar leaves the +flow (`position: fixed`, off-canvas at `x=-280`), so `.main-inner` JUMPS from +526px to 750px. The sidecar grid re-splits into two columns at 349px each and the +22.5px misalignment returns — on tablet widths, below the width where it was last +believed fixed. Any fix must be verified at 760/740, not only at desktop widths. + diff --git a/devlog/_plan/260829_gui_dashboard_slop/012_shipped_fix_and_subgrid_postmortem.md b/devlog/_plan/260829_gui_dashboard_slop/012_shipped_fix_and_subgrid_postmortem.md new file mode 100644 index 0000000000..19db7c2910 --- /dev/null +++ b/devlog/_plan/260829_gui_dashboard_slop/012_shipped_fix_and_subgrid_postmortem.md @@ -0,0 +1,72 @@ +# 012 — What actually shipped, and why subgrid could not + +`010` proposed subgrid; `011` corrected its premise but still recommended shared +row tracks. Both were wrong about the mechanism. This is the record of what the +experiments showed, kept because the failed attempts are the reason the shipped +fix is one line. + +## The shipped fix + +```css +.dash-sidecar-row-card { align-content: start; } +``` + +Measured: worst paired offset **0.0px** (was 27.8px) across `en/ko/ru/fr/ja/zh/de/tr` +at 1024 and 1100, plus the regime boundaries 1600/1440/1010/760/1000/992/430 on +the two longest-hint locales. Card heights and the one-column horizontal row are +unchanged. + +## Why the copy band was never the cause + +The band (`min-height: 3.9375rem`) looked like the culprit and the plan called for +deleting it. The decisive experiment says otherwise: with the band in place and +`align-content` still at its default, the two copy blocks measured **equal** +(63/63) while the control rows were still **27.4px apart**. Equal copy height is +therefore necessary but not sufficient — the mis-distributed thing is the wrapped +**lines**, not the copy. + +So the band stays. It is load-bearing, just for a different reason than its +comment claimed: it equalises the copy row that `align-content: start` then packs +against. Deleting it would have re-broken the pair while the new rule kept +measuring 0.0px at the locales that happen to wrap identically. + +## Why subgrid is unavailable here + +Shared row tracks are the textbook fix, and the independent auditor recommended +them. They cannot work in this tree: + +| attempt | result | +|---------|--------| +| card as subgrid, `container-type` on the card | never applied; computed `display` stayed `flex` | +| `container-type` moved to `.dash-sidecar-grid` | card's computed `grid-template-rows` = `none`; rows collapsed to 19px; cards 54px tall; controls overflowing 43-80px | +| `container-type` on `.dash-overview-stack` | same collapse | +| `min-content` / `max-content` / `auto` row sizing | no effect; the rejection is of `subgrid` itself, not the track sizing | +| isolated clone with no container ancestor | worked perfectly, delta 0 — which is what identified containment as the cause | + +Chrome rejects a child's `grid-template-rows: subgrid` when an ancestor +establishes layout containment via `container-type: inline-size`. This surface has +two such containers (`.dash-sidecar-grid` and the per-card `sidecar-card` used by +the existing narrow-card queries), so there is no position for the container that +does not also block the subgrid. Removing the queries to make room would trade a +27px offset for the wrong-axis bug they were introduced to fix. + +## The measurement lesson + +The subgrid collapse **passed the alignment gate**: `ctrlYDelta` read 0.0px while +cards rendered 54px instead of 215px, because both cards were broken *identically*. +A relative metric cannot see a symmetric failure. The gate now also asserts +absolute card height and that no child overflows its panel, which is what caught +it. + +## Deferred, per the audit + +Auditor blockers 6 and 7 are accepted and remove work from `020`/`030` rather than +adding it: + +- The `0px` third track is **normal** `auto-fit` behaviour for a collapsed empty + track, not a defect. Replacing `auto-fit` with a fixed two-up would change + future three-card behaviour for no present gain. `020` is withdrawn. +- `dvw` does not subtract a classic scrollbar, so the toast fix must use + containing-block insets / `max-inline-size: 100%`, not a unit swap. Only the + `.logs-table-wrap` `vh` → `dvh` change survives from `030`. + diff --git a/devlog/_plan/260829_gui_dashboard_slop/013_final_shipped_and_measurement_lessons.md b/devlog/_plan/260829_gui_dashboard_slop/013_final_shipped_and_measurement_lessons.md new file mode 100644 index 0000000000..c00bfcaa13 --- /dev/null +++ b/devlog/_plan/260829_gui_dashboard_slop/013_final_shipped_and_measurement_lessons.md @@ -0,0 +1,92 @@ +# 013 — Final: what shipped and what the measurement taught + +Supersedes the mechanism proposed in `010`/`011`/`012`. Those documents are kept +because the failed attempts are why the shipped fix is one declaration. + +## Shipped + +```css +.dash-sidecar-row-card .dash-sidecar-copy .setting-hint { min-height: 3lh; } +``` + +replacing `min-height: 3.9375rem` on the copy block. + +## The defect, stated exactly + +The pair's control rows sat **19.5px** apart — one line box — at every two-up width +from 1600 down to 740, in **ru** and **fr** only. Six other locales measured 0px. + +The old band was 63px, documented as "21px title + 3px hint margin + two 19.5px +hint lines". It encodes a two-line assumption. The ru/fr vision hint wraps to +**three** lines at a two-up card (82.5px of copy against 63px), so the band stopped +describing the taller card and each card's control line followed its own copy. + +`3lh` states the real constraint — reserve three line boxes of the hint's own +line-height — so the shorter hint reserves the same three lines, and a font or +line-height change cannot invalidate the number. + +| locale | hint lines (web search / vision) | before | after | +|--------|----------------------------------|--------|-------| +| en | 2 / 2 | 0px | 0px | +| ko | 1 / 2 | 0px | 0px | +| ja | 2 / 2 | 0px | 0px | +| zh | 1 / 1 | 0px | 0px | +| de | 2 / 2 | 0px | 0px | +| tr | 2 / 2 | 0px | 0px | +| **ru** | **2 / 3** | **19.5px** | **0px** | +| **fr** | **2 / 3** | **19.5px** | **0px** | + +## Why not subgrid + +Shared row tracks are the textbook fix and the independent auditor recommended +them. They are unavailable here, and the evidence is unambiguous: + +| attempt | measured result | +|---------|-----------------| +| card as subgrid, `container-type` on the card | never applied; computed `display` stayed `flex` | +| `container-type` moved to `.dash-sidecar-grid` | card's computed `grid-template-rows` = `none`; tracks 19px; cards 54px tall; controls overflowing 43-80px | +| `container-type` on `.dash-overview-stack` | same collapse | +| `auto` / `min-content` / `max-content` rows | no effect — the rejection is of `subgrid`, not the sizing | +| isolated clone, no container ancestor | worked, delta 0 — which is what identified containment as the cause | + +Chrome rejects a child's `grid-template-rows: subgrid` when an ancestor +establishes layout containment via `container-type`. This surface has two such +containers (`.dash-sidecar-grid` and the per-card `sidecar-card` that the existing +narrow-card queries depend on), so there is no placement that does not block it. + +## Two measurement failures worth keeping + +Both produced confident, wrong "all clear" results. The harness now defends +against each. + +**1. A symmetric break passes a relative gate.** The subgrid collapse reported +`ctrlYDelta = 0.0px` while cards rendered 54px instead of 215px, because both +cards were broken identically. Alignment deltas cannot see that. The gate now also +asserts absolute card height, child-vs-panel overflow, and hint truncation. + +**2. A leftover probe stylesheet fakes a pass.** An earlier round reported "ALL +OK" for `align-content: start` across 30 cells. The number was real; the page was +not the shipped page — an injected experiment sheet from a previous probe was still +attached. The harness now strips every probe sheet before measuring, counts what +remains, and **fails** if the count is not what the run expects. + +The second one is why `align-content: start` was briefly committed as the fix. It +is not in the shipped diff: re-measured on a clean page it leaves the full 19.5px, +because packing lines from the top does nothing when the copy rows themselves are +unequal. + +## Deferred, per the audit + +- `020` **withdrawn.** The `0px` third track is normal `auto-fit` behaviour for a + collapsed empty track, not a defect. Replacing `auto-fit` with a fixed two-up + would change future three-card behaviour for no present gain. +- `030` **reduced.** `dvw` does not subtract a classic scrollbar, so the toast fix + must use containing-block insets / `max-inline-size`, not a unit swap. Only + `.logs-table-wrap`'s `vh` → `dvh` survives. + +## Evidence + +- Harness: `.tmp/uiux/measure.ts` (scratch, not committed) +- Screenshots with control-row guides: before `-19.5px` / after `0px` at ru and fr, 1024 +- Regression: `gui/tests/sidecar-layout.test.ts`, red on the previous CSS (2 fail), green on this one (8 pass) + diff --git a/devlog/_plan/260829_gui_dashboard_slop/020_phantom_grid_track.md b/devlog/_plan/260829_gui_dashboard_slop/020_phantom_grid_track.md new file mode 100644 index 0000000000..dc05722662 --- /dev/null +++ b/devlog/_plan/260829_gui_dashboard_slop/020_phantom_grid_track.md @@ -0,0 +1,44 @@ +# 020 — Phantom zero-width auto-fit track (wp2) + +## Defect + +At vw ≥ 1440 both dashboard grids compute a third, zero-width column: + +``` +grid-template-columns: 555px 555px 0px +``` + +from `repeat(auto-fit, minmax(min(100%, 21rem), 1fr))`. + +`auto-fit` collapses empty tracks but still *generates* one here because +`min(100%, 21rem)` lets the hypothetical third track floor at 0 once the +container is wide enough to nominally fit it. With only two children the track +collapses to 0 and the trailing gap measures 0, so nothing shifts today. It +becomes a real phantom gap the moment a third card is added. + +## Change + +Both grids hold a *known* number of cards, so express that instead of asking +`auto-fit` to guess: + +```css +grid-template-columns: repeat(auto-fit, minmax(min(100%, 21rem), 1fr)); +``` + +becomes an explicit two-up that collapses to one column by container width: + +```css +grid-template-columns: 1fr; /* narrow: stack */ +@container / min-width: two-up → 1fr 1fr /* wide: matched pair */ +``` + +Applies to `.dash-sidecar-grid` and `.dash-overview-tools`. The wrap width stays +`21rem` per card so the responsive behaviour is unchanged — verified by the same +sweep, which must keep reporting STACKED at 900/430 and PAIRED at 1024+. + +## Acceptance + +- No `0px` track in either grid's computed columns at any swept width. +- The PAIRED/STACKED pattern per width matches the baseline exactly (no + behavioural change, only the phantom track removed). + diff --git a/devlog/_plan/260829_gui_dashboard_slop/030_dynamic_viewport_units.md b/devlog/_plan/260829_gui_dashboard_slop/030_dynamic_viewport_units.md new file mode 100644 index 0000000000..eb5870bc11 --- /dev/null +++ b/devlog/_plan/260829_gui_dashboard_slop/030_dynamic_viewport_units.md @@ -0,0 +1,41 @@ +# 030 — Dynamic viewport units in scroll surfaces (wp3) + +## Defect + +`gui/src/styles.css:2003`: + +```css +.logs-table-wrap { max-height: calc(100vh - 260px); } +``` + +`vh` is the *large* viewport: it ignores mobile browser chrome, so the log table +is capped for a viewport taller than the one the user can see, pushing the last +rows under the browser UI. The rest of the shell already moved to `100dvh` +(styles.css:244, 247, 411, 412, 2198), so this line is an outlier, not a +convention. + +`styles.css:755` and `1222` cap toast width with `calc(100vw - Npx)`. `100vw` +excludes a classic scrollbar's width, so on a scrollbar-reserving platform the +toast can exceed the visible area. + +## Change + +- `.logs-table-wrap` → `max-height: calc(100dvh - 260px)`. +- Toast caps → `min(, calc(100dvw - Npx))`, keeping each existing pixel + inset. +- `styles.css:2003` is the only static `vh` in a scroll surface; the `12vh` + padding on the toast wrapper is decorative offset, not a size cap, and stays. + +## Verification + +Behavioural, not textual: the probe compares each scroll container's computed +`max-height` against `visualViewport.height` and counts any cap that exceeds it +(`staticVh`). The gate fails on a non-zero count, so the assertion survives a +selector rename. Measured at a mobile profile where the visual viewport is +smaller than the large viewport. + +## Acceptance + +- `staticVh = 0` at every swept cell, including the 430-wide mobile profile. +- No `calc(100vh` remaining in a scroll-surface cap. +