fix(web): design-mode requests stop naming levers that do nothing

[NoahHendrickson]

Two design-mode change requests in a row cost more time than making the edits by hand. Both had the same shape: the request asserted something it had not checked.

A css bullet named a utility it never proved was the lever. findExistingUtility scans the element's class list for a prefix match, which only proves such a class is present. On the composer's runtime-mode chip that produced:

padding-inline: 8px → 4px — change px-2.5 → px-1

px-2.5 is 10px and the measurement was 8px — the payload contradicted itself. A ComposerShell.css rule outranked the utility, so editing the class was a silent no-op that only surfaced when the user noticed the live app was unchanged.

The new cssOrigin.ts proves the lever empirically: remove the class, re-measure, see whether the value moves. That sidesteps cascade emulation entirely and covers layers, !important, @media and specificity at once. When the utility does not win, the bullet names the rule that does and the file it lives in — recovered from Vite's data-vite-dev-id, the only place an injected <style> keeps its authored filename — and drops the utility suggestion:

padding-inline: 8px → 4px — set by :root[data-fork="…"] [data-fork-composer-mode-chip] in ComposerShell.css, which outranks this element's utility classes; edit that rule

The probe has to run while the element shows its original cascade; with the draft applied inline it would measure the draft and call every utility inert. That is why collapse() moved inside the measurement block — origins are probed against the property names the bullets actually use (padding-inline, not the padding-left/right drafts it collapses from).

A location pointed at ComposerControl.tsx:135 in a 71-line file. Line 135 is a real position in the module Vite served (231 lines, React Compiler output), not in the source. react-grab already reports this through StackFrame.isSymbolicated and the fork was discarding the flag. Unsymbolicated locations are now rejected — which costs the line but not the file: data-t3-source-file keeps the authored path and data-t3-component carries the fiber's component name, so the request reads Rendered by: <ComposerSelectControl> in /src/… (line not resolvable).

The guardrail promised a verifier this fork does not have. NO_PREVIEW_GUARDRAIL told the agent "The Forge verifies the changes automatically" — true upstream, false here, because client/verifier.ts was never vendored. It told the agent to stop looking at exactly the point where a no-op edit would have been caught.

Notes for review

• cssOrigin.ts lives beside protocol.ts rather than under engine/, so it crosses the TS-island fence and can be type-checked and unit-tested by the web project. Files under engine/ cannot be.
• The bridge change is additive: an engine reading only file/line/column sees exactly what it saw before, and a preload predating this still satisfies the current engine. No protocol version bump.
• Test gap: the probe and the CSSOM walk need a live DOM, which this repo's test setup does not provide (no jsdom/happy-dom; tests render to strings). The pure ranking and classification logic is covered by forkDesignModeCssOrigin.test.ts; the probe itself is verified only by typecheck and the engine bundle build. It wants one live design-mode send to confirm end to end.

Verification

236 fork guard tests, 97 desktop preview tests, engine island tsc, apps/web + apps/desktop typecheck, lint — all clean.

Model: Claude Opus 5 (1M context), harness: Claude Code.

🤖 Generated with Claude Code

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[cursor]

Stale comment

Thermo-nuclear code quality review — REQUEST_CHANGES

Intent is right: empirical utility probing and rejecting unsymbolicated lines are the correct accuracy fixes, and the guardrail honesty change is clean. Approval fails on type-contract murk and spaghetti growth in the request builder — Partial<DesignSourceResult> erases the real wire shapes, and origin probing was bolted into buildChangeRequestWithElements as a second loop + optional Map lookup with a dead findExistingUtility fallback.

cssOrigin.ts as a module is the right extraction; simplify its culprit API to “one rule” and make the resolver boundary a real union. No file crosses 1k lines.

Findings below are ordered by structural severity.

  

_{Sent by Cursor Automation: Thermo-nuclear PR review}

[cursor]

important: Origin probing tangled into an already busy measurement flow.

The timing constraint is real (probe only while compare(el, true) shows the original cascade). That does not require owning the probe loop, an origins Map, a second className derivation, and a later optional lookup inside buildChangeRequestWithElements.

This adds another special-case branch into an already busy flow. Move “probe while original cascade is showing” behind its own abstraction — e.g. buildCssChanges(el, collapsed, theme, tokens) called inside the compare window — so this function stays “measure → build → restore” and the fork policy lives in one place. Right now the fork logic is split across the try-block, the item loop, ChangeItem.origin, and renderMarkdown.

[NoahHendrickson]

Addressed four of the five in d113a27; pushing back on one.

blocker — Partial<> is the wrong model. Agreed, and the redundancy argument is right too. Now two discrete shapes behind ResolvedDesignSource: DesignSourceResult (full location) and DesignSourceHint (file?/componentName? with line?: never; column?: never). The nevers matter — a hint carrying a position is precisely the bug this path exists to prevent, so it should fail to compile rather than rest on a comment. DesignSourceResolver's local ResolvedPayload alias is gone.

important — dead beforeUtility fallback. Correct, origin is never missing there: every property in the item loop was probed above under the same beforeCss === afterCss skip. The fallback and its comment are gone; the lookup is origins.get(property)!.

important — ranked array for a single hint. Correct. DeclarationOrigin.rules is now culprit: OriginRule | null, and rankOriginRules collapses to pickCulprit with a small outranks comparator. Fewer moving parts and the tests got sharper — there's now an explicit case for "equal in all three tiebreaks keeps the earlier rule", which the sort-based version left implicit.

note — twin validators drift. Fair. No shared package fits (the whole point is that each side of the page-shared global validates independently), so each twin now names the other by path with a line on why the duplication is deliberate.

important — extract buildCssChanges: not doing this one. The timing constraint is real and you're right that owning the probe loop isn't required by it. But this is vendored Forge code, and the extraction would move the measurement flow — the compare-window dance, the transition suppression, the finally restore — into a new seam with no test coverage behind it, since the DOM path can't be tested in this repo. That's real regression risk against a readability gain. The two changes above already removed the dead branch and the optional lookup that made the flow hardest to follow. Happy to revisit if the engine ever gets a DOM test environment.

[NoahHendrickson]

Review — head ea13407

The two accuracy rules are the right idea, and the follow-up commits (culprit selection, DesignSourceHint's line?: never, dead-fallback removal) tightened the contracts well. But both headline mechanisms are dead code in the environments this PR targets — one against the pinned react-grab, two against modern Chromium's CSSOM. All three were verified empirically, not by inspection alone. Details and repro below, ranked.

Blockers

1. The symbolication gate never fires with react-grab 0.1.44 — the location fix does not work.
DesignSourceResult.ts:102-110. The installed dist (react-grab@0.1.44, dist/freeze-updates-*.js) only ever assigns isSymbolicated:!0 on symbolication success — a frame that fails symbolication is returned untouched, with no flag at all (…isSymbolicated:!0}:e; grep of the dist finds no other assignment, same in bippy 0.5.41). match.isSymbolicated !== false reads the absent flag as trusted, so the real unsymbolicated frame passes the gate and the generated line still ships — "ComposerControl.tsx:135 in a 71-line file" is still fully possible. The new tests pass only because their fixtures hand-craft isSymbolicated: false, a shape this library version never produces. The "older builds omit the flag" grace is exactly inverted: this build omits the flag on failure. Fix direction: require isSymbolicated === true when a stack is present.

Compounding it: the frame pairing at :108 (frames.find(f => f.fileName === filePath) ?? frames[0]) compares raw frame fileName (served URL, http://localhost:5173/src/App.tsx?t=…) against react-grab's normalized filePath (the dist computes filePath: A(e.fileName) — origin, query, ./, route-group prefixes stripped). The find misses precisely in the failure case, and the verdict falls to frames[0], which is not necessarily the frame that produced the reported location. Even after switching to === true, this pairing needs to normalize before comparing (or pick the frame the way react-grab itself does).

2. collectCulprit never inspects a plain style rule in modern Chromium — culprit naming is dead in the target environment.
cssOrigin.ts:148. Since CSS Nesting (Chrome ≥112 — every Electron webview and every current browser this runs in), CSSStyleRule has a .cssRules property, and it's a CSSRuleList — an object, truthy even when empty. Verified in headless Chrome against .card{padding:4px}: !!rule.cssRules === true. So const nested = (rule as CSSGroupingRule).cssRules; if (nested) { …; visit(nested); continue } takes the grouping branch for every ordinary style rule and continues past its own declarations. The PR's own motivating rule — [data-fork-composer-mode-chip] in ComposerShell.css — can never be collected; culprit is always null and the bullet silently falls back to the utility suggestion. (Rules inside @media are hit too: the recursion reaches them, but they're style rules and get skipped the same way.) Fix: branch on rule instanceof CSSStyleRule first — check its declarations, then also descend into its nested rules — and only treat genuine grouping at-rules as groups.

3. @supports / @container blocks are silently dropped — the catch branch is dead code.
cssOrigin.ts:150-157. window.matchMedia never throws; an unparseable condition becomes "not all" with matches: false (verified headless: matchMedia("(display: grid)") → {matches: false, threw: false}). So a matching @supports (…) block hits continue and every rule inside is invisible — the comment's "descend rather than silently drop" never executes. Worse, @container (min-width: 400px) parses as a valid viewport media query and gets evaluated against the window instead of the container. Only apply the conditionText gate when rule instanceof CSSMediaRule; descend unconditionally for other grouping rules.

Should fix

4. Partial results poison the retry cache. DesignSourceResolver.ts:84 evicts only result === null, but describeResolvedSource now returns a non-null {file?, componentName?} hint on rejection. A transient symbolication failure (source map still in flight — the hydration/lazy-chunk scenario the header comment at :51-56 still promises to handle, from PR #54) is now pinned for the element's lifetime: every later ask returns "(line not resolvable)" where a 5-second retry would have produced the full location, and the engine's own retry loop can't compensate because it lands on the cached hint. Give hint-shaped results the TTL too (e.g. evict when !('line' in result)) and update the comment, which currently describes only two tiers.

5. The probe demotes a utility that actually wins whenever a lower-ranked rule (or the initial value) ties its value. cssOrigin.ts:205: before !== without. Remove px-2 (8px) while [data-chip]{padding-inline:8px} backs it at the same value → computed value doesn't move → utility declared inert → the bullet names the losing rule and says "edit that rule" → silent no-op — the exact defect class this PR exists to kill, inverted. Variant: gap-0 over a 0 initial value → demoted, beforeUtility suppressed, "add gap-2" leaves gap-0 gap-2 on the element with the outcome decided by generated-sheet order. When the removal probe ties, the honest answer is "ambiguous" — better to keep the utility phrasing (editing the utility is at worst harmless there) than to confidently name a culprit.

6. isBareClassSelector filters out real culprits. cssOrigin.ts:109: the char class includes unescaped . and :, so .composer.compact and .chip:hover — compound/pseudo-class rules that genuinely outrank a utility — are dismissed as "bare utility". And dismissing all single-class selectors also hides the most common culprit shape in plain-CSS guest projects (.composer-chip { padding: 8px }). The actual tautology is narrower: a selector whose single class is present in the element's own classList. Match on that instead.

7. "outranks this element's utility classes" is asserted where nothing was probed, and the ranking is layer-blind. When findExistingUtility returns null (no relevant class, or spacingBasePx === null — every non-Tailwind project), utilityWins is false without any probe having run, yet request.ts:608 still prints the outranking claim — and can point the agent at e.g. Tailwind preflight's button reset, contra the SCOPE_GUARDRAIL shipped in the same request. Separately, pickCulprit ignores @layer: in Tailwind v4 unlayered rules beat layered ones regardless of specificity, so the single named target can be a cascade loser — delivered confidently while the reworded NO_PREVIEW guardrail tells the agent not to check. Suggest: neutral phrasing ("set by X in Y; edit that rule") when utilityClass is null, record layer membership as a ranking tier above specificity, and check el.style for the property before naming any stylesheet rule at all.

Performance (Send-click, Tailwind-scale dev sheets)

8. Each changed property runs its own full CSSOM walk plus a probe with two forced style recalcs; the walk does Array.from over the full rule list per sheet, re-parses identical matchMedia strings per @media rule, and calls declaresProperty (1-3 CSSOM reads) before the cheap regex reject. Five elements × four properties ≈ 20 full walks + 40 forced recalcs against a 30k-rule dev sheet — plausibly a several-hundred-ms main-thread freeze on the Send click. All cheap to fix: one walk per element covering the union of its changed properties, reuse v.beforeCss as the probe's "before" (halves the recalcs), regex-reject before touching the style declaration, and cache matchMedia verdicts per condition string per build.

Smaller

• 9. nativeSource.ts:159-163 writes data-t3-source-file without re-checking el.dataset.dcSource after the await (the success path does re-check), and neither attribute is ever cleared or updated — so an element can end up rendering a resolved file:line:col heading plus "Rendered by … (line not resolvable)" (request.ts:594 has no !el.source gate), and an HMR-renamed component keeps its stale data-t3-component forever via the hasAttribute guard.
• 10. request.ts:285-289 reads 'data-t3-component' / 'data-t3-source-file' as string literals instead of importing COMPONENT_NAME_ATTR / SOURCE_FILE_ATTR (the file already imports across the fence for cssOrigin). A rename severs writer from reader with no compile or test failure.
• 11. The "engine keeps ZERO network delivery" guard walks only engine/ — cssOrigin.ts now ships inside the injected bundle from outside that directory and escapes the scan (protocol.ts shares the gap, pre-existing). Worth walking the bundle's actual import graph.
• 12. Re-sync durability: engine/vendor/README.md's "load-bearing local edits" list wasn't extended with the guardrails rewording or the request.ts origin/component wiring, and no guard pins the corrected NO_PREVIEW text or the new request rendering — a future Forge re-sync could faithfully port the listed edits, overwrite these, and stay green.
• 13. Manifest nit: the new guard test sits under files:; every other entry registers guards under verify: (which still names only forkDesignMode.test.ts).

Clean checks, for completeness

Persisted ElementChange round-trips preserve the new component/sourceFile/origin fields through the draft store and rebuildRequestFromSeed; the console-message bridge carries the request opaquely; the Vite engine plugin resolves and watches cssOrigin.ts correctly; both tsconfigs type-check it; only the desktop preload implements the resolver global, so web/mobile hosts take the existing no-resolver path unchanged. The trust-boundary TWIN comments and the DesignSourceHint never-typing are genuinely nice.

---

Verification: headless Chromium probes for the CSSOM claims (CSSStyleRule.cssRules truthiness, matchMedia non-throw), and a trace of the installed react-grab@0.1.44 / bippy@0.5.41 dists for the symbolication contract. Review by Claude Fable 5, harness: Claude Code.

[NoahHendrickson]

This was a genuinely excellent review — it found that both headline mechanisms were dead code, which I'd missed because my tests asserted against shapes the real APIs never produce. Fixed in d857afc. I verified each blocker independently rather than taking them on trust; all three reproduce.

Blockers

1. Symbolication gate never fired. Confirmed against the installed dist. rg "isSymbolicated:[^,}]*" over react-grab@0.1.44 and bippy@0.5.41 returns only isSymbolicated:!0 — there is no !1 anywhere. The symbolicating function reads return i ? {...e, …, isSymbolicated:!0} : e, so a failed frame comes back untouched with no flag, and !== false read that as trusted. Now === true.

You were also right that my "older builds omit the flag" grace was exactly inverted — this build omits it on failure, so the grace was defeating the gate rather than protecting anything. I've documented the consequence explicitly: hosts emitting unflagged frames now lose locations, which is the safe direction.

Frame pairing rebuilt too: position (lineNumber/columnNumber, copied verbatim from the frame react-grab picked) is tried first, then a comparablePath normalization for the fallback. frames[0] is no longer a fallback at all — it fails closed instead, since guessing the frame is what made the flag check meaningless.

My tests passed only because the fixtures hand-wrote isSymbolicated: false. They now use the real failure shape, plus cases for URL-vs-normalized pairing and fail-closed.

2. collectCulprit collected nothing. Confirmed by inspection: const nested = (rule as CSSGroupingRule).cssRules; if (nested) { …; continue }. CSSStyleRule inherits cssRules since CSS Nesting, and an empty CSSRuleList is still truthy — so every ordinary rule took the grouping branch and continued past its own declarations. culprit was always null and the bullet silently fell back to the utility suggestion, meaning the PR's own motivating rule could never be found. Style rules are now handled first, then descended into for nested children.

3. @supports / @container dropped. Right that matchMedia never throws, so the catch was dead and the continue swallowed those blocks. Only CSSMediaRule is condition-gated now; other grouping rules descend unconditionally.

Should fix — all applied

5 is the sharpest catch: the probe genuinely cannot decide a tie, and demoting on before !== without inverted this PR's own defect. Rather than keep the utility phrasing, ambiguous now names both — "px-2 and [data-chip] (file) both declare this at the same value, so removing either alone changes nothing" — which is the honest answer and still solves the motivating case, where the two disagree about which is authoritative.

6 — narrowed to "a lone class the element itself carries", which keeps .composer-chip in plain-CSS projects and stops swallowing .a.b / .chip:hover. 7 — phrasing is now a four-way kind (overrides / ambiguous / plain / inline), so the outranking claim is only printed when a probe actually established it; inline styles are named as themselves; and pickCulprit has a layer tier above specificity. 4 — hints get the retry TTL, with the three-tier comment. 8 — one walk per element across all changed properties, beforeCss reused as the probe baseline, cheap rejects before CSSOM reads, memoised matchMedia.

9, 10, 12, 13 applied: overwrite-not-first-write for data-t3-component, hint path re-checks dcSource after its await, SOURCE_FILE_ATTR cleared when a real tag lands, Rendered by gated on !el.source, attribute constants imported, vendor README extended, a guard pinning the NO_PREVIEW wording, and the guard test moved from files: to verify:.

11 (bundle-graph scan) I've left — it's a pre-existing gap that protocol.ts already shares, and widening the guard from a directory walk to an import-graph walk is its own change rather than something to slip into this one.

Verification: 245 fork guards, 100 desktop preview tests, .fork/lint-owned.mjs clean (142 files), engine tsc, both project typechecks, lint.

The standing caveat is unchanged and now matters more: the probe and CSSOM walk still have no DOM test, which is exactly why blockers 2 and 3 survived to review. Everything above is verified by inspection and against the dist, not by executing the walk. It wants one live design-mode send before this is trusted.

[NoahHendrickson]

Follow-up — verified d857afc by executing the walk

The standing caveat was that the probe and CSSOM walk had never run in a DOM. I bundled this head's cssOrigin.ts with esbuild (IIFE, unmodified source) and drove resolveDeclarationOrigins in headless Chromium against fixture stylesheets. Results:

Confirmed fixed, observed live:

• Plain style rule collected: [data-chip] in a data-vite-dev-id sheet is found and named with its filename — blocker 2 no longer reproduces.
• @supports (display: grid) block descended; its rule is named — blocker 3 no longer reproduces. @container descends too.
• @media gating is real: the matching breakpoint's rule is named, the non-matching one is excluded.
• Layer tier works: unlayered [data-l] (specificity 100) beats layered div[data-l] (101), agreeing with the actual computed value.
• A utility that genuinely controls its property probes as utilityWins: true; an inline style is reported as inlineStyle: true.
• The resolver-side fixes (isSymbolicated === true, position-first pairing that fails closed, TTL on hints, attr re-check/clear in nativeSource.ts, !el.source gate) all match the review asks by inspection.

Two accuracy issues remain, both observed in the same run:

1. The PR's own motivating case now renders a false "same value" claim. Element with px-2.5 (declares 10px) beaten by unlayered [data-chip] { padding-inline: 8px }, computed 8px — the probe returns ambiguous: true (removal doesn't move the value), so the bullet reads "px-2.5 and [data-chip] (ComposerShell.css) both declare this at the same value, so removing either alone changes nothing". Both clauses are false here: they declare 10px vs 8px, and removing the chip rule would move the value (8px → 10px). This case is decidable, not ambiguous: when the utility's declared value differs from the measured beforeCss, the utility is provably not the lever — that's overrides, the phrasing ea13407 already had for it. The theme knows the utility's implied value (spacingBasePx × scale), or the walk could read the tautology-filtered utility rule's own declaration before discarding it. Genuine ties (declared == measured) keep the ambiguous phrasing, which is right for them.

2. The tautology filter suppresses the plain-CSS culprit it was narrowed to keep. isTautologicalSelector dismisses any single class the element itself carries — but in a plain-CSS guest project, .composer-chip { padding: 8px } on an element with class="composer-chip" is the element's own class, so the culprit comes back null and the bullet names nothing (observed: element carrying .plainchip, rule declaring the property, culprit: null). The comment in the same function names this exact shape as the one that "must survive". The tautology is narrower still: only the probed utility class is uninformative to name. Dismissing just utilityClass (when one exists) instead of the whole classList keeps .composer-chip nameable while still dropping .px-1.

Neither regresses below the pre-PR baseline — both degrade to the old suggestion or to silence rather than to a wrong-line pointer — so they're follow-up material by the same accuracy standard the PR sets for itself, not re-blockers of the shipped fixes. Repro fixtures are trivial to reconstruct from the descriptions above.

CI: Test is green on d857afc; the only red check is the PR-size labeling job, which is unrelated to the code.

Verification: esbuild-bundled cssOrigin.ts from this head executed in headless Chromium. Review by Claude Fable 5, harness: Claude Code.

[NoahHendrickson]

Both follow-up findings fixed in 556e9f6a1

1. Ties are now decided by value, not hedged. A removal-probe tie gets a second probe: the utility's own declared value — read from its single-class rule in the sheets — is applied inline with !important and re-measured. Applying rather than string-comparing is the point: calc(var(--spacing) * 2.5) only becomes comparable to a measured 8px by resolving it in the element's own context. If it computes to something other than the measured value, the utility provably lost and the bullet carries the confident overrides phrasing again; only a genuine same-value tie renders as ambiguous. The motivating case is decided, not hedged.

2. Culprit exclusion is now exactly one selector. isTautologicalSelector (any carried single class) is replaced by singleClassName, and the only rule excluded from culprit naming is the probed utility's own. A plain-CSS project's .composer-chip is nameable again (plain kind), and a competing utility on the element can be named as a finding. No render changes were needed — the existing kind mapping produces the right sentences once ambiguous is truthful.

Verified the same way as the last round — the bundled module executed in headless Chromium:

fixture	result
px-2.5 = calc(var(--spacing)*2.5) (10px, layered) vs unlayered rule at 8px	ambiguous: false, culprit named → renders overrides ✓
mt-3 = calc 12px vs rule at literal 12px (true tie)	ambiguous: true, backup rule named ✓
utility genuinely controlling its property	utilityWins: true ✓
element carrying .plainchip, rule in app.css	culprit .plainchip @ app.css ✓
all cases	computed value and style attribute fully restored after probing ✓

Checks: 244 fork guard tests, engine island tsc, apps/web typecheck, .fork/lint-owned.mjs (142 files) — all clean. The guard count went 245 → 244 because the five isTautologicalSelector tests consolidated into four singleClassName tests. Manifest intent updated to describe the two-probe tie rule and the narrowed exclusion.

Fix and verification by Claude Fable 5, harness: Claude Code.

[cursor]

REQUEST_CHANGES — thermo-nuclear code quality review

The intent is right: empirical class probes, a hard symbolication gate, and dropping the false Forge-verifier claim. Prior review debt on Partial<>, the dead findExistingUtility fallback, array-shaped culprits, and twin-validator comments is largely cleaned up.

What remains is structural. DeclarationOrigin is still independent-boolean soup that request.ts re-derives into a discriminated union, and the unmapped cases fall back into the exact “change utility X → Y” path this PR exists to kill. That is a code-judo miss with a real accuracy hole (ambiguous / utility-lost without a culprit still names a lever). Origin probing is also still a fat fork block inside buildChangeRequestWithElements rather than one helper that owns probe → item → phrasing.

No file crosses 1k lines; cssOrigin.ts as a dedicated module is the right layer. Do not approve until the origin model is exhaustive and the request builder stops re-encoding flags into sentences.

Prior threads: Partial model / dead fallback / array culprits / twin-validator note → addressed. Probe tangled in buildChangeRequestWithElements → still open.

  

_{Sent by Cursor Automation: Thermo-nuclear PR review}

[cursor]

blocker (code-judo / types): DeclarationOrigin is boolean soup (utilityWins / ambiguous / inlineStyle / nullable culprit) while ChangeItem.origin is already a discriminated union — then request.ts re-derives the union from flags. That dual model is how you get illegal combinations and silent fallthroughs.

Code-judo: make resolveDeclarationOrigins return the union the markdown path already wants, e.g. utility | inline | overrides | ambiguous | plain | unresolved, with utilityClass / culprit only on the arms that have them. Delete the remap. Why keep two representations of one decision?

[NoahHendrickson]

Declining the union refactor for this PR. The point is fair — the flag record can express states the union can't — but the concrete harm it enabled (verdicts dropped when no culprit was nameable) is fixed in 4543bdc by making the flag→union mapping exhaustive over the probe's verdicts, and the one flag combination that was genuinely illegal (a 'lost' verdict from a probe that never ran) is now unrepresentable at the source. Collapsing DeclarationOrigin into the wire union would churn cssOrigin.ts and its 17 guard tests for shape alone; noting it as follow-up material rather than growing this diff.

[cursor]

blocker (structural): This mapping still violates the PR’s own claim. beforeUtility is set when utilityWins || ambiguous, but origin is only set for inlineStyle or when culprit exists. So ambiguous && !culprit (and the “utility lost via value probe but no nameable rule” case) leave origin unset and fall through to change \${beforeUtility}` → …/add …` — naming a lever the probe already proved does not uniquely win.

If the class did not move the value, the utility-suggestion arm must be unreachable. Exhaustive switch on a real origin union; no default that pretends the class is the edit surface.

[NoahHendrickson]

Confirmed real — both culprit-less paths were reachable whenever the winning/tying declaration lives in a cross-origin sheet (the CSSOM walk skips those by design). Fixed in 4543bdc: ChangeItem.origin gains ambiguous-unnamed / overrides-unnamed arms, the builder maps every probe verdict (with resolveDeclarationOrigins now nulling utilityClass when the removal probe never actually ran, so a non-null class always means a verdict), and the renderer states the tie or the untraceable winner instead of falling through to change/add.

[cursor]

important (spaghetti): Prior thread still stands. The timing constraint (probe only under compare(el, true)) does not require owning collapse, an origins Map, className re-derivation, non-null origins.get(property)!, flag→union remap, and later renderMarkdown kind chains inside buildChangeRequestWithElements.

This adds another special-case branch into an already busy flow. Extract one helper called inside the compare window — e.g. buildCssChangeItems(el, collapsed, theme, tokens) — so this function stays measure → build → restore and fork policy lives in one place. Right now the same decision is split across try-block, item loop, ChangeItem.origin, and markdown.

[NoahHendrickson]

Declining the extraction. buildChangeRequestWithElements is vendored Forge code (engine/vendor/, kept close to upstream with t3-fork edit markers per vendor/README.md); restructuring its measure→build→restore body into new helpers maximizes drift against the vendor for a readability gain the fork's policy already trades away. The timing constraint the inline placement encodes (probe only while compare(el, true) shows the original cascade) is load-bearing and documented at the site.