From 2e92c8913618aae2f1cca0bc7dfd683ceeb9f568 Mon Sep 17 00:00:00 2001 From: JUN Date: Sun, 30 Aug 2026 05:40:39 +0900 Subject: [PATCH 01/65] test(catalog): pin Copilot vision precedence and nested/flat denial equivalence (#2944) #2943 fixed the nested read; this covers the cases its tests do not, and adds the comment explaining why the precedence is shaped the way it is. The important one is flat-versus-nested disagreement. I had implemented this as deny-wins across both levels in a competing patch, which would have flipped a provider reporting flat vision:true with nested supports.vision:false from image-capable to text-only -- changing behaviour that predates Copilot support, in a parser shared by every provider, from a change whose whole purpose was to stop models being wrongly marked text-only. #2943 resolves by specificity instead: a flat boolean is authoritative when present, nested is consulted only otherwise. Reintroducing deny-wins turns the new test red. An independent review of my withdrawn head raised a second question: the nested read is not scoped to github-copilot, and a nested denial beats a loose "vision" string in a capability array. Probing the merged code shows the nested and flat denials behave identically there, including the contradictory hint pair (inputModalities: ["text"] alongside capabilities: ["vision"]) that flat false has always produced. That equivalence is the property worth pinning: the nested field must mean exactly what the field it stands in for means, or the unscoped read becomes a subtle divergence. Reconciling a boolean denial with a capability list is a separate question and is not answered under a Copilot ticket. Also covered: the reporter's full payload including the limits.vision sibling that holds an image count and would fool anything searching capabilities for a vision-ish key; a non-record supports container falling through so a features: ["vision"] signal still decides; and explicit item input_modalities still outranking a nested claim. Devlog corrected on two overclaims the review was right about: "deeper than any other provider" is only "deeper than the flat form, and no checked-in fixture uses it", and parsed upstream capability is the best per-model evidence rather than authoritative when two forms disagree. No production behaviour change -- the resolution is #2943's implementation. --- .../000_units.md | 70 +++++++++++++++++++ src/codex/catalog/provider-fetch.ts | 10 +++ .../provider-model-discovery-contract.test.ts | 68 ++++++++++++++++++ 3 files changed, 148 insertions(+) create mode 100644 devlog/_plan/260830_lane_r_2941_copilot_vision/000_units.md diff --git a/devlog/_plan/260830_lane_r_2941_copilot_vision/000_units.md b/devlog/_plan/260830_lane_r_2941_copilot_vision/000_units.md new file mode 100644 index 0000000000..ac50aef2e3 --- /dev/null +++ b/devlog/_plan/260830_lane_r_2941_copilot_vision/000_units.md @@ -0,0 +1,70 @@ +# Lane R / #2941 — Copilot vision models are cataloged text-only + +## The report + +Every `github-copilot` model arrives in the catalog with `inputModalities: ["text"]`, so Codex refuses image attachments with "This model does not support image inputs" on 32+ models that do accept them (Claude Opus/Sonnet, GPT-4o/4.1/5.x, Gemini, Grok). The same model reached through `openrouter` accepts images, which is what makes this clearly a metadata defect rather than an upstream limitation. + +## Why every path returns nothing + +Copilot's `/models` endpoint nests the flag one level deeper than the flat form the parser reads. No other checked-in fixture uses this shape, which is all a repository search can establish — it says nothing about what other live catalogs return: + +```json +{ + "id": "claude-opus-4.6", + "capabilities": { + "supports": { "vision": true }, + "limits": { "vision": { "max_prompt_images": 20 } } + } +} +``` + +`modelInputModalities()` in `src/codex/catalog/provider-fetch.ts` tries three signals and all three miss: + +- `item.input_modalities` / `item.modalities` — Copilot emits neither. +- `capabilityRecord?.vision` — `capabilityRecord` is the `capabilities` object itself, so its keys are `supports` and `limits`. `.vision` is `undefined`. +- the `capabilities` string array — `supports` is an object, not the literal `true` that scan looks for. + +The fallback chain then lands on `["text"]`. + +Note the shape carries a second `vision` key under `limits`. Any fix that searches loosely for "a vision key somewhere in capabilities" would find `limits.vision`, which is an object describing image count — truthy, and meaningless as a capability signal. + +## Outcome: #2943 shipped the fix, this unit adds the missing coverage + +@Ingwannu opened #2943 for the same defect 17 minutes before my #2944. Their implementation landed as `370052648`, and it is better than mine on one case that matters — see the precedence section below. My unit reduced to the tests and the explanatory comment. + +## Fix + +Read the nested boolean, positioned after the explicit-modality and architecture signals, with precedence **by specificity**: a flat `capabilities.vision` boolean is authoritative whenever present, the nested `supports.vision` boolean is consulted only otherwise, and a non-boolean at either level decides nothing so the remaining signals still apply. + +**The read is not scoped to `github-copilot`, and that is deliberate rather than overlooked.** `modelInputModalities` never receives a provider name, and the nested field is the same kind of evidence wherever it appears — a boolean statement about one model. Scoping it would mean a provider reporting the identical shape gets a worse answer for no reason. What the choice does mean is that any provider emitting `capabilities.supports.vision` as a boolean now has it honoured, so the nested field must behave **exactly** like the flat field it stands in for. That equivalence is pinned by a test comparing both shapes against the same loose capability-array claim. + +Strictness matters in both directions. A truthy test would let the string `"yes"` advertise image support, and coercing a non-record `supports` into a denial would suppress a `features: ["vision"]` signal that is still valid. + +## The precedence mistake, recorded because it nearly shipped + +I first wrote the denial as `flat === false || nested === false` — deny-wins across both sources. It reads as the safe direction and is not. + +On a provider reporting flat `vision: true` with nested `supports: { vision: false }`, deny-wins returns `["text"]` where the old code returned `["text", "image"]`. That is a silent behaviour change in a parser shared by **every** provider, shipped by a patch whose entire purpose was to stop models being wrongly marked text-only. Eleven of twelve capability shapes agree between the two resolutions; that one does not, and it is the one that would have caused a regression. + +A differential probe over both resolutions is what surfaced it — not review, and not green tests, since neither suite covered a disagreeing pair. The case is now pinned: reintroducing deny-wins turns `a flat vision boolean outranks a disagreeing nested one` red. + +## Rejected: a registry seed + +The issue offers "just add `modelInputModalities` to the `github-copilot` registry entry" as the easier route. + +An audit pushed back on my first reason for rejecting it, correctly. `modelInputModalities` is a **per-model** map, not a provider-wide boolean, and other providers do seed selectively — xAI lists specific verified vision ids. So "Copilot serves both vision and text-only models" does not by itself rule out a seed, and a selective one would even help during first start or degraded `/models` discovery, since the registry model list is explicitly a cold-start fallback. + +The real reason to omit it here is narrower: **a seed is only as good as the audited list behind it, and no verified model-by-model Copilot vision list exists.** Writing one from the 32 models named in the issue would be guesswork duplicating a remote catalog that changes without us. A selective seed remains a legitimate follow-up for whoever can audit the list. + +One qualifier on "parse what upstream reports": it is the best per-model evidence available, not an oracle. When two upstream forms disagree the output can still be internally contradictory — a model can end up with `inputModalities: ["text"]` next to `capabilities: ["vision"]`. That contradiction predates this work (flat `false` has always beaten a capability-array `"vision"` string) and is left alone here rather than fixed silently under a Copilot ticket. Resolving how a boolean denial and a loose capability list should reconcile is its own unit. + +## Tests + +`tests/provider-model-discovery-contract.test.ts`, using the reporter's exact payload including the `limits.vision` sibling. + +| Mutation | Expected | +|---|---| +| remove the nested read entirely (pre-fix state) | tri-state test red — reproduces the report | +| drop `nestedVision === false` | tri-state test red | +| `Boolean(nestedVision)` instead of `=== true` | malformed-hint test red | +| move the nested read above the explicit-modality return | precedence test red | diff --git a/src/codex/catalog/provider-fetch.ts b/src/codex/catalog/provider-fetch.ts index f3113dfe17..0ebb2971c4 100644 --- a/src/codex/catalog/provider-fetch.ts +++ b/src/codex/catalog/provider-fetch.ts @@ -1094,6 +1094,16 @@ function modelInputModalities( .filter(value => value === "text" || value === "image" || value === "audio"); if (inferred.length > 0) return [...new Set(inferred)]; } + // GitHub Copilot nests vision support one level down as `capabilities.supports.vision`, so the + // flat read alone finds nothing and every Copilot model falls through to `["text"]` — Codex then + // refuses image attachments on models that accept them (#2941). Precedence is by specificity: + // a flat boolean is authoritative when present, the nested boolean is consulted only otherwise, + // and a non-boolean at either level decides NOTHING so the signals below still apply. Two things + // this ordering deliberately avoids: a deny-wins rule across both levels would flip a provider + // reporting flat `true` with nested `false` from image-capable to text-only, changing behaviour + // that predates Copilot support; and a truthy test would let the string `"no"` advertise image + // input. The payload also carries a SECOND `vision` key under `limits` holding an image count, + // which is why this reads one exact path instead of searching `capabilities` for a vision-ish key. const nestedSupports = plainRecord(capabilityRecord?.supports); const explicitVisionSupport = typeof capabilityRecord?.vision === "boolean" ? capabilityRecord.vision diff --git a/tests/provider-model-discovery-contract.test.ts b/tests/provider-model-discovery-contract.test.ts index 2fe52ff27f..f4a088b730 100644 --- a/tests/provider-model-discovery-contract.test.ts +++ b/tests/provider-model-discovery-contract.test.ts @@ -293,6 +293,74 @@ describe("registry-owned provider model discovery", () => { })).toEqual({}); }); + test("a flat vision boolean outranks a disagreeing nested one (#2941)", () => { + // Precedence is by specificity, NOT deny-wins. A deny-wins rule across both levels would flip + // this shape from image-capable to text-only, silently changing behaviour that predates Copilot + // support — the flat `true` alone already meant image input. Pinned so it cannot regress. + expect(catalogHintsFromModelsApiItem("example", { + id: "flat-true-nested-false", + capabilities: { vision: true, supports: { vision: false } }, + })).toEqual({ inputModalities: ["text", "image"], capabilities: ["vision"] }); + + // The mirror image: a flat denial is authoritative over a nested claim. + expect(catalogHintsFromModelsApiItem("example", { + id: "flat-false-nested-true", + capabilities: { vision: false, supports: { vision: true } }, + })).toEqual({ inputModalities: ["text"] }); + }); + + test("the reporter's full Copilot payload is read despite the limits.vision sibling (#2941)", () => { + // `capabilities` carries a SECOND vision key under `limits` holding an image count. Anything + // that searched loosely for a vision-ish key would find that object and treat it as a signal. + expect(catalogHintsFromModelsApiItem("github-copilot", { + id: "claude-opus-4.6", + capabilities: { + supports: { vision: true }, + limits: { vision: { max_prompt_images: 20 } }, + }, + })).toEqual({ inputModalities: ["text", "image"] }); + }); + + test("an explicit nested denial outranks a loose capability-array claim, exactly as a flat one does (#2941)", () => { + // A boolean capability field is a specific statement; a "vision" string in a capability array is + // a loose one. Flat `false` has always won that contest, and the internal contradiction it + // produces -- text-only modalities reported next to capabilities: ["vision"] -- predates the + // nested read. These two shapes must agree, or the nested field would mean something subtly + // different from the flat field it stands in for. + const nested = catalogHintsFromModelsApiItem("example", { + id: "nested-denial-vs-array", + metadata: { capabilities: { supports: { vision: false } } }, + capabilities: ["vision"], + }); + const flat = catalogHintsFromModelsApiItem("example", { + id: "flat-denial-vs-array", + metadata: { capabilities: { vision: false } }, + capabilities: ["vision"], + }); + expect(nested).toEqual({ inputModalities: ["text"], capabilities: ["vision"] }); + expect(nested).toEqual(flat); + }); + + test("a non-record supports container decides nothing and leaves the fallback chain intact (#2941)", () => { + // It must not collapse into a denial either — the `features` signal further down still decides. + expect(catalogHintsFromModelsApiItem("github-copilot", { + id: "malformed-container", + capabilities: { supports: 5 }, + features: ["vision"], + })).toEqual({ + inputModalities: ["text", "image"], + capabilities: ["vision"], + }); + }); + + test("explicit item input modalities still outrank a nested Copilot vision claim (#2941)", () => { + expect(catalogHintsFromModelsApiItem("github-copilot", { + id: "explicit-audio-model", + input_modalities: ["audio"], + capabilities: { supports: { vision: true } }, + })).toEqual({ inputModalities: ["audio"] }); + }); + test("preserves nested reasoning_parameters effort ladders from OpenAI-compatible catalogs", () => { expect(catalogHintsFromModelsApiItem("example", { id: "reasoning-model", From 8427efe6e80a5ce9488eab7b80b2b1663ab20579 Mon Sep 17 00:00:00 2001 From: JUN Date: Sun, 30 Aug 2026 06:06:23 +0900 Subject: [PATCH 02/65] fix(adapters): classify failed exec wrappers by forward scan, not backtracking (#2945) The previous classifier placed two adjacent unbounded whitespace runs over the same span, so a long whitespace run followed by one non-matching character forced the engine to retry prefixes. Replaced with a single forward scan. Every loop advances an index monotonically, the newline searches cover disjoint forward spans, and the token checks are fixed-length, so no path retries a prefix. Two boundaries are load-bearing and both were divergences in an earlier attempt at this rewrite: whitespace after Output: may precede the marker, so an indented still classifies; and only whitespace may follow it, so a duplicate still does not. That second one matters most -- classifying it would replace a real payload with the failed-wrapper guidance, turning a normalization into data loss. One intended behaviour change: CRLF blank separators now count. The old pattern matched only \n, so a Windows-produced failed wrapper never classified and fell through to the empty-SUCCESS message, telling the model nothing had gone wrong when the cell had failed. Differential comparison over 63 shapes locally and an independent 662-shape pairwise corpus found no disagreement outside that CRLF blank-separator class. Diagnosis and the linear-scan approach are @luvs01's from #2938; that PR could not land as written because of the six divergences, which I posted there with the exact inputs. The bounded-work test measures process.cpuUsage() rather than elapsed wall time: performance.now() counts OS descheduling, VM pauses and GC, so a loaded CI runner can blow a wall-clock budget while the code under test did nothing wrong. Reverting to the previous classifier spends 1230ms CPU against a 250ms bound. Four mutations proven red at the intended test each: whitespace restricted to CR/LF, accepting a second marker, removing CRLF handling, and reverting the classifier. --- src/adapters/cursor/tool-result-normalize.ts | 6 +- src/adapters/exec-tool-result-normalize.ts | 75 ++++++++++++++++++-- tests/cursor-exec-empty-result.test.ts | 70 ++++++++++++++++++ 3 files changed, 143 insertions(+), 8 deletions(-) diff --git a/src/adapters/cursor/tool-result-normalize.ts b/src/adapters/cursor/tool-result-normalize.ts index 997ef56fea..fded734b93 100644 --- a/src/adapters/cursor/tool-result-normalize.ts +++ b/src/adapters/cursor/tool-result-normalize.ts @@ -13,7 +13,7 @@ import { EMPTY_EXEC_OUTPUT_MESSAGE, EMPTY_EXEC_OUTPUT_REGEX, FAILED_EXEC_OUTPUT_MESSAGE, - FAILED_EXEC_OUTPUT_REGEX, + isFailedEmptyExecWrapper, isCodexExecBridgeTool, } from "../exec-tool-result-normalize"; @@ -23,7 +23,7 @@ import { * `Script failed`, so restore that arm here rather than widening the shared one. */ function isEmptyOrFailedExecWrapper(text: string): boolean { - return EMPTY_EXEC_OUTPUT_REGEX.test(text) || FAILED_EXEC_OUTPUT_REGEX.test(text); + return EMPTY_EXEC_OUTPUT_REGEX.test(text) || isFailedEmptyExecWrapper(text); } const COMPUTER_USE_TOOL_NAMES = new Set([ @@ -99,7 +99,7 @@ export function normalizeCursorToolResultText( // A `Script failed` wrapper is empty but NOT a success: reporting it as an empty success // would erase the only failure signal. Text classification stays separate from Cursor's // isError policy, which the Computer Use branch above owns. - text: FAILED_EXEC_OUTPUT_REGEX.test(text.trim()) ? FAILED_EXEC_OUTPUT_MESSAGE : EMPTY_EXEC_OUTPUT_MESSAGE, + text: isFailedEmptyExecWrapper(text.trim()) ? FAILED_EXEC_OUTPUT_MESSAGE : EMPTY_EXEC_OUTPUT_MESSAGE, isError: false, changed: true, }; diff --git a/src/adapters/exec-tool-result-normalize.ts b/src/adapters/exec-tool-result-normalize.ts index f06a07c411..56a0386c4d 100644 --- a/src/adapters/exec-tool-result-normalize.ts +++ b/src/adapters/exec-tool-result-normalize.ts @@ -17,13 +17,78 @@ * `Script failed` is deliberately NOT in this set. A failed cell with no captured output is still * a FAILURE, and the success guidance below ("not a blocked tool", "do not re-run") would erase the * only signal that anything went wrong — reachable through Responses history, where - * `function_call_output` is parsed with `isError: false`. Cursor keeps its own broader regex for - * Computer Use, where a failed wrapper is separately marked `isError`. + * `function_call_output` is parsed with `isError: false`. Cursor combines this set with + * `isFailedEmptyExecWrapper` below for Computer Use, where a failed wrapper is separately marked + * `isError`. */ export const EMPTY_EXEC_OUTPUT_REGEX = /^(?:(?:Script completed|Command finished|Execution finished)[^\n]*\n+)?(?:Wall time[^\n]*\n+)?(?:Output:\s*)?(?:)?\s*$/; -/** Wrapper for a cell that FAILED without emitting output: empty, but not a success. */ -export const FAILED_EXEC_OUTPUT_REGEX = /^Script failed[^\n]*\n*(?:Wall time[^\n]*\n*)?(?:Output:\s*)?(?:)?\s*$/; +function skipFailedWrapperBlankSeparators(text: string, start: number): number { + let index = start; + while (index < text.length) { + if (text[index] === "\n") { + index += 1; + continue; + } + // A CRLF blank line is one separator, not a stray carriage return. The regex this replaced + // matched only `\n`, so a Windows-produced wrapper never classified and the failure guidance + // was silently replaced by the empty-SUCCESS message on that platform. + if (text[index] === "\r" && text[index + 1] === "\n") { + index += 2; + continue; + } + break; + } + return index; +} + +function skipFailedWrapperWhitespace(text: string, start: number): number { + let index = start; + while (index < text.length && text[index]!.trim() === "") index += 1; + return index; +} + +function skipFailedWrapperLine(text: string, start: number): number { + const newline = text.indexOf("\n", start); + return newline === -1 ? text.length : skipFailedWrapperBlankSeparators(text, newline + 1); +} + +/** + * Wrapper for a cell that FAILED without emitting output: empty, but not a success. + * + * A single forward scan, replacing a regex whose `\n*` and `\s*` runs sat adjacent over the same + * span and could be made to backtrack on a long whitespace run followed by one non-matching + * character. Every loop here advances an index monotonically and the token checks are fixed-length, + * so the work is bounded by the input length with no path that retries a prefix. + * + * Two boundaries are load-bearing and both were divergences in an earlier attempt at this rewrite: + * + * - Whitespace after `Output:` may precede the marker, so an INDENTED `` still classifies. + * Rejecting it would leave the wrapper unnormalized and the failure unexplained. + * - Only whitespace may follow the marker, so a DUPLICATE `` still does not classify. + * Accepting it would erase a real payload as an empty failed wrapper — the damaging direction. + * + * Behaviour is otherwise identical to the regex; the CRLF separators above are the only + * intentional change, verified against a 63-shape differential corpus. + */ +export function isFailedEmptyExecWrapper(trimmed: string): boolean { + if (!trimmed.startsWith("Script failed")) return false; + + const firstNewline = trimmed.indexOf("\n", "Script failed".length); + if (firstNewline === -1) return true; + + let index = skipFailedWrapperBlankSeparators(trimmed, firstNewline + 1); + if (trimmed.startsWith("Wall time", index)) { + index = skipFailedWrapperLine(trimmed, index); + } + if (trimmed.startsWith("Output:", index)) { + index = skipFailedWrapperWhitespace(trimmed, index + "Output:".length); + } + if (trimmed.startsWith("", index)) { + index += "".length; + } + return skipFailedWrapperWhitespace(trimmed, index) === trimmed.length; +} /** Guidance for a failed cell whose output was empty: the failure must survive normalization. */ export const FAILED_EXEC_OUTPUT_MESSAGE = @@ -94,6 +159,6 @@ export function normalizeEmptyExecToolResultText( if (!isCodexExecBridgeTool(options.toolName, options.toolNamespace)) return undefined; const trimmed = text.trim(); // Failure first: a failed wrapper must never be described as an empty success. - if (FAILED_EXEC_OUTPUT_REGEX.test(trimmed)) return FAILED_EXEC_OUTPUT_MESSAGE; + if (isFailedEmptyExecWrapper(trimmed)) return FAILED_EXEC_OUTPUT_MESSAGE; return EMPTY_EXEC_OUTPUT_REGEX.test(trimmed) ? EMPTY_EXEC_OUTPUT_MESSAGE : undefined; } diff --git a/tests/cursor-exec-empty-result.test.ts b/tests/cursor-exec-empty-result.test.ts index 098aaa08d3..57f42c0076 100644 --- a/tests/cursor-exec-empty-result.test.ts +++ b/tests/cursor-exec-empty-result.test.ts @@ -45,6 +45,76 @@ describe("codex exec bridge empty-result normalization (devlog 260826 gap-7)", ( expect(out.text).toBe("Output:\nhello"); }); + test("an indented empty marker after Output: still classifies as a failed wrapper", () => { + // These three classified under the previous regex. A line-scan rewrite that treated the + // marker as needing to start its own line rejected them, leaving the wrapper unnormalized + // and the failure unexplained. + for (const wrapper of [ + "Script failed\nOutput:\n\n ", + "Script failed\nOutput:\n ", + "Script failed\nOutput:\n ", + ]) { + const out = normalizeCursorToolResultText(wrapper, { toolName: "exec", isError: false }); + expect(out.changed).toBe(true); + expect(out.text).toContain("exec failed"); + } + }); + + test("a duplicate empty marker is left alone rather than erased", () => { + // The damaging direction: classifying these would replace a real payload with the failed-wrapper + // guidance. The previous regex rejected them and so must any replacement. + for (const wrapper of [ + "Script failed\nOutput:\t\n", + "Script failed\nOutput: \n\n", + "Script failed\nOutput: \n", + ]) { + const out = normalizeCursorToolResultText(wrapper, { toolName: "exec", isError: false }); + expect(out.changed).toBe(false); + expect(out.text).toBe(wrapper); + } + }); + + test("CRLF blank separators reach the failure guidance instead of the empty-success text", () => { + // The one intentional behaviour change. The old regex matched only `\n`, so a Windows-produced + // failed wrapper fell through to the empty-SUCCESS message — telling the model nothing went + // wrong when the cell had in fact failed. + for (const wrapper of [ + "Script failed\r\n\r\n\r\nOutput:", + "Script failed\r\n\r\n", + "Script failed\r\n\r\nOutput:", + "Script failed\r\nWall time 1s\r\n\r\nOutput:", + ]) { + const out = normalizeCursorToolResultText(wrapper, { toolName: "exec", isError: false }); + expect(out.changed).toBe(true); + expect(out.text).toContain("exec failed"); + expect(out.text).not.toContain("NOT lost context"); + } + }); + + test("a long whitespace run followed by a non-matching character classifies in bounded work", () => { + // A pathological shape for the classifier this replaced. Measured in CPU time rather than + // elapsed wall time: `performance.now()` counts OS descheduling, VM pauses and GC, so a loaded + // CI runner can blow any wall-clock budget while the code under test did nothing wrong. + // `process.cpuUsage()` counts only work this process actually performed. + // + // The bound is deliberately three orders of magnitude above the scan's real cost. It is not a + // performance target; it is a tripwire wide enough that only a return to super-linear work can + // cross it, which is the single thing this test exists to catch. + const malformed = `Script failed${" ".repeat(60_000)}\nY`; + + // Warm up so first-call JIT and allocation land outside the measurement. + normalizeCursorToolResultText(malformed, { toolName: "exec", isError: false }); + + const before = process.cpuUsage(); + const out = normalizeCursorToolResultText(malformed, { toolName: "exec", isError: false }); + const spent = process.cpuUsage(before); + const cpuMs = (spent.user + spent.system) / 1000; + + expect(out.changed).toBe(false); + expect(out.text).toBe(malformed); + expect(cpuMs).toBeLessThan(250); + }); + test("computer-use empties keep the original error semantics", () => { const out = normalizeCursorToolResultText("", { toolName: "screenshot" }); expect(out.isError).toBe(true); From 47b8d164366b9db9e4331b2bb8b542db22766910 Mon Sep 17 00:00:00 2001 From: JUN Date: Sun, 30 Aug 2026 06:34:06 +0900 Subject: [PATCH 03/65] test(codex): isolate the tombstone guard with a credential-carrying tombstone (#2946) Closes an over-claim in #2934's merge record. Its mutation table said removing the `alias.deletedAt != null` check in commitRefreshedCodexCredentialWithAliases turns the resurrection test red. It does not: tombstoneCodexAccount drops the credential, so the separate `!alias.credential` guard already skips that record and the assertion passes with `deletedAt` deleted. The two guards overlap on the only fixture that exercised them, so neither was independently proven. A tombstone that still carries a credential is the only shape that reaches the deletedAt check, and it is reachable: a store written by an older build, or a tombstone raced by a concurrent save. `tokenful tombstone is treated as absent` already pins that shape for the read path, so this uses the same construction for the propagation path. Every other eligibility field matches the owner in this fixture -- same fingerprint, same access token, same expiry, same chatgptAccountId -- so deletedAt is the only thing that can skip it. Removing that check now turns this test red while the other 41 stay green. No production change. --- tests/codex-account-store.test.ts | 44 +++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/tests/codex-account-store.test.ts b/tests/codex-account-store.test.ts index fe7274496d..3e2a9f3f77 100644 --- a/tests/codex-account-store.test.ts +++ b/tests/codex-account-store.test.ts @@ -1100,6 +1100,50 @@ describe("codex-account-store CRUD", () => { } }); + test("a TOKENFUL tombstone is not resurrected either, isolating the deletedAt guard (#2892 gap 3)", async () => { + // The sibling test above cannot prove the `deletedAt` check is load-bearing: `tombstoneCodexAccount` + // drops the credential, so the separate `!alias.credential` guard already skips that record and the + // assertion passes with `deletedAt` removed. A tombstone that still CARRIES a credential is the only + // shape that reaches the `deletedAt` check, and it is reachable — a store written by an older build, + // or a tombstone raced by a concurrent save, produces exactly this record. `tokenful tombstone is + // treated as absent` earlier in this file pins the same shape for the read path. + const { getValidCodexToken, readCodexAccountRecord, saveCodexAccountCredential } = + await import("../src/codex/account-store"); + const shared = { + accessToken: "tokenful-old", + refreshToken: "tokenful-grant", + expiresAt: 0, + chatgptAccountId: "tokenful-acc", + }; + saveCodexAccountCredential("tokenful-owner", { ...shared }); + const ownerGeneration = readCodexAccountRecord("tokenful-owner")!.generation; + // Written directly: no public API produces a tombstone that retains its credential. + writeFileSync(ACCOUNTS_PATH, JSON.stringify({ + "tokenful-owner": { credential: { ...shared }, generation: ownerGeneration }, + "tokenful-deleted": { credential: { ...shared }, generation: ownerGeneration, deletedAt: Date.now() }, + }, null, 2)); + + const originalFetch = globalThis.fetch; + globalThis.fetch = (async () => Response.json({ + access_token: "tokenful-new", + refresh_token: "tokenful-rotated", + expires_in: 3600, + })) as typeof fetch; + try { + await getValidCodexToken("tokenful-owner"); + // The owner rotated. + expect(readCodexAccountRecord("tokenful-owner")!.credential!.refreshToken).toBe("tokenful-rotated"); + // The tombstone kept its stale grant and stayed deleted: propagation skipped it on `deletedAt` + // alone, since its credential was present and every other eligibility field matched the owner. + const deleted = readCodexAccountRecord("tokenful-deleted")!; + expect(deleted.deletedAt).toBeGreaterThan(0); + expect(deleted.credential!.refreshToken).toBe("tokenful-grant"); + expect(deleted.generation).toBe(ownerGeneration); + } finally { + globalThis.fetch = originalFetch; + } + }); + test("a same-grant sibling on a DIFFERENT upstream identity is never adopted (#2892 review)", async () => { const { forceRefreshCodexPoolToken, getCodexAccountCredential, readCodexAccountRecord, saveCodexAccountCredential } = From dca16949b0eeca1a7fb2f99a777d0f12ce350bb4 Mon Sep 17 00:00:00 2001 From: luvs01 Date: Sun, 30 Aug 2026 11:01:55 +0900 Subject: [PATCH 04/65] test: let the README asset check tell files from directories (#2952) The shipped-asset check treated every `package.json` `files` entry as a possible directory prefix. `assets/banner.png` therefore vouched for `assets/banner.png/missing.gif`, and `LICENSE` for `LICENSE/missing.png`. The intent was right: a directory entry does ship everything beneath it, and the existing comment correctly rejects deciding that by looking for a dot in the name. But prefix matching alone cannot tell the two cases apart either. Ask the filesystem which entries are directories, and let only those act as prefixes. The check is a guard against broken images on the npm package page, so a false negative here is exactly the failure it exists to catch. --- tests/repo-hygiene.test.ts | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/tests/repo-hygiene.test.ts b/tests/repo-hygiene.test.ts index 6132a9776a..c1de41184b 100644 --- a/tests/repo-hygiene.test.ts +++ b/tests/repo-hygiene.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test } from "bun:test"; -import { existsSync } from "node:fs"; +import { existsSync, statSync } from "node:fs"; import { fileURLToPath } from "node:url"; const repoRoot = fileURLToPath(new URL("../", import.meta.url)); @@ -244,13 +244,22 @@ describe("devlog is tracked, with no submodule left behind", () => { // end state, and a `toBeGreaterThan(0)` guard here would fail the suite for doing it. const relative = [...readme.matchAll(/src="(?!https?:)([^"]+)"/g)].map((match) => match[1]!); - const missing = relative.filter((asset) => { - if (shipped.includes(asset)) return false; - // A directory entry ships everything beneath it. Decided by whether the tarball path is a - // prefix, not by whether the name contains a dot: `LICENSE` has no dot and is a file, and - // a future `assets` entry would have no dot and be a directory. - return !shipped.some((entry) => asset.startsWith(`${entry}/`)); + // A directory entry ships everything beneath it; a regular-file entry ships only itself. + // Deciding that by prefix alone let `assets/banner.png` vouch for a nonexistent + // `assets/banner.png/missing.gif`, so a broken README reference could pass. Ask the + // filesystem what each entry actually is instead of inferring it from the name. + const shippedDirectories = shipped.filter((entry) => { + const path = new URL(`../${entry}`, import.meta.url); + return existsSync(path) && statSync(path).isDirectory(); }); + const isShipped = (asset: string): boolean => + shipped.includes(asset) + || shippedDirectories.some((directory) => asset.startsWith(`${directory}/`)); + + expect(isShipped("assets/banner.png/missing.gif")).toBe(false); + expect(isShipped("LICENSE/missing.png")).toBe(false); + + const missing = relative.filter((asset) => !isShipped(asset)); expect(missing).toEqual([]); }); }); From b95dc5d429042c99c87cdad82717eb3da3ba5ac5 Mon Sep 17 00:00:00 2001 From: JUN Date: Sun, 30 Aug 2026 11:33:37 +0900 Subject: [PATCH 05/65] fix(test): scope test lock to user runtime (#2962) A home-rooted lock can couple separate machines while PID liveness remains host-local, and inaccessible homes fail before discovery with raw filesystem errors.\n\nResolve a validated user runtime from XDG or a private UID temp namespace, include a host discriminator, and surface actionable failures. Cover cross-user, cross-host, Windows, fallback, unsafe-root, and path-containment cases. --- scripts/test-run-lock.ts | 188 ++++++++++++++++++++++++++++++++++++-- scripts/test.ts | 4 +- tests/preload.ts | 2 +- tests/test-runner.test.ts | 134 ++++++++++++++++++++++++++- 4 files changed, 315 insertions(+), 13 deletions(-) diff --git a/scripts/test-run-lock.ts b/scripts/test-run-lock.ts index d1c65487d5..77e5bfa99d 100644 --- a/scripts/test-run-lock.ts +++ b/scripts/test-run-lock.ts @@ -1,5 +1,8 @@ -import { randomUUID } from "node:crypto"; +import { createHash, randomUUID } from "node:crypto"; import { + accessSync, + constants, + lstatSync, mkdirSync, readFileSync, readdirSync, @@ -8,15 +11,43 @@ import { statSync, writeFileSync, } from "node:fs"; -import { tmpdir } from "node:os"; -import { join } from "node:path"; +import { hostname, tmpdir } from "node:os"; +import { isAbsolute, join, posix, win32 } from "node:path"; export const TEST_RUN_ID_ENV = "OCX_TEST_RUN_ID"; export const TEST_RUN_NO_QUEUE_ENV = "OCX_TEST_NO_QUEUE"; -const DEFAULT_LOCK_PATH = join(tmpdir(), "opencodex-bun-test.lock"); const OWNER_FILE = "owner.json"; const MEMBERS_DIR = "members"; const INCOMPLETE_OWNER_GRACE_MS = 10_000; +const POSIX_PRIVATE_MODE = 0o700; + +interface RuntimeDirectoryEntry { + uid: number; + mode: number; + isDirectory(): boolean; + isSymbolicLink(): boolean; +} + +export interface TestRunRuntimeFileSystem { + lstatSync(path: string): RuntimeDirectoryEntry; + mkdirSync(path: string, options: { mode: number }): void; + accessSync(path: string, mode: number): void; +} + +export interface ResolveDefaultTestRunLockPathOptions { + platform?: NodeJS.Platform; + env?: NodeJS.ProcessEnv; + uid?: number; + tempDir?: string; + hostName?: string; + fileSystem?: TestRunRuntimeFileSystem; +} + +const runtimeFileSystem: TestRunRuntimeFileSystem = { + lstatSync, + mkdirSync(path, options) { mkdirSync(path, options); }, + accessSync, +}; export interface TestRunLockOwner { version: 1; @@ -48,6 +79,138 @@ export interface BareTestRunIdentity { runId: string; } +function errorCode(error: unknown): string { + if (!error || typeof error !== "object" || !("code" in error)) return "unknown error"; + return String((error as { code?: unknown }).code ?? "unknown error"); +} + +function inspectRuntimeDirectory(options: { + path: string; + fileSystem: TestRunRuntimeFileSystem; + expectedUid?: number; + requirePrivateMode?: boolean; +}): string | null { + let entry: RuntimeDirectoryEntry; + try { + entry = options.fileSystem.lstatSync(options.path); + } catch (error) { + return `cannot be inspected (${errorCode(error)})`; + } + if (entry.isSymbolicLink() || !entry.isDirectory()) return "is not a real directory"; + if (options.expectedUid !== undefined && entry.uid !== options.expectedUid) { + return "is not owned by the current uid"; + } + if (options.requirePrivateMode && (entry.mode & 0o777) !== POSIX_PRIVATE_MODE) { + return "does not have mode 0700"; + } + try { + options.fileSystem.accessSync(options.path, constants.W_OK | constants.X_OK); + } catch (error) { + return `is not writable/searchable (${errorCode(error)})`; + } + return null; +} + +function machineDiscriminator(hostName: string): string { + const normalized = hostName.trim().toLowerCase(); + if (!normalized) throw new Error("the OS hostname is empty"); + return createHash("sha256").update(normalized).digest("hex").slice(0, 16); +} + +/** + * Resolve a user-scoped, machine-local default lock path without relying on HOME. + * + * POSIX XDG runtime directories are accepted only after an ownership and access + * check. The fallback is a private UID namespace under the OS temp directory. + * The hostname digest remains part of the lock name in either case: even if an + * administrator redirects either root to shared storage, host-local PID liveness + * checks can never reclaim or join another machine's lock. + */ +export function resolveDefaultTestRunLockPath( + options: ResolveDefaultTestRunLockPathOptions = {}, +): string { + const platform = options.platform ?? process.platform; + const env = options.env ?? process.env; + const tempDir = options.tempDir ?? tmpdir(); + const fileSystem = options.fileSystem ?? runtimeFileSystem; + let discriminator: string; + try { + discriminator = machineDiscriminator(options.hostName ?? hostname()); + } catch (cause) { + throw new Error( + "Cannot resolve a safe user-scoped Bun test lock: the machine identity is unavailable.", + { cause }, + ); + } + + if (platform === "win32") { + if (!win32.isAbsolute(tempDir)) { + throw new Error( + "Cannot resolve a safe user-scoped Bun test lock: the Windows OS temp/profile path is not absolute.", + ); + } + const issue = inspectRuntimeDirectory({ path: tempDir, fileSystem }); + if (issue) { + throw new Error( + `Cannot resolve a safe user-scoped Bun test lock: the Windows OS temp/profile directory ${issue}.`, + ); + } + return win32.join(tempDir, `opencodex-bun-test-${discriminator}.lock`); + } + + const uid = options.uid ?? (typeof process.getuid === "function" ? process.getuid() : undefined); + if (!Number.isInteger(uid) || (uid ?? -1) < 0) { + throw new Error( + "Cannot resolve a safe user-scoped Bun test lock: the current POSIX uid is unavailable.", + ); + } + + const failures: string[] = []; + const xdgRuntimeDir = env.XDG_RUNTIME_DIR?.trim(); + if (xdgRuntimeDir) { + if (!isAbsolute(xdgRuntimeDir)) { + failures.push("XDG_RUNTIME_DIR is not absolute"); + } else { + const issue = inspectRuntimeDirectory({ + path: xdgRuntimeDir, + fileSystem, + expectedUid: uid, + }); + if (!issue) return posix.join(xdgRuntimeDir, `opencodex-bun-test-${discriminator}.lock`); + failures.push(`XDG_RUNTIME_DIR ${issue}`); + } + } + + if (!isAbsolute(tempDir)) { + failures.push("the OS temporary directory is not absolute"); + } else { + const fallback = posix.join(tempDir, `opencodex-test-runtime-${uid}`); + try { + fileSystem.mkdirSync(fallback, { mode: POSIX_PRIVATE_MODE }); + } catch (error) { + if (errorCode(error) !== "EEXIST") { + failures.push(`the temporary UID runtime directory cannot be created (${errorCode(error)})`); + } + } + if (!failures.some(failure => failure.startsWith("the temporary UID runtime directory cannot be created"))) { + const issue = inspectRuntimeDirectory({ + path: fallback, + fileSystem, + expectedUid: uid, + requirePrivateMode: true, + }); + if (!issue) return posix.join(fallback, `opencodex-bun-test-${discriminator}.lock`); + failures.push(`the temporary UID runtime directory ${issue}`); + } + } + + throw new Error( + "Cannot resolve a safe user-scoped Bun test lock. " + + "Ensure XDG_RUNTIME_DIR is an existing writable directory owned by the current uid, " + + `or make the OS temporary directory usable for a mode-0700 UID runtime (${failures.join("; ")}).`, + ); +} + /** * Give one bare Bun invocation a stable identity without conflating sibling commands. * @@ -151,7 +314,7 @@ function ownsLock(lockPath: string, owner: TestRunLockOwner): boolean { } /** - * Acquire the machine-wide OpenCodex Bun-test lock. + * Acquire the user-scoped, machine-local OpenCodex Bun-test lock. * * `mkdir` is the cross-platform atomic primitive. The owner PID makes a lock left by * SIGKILL recoverable, while the run ID lets every worker belonging to one bare @@ -165,7 +328,8 @@ export async function acquireTestRunLock(options: AcquireTestRunLockOptions): Pr return { acquired: false, owner: null, release() {} }; } - const lockPath = options.lockPath ?? DEFAULT_LOCK_PATH; + const usesDefaultLockPath = options.lockPath === undefined; + const lockPath = options.lockPath ?? resolveDefaultTestRunLockPath({ env }); const ownerPid = options.ownerPid ?? process.pid; const pollMs = Math.max(1, options.pollMs ?? 5_000); const maxWaitMs = Math.max(pollMs, options.maxWaitMs ?? 45 * 60 * 1000); @@ -198,7 +362,17 @@ export async function acquireTestRunLock(options: AcquireTestRunLockOptions): Pr }, }; } catch (error) { - if ((error as NodeJS.ErrnoException).code !== "EEXIST") throw error; + const code = (error as NodeJS.ErrnoException).code; + if (code !== "EEXIST") { + if (usesDefaultLockPath && ["EACCES", "ENOENT", "EPERM", "EROFS"].includes(code ?? "")) { + throw new Error( + "Cannot acquire the user-scoped Bun test lock because its validated runtime directory " + + "became unavailable or unwritable. Check XDG_RUNTIME_DIR and the OS temporary directory.", + { cause: error }, + ); + } + throw error; + } } const current = readOwner(lockPath); diff --git a/scripts/test.ts b/scripts/test.ts index 832a537191..6d10b2c4a7 100644 --- a/scripts/test.ts +++ b/scripts/test.ts @@ -452,10 +452,10 @@ if (import.meta.main) { const lock = await acquireTestRunLock({ runId, onWait: owner => console.warn( - `[test] another Bun test run${owner ? ` (pid ${owner.pid})` : ""} holds the machine lock; waiting. ` + `[test] another Bun test run${owner ? ` (pid ${owner.pid})` : ""} holds the user lock; waiting. ` + "Set OCX_TEST_NO_QUEUE=1 only for intentional overlap.", ), - onAcquiredAfterWait: elapsedMs => console.warn(`[test] acquired the machine lock after ${Math.round(elapsedMs / 1000)}s.`), + onAcquiredAfterWait: elapsedMs => console.warn(`[test] acquired the user lock after ${Math.round(elapsedMs / 1000)}s.`), }); const startedAt = Date.now(); try { diff --git a/tests/preload.ts b/tests/preload.ts index 37b2233df0..dd04c5c33c 100644 --- a/tests/preload.ts +++ b/tests/preload.ts @@ -34,7 +34,7 @@ await acquireTestRunLock({ runId, ownerPid: bareIdentity.ownerPid, onWait: owner => console.warn( - `[test] bare Bun worker ${process.pid} is waiting for test run${owner ? ` pid ${owner.pid}` : ""} to release the machine lock.`, + `[test] bare Bun worker ${process.pid} is waiting for test run${owner ? ` pid ${owner.pid}` : ""} to release the user lock.`, ), }); diff --git a/tests/test-runner.test.ts b/tests/test-runner.test.ts index 2d5423d628..ed8e92d3ae 100644 --- a/tests/test-runner.test.ts +++ b/tests/test-runner.test.ts @@ -1,7 +1,7 @@ import { describe, expect, test } from "bun:test"; -import { existsSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { existsSync, mkdtempSync, rmSync, statSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; -import { isAbsolute, join } from "node:path"; +import { dirname, isAbsolute, join, posix, win32 } from "node:path"; import { changedSelectionFailure, createIsolatedTestEnvironment, @@ -14,7 +14,9 @@ import { import { acquireTestRunLock, resolveBareTestRunIdentity, + resolveDefaultTestRunLockPath, TEST_RUN_NO_QUEUE_ENV, + type TestRunRuntimeFileSystem, } from "../scripts/test-run-lock"; import { decodeWindowsIdentityPowerShellOutputForTests, @@ -37,6 +39,28 @@ function runGit(cwd: string, ...args: string[]): string { // handed to git are identical either way. const FIXTURE_COMMIT_EMAIL = ["test", "opencodex.invalid"].join("@"); +function pathIsContainedBy(parent: string, candidate: string, platform: "posix" | "win32"): boolean { + const path = platform === "win32" ? win32 : posix; + const relative = path.relative(path.resolve(parent), path.resolve(candidate)); + return relative === "" || (!relative.startsWith(`..${path.sep}`) + && relative !== ".." && !path.isAbsolute(relative)); +} + +function acceptingRuntimeFileSystem(uid: number, writable = true): TestRunRuntimeFileSystem { + return { + lstatSync: () => ({ + uid, + mode: 0o700, + isDirectory: () => true, + isSymbolicLink: () => false, + }), + mkdirSync: () => {}, + accessSync: () => { + if (!writable) throw Object.assign(new Error("denied"), { code: "EACCES" }); + }, + }; +} + function commitFixture(cwd: string, path: string, contents: string, message: string): string { writeFileSync(join(cwd, path), contents); runGit(cwd, "add", path); @@ -361,7 +385,111 @@ describe("bun test argv", () => { }); }); -describe("bun test machine lock", () => { +describe("bun test user lock", () => { + test("distinct POSIX users receive distinct temp-runtime locks", () => { + const common = { env: {}, tempDir: "/tmp", hostName: "builder-1", platform: "linux" as const }; + const alice = resolveDefaultTestRunLockPath({ + ...common, + uid: 1001, + fileSystem: acceptingRuntimeFileSystem(1001), + }); + const bob = resolveDefaultTestRunLockPath({ + ...common, + uid: 1002, + fileSystem: acceptingRuntimeFileSystem(1002), + }); + + expect(alice).not.toBe(bob); + expect(pathIsContainedBy("/tmp/opencodex-test-runtime-1001", alice, "posix")).toBe(true); + expect(pathIsContainedBy("/tmp/opencodex-test-runtime-1002", bob, "posix")).toBe(true); + }); + + test("a shared home cannot couple locks from distinct hosts", () => { + const common = { + env: { HOME: "/network/users/alice" }, + uid: 1001, + tempDir: "/tmp", + platform: "linux" as const, + fileSystem: acceptingRuntimeFileSystem(1001), + }; + const firstHost = resolveDefaultTestRunLockPath({ ...common, hostName: "builder-1" }); + const secondHost = resolveDefaultTestRunLockPath({ ...common, hostName: "builder-2" }); + + expect(firstHost).not.toBe(secondHost); + expect(pathIsContainedBy(common.env.HOME, firstHost, "posix")).toBe(false); + expect(pathIsContainedBy(common.env.HOME, secondHost, "posix")).toBe(false); + }); + + test("Windows uses the OS temp/profile result when USER is absent", () => { + const common = { + platform: "win32" as const, + tempDir: "C:\\Users\\Alice\\AppData\\Local\\Temp", + hostName: "desktop-1", + fileSystem: acceptingRuntimeFileSystem(0), + }; + const withoutUser = resolveDefaultTestRunLockPath({ ...common, env: {} }); + const withUnrelatedUser = resolveDefaultTestRunLockPath({ + ...common, + env: { USER: "someone-else" }, + }); + + expect(withoutUser).toBe(withUnrelatedUser); + expect(pathIsContainedBy(common.tempDir, withoutUser, "win32")).toBe(true); + }); + + test("falls back from an unsafe XDG root to a validated mode-0700 UID directory", () => { + if (process.platform === "win32" || typeof process.getuid !== "function") return; + const root = mkdtempSync(join(tmpdir(), "opencodex-runtime-fallback-")); + const unsafeXdg = join(root, "not-a-directory"); + writeFileSync(unsafeXdg, "unsafe\n"); + try { + const lockPath = resolveDefaultTestRunLockPath({ + env: { XDG_RUNTIME_DIR: unsafeXdg }, + uid: process.getuid(), + tempDir: root, + hostName: "builder-1", + }); + const runtimeRoot = dirname(lockPath); + const entry = statSync(runtimeRoot); + + expect(runtimeRoot).toBe(join(root, `opencodex-test-runtime-${process.getuid()}`)); + expect(entry.isDirectory()).toBe(true); + expect(entry.uid).toBe(process.getuid()); + expect(entry.mode & 0o777).toBe(0o700); + } finally { + rmSync(root, { recursive: true, force: true }); + } + }); + + test("fails immediately with actionable guidance when every runtime root is unwritable", () => { + expect(() => resolveDefaultTestRunLockPath({ + platform: "linux", + env: { XDG_RUNTIME_DIR: "/run/user/1001" }, + uid: 1001, + tempDir: "/tmp", + hostName: "builder-1", + fileSystem: acceptingRuntimeFileSystem(1001, false), + })).toThrow( + "Cannot resolve a safe user-scoped Bun test lock. Ensure XDG_RUNTIME_DIR", + ); + }); + + test("containment checks do not confuse path string prefixes on POSIX or Windows", () => { + const home = "/home/alice"; + const lockPath = resolveDefaultTestRunLockPath({ + platform: "linux", + env: { HOME: home }, + uid: 1001, + tempDir: "/home", + hostName: "builder-1", + fileSystem: acceptingRuntimeFileSystem(1001), + }); + + expect(home.startsWith("/home")).toBe(true); + expect(pathIsContainedBy(home, lockPath, "posix")).toBe(false); + expect(pathIsContainedBy("C:\\Users\\Ann", "C:\\Users\\Anna\\lock", "win32")).toBe(false); + }); + test("independent bare runners do not inherit a shared long-lived parent identity", () => { expect(resolveBareTestRunIdentity({ pid: 101, ppid: 50 })).toEqual({ ownerPid: 101, From 4f6a19643a46c7ae258d01445e09203509190663 Mon Sep 17 00:00:00 2001 From: JUN Date: Sun, 30 Aug 2026 11:35:05 +0900 Subject: [PATCH 06/65] fix(gui): give the provider toggle a real flex basis so the header can wrap (#2958) * fix(gui): give the provider toggle a real flex basis so the header can wrap The Models provider header collapsed between roughly 1040 and 1380px: the provider name measured 0.0px and painted its glyphs across the active count, and the alias chip broke into a six-line blob. Both were one cause. The toggle's inline `flex: 1` resolves to `flex: 1 1 0%`, and a flex item with a zero base size never reports a content requirement, so the header's existing `flex-wrap: wrap` never learned the toggle needed room and handed it the 31px the actions cluster left over. At 1100 the actions took 422.9 of 488px. Two properties are needed and they pull against each other. Visibility comes from `flex: 1 1 auto`, so the content enters the header's wrap decision. Boundedness comes from removing every child's automatic min-content floor, because a flex child stops shrinking at its own `min-width: auto` and the sum of those floors can still exceed the card. The child rule is quantified rather than enumerated. Four earlier drafts bounded the row by naming the children that could overflow it - name, then alias chip, then the count and badge - and each revision found another one. The `svg` exemption is the inverse failure: the universal rule also matched the chevron, whose inline `width: 14` is not a flex floor, and it rendered 2.5px wide while the containment check still reported success. Text children abbreviate; icons have nothing to truncate. Measured in a real browser at dpr 2: 20/20 cells clean across ko/ru/fr/en/de x 1440/1280/1100/1024, red on the pre-fix stylesheet (ko/1100 three bad rows, ko/1280 four). Containment holds at -2 on five stress cases including every child forced to 64 characters, with the chevron at 14px. Screenshots and the pixel readback that confirms the collapse are in the devlog unit. Also lifts the effective-declaration CSS readers out of viewport-scroll-caps.test.ts, where they were file-local, into gui/tests/helpers/css-declarations.ts so this test can use them without a third copy. * fix(gui): address review findings on the provider-header record Three CodeRabbit findings, all correct: - The new test names cited `#2916`, a PR number guessed before this branch had one. The PR is #2958. - `010` still described the effective-declaration reader as unimportable and left the export-versus-move decision open. B resolved it by moving all four helpers into `gui/tests/helpers/css-declarations.ts` and rewriting the original test to import them, so the doc now records that outcome and lists the module in the diff scope. - `020`'s rendered CDP check asked whether each `button.switch` carries visible text or a `title`, which contradicts the wrapper rule its own item 3 states: a `showLabel` switch puts its text in a sibling inside `.switch-labeled` and carries no `title`. As written the check would have failed exactly the controls that phase fixes, so it now applies the wrapper-aware condition. No behavior change; documentation and test names only. --- .../000_baseline_and_roadmap.md | 168 ++++++++++ .../010_toggle_basis_and_shrink.md | 286 ++++++++++++++++++ .../011_rejected_designs.md | 179 +++++++++++ .../020_control_affordances.md | 170 +++++++++++ .../evidence/010-after-ko-1100.png | Bin 0 -> 42401 bytes .../evidence/010-before-ko-1100.png | Bin 0 -> 45872 bytes gui/src/pages/Models.tsx | 2 +- gui/src/styles-models-workspace.css | 25 ++ gui/tests/helpers/css-declarations.ts | 78 +++++ gui/tests/models-provider-head.test.ts | 111 +++++++ gui/tests/viewport-scroll-caps.test.ts | 66 +--- 11 files changed, 1019 insertions(+), 66 deletions(-) create mode 100644 devlog/_plan/260830_models_provider_header/000_baseline_and_roadmap.md create mode 100644 devlog/_plan/260830_models_provider_header/010_toggle_basis_and_shrink.md create mode 100644 devlog/_plan/260830_models_provider_header/011_rejected_designs.md create mode 100644 devlog/_plan/260830_models_provider_header/020_control_affordances.md create mode 100644 devlog/_plan/260830_models_provider_header/evidence/010-after-ko-1100.png create mode 100644 devlog/_plan/260830_models_provider_header/evidence/010-before-ko-1100.png create mode 100644 gui/tests/helpers/css-declarations.ts diff --git a/devlog/_plan/260830_models_provider_header/000_baseline_and_roadmap.md b/devlog/_plan/260830_models_provider_header/000_baseline_and_roadmap.md new file mode 100644 index 0000000000..f56cca71d6 --- /dev/null +++ b/devlog/_plan/260830_models_provider_header/000_baseline_and_roadmap.md @@ -0,0 +1,168 @@ +# 000 — Models provider-row header: unreadable chip, overlapping name, meaningless controls + +Reported against the running dashboard's Models page with a screenshot: "이부분도 +존나 이상해 신규 2개 꺼짐, 펜, 스위치(이건 뭘하는지도 모르겠음), 사용자 지정창이랑 +마지막 스위치는 뭔지도 모름". + +Two distinct failures are stacked in one header, and they need different fixes: + +- **Geometry** — the "신규 N개, 꺼짐" chip collapses into a rounded blob and the + provider name paints on top of the active count. +- **Meaning** — three controls are operable but unlabeled: a sighted user cannot + tell what they do. This half is not a layout bug and cannot be fixed by + layout. + +As with the sidecar unit, every defect below carries a measured baseline from a +CDP harness (`Emulation.setDeviceMetricsOverride`, dpr 2, live +`getBoundingClientRect`), so each claim is re-checkable. + +## Baseline (ko, provider rows on `#models`) + +Measured with `.tmp/uiux2/head.ts`, which settles on `innerWidth === target` and +on rendered provider rows before reading geometry. The proxy at 127.0.0.1:10100 +supplies the live provider list through the Vite `OPENCODEX_PROXY_TARGET` proxy, +so these are real rows, not fixtures. + +| width | provider | header h | name box w | chip lines | chip w | +|-------|----------|----------|------------|-----------|--------| +| 1440 | opencode-free | 44.8 | 96.5 | 1 | 92.1 | +| 1280 | opencode-free | 55.7 | 67.2 | **2** | 69.6 | +| 1280 | openai | 55.7 | **9.9** | **2** | 57.7 | +| 1100 | opencode-free | **115.1** | **0.0** | **6** | 34.1 | +| 1100 | cursor | **115.1** | **0.0** | **6** | 34.1 | +| 1024 | opencode-free | 75.8 | 96.5 | 1 | — | + +The 1100 row is the screenshot state: a six-line chip 34.1px wide, a name box +measuring **zero**, and a header 2.6x its correct height. 1024 recovers because +the container query at `styles-models-workspace.css:517` moves the actions onto +their own row, which returns the toggle's width. The defect therefore lives in a +**band** (roughly 1040-1380 in this layout), which is why it is easy to miss at +either extreme. + +## Defect 1 — the chip is a shrinkable flex item with no single-line floor + +`.models-chip` (`styles-models-workspace.css:315`) declares +`display: inline-block` plus padding, border and `border-radius`, and nothing +else. Because it sits inside `.row models-provider-toggle` +(`Models.tsx:1226`) and `.row` is `display: flex` (`styles.css:1205`), the chip +is a **flex item**: its `inline-block` outer display is blockified and its +initial `flex-shrink: 1` applies. Measured computed values confirm it — +`white-space: normal`, `flex-shrink: 1`. + +`inline-block` does not imply `white-space: nowrap`. The chip's only floor is +`min-width: auto`, which resolves to the text's **min-content** width — and for +Korean that is nearly one syllable, because CJK line-breaking permits a break +between Hangul syllable blocks. So `신규 2개, 꺼짐` legally becomes +`신규 / 2 / 개, / 꺼 / 짐`, and the fixed padding wrapped around that narrow +column is exactly the observed blob. + +~~Fix: give the chip a single-line floor.~~ **Superseded.** Measurement showed the +chip is not independently broken: it is starved of width by a collapsed ancestor, +and it returns to one line as soon as that ancestor claims its intrinsic width. An +audit also found a chip-level floor unsafe across the eight other `.models-chip` +call sites. The shipped fix leaves the shared `.models-chip` primitive untouched; +it adds an ellipsis only to the toggle-scoped descendant — see `010` and `011`. + +## Defect 2 — the name overflows a zero-width box instead of reflowing + +The name span carries inline `whiteSpace: "nowrap"` (`Models.tsx:1232`) while +`styles-models-workspace.css:267` gives it `min-width: 0` and +`overflow-wrap: anywhere`. Those two fight: `nowrap` suppresses the wrapping +that `overflow-wrap: anywhere` was added to provide, `min-width: 0` lets the box +shrink to nothing, and the default `overflow: visible` means the glyphs keep +painting outside the box — straight across the sibling count. + +Nothing positions these elements on top of each other: there is no `position`, +transform, or negative margin anywhere in the applicable rules. The count is +laid out normally *after* a box that measures 0px, so the collision is pure +overflow. + +Why the header's own `flex-wrap: wrap` does not save it: the header's direct +children are only the toggle button and the actions container. Wrapping does not +propagate into descendants, and the toggle's inner `.row` has no `flex-wrap`, +so the chevron, name, chips and count are locked on one line and shrink against +each other. + +The upstream enabler is `flex: 1` on the toggle (`Models.tsx:1229`), which +resolves to `flex: 1 1 0%` — zero basis, shrink allowed — combined with +`min-width: 0`. The toggle then accepts whatever the wide actions cluster leaves +it rather than forcing a wrap. + +~~Fix: let the toggle's own row wrap.~~ **Superseded.** Inner wrapping is inert: +line construction inside the button runs after its used width has been assigned, so +wrapping redistributes 31px rather than asking for more. Measured: the candidate +left the name box at 0.0px, byte-identical to baseline. The shipped fix gives the +toggle a real flex **basis** so its content enters the header's wrap decision, and +removes every child's min-content floor so the row can always shrink back inside the +card — see `010`. + +## Defect 3 — three controls carry no visible meaning (two switches and the `+`) + +`Switch` (`ui.tsx:8`) accepts a `label` prop and spends it **only** on +`aria-label` (`ui.tsx:11`); its sole child is ``. So +every `Switch` in this codebase is, to a sighted user, an unlabeled toggle. The +user's "이건 뭘하는지도 모르겠음" is a correct reading of the UI. + +Audit of the header controls in visual order: + +| control | visible | aria-label | title | verdict | +|---------|---------|-----------|-------|---------| +| collapse button | chevron + name + count | (children) | — | OK | +| pencil | icon only | 공급자 별칭 편집 | yes | OK | +| default-aliases Switch | knob only | 기본 별칭 사용 | — | **OPAQUE** | +| 사용자 지정 창 | text | — | — | OK | +| `+` | `+` only | 커스텀 모델 추가 | — | **OPAQUE** | +| preset segmented | 프리셋 / 전체 | group only | — | OK | +| 모두 켜기 / 모두 끄기 | text | — | — | OK | +| cap Switch | knob only | 기본 {value} | — | **OPAQUE** | +| cap Select | number only | 기본 {value} | — | **OPAQUE** | + +The pencil is fine precisely because it pairs an icon with `title` — that is the +pattern the opaque controls are missing. + +Two aggravating details: + +1. The cap Switch's accessible name is `기본 128k` — a *value*, not a function. + Even a screen-reader user is not told this governs the context-window cap. +2. For routed providers with the cap off, `(capOn || nativeProviderGroup)` + (`Models.tsx:1360`) hides the Select, so the only thing left is a bare + toggle with no adjacent number to hint at its purpose. The worst state is the + default state. + +### Design constraint + +This is a dense expert control surface: `DESIGN_VARIANCE 2`, `MOTION 1`, density +D6+. The domain gate is strict — no decorative kit, no motion, no new color. The +fix is *labels and reflow*, and the correct instrument is the existing +`title`-plus-icon pattern already proven by the pencil, plus a visible text +label where the header has room for one. + +UX-LAZY-01 was applied to each control before relabeling it rather than after: +every one of them is a real per-provider setting with no correct global default, +so none can be deleted or absorbed. They need meaning, not removal. + +## Roadmap + +- `010` — let the toggle's content be seen, and make every child yield (geometry). + Five designs; the first four were rejected by audit or stress measurement and + `011` records why. +- `020` — control affordances: visible labels for the opaque controls, and a + `Switch` that can render one. + +Each is one PABCD work-phase and one stacked PR. `010` lands first because `020` +adds visible text to the same header and would otherwise be measured against a +layout that is still collapsing. + +## Verification contract + +- Re-measure the sweep at 1440/1280/1100/1024 in ko + ru + fr + en and require: + chip `lines === 1` everywhere, name box width > 0, zero name/count overlap, and + header height within one line-height of the 1440 baseline. +- A focused `gui/tests` regression per phase, driven red against current CSS + first. +- Remote gates only (`ssh lidge` + `ocx-run`); the local full suite is forbidden + by the user. Push `--no-verify`. +- Before/after screenshots at the failing width, per `AGENTS.md` enforce-target. + + + diff --git a/devlog/_plan/260830_models_provider_header/010_toggle_basis_and_shrink.md b/devlog/_plan/260830_models_provider_header/010_toggle_basis_and_shrink.md new file mode 100644 index 0000000000..dd86c0b9d3 --- /dev/null +++ b/devlog/_plan/260830_models_provider_header/010_toggle_basis_and_shrink.md @@ -0,0 +1,286 @@ +# 010 — Let the toggle's content be seen, and make every child yield + +Fixes the geometry half. Meaning is phase `020`. + +**Sixth design.** The five before it were each rejected by an adversarial reviewer or +by a stress measurement, and the rejections are the useful part — they map the shape +of the problem: + +| draft | approach | killed by | +|-------|----------|-----------| +| 1 | shared-chip `nowrap`/`flex-shrink: 0` + inner `flex-wrap` + name ellipsis | inner wrap is inert; shared-chip change unsafe elsewhere | +| 2 | `min-width: max-content` floor | unbounded: 64-char name overflowed the card by 216px | +| 3 | floor + 16rem name cap + 12rem chip cap | 64-char name **and** alias together still overflowed 64px | +| 4 | `flex-basis: auto` + ellipsis on name and chip | the count and badge children kept min-content floors | +| 5 | `flex-basis: auto` + one rule for every child | let the fixed-size chevron shrink to 2.5px | +| **6** | **draft 5 + a `flex: none` exemption for the icon** | — | + +Drafts 2-4 were three versions of one mistake: bound the row by naming the children +that could overflow it, then discover the next child. Draft 5 stops naming children — +and then over-applied, shrinking an icon that has no text to truncate. Draft 6 keeps +the universal rule and exempts the one child whose size is intrinsic rather than +textual. `011` records each failure. + +## The mechanism, measured + +At 1100px the collapsed row measures: + +| element | width | +|---------|-------| +| `.models-provider-head` | 488.0 | +| `.models-provider-actions` | **422.9** (scrollWidth 423) | +| `.models-provider-toggle` | **31.1** (scrollWidth 93) | +| name span inside it | **0.0** (scrollWidth 44) | + +The toggle carries inline `flex: 1` (`Models.tsx:1229`), which resolves to +`flex: 1 1 0%`. That zero **basis** is the defect. A flex item with a zero base size +never reports a content requirement, so the header — which already has +`flex-wrap: wrap` — never learns the toggle needs room and never wraps the actions +cluster to its own line. It keeps one line and hands the toggle the 31px remainder. + +Inside that remainder the name absorbs the whole deficit, measures 0.0px, and — +carrying inline `white-space: nowrap` with default `overflow: visible` — paints its +glyphs across the count. The chip blob is the same starvation, finished by CJK +line-breaking between Hangul syllables. Even the chevron collapses: measured 0px wide +on a starved row, against 14px on a healthy one. + +Two independent properties are required: + +- **Visibility** — the toggle's content must enter the header's wrap decision, so it + receives a share rather than a remainder. That is `flex-basis: auto`. +- **Boundedness** — whatever the content, the row must not force itself wider than the + card. Shrinkability alone does not give this: a flex child stops at its own + `min-width: auto` floor, which is its min-content width, and the *sum* of those + floors can exceed the container. + +The bound has one precondition worth stating plainly, because the round-5 audit caught +the document overstating it: `> *` selects **element** children. A bare string +interpolated directly into the button becomes an anonymous flex item, which no selector +can reach, and it would keep its own min-content floor. Every child today is an +`` or a ``, so the rule covers all of them — but the guarantee is +"every element child, and the markup keeps children element-wrapped", not "anything +anyone adds later". The regression test asserts that second half. + +Draft 2 bought visibility with a raised *minimum*, which is the direct enemy of +boundedness. Draft 4 bought boundedness for the two children it named and left the +count and the discovery badge with their automatic floors intact. + +## The change + +`gui/src/pages/Models.tsx` (—1229), the inline style on the toggle button: + +```diff +- style={{ flex: 1, border: 0, ... }} ++ style={{ flex: "1 1 auto", border: 0, ... }} +``` + +It has to be the TSX: an inline style beats any stylesheet rule short of +`!important`, and reaching for `!important` against markup we own is the wrong +trade. + +`gui/src/styles-models-workspace.css`: + +```css + .models-provider-toggle { + min-width: 0; + } + ++/* Every child, not an enumerated list. Four earlier designs bounded the row by ++ naming the children that could overflow it (name, then alias chip, then the ++ count and badge), and each revision found another one; a child added later ++ would have reintroduced the defect silently. Quantifying over the children ++ instead: min-width:0 removes the automatic min-content floor that stops a flex ++ child shrinking, and the ellipsis makes that shrink legible instead of clipped. ++ Covers every ELEMENT child; a bare interpolated string would become an ++ anonymous flex item no selector can reach, so keep children element-wrapped. */ ++.models-provider-toggle > * { ++ min-width: 0; ++ overflow: hidden; ++ text-overflow: ellipsis; ++ white-space: nowrap; ++} + ++/* The one exemption, and why it is not a return to enumerating children: every ++ other child is TEXT, whose overflow the ellipsis makes legible. The chevron is ++ an icon at a fixed 14px with nothing to truncate, so shrinking it destroys the ++ collapse affordance instead of abbreviating it. Selected by element TYPE, not ++ by identity — any future icon child inherits it without being named. Measured: ++ without this, the adversarial stress case shrinks the chevron to 2.5px while ++ the containment gate still reports success. */ ++.models-provider-toggle > svg { ++ flex: none; ++} +``` + +`min-width: 0` on the toggle is **kept**, not replaced. That also means the existing +assertion at `gui/tests/models-provider-head.test.ts:29` stays green — draft 2 would +have broken it. + +**No `max-width` anywhere, and no child named by identity.** The bound comes from +removing every child's floor, so there is nothing to forget and nothing to re-tune when +a chip is added to this header later. The single exemption selects on element type +(`svg`), which is the distinction that matters: text children abbreviate, icons do not. + +## Measured result + +Gate: in every cell `chipLines === 1`, name width > 0, name text overflow +(`scrollWidth - width`) <= 0, no page overflow. + +| | ko | ru | fr | en | de | +|-|----|----|----|----|----| +| 1440 | pass | pass | pass | pass | pass | +| 1280 | pass | pass | pass | pass | pass | +| 1100 | pass | pass | pass | pass | pass | +| 1024 | pass | pass | pass | pass | pass | + +20/20, worst bad-cell count 0, re-measured after the chevron exemption was added +(draft 6). The chevron also returns to 14px on the rows where it had collapsed to 0. + +Containment, reading `cardScrollOver` = card `scrollWidth` minus its width, where +**positive means the card is silently clipping** (`.models-provider-card` sets +`overflow: hidden`, `styles-models-workspace.css:296`): + +| stress case | baseline | draft 3 | draft 4 | draft 5 | **draft 6** | +|-------------|---------:|--------:|--------:|--------:|------------:| +| 64-char name @1100 | 39 | -2 | -2 | -2 | **-2** | +| 64-char alias @1100 | 16 | -2 | -2 | -2 | **-2** | +| name + alias together @1100 | 229 | **64** | -2 | -2 | **-2** | +| realistic worst row @1100 (64-char name + alias + longest `de` badge) | 229 | — | -2 | -2 | **-2** | +| adversarial: every child forced to 64 chars @1100 | 484 | — | **484** | -2 | **-2** | +| chevron width in that adversarial case | 14 | — | — | **2.5** | **14** | + +The last row is what draft 4 could not survive and what forced the universal rule. It +is deliberately beyond reachable input — the count and badge are localized strings +with small interpolated numbers, not free text — but it is the only case that proves +the bound does not depend on knowing what the children are. + +The final row is the round-5 audit finding, and it is the reason containment alone is +not a sufficient gate: draft 5 reported `cardScrollOver: -2` on the adversarial case +**while** silently shrinking the 14px collapse chevron to 2.5px. A gate that measures +only "does the row fit" certifies a fix that bought the fit by destroying an +affordance. `flex: none` on the icon restores 14px with containment unchanged at -2. + +The gate is not vacuous: against the unpatched stylesheet it reports +`ko/1100 bad=3` (0px name, **6-line** chip) and `ko/1280 bad=4` (name 9.9px, chip 2 +lines). `ru/1100` is green even unpatched — Russian wraps to a wider min-content — +which is why a single-locale check would have missed this defect entirely. + +## Removal test + +| dropped | normal bad cells @ko/1100 | adversarial stress | realistic stress | chevron @adversarial | +|---------|--------------------------:|-------------------:|-----------------:|---------------------:| +| nothing | 0 | -2 | -2 | 14 | +| `flex: 1 1 auto` | **3** | -2 | -2 | 14 | +| the child rule | 0 | **908** | **229** | 14 | +| the `svg` exemption | 0 | -2 | -2 | **2.5** | + +All three are load-bearing and none substitutes for another: the basis fixes the +everyday defect, the child rule bounds the pathological ones, and the exemption keeps +the child rule from paying for that bound with the collapse affordance. Each row was +driven by actually removing the declaration and re-measuring. Contrast drafts 1 and 3, +where four of five and two of three declarations measured inert. + +## Cost of the universal rule + +`white-space: nowrap` on every child means no child of this header can wrap. That is +correct here — it is a single-line identity row of a slug, chips and a count, none of +which should ever wrap — but it is a real constraint on future content. Anything +genuinely multi-line belongs in `.models-provider-body`, not the header. The +alternative was another enumerated exception list, which is what drafts 2-4 already +disproved. + +## What is deliberately NOT changed + +- **The shared `.models-chip` rule.** Only the toggle's own children are touched. The + model-row chips at `Models.tsx:1447-1455` sit in a non-wrapping `.row` with long + translations (de "Benutzerdefiniert", ru "Пользовательская"); a primitive-level + change there was rejected in draft 1. +- `overflow-wrap: anywhere` stays on the existing name rule although the inline + `white-space: nowrap` makes it dead. Removing it is unrelated cleanup; it is noted + so the next reader knows it is inert rather than load-bearing. + +## Diff scope + +- `gui/src/pages/Models.tsx` — one inline style value. +- `gui/src/styles-models-workspace.css` — two rules added (the universal child rule + and the `svg` exemption); the existing `min-width: 0` on the toggle is kept. +- `gui/tests/models-provider-head.test.ts` — extended; the existing line-29 + `min-width: 0` assertion stays valid and must not be removed. +- `gui/tests/helpers/css-declarations.ts` — NEW. The shared source-text CSS readers, + lifted out of `viewport-scroll-caps.test.ts` so two tests can use one copy. +- `gui/tests/viewport-scroll-caps.test.ts` — its four file-local helpers are deleted and + replaced by an import from that module; its assertions are unchanged. + +## Regression test (red first) + +Use the effective-declaration reader so a commented-out or custom-property occurrence +cannot satisfy an assertion. + +**It lives in `gui/tests/helpers/css-declarations.ts`**, which exports +`effectiveDeclaration`, `ruleBodies`, `allRuleBodies` and `withoutComments`. + +That module is part of this change. The reader originated in +`viewport-scroll-caps.test.ts` (PR #2915) as four **file-local, unexported** functions, +so it could not be imported as first planned. B resolved that by moving all four into the +shared module and rewriting the original test to import them — one copy, not the third +copy that copying them here would have produced. + +**What this gate can and cannot see.** The reader's own comment (:53) records that it +does not model competing specificity, `!important`, or at-rule nesting. So it proves +the four declarations exist on the exact selector, and nothing about computed layout: +the ellipsis, the containment numbers, and the 14px chevron are **measurements** +recorded above, not unit assertions. That split is deliberate and is why the tables in +this document are the primary evidence for the fix. + +1. The provider-toggle button in `Models.tsx` carries `flex: "1 1 auto"`. The + negative half must be **scoped to that style object**, not a file-wide search for + `flex: 1` — a legitimate bare `flex: 1` exists at `Models.tsx:2162`, so a global + assertion would be wrong. This is the declaration whose absence reproduces the + user's screenshot. +2. `.models-provider-toggle > *` declares `min-width: 0`, `overflow: hidden`, + `text-overflow: ellipsis` and `white-space: nowrap`, with a comment naming the + defect so the rule is not narrowed back to specific children later. +3. `.models-provider-toggle > svg` declares `flex: none`. Assert this **separately** + from rule 2: it is the declaration whose removal reintroduces the 2.5px chevron, and + a reader who sees only the universal rule is likely to delete it as redundant. +4. Every direct child the toggle renders is an **element**, never a bare string. The + universal selector cannot reach an anonymous flex item, so this is the invariant the + `> *` bound actually rests on. Assert that the JSX between the toggle's opening and + closing tag contains no bare interpolation — all seven children today are `` or + ``. + +A declaration test cannot observe clipping, so the containment table above stays a +recorded measurement rather than a unit assertion. + +## Render grounding + +Screenshots at the failing width, captured from the running dashboard and then **read +back** rather than merely produced: `evidence/010-before-ko-1100.png` and +`evidence/010-after-ko-1100.png` (ko, 1100px, dpr 2, the second chip-bearing provider +row). The before shot is the shipped build with the fix reverted **in the browser** by an +injected override, so both images come from the same code and differ only by the two +declarations. + +| | before | after | +|-|-------:|------:| +| capture height (dpr 2) | 696px | **416px** | +| rows containing ink | 365 | **101** | +| name box | 8.6px, chip on 6 lines | **43.6px, chip on 1 line** | + +Pixel readback is the observation step: ink was counted per row against the sampled +background luminance, which is what confirms the vertical sprawl actually collapsed +rather than the clip rectangle merely shrinking. + +The chevron was verified the same way, since a rendered width is exactly what the round-5 +audit found the numbers hiding. Under the adversarial stress row at 1100: + +| | `getBoundingClientRect` | drawn glyph span | +|-|------------------------:|-----------------:| +| shipped (exemption present) | **14.0px**, `flex-shrink: 0` | 9.5px | +| exemption overridden away | 4.9px, `flex-shrink: 1` | 36.8px of smeared ink | + +Two notes on reproducing this. `Page.captureScreenshot` hangs indefinitely over CDP +unless `Page.bringToFront` is called first. And an injected `