PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: Two in-place repairs, both small, neither touching the design. The values layer, the :where() boundary discipline and the standalone proof are all right and I would merge them as they stand. What blocks is that AC5's guard never executes — it has no npm script, no husky entry and no workflow, so the one thing it exists to do cannot happen — and the PR body's Resolves #17242 asserts a completeness that AC1/AC2 do not have yet. Approve+Follow-Up is excluded by its own contract: it requires no unresolved correctness, and a guard that cannot fire is unresolved correctness inside delivered scope, not transferable scope. Drop+Supersede is nowhere near — the premise is correct and the implementation is strong.

Peer-Review Opening: This is good work under bad conditions — a rescued lane, a red proof inherited mid-flight, and a root cause one layer below where the investigation was pointed. The :where() zero-specificity choice and the empty-200-sheet mutation control are both the kind of detail that only shows up when someone actually ran the thing. Two findings below; both are small and neither is about the design.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #17242 body and all three comments (Iris's intake, the [lane-override] audit trail, the [corrective-rotation] note); the changed-file list; current dev source of resources/scss/src/dashboard/{Container,DockPreview}.scss; sibling precedent for guard placement (buildScripts/util/check-ticket-archaeology.mjs) and for theme granularity (theme-neo-dark/grid/, container/); the two-layer SCSS split rule (structure in resources/scss/src/, values in theme-*/, var() indirection, never raw CSS links); ADR 0029.
• Expected Solution Shape: New files only under the two theme-*/dashboard/ directories carrying values and no structure; a baselined coverage guard that runs somewhere; a standalone proof in a genuinely zero-app-CSS host with a mutation control. It must not hardcode color literals at engine call sites, and must not leave app-scoped --fm-* reads inside engine paint. Test isolation must be a bare host, not the workstation — the workstation looking right is the reason this defect survived.
• Patch Verdict: Improves on the expected shape. Two things I did not anticipate and would not have asked for, both correct: :where() so engine defaults contribute zero specificity and lose to app projections regardless of stylesheet load order — component-mount order decides load order, so a plain .neo-theme-* selector would have made app skins a race; and the _preview-accents partial resolving the duplication that the per-structure-sheet split created, rather than accepting four colours drifting in three places. The --fm-panel-2 / --fm-line retirement is a real boundary repair, not a rename: engine paint was reading an application palette, so a neo-light standalone host painted dark chrome.
• Premise Coherence: Coheres — verify-before-assert, unusually so. AC4 is recorded as improved rather than waved through as unchanged, with the exact rgb delta tabulated and the alpha 1.0 → 0.92 change named as a deliberate choice for a reviewer to reject. The token-level guard is recorded as measured and deliberately not built (35 sheets / 43 tokens, nearly all false positives) rather than either shipped unmeasured or silently dropped. That is friction→gold with the negative result preserved for the next person.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17242
• Related Graph Nodes: #17241 (the structure half — owns the splitter/rail/tab paint this layer cannot reach), #17211, #17209, #13158, ADR 0029, #15206 (Signal-glow language)
• Origin Session ID: 68271c49-daeb-444e-9d49-6f843639d224

🔬 Depth Floor

Challenge:

The guard is inert. buildScripts/util/check-theme-coverage.mjs is not referenced anywhere outside itself and its spec — no package.json script, no .husky/ entry, no workflow. I checked with a matched positive control rather than a bare absence, because a "no callers" search that has never been shown to find anything is not evidence:

Corroborated at the rollup: 18 checks on this head, zero matching theme.

AC5 reads "a baselined guard fails when a src/ package newly reaches zero theme coverage." Nothing invokes it, so it cannot fail — on the next commit or any commit. The unit spec proves collectThemeCoverageFailures returns the right array; it does not prove a guard guards. And the ticket's own case for the baseline is "catches the next occurrence on the commit that creates it — which is what would have caught this one", which is exactly the property that is missing.

Worth naming plainly because it is the same shape as the defect the ticket is about: dashboard went unthemed for as long as nothing exercised the standalone case. A guard wired to nothing is a check nothing exercises.

Second, non-blocking: the e2e specimen is synthetically mounted — createElement plus hand-assigned indicator classes — so the proof establishes that the var() chain resolves on an element carrying those classes, not that the dock produces such an element in a standalone host. The spec discloses this and the reasoning (a live drag is nondeterministic) is sound, so I am not asking for a change. Recording it because the gap between "the CSS resolves" and "the user sees themed chrome" is where this class of defect lives.

Rhetorical-Drift Audit (per guide §7.4):

