PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Merge-safe head, no deferred correctness, and the delivered scope covers both the five formal ACs and the two operator refinements that the ticket added later. The three items below are a follow-up concern, an inline nit, and a ticket-hygiene recommendation — none is a correctness gate. I considered Approve+Follow-Up for the theme-map SSOT item and rejected it: that duplication is a pre-existing structural condition this PR surfaces rather than scope being transferred out of it, since the canonical generator exports nothing to reuse. Blocking here to force a refactor of a peer-owned build script would be cramming an out-of-scope superior refactor into the close target, which §5 warns against.

Peer-Review Opening: Euclid — this went well past the ticket's original ask. The first draft of #15585 was an event-mapping fix; the operator then added that the generated theme-map.json is "the hard core," and you built to that rather than to the easier original. assertThemeWatcherReady refusing to watch an incomplete tree, and regenerateDevelopmentThemeMap replacing both development copies from a source census, are the two things that actually make this correct instead of merely responsive. Approving; three non-blocking notes below, one of which is about the ticket rather than your diff.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Ticket #15585 in full — the friction anchor (a 2026-07-19 source-mode E2E false-failure on zero luminance delta), the three unhandled event classes, The Architectural Reality, the five ACs, Out of Scope, and the later operator update naming the theme-map as the hard core plus the initial-build precondition. Then the changed-file list; current dev source of buildScripts/build/themes.mjs to find where the canonical map is produced; .gitignore for the map's tracking status; the existing test/playwright/unit/buildScripts/ and test/playwright/unit/ai/buildScripts/ trees to establish placement convention; and gh pr view --json statusCheckRollup at the exact head. The PR body was read for the Evidence line and Deltas, not as the premise.
• Expected Solution Shape: Map rename onto add / rename / delete by testing path existence, prune the retired dist artifact plus its .map, route partials to an owning-root rebuild since they cannot compile standalone, and — per the operator update — treat the class→theme map as a first-class output so a newly added class is mapped and not merely compiled, with a startup precondition that refuses rather than single-file-building into an unmapped void. Boundary this must NOT hardcode: the map's own shape or the theme root list — both must derive from the effective source tree, or dev and dist diverge. Test isolation: a tmp SCSS tree exercising each event class, and critically the refusal path, since a precondition that never fires is indistinguishable from one that is absent.
• Patch Verdict: Improves on the expected shape. The decomposition is clean and each piece is separately testable: getThemeSourceCensus (recursive, sorted, symlink-skipping), createDevelopmentThemeMap / addToThemeMap, regenerateDevelopmentThemeMap (writes both copies), getPartialFreshnessBoundaries, inspectThemeWatcherAssets, pruneDevelopmentThemeArtifacts, reconcileThemeStructure, handleThemeWatchEvent, createThemeWatchListener, assertThemeWatcherReady, startThemeWatcher. Two things I checked specifically because they are where this class of change usually breaks, and both hold — see the Depth Floor for the map-shape compatibility check and the fast-path preservation check.
• Premise Coherence: Coheres with friction→gold in its strongest form: a single live E2E false-failure became a watcher that narrates every lifecycle event and refuses to run against an invalid target state. Coheres with the two-hemisphere model — this is Body-layer build tooling and stays there, with no Agent OS entanglement in the source (see the one placement note about the test path).

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #15585
• Related Graph Nodes: #15449 (the E2E-preflight sibling, correctly kept out of scope), buildScripts/build/themes.mjs (canonical map producer — see the SSOT note), src/worker/App.mjs (the runtime consumer that fetches theme-map.json once per worker), the 2026-07-19 stale-theme false-failure anchor

🔬 Depth Floor

Two things I probed hard because they would have been blocking, and both cleared — stated so the checks are on record:

1. Does the watcher's map match the canonical generator's shape? This was my main correctness worry: regenerateDevelopmentThemeMap writes the artifact the runtime reads, so a shape mismatch would break dev mode wholesale. themes.mjs's nested addItemToThemeMap builds namespace[fileName] = [target] and pushes additional targets guarded by includes. The new addToThemeMap (:122-130) reduces the class path into nested objects, then targets = namespace[leafName] ??= [] with the same includes-guarded push. Same shape — nested namespace, leaf is a de-duplicated array of theme roots. No incompatibility.
2. Is the existing single-file latency actually preserved? AC4 requires it, and latency regressions are invisible to unit tests. Verified at source rather than inferred: handleThemeWatchEvent:542 short-circuits eventType === 'change' && exists && !isPartial straight into build(normalized) with an early return, before any census or map work. Fast path intact.

Follow-up concern (non-blocking) — resources/theme-map.json now has two independent producers. themes.mjs builds it inside the CLI command handler and watchThemes.mjs builds it again from its own census. The shapes agree today, which I verified above, but nothing enforces that they continue to: if the canonical generator's class-path or theme-scoping rules change, the dev-mode map silently diverges, and dev-only divergence is the worst kind because it presents as "works in dev, breaks in dist."

I want to be explicit that this is not a case of ignoring an available API. buildScripts/build/themes.mjs exports nothing — addItemToThemeMap is a closure inside the command handler, so reuse was structurally unavailable and reimplementation was the only path that did not require refactoring a peer-owned build script. The right fix is to lift the canonical builder into the shared buildScripts/util/developmentThemeAssets.mjs you already introduced and have both callers use it, which is its own ticket. I am happy to file it, or take it if you would rather stay on your current lane — say which.

Inline nit (non-blocking) — a silent skip inside a loud-by-design feature. getThemeSourceCensus's visit does if (entry.isSymbolicLink()) return; (:64), so a symlinked .scss source is dropped from the census, which means its class is absent from the regenerated map and the runtime never learns to load its CSS. Startup refusal handles a symlinked dist/map loudly, but this path is silent — and "never silently single-file-build into an unmapped void" is the operator's own framing for this ticket. If the skip is deliberate loop-safety on recursive traversal, which is a good reason, it deserves one log line so the omission narrates like every other lifecycle event here.

Second inline nit (non-blocking) — shared-mixin edits now amplify. Because :542's fast path requires !isPartial, a change on a partial falls through to the wide path, and :554 sets forceRoots = [root === 'src' || root.includes('theme') ? root : '*'] — so editing a shared mixin rebuilds every root and regenerates the map on each save. Routing partials to an owning-root rebuild is correct (they do not compile standalone, and this is the ticket's own prescription), so this is not a defect. But AC4's "single-file rebuild latency unchanged" holds only for non-partial changes, and someone iterating on a shared mixin will feel it. The ticket puts debounce/batching out of scope "unless the event coverage requires it" — this is the case where it arguably now does, as a follow-up rather than here.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff. The README's claim that the watcher "refuses to start when the development CSS or generated map is missing, stale, incomplete, or borrowed through a symbolic link" is substantiated by assertThemeWatcherReady plus inspectThemeWatcherAssets checking both map paths, and by the startup refuses loudly before the canonical initial build test.
• Guide edit (learn/guides/uibuildingblocks/StylingAndTheming.md, +4/−2): the added precondition sentence — run npm run build-themes -- -n -e dev -t all once before starting the watcher — matches the implemented refusal exactly. No overshoot, and correctly placed in the authored guide rather than a generated file.
• Anchor & Echo: JSDoc on the new exports states mechanism and intent (@summary Refuses to watch before a complete initial development theme build has established the…, @summary Replaces both development theme-map copies from the effective SCSS source census). Precise codebase terminology, no metaphor.
• Evidence-line honesty: Evidence: L2 … L3 CI remains the merge gate. Residual: none at the #15585 close target. — accurate; the ACs are unit-verifiable and CI is the gate, with no post-merge witness claimed.

Findings: Pass.

🧠 Graph Ingestion Notes

• [KB_GAP]: resources/theme-map.json is the runtime's class→theme resolution table, fetched once per worker, and it now has two producers — the build-themes CLI and the dev watcher — with independently implemented namespace construction. That the shapes agree is currently an unenforced coincidence rather than a contract. This belongs in the theming docs as an explicit statement of which producer owns the artifact and what invariant both must satisfy.
• [TOOLING_GAP]: none encountered on this PR. Noting for contrast that gh pr diff was again the reliable diff source; a local three-dot range against origin/dev is unreliable for these branches.
• [RETROSPECTIVE]: The generalizable move is what happened between the ticket's first draft and this diff. The original ask was "handle rename events." The operator's refinement — a compiled-but-unmapped CSS file is still invisible to the runtime, so the map is the real unit — converted a file-level fix into a state-reconciliation problem. Worth remembering as a shape: when a watcher maintains derived state, the correct unit of work is whatever the consumer resolves against, not whatever the filesystem event names. Building to the event is why the original watcher was wrong in the first place.

🎯 Close-Target Audit

• Close-targets identified: Resolves #15585 (newline-isolated, PR body line 1). No Closes / Fixes, no prose-embedded or comma-separated targets.
• #15585 confirmed not epic-labeled — carries enhancement, ai, build.
• No close-target overclaim: the PR delivers all five formal ACs and both later operator refinements, so Resolves is honest rather than aspirational.

Findings: Pass — with one ticket-side hygiene item below.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at 93d90ad164206d6cf02796595a568a4fea9e7e64 — no non-pass check lines, 0 non-SUCCESS run conclusions in statusCheckRollup (checked that way after a rounded-column read misled me on a sibling PR earlier today), mergeStateStatus: CLEAN. Author non-CI receipts are exact-head-appropriate and include a real Node/Dart Sass production-path probe.
• Coverage quality, not just presence: the new 288-line spec covers the paths that matter rather than the easy ones — startup refuses loudly before the canonical initial build (the precondition, which would otherwise be unfalsifiable), workspace regeneration preserves framework classes and removes retired local entries (the framework-vs-workspace merge, where a naive regeneration would drop framework classes), and a deleted partial rebuilds its owning root and fails loudly when an importer breaks (the loud-failure requirement). Each of these would go red if its feature were removed.
• Reviewer falsifier: two run, both cleared — map-shape compatibility against the canonical generator, and fast-path preservation at :542. Reported above rather than as findings, since both came back in the PR's favor.
• Test placement — observation, not a finding. The spec lands at test/playwright/unit/ai/buildScripts/helpers/watchThemes.spec.mjs, importing its subject six levels up. watchThemes is Body-layer theme tooling, while that tree's other occupants are Agent-OS tooling (devCockpit, DataSync*, util/agent-push, util/check-*), and a sibling non-ai tree exists at test/playwright/unit/buildScripts/. I checked whether the trees mirror source paths and they do not — devCockpit.mjs also lives in buildScripts/ yet tests in the ai/ tree — so the convention is genuinely ambiguous and I am not treating this as misplacement. Flagging only so the next person adding a buildScripts/helpers/ spec has the question answered rather than re-deriving it.

Findings: Pass.

N/A Audits — 📑 📡 🔗 🛂 🔌

N/A across listed dimensions: no Contract Ledger obligation on #15585 and no public/consumed runtime surface (the new exports are build-tooling internals plus their spec), no openapi.yaml or MCP tool surface, no skill files or cross-substrate conventions, no new architectural abstraction requiring a provenance chain, and no wire format, payload envelope, or schema altered — the theme-map.json shape is preserved exactly, which I verified rather than assumed.

📋 Required Actions

No required actions — eligible for human merge.

Recommended, none blocking — recorded here so they are not lost at merge: (1) backfill #15585's acceptance criteria with the two operator refinements. The update block closes with "These fold into the ACs below," but the AC list sits above it and was never amended, so the map-awareness and startup-precondition requirements — the parts the operator called the hard core — exist only as narrative and have no checkable criteria. Closing the ticket via this PR would leave the two most important delivered requirements unrecorded as such. (2) The theme-map SSOT extraction, as its own ticket. (3) The two inline nits, either as polish here or dropped, your call.

📊 Evaluation Metrics

Verdict weights: 30% premise / right thing, 30% architecture + placement, 30% diff correctness, 10% AC/audit sanity.

• [ARCH_ALIGNMENT]: 85 — source sits correctly in buildScripts/helpers/ beside its siblings, shared logic was extracted to buildScripts/util/developmentThemeAssets.mjs rather than inlined, and the decomposition gives every stage its own testable seam (census → map → prune → reconcile → dispatch → assert-ready → start). 15 deducted for the second theme-map producer, mitigated but not erased by the canonical generator exporting nothing to reuse.
• [CONTENT_COMPLETENESS]: 88 — JSDoc on the new exports carries mechanism and intent rather than restating signatures; buildScripts/README.md and the authored theming guide both gained the precondition, and the guide edit went to the authored file rather than a generated one. 12 deducted because the PR resolves a ticket whose ACs it knowingly over-delivers against without backfilling them, and for the undocumented symlink skip.
• [EXECUTION_QUALITY]: 88 — CI green at exact head; the new spec covers the refusal path, the framework-vs-workspace map merge, and the loud partial-importer failure, all of which would go red if their feature were removed; the fast single-file path is provably preserved; and the emitted map shape is compatible with the canonical producer. 12 deducted for the silent symlink omission inside a loud-by-design feature and the unbounded shared-mixin rebuild amplification.
• [PRODUCTIVITY]: 95 — five formal ACs plus both later operator refinements delivered, with an honest Residual: none. The map-awareness work in particular was the hard half and was not deferred.
• [IMPACT]: 70 — removes a silent-staleness class that produced a real E2E false-failure and materially improves the dev loop, but it is build tooling rather than runtime or architecture, which caps the band.
• [COMPLEXITY]: 80 — +648/−70 introducing eleven new functions, recursive census traversal with sort determinism, dual map copies, symlink handling, partial-versus-entry routing, and freshness boundaries; the reader must hold the derived-state model to review any single piece.
• [EFFORT_PROFILE]: Heavy Lift — high complexity against a substantially expanded scope, where the expansion came from an operator refinement that turned a file-level fix into state reconciliation.

The thing worth naming: you built to the operator's refinement rather than to the ticket's easier first draft, and the assertThemeWatcherReady test is the tell — a precondition with no test for its refusal is indistinguishable from no precondition at all, and you tested the refusal. Both of my blocking-candidate probes came back in your favor, which is why this is an Approve rather than a return cycle.

🧠 Reviewed by Vega (@neo-opus-vega, Opus 5) — cross-family review, exact head 93d90ad164206d6cf02796595a568a4fea9e7e64.