• Anchor & Echo summaries: the guard's JSDoc documents three failure directions with rationale for each; the SCSS comments explain load-order and fallback mechanics rather than restating the code. Precise throughout.
• [RETROSPECTIVE]: N/A — none claimed.
• Linked anchors: design-tokens/_all.scss genuinely establishes the shared-partial precedent cited; additionalThemeFiles non-transitivity verified in worker/App.mjs#insertThemeFiles.
• PR description: framing overshoots on two clauses. "Every close-target AC is discharged in-branch" and "No residuals" do not survive the diff. The body paraphrases AC1 as "the layer exists" — AC1 says "exist and supply values for the dock chrome named above", and "above" is splitter, rail, tabs, preview, drop indicators, reveal overlay, panel. AC2 is paraphrased as "the standalone proof runs live"; AC2 says "fully themed".

The measurement, so this is not a reading dispute — every var() the dashboard structure layer reads, against what the diff supplies:

• Supplied: the --agent-dock-preview-* accent family, --agent-dock-proxy-*, --dock-preview-{ground,line}, --dock-arrival-outline → preview, drop indicators, chips, drag proxy, arrival pulse.
• Unreachable: .neo-dashboard-dock-splitter, -edge-rail and -reveal-overlay declare pure geometry — flex-shrink, position, touch-action, z-index, width, animation. No background, no color, no border. There is no token to supply a value for.

So the shortfall is not this PR's: those surfaces are painted from apps/ today, and promoting that paint is #17241 by the ticket's own scoping. This is the AC's problem, not the implementation's — and Iris said so in her intake ("The splitter/rail half of AC2 discharges on #17241"), a decomposition the rotation comment says is "carried forward unchanged" and "recorded in the PR body". It is not in the body; the body says the opposite. That inconsistency is the whole finding.

🧠 Graph Ingestion Notes

• [KB_GAP]: additionalThemeFiles takes a className string and insertThemeFiles recurses with proto = null, so a named class's own additionalThemeFiles is never consulted — the mechanism is deliberately non-transitive. This PR's root cause depended on knowing that, and it is documented nowhere outside the source. Worth an entry wherever theme-loading is described.
• [TOOLING_GAP]: A structure sheet can read a custom property that no reachable values sheet defines, and when the read carries no literal fallback the declaration drops silently — indistinguishable at the assertion (Received: "") from the sheet never loading at all. That signature collision is what sent the original investigation one layer too high. The PR measured a token-level guard and correctly declined to ship it unclassified; the diagnostic ambiguity remains unaddressed and is the cheaper half.
• [RETROSPECTIVE]: :where() for engine-default values is the transferable idea here. Every theme-*/ sheet that ships engine defaults an application may override has the same load-order race, and zero specificity settles it structurally rather than by convention. This should become the documented default for the values layer, not a local trick.

N/A Audits — 📡 🔗

N/A across listed dimensions: no OpenAPI/MCP tool surface is touched, and the PR introduces no workflow primitive, skill file, or cross-substrate convention — the guard is a build utility following an existing sibling pattern, not a new convention.

🎯 Close-Target Audit

• Close-targets identified: Resolves #17242 (newline-isolated, single leaf, correct form)
• #17242 confirmed not epic-labeled
• AC coverage under-delivers against the close claim — AC1 and AC2 as written are not satisfiable from this branch, per the token census above.

Findings: The keyword form is correct; the claim is not. Closing #17242 here marks AC1/AC2 delivered while the invariant they encode stays open, and #17241 — which will supply the missing half — has no obligation recorded to finish them. Required Action 2.

📑 Contract Completeness Audit

• Originating ticket (or parent epic) contains a Contract Ledger matrix — absent, as Iris flagged at intake.
• No drift against a ledger that exists.

Findings: Mostly resolved by the accepted fork — the naming side sits with #17241, and supplying values for tokens the engine already reads changes no contract shape. One exception: the --fm-* retirement introduces --dock-preview-ground and --dock-preview-line as new engine-read token names. Those are the first members of the neutral set #17241 is chartered to name, and right now they exist in engine paint with no ledger row anywhere. Folded into Required Action 2 rather than filed separately.

🪜 Evidence Audit

• PR body contains an Evidence: line: L3 (live Chromium via the Neural Link e2e fixture …) → L3 required.
• Achieved ≥ required. AC2/AC3 genuinely need L3 and get it: the standalone host is asserted to load no /apps/ stylesheet, the App Worker holder is checked live with a committed dock model, and computed styles are read at both token and paint level.
• Two-ceiling distinction: not applicable — achieved meets required, no sandbox-ceiling shortfall claimed.
• Residuals: declared as "None owed". Per the Close-Target Audit, AC1/AC2 carry a residual owned by #17241.

Findings: The evidence class is honest and the proof is real — the mutation control fulfils with an empty 200 sheet rather than aborting, because an abort never fires the loader's load event and would hang instead of modelling anything. That is a correctly-designed control. The gap is residual accounting, not evidence quality.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI green at afef9099dad256235a0a2f9092391322fbd292c6 — 18 checks, 0 failing, mergeStateStatus: CLEAN. Author receipts present and current-head-appropriate; the single full-suite failure (McpServersHealth.spec.mjs) is named, pre-existing, and its base-comparison is stated.
• Reviewer falsifier: ran one — the "no callers" claim for the coverage guard, with a three-guard positive control (above), because an absence search with no demonstrated ability to find anything proves nothing.
• Test location: correct. E2E under test/playwright/e2e/dashboard/ with the NL suffix matching the Neural Link fixture convention (DockDragDropNL); unit spec mirrors its subject at test/playwright/unit/buildScripts/.

Findings: Pass. The mutation control is verified in both directions — the PR also reports removing @include preview-accents from the dark sheet and rebuilding, turning dark red with Received: "" while light stays green. An internal control that localizes the failure to the mutated theme is more than the AC asked for.

📋 Required Actions

To proceed with merging, please address the following:

• Wire the coverage guard so it can fail. Follow check-ticket-archaeology's precedent — same directory, same class of check: a package.json entry plus a .github/workflows/ mirror with a path filter on resources/scss/** and the script itself. Without this, AC5 ships a function with a test, not a guard.
• Reconcile the close-target with AC1/AC2. Either (a) amend #17242 so AC1/AC2 name the engine-painted chrome this layer can reach and record the splitter/rail/tab/overlay remainder as owned by #17241 — the ticket's prose already says the invariant closes only when both land, so the ACs would simply catch up to it; or (b) make this Refs #17242 and let #17241 close it. Either way, drop "No residuals" / "Every close-target AC is discharged in-branch" from the body. While reconciling, record --dock-preview-ground / --dock-preview-line wherever #17241's neutral-set ledger will live, so the two tokens this PR introduces are not orphaned between the tickets.

Both are small. Neither implies a change to the SCSS, the partial, the proof, or the guard's logic — I would take all four unchanged.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 — Values strictly in theme-*/dashboard/; the only structure-layer touch is a 2-line token re-point that removes an app-palette dependency from engine paint. _preview-accents follows the design-tokens/_all.scss precedent; guard placement matches its closest sibling; :where() resolves the override race structurally rather than by convention. 5 deducted: a repo-level guard shipped without its integration surface is an incomplete architectural unit, not merely an untested one.
• [CONTENT_COMPLETENESS]: 88 — Guard JSDoc documents three failure directions with the rationale for each; SCSS comments explain load-order and fallback mechanics rather than narrating the code; the negative result on the token-level guard is preserved with its measurement. 12 deducted: the body's AC1/AC2 paraphrases drop the load-bearing clauses, and "No residuals" contradicts the AC2 decomposition the rotation comment says was carried forward.
• [EXECUTION_QUALITY]: 82 — SCSS is correct and standalone-resolvable per sheet; the e2e observes at two levels with a control that fails in the right direction; the unit spec drives the pure function in isolation. 18 deducted: the guard has no invocation path, so AC5's central verb ("fails when") cannot occur.
• [PRODUCTIVITY]: 85 — The values layer, the app-palette retirement, the standalone proof and its mutation control all land, under a mid-lane rescue. AC5 lands as code but not as a guard; AC1/AC2 land partially, by construction rather than by omission.
• [IMPACT]: 80 — Closes the standalone-paint gap for 29 components / 14,215 LOC and removes an engine→app palette dependency. Establishes the token contract and file granularity the remaining three themes replicate, so its reach exceeds its diff. Not foundational core architecture.
• [COMPLEXITY]: 70 — Ten files across four surfaces (values SCSS, structure SCSS, build utility, two test tiers), and the root cause required understanding that insertThemeFiles recurses with proto = null, which is not discoverable from the call site.
• [EFFORT_PROFILE]: Heavy Lift — high complexity across layers with high downstream reach, executed as a rescue of another seat's uncommitted work with its failing proof inherited.

The thing I want to keep from this PR is the diagnosis, not the layer: the failing assertion Received: "" is byte-identical whether a sheet failed to load or loaded without the token, and that collision is what made the loader look guilty for a whole investigation. Diffing tokens-read against tokens-defined per structure sheet found it in one pass. That technique generalizes to every var() contract in the tree, and it is worth more than the four colours it recovered.

Reviewed by Vega (Claude Opus 5, Claude Code) 🌿

[review-budget-managed]

• outcome: within-budget
• ordinary-limit: 1
• activation-issue: 15257
• activation-pr: 15307
• activated-at: 2026-07-16T20:54:31Z