ci(theme): pin the compiled theme seed against the bundled JSON

[nigelfenton]

Groundwork for #3184 item 2 ("generate the seed table from the resource at build time — the
high-leverage one; kills the drift class permanently").

This PR changes no colours. It proves the drift class is real, pins it, and stops it widening,
so the generator lands against a floor and gets a regression test for free.

The drift already happened

ThemeManager::seedBuiltinDefaults() compiles a copy of the default theme into the binary so the
UI works with zero theme files on disk. It is kept in sync with resources/themes/default-dark.json
by hand, and the source says so twice — including this warning:

KEEP IN SYNC: ... If those primitives shift, update both sites or the seeded look will drift
from the JSON-defined look on bundled themes (silently — both layers resolve, the JSON wins,
but the visible vs. seeded values diverge for pre-PR user themes).

Nine tokens on main @ d50377f6 disagree today:

token	compiled seed	default-dark.json
color.accent.dim	#0090e0	#0070c0
color.slice.a	#ff4040	#00d4ff
color.slice.b	#ff8c00	#ff40ff
color.slice.c	#ffd040	#40ff40
color.slice.d	#40c060	#ffff00
color.slice.e	#00b4d8	#ffa000
color.slice.f	#4080ff	#00e0c0
color.slice.g	#c060ff	#ff6080
color.slice.h	#ff60a0	#b080ff

The slice colours explain themselves — the seed's own comment reads "Preliminary values — a
dedicated slice-colour audit may tune these in a follow-up." The JSON was later tuned; the seed
never was. Both bundled themes agree slice A is cyan/blue; only the compiled seed still says red.

Why it stayed hidden: on a normal run the JSON wins, so a drifted seed is invisible. It only
surfaces for a user whose theme predates a token — the seed then supplies the value the JSON would
have overridden — or when no theme file loads at all. Both are exactly the situations nobody is
watching, which is what makes this worth a gate rather than a one-off correction.

What this adds

• tools/check_theme_seed.py — parses both sides (resolving {color.red.500} aliases against
  the primitives palette) and diffs them. The nine existing divergences are frozen in
  KNOWN_SEED_DRIFT; --strict fails on anything else. The list may only shrink: a token that
  stops drifting is reported as stale so it gets removed rather than quietly padding the
  baseline.
• .github/workflows/theme-seed-check.yml — runs it on PRs touching ThemeManager.cpp,
  resources/themes/**, or the checker. Modelled directly on engine-boundary.yml: same
  concurrency block, same pinned action SHAs, same known-warn / new-fail shape.

The checker refuses to report a clean run if it parses zero shared tokens, so a future refactor
of seedBuiltinDefaults() that breaks the regex fails loudly instead of silently passing. That
failure mode is the whole risk of a parser-based gate, so it is guarded explicitly.

Verification

All three paths exercised, not just the happy one:

case	result
current main	9 known, 0 new → exit 0
inject a new divergence (color.spectrum.grid → #deadbe)	reported as NEW, separate from the nine → exit 1
repair a known one (slice.a → JSON value)	reported as stale, prompting its removal

Same results on Windows/Python 3.14 and on Linux/Python 3.12 — the version CI pins.

Deliberately not in this PR

Fixing the nine. Which side wins is a design call, not a mechanical one: the JSON is almost
certainly right (it is what every user actually sees, and both bundled themes agree), but the seed
is what pre-v2 user themes fall back to, so changing it is a visible change for those users. That
belongs with whoever owns the slice-colour audit the comment refers to — happy to do it in a
follow-up once someone says which way.

The generator itself. Once the seed is generated from the resource, KNOWN_SEED_DRIFT empties
and this checker becomes the test that the generator stays correct.

Refs #3184.

[aethersdr-agent]

Good PR. I ran the checker against main @ d50377f6 and reproduced your results exactly — 87 tokens compared, the same nine divergences, --strict exit 0 — so the drift table in the description is accurate and this genuinely changes no colours. The workflow is a faithful copy of engine-boundary.yml (same pinned SHAs, same concurrency block, same known-warn/new-fail shape), and leaving the nine unfixed is the right call for a pinning PR. Non-blocking stale reporting matches the EB3-stale precedent in check_engine_boundary.py, so no objection there either.

One real gap: the zero-token guard you added covers total parser failure but not partial failure, which is the more likely refactor outcome. I simulated three plausible edits to seedBuiltinDefaults() — only one of them trips the guard:

refactor	tokens parsed	result
QString( → QStringLiteral(	0 / 87	guard trips ✅
scoped block closing brace reindented	81 / 87	passes silently
m_tokens → rootTokens() rename	9 / 87	passes silently

That last one is the bad case: all 78 root tokens vanish, which includes all nine known-drift entries, so the run reports them as stale and still exits 0 — the gate inverts into "please delete your baseline." Concrete fix suggested inline.

Would like fixed before merge

• Partial parse failure passes --strict silently; assert every hex literal in the body was captured, not just that the count is non-zero (tools/check_theme_seed.py:132).

Polish

• 17 seeded non-colour tokens (font.size.*, sizing.*) are outside the parser, but the seeded only in C++ : 0 line reads as full coverage (tools/check_theme_seed.py:163).

Non-blocking notes

• Short-form hex would report as false drift (tools/check_theme_seed.py:66).

---

_{🤖 aethersdr-agent · cost: $2.9168 · model: claude-opus-5}

[ten9876]

Reviewed by checking it out and running the checker rather than reading it cold. The premise is right, the shape is right, and the claims in the description hold up. Two things I'd want fixed before merge, and two scope calls I'd rather see stated than fixed.

Answers to your two questions are on #3184 — short version: the JSON wins, and generation is commit-time, which makes this checker the freshness gate for the generator rather than something the generator supersedes. That raises the value of getting the coverage right here, hence finding 1.

Verified

Reproduced exactly on the branch: 87 tokens compared, 9 known, 0 new, exit 0. The nine divergences are real. Alias resolution is clean — zero unresolved {…} across the 87, so no false positives lurking. The applet/tx|rx|comp scope-path construction matches the C++ scopeOrCreate keys and all 9 scoped tokens parse. The workflow is a faithful clone of engine-boundary.yml: same pinned action SHAs, same concurrency block, permissions: contents: read, path-filtered. It isn't in branch protection's required set (build, check-windows, check-macos), so it won't hang unrelated PRs — same advisory-by-convention status as the engine-boundary ratchet, which is the right call.

1. The parser silently skips 17 of the 95 root inserts

HEX = r"#[0-9a-fA-F]{3,8}" means only hex-colour literals are extracted. Dropped:

• seven font.family.* (string values)
• nine numeric — font.size.tiny|small|normal|large, sizing.panel.padding|spacing|cornerRadius, sizing.border.subtle|strong
• color.meter.bar.fillGradient — a colour token, squarely in scope, skipped only because its insert wraps to a second line

I checked the nine numeric ones against default-dark.json: they all exist there with matching values today. So they are comparable and currently clean — they are just not covered. Change sizing.panel.cornerRadius to 6 in the JSON alone and this checker still prints a clean run. That is precisely the drift class the PR exists to pin.

It also undercuts the description's strongest claim — that the zero-shared-token guard means a refactor which breaks the regex "fails loudly instead of silently passing." True only for total breakage. A reformat that moves QString( to the next line for some subset drops those tokens and still exits 0, while the summary line seeded only in C++: 0 reads as full coverage.

Suggested fix, cheaper and stricter than a magic minimum: count tokens.insert( occurrences inside the extracted function body and fail when the number extracted is lower. That converts partial-parse into a hard error, which is the failure mode you correctly identified as the whole risk of a parser-based gate — it just needs to cover partial as well as total.

2. stale never fails, even under --strict

The description says the baseline "may only shrink" and that a repaired token is reported "so it gets removed rather than quietly padding the baseline." But main() returns 1 only on new. A repaired token prints an advisory line inside a green job's log and stays in KNOWN_SEED_DRIFT indefinitely — nobody reads a passing job.

Since engine-boundary.yml is the cited model and its baselines genuinely only shrink, make stale non-zero under --strict too. If you'd rather keep it advisory, drop the ratchet language from the docstring so the two agree.

3 and 4 — scope calls, fine to leave, worth saying out loud

69 JSON tokens have no seed entry at all — color.slice.dim.*, color.button.*.disabled, color.highlight.rx|tx|fg|message. That's arguably a sharper form of the failure this PR describes: the pre-v2 user's theme falls back not to a stale value but to nothing. It's printed as a bare count and never gated, so it can grow without a signal. Note in the docstring why it isn't gated. It also stops being a question once the generator emits all 147 — see the scope note on #3184.

default-light.json isn't checked. The description reasons from both bundled themes agreeing; only dark is gated.

Verdict

Fix 1 and 2 and this is good to merge. 1 is the one that matters — a coverage gap in a gate is worse than no gate, because the green check is read as an assurance the tool isn't actually making. Neither is more than a few lines.

— Claude (AI dev partner)

[ten9876]

Nigel — reconciling my review with AetherClaude's, since two agent reviews landed on this within two hours and one line of mine would send you the wrong way if you applied both literally.

We agree on substance. Both reviews independently found the same primary defect (the parser's partial-parse blind spot) and the same 17 uncovered seeded tokens — same breakdown, seven font.family.*, nine numeric across font.size.* / sizing.*, and the color.meter.bar.fillGradient insert. No contradiction there. AetherClaude's #fff vs #ffffff normalisation point is additive and I'd take it.

Where I was wrong. I suggested asserting on tokens.insert( occurrences in the function body and failing when fewer were extracted. I checked that against origin/main after the fact and it does not hold: the body contains 104 tokens.insert( calls but only 87 hex literals. That assertion fails CI immediately on a correct tree. AetherClaude's version is the right one and I've verified its equality claim exactly — QString("#hex") occurrences in the body: 87; parser-captured: 87. Take its inline suggestion as written.

The two only reconcile in a specific order, which is worth being explicit about since they assert different invariants:

1. Now: adopt AetherClaude's hex-literal equality guard. Correct today, and correct as long as the checker's scope stays hex-only.
2. Only if you widen the parser to cover the non-hex seeds (the bare integers, and plain QString("…") values): then upgrade to the insert-count invariant, because at that point 104 becomes the right expectation and the hex equality becomes too loose.

Doing both at once, or the second without the first, breaks. If you keep the scope hex-only, my step 2 simply doesn't apply — take AetherClaude's and relabel the counter to "colour tokens compared" per its second comment.

One token stays out of reach under either: color.meter.bar.fillGradient inserts QVariant::fromValue(g) with no hex literal in the call at all, so it is invisible to both assertions. It needs explicit handling or an explicit note saying gradients are out of scope — silence is the thing worth avoiding.

One compounding interaction neither review caught alone. AetherClaude observed that a partial parse which loses the root tokens makes all nine KNOWN_SEED_DRIFT entries report as stale. I separately flagged that stale never fails under --strict. Together those are worse than either: the job goes green and prints an instruction to empty the baseline. That turns my finding 2 from tidiness into a real safety property — I'd now treat making stale fatal under --strict as blocking rather than optional.

Net: two blocking items, unchanged in count. Use AetherClaude's fix for the first.

— Claude (AI dev partner)

[nigelfenton]

Both blocking items addressed in 2fadb56, in the order the reconciliation comment prescribes:

1. Completeness guard — took AetherClaude's inline suggestion as written: equality against the QString("#hex") literal count (87 == 87 today), not the insert count. Red-tested both failure modes from the review: an m_tokens rename dies with captured 9 of 87, and a scoped-brace reindent fires the same guard instead of passing 81/87 silently.

2. stale is now fatal under --strict — upgraded to blocking per the reconciliation's compounding case (partial parse ⇒ all-stale ⇒ green run that says "empty the baseline"). Verified: an injected never-drifts entry exits 1 with an explicit message telling you to remove the entry or fix the parser if tokens went missing rather than converged.

Also picked up the two additive points: hex normalisation before comparison (#fff == #ffffff, opaque leading alpha stripped — Qt's alpha is leading), and the counters/docstring now say colour tokens only, listing the 17 out-of-scope seeds explicitly. color.meter.bar.fillGradient gets its own callout as invisible to any literal-based guard, and the docstring carries the invariant-ordering note (hex-equality now; switch to insert-count only if the parser is ever widened) so the two guards don't get combined by a future editor.

Green run on the branch: 87 colour tokens compared, 9 known drifts, 0 new, exit 0.

[ten9876]

Approving, with the fixes pushed as 3dc5d0c3 rather than handed back as another round of review notes — Jeremy's call, since this had already been through three rounds and the remaining work was mechanical.

Disclosure up front: I wrote that commit, so this approval is not an independent review of it. Nigel, please read 3dc5d0c3 as a proposal on your branch and say if you disagree with any of it — you know this checker better than I do and I'd rather revert a hunk than have it land over your objection.

What the last round got wrong, including two things I told you to do

The hardening in 2fadb56d did what the previous reviews asked. The problem is that one of those reviews was mine and it was wrong.

The completeness guard I endorsed was tautological. I told you to take the expected = len(re.findall(rf'QString\("{HEX}"\)', body)) snippet "as written," and explicitly retracted my own tokens.insert(-counting version. But that guard counts the same pattern the capture regexes match, so any other spelling drops out of both in lockstep. Verified: respelling one seed as QStringLiteral("#deadbe") — the idiom already used for scopeOrCreate() a few lines down in that same function — took the token out of the comparison entirely, left the guard silent, and reported drifted (NEW): 0 with real drift compiled into the binary. That defeated the PR's whole purpose, and my endorsement is why it shipped that way.

The fix is better than the guard I originally proposed, too: a colour seed is now anything whose value carries a hex literal in any wrapper, so a respelling is captured rather than merely detected. Case 1 now reports the drift correctly instead of hard-failing on a count.

My advice to widen the parser to font.family.* was also wrong. Those can't be compared key-for-key: the JSON stores them as compound objects (font.family.ui = {family, size, color}, flattened to font.family.ui.family) while the C++ side inserts a flat string, so the keys never intersect. A wider value pattern would cover none of them — it needs a shape-aware rule. The docstring now says so, and corrects "three non-colour tokens on both sides" to nine (4× font.size.*, 5× sizing.*).

Also fixed

• Comments were matched, both directions — a commented-out insert produced a phantom NEW divergence and failed --strict on a PR that changed nothing, and conversely a commented-out old line could supply the compared value while a differently-spelled live line carried the drift. Comments are now blanked before matching, string-literal aware, offsets preserved so line numbers don't shift.
• The guard false-failed legitimate edits. A local const QString kAccent = QString("#00b4d8"); reported "captured 87 of 88 — the parser is out of date" when nothing had mis-parsed. Duplicate seeds are now diagnosed for what they are.
• Parsing no longer depends on formatting. The scoped-block terminator was the literal \n \}, so reindenting a block dropped its tokens. Body extraction and call parsing now use brace/paren matching, and a token's scope comes from the nearest preceding scopeOrCreate() rather than from indentation.
• The mislabeled stat. "colours present only in JSON: 69" counted every flattened JSON key. Restricted to actual colours it's 25 — which independently matches the "~24 unseeded tokens" from the verification pass on #3184, a good sign both numbers are now right.

Verification

Ten adversarial mutations, each asserted in the required direction rather than just run:

case	required	result
respell QStringLiteral + real drift	fail	✅ caught as NEW drift
bare-string value + drift	fail	✅ caught as NEW drift
commented-out bogus insert	clean	✅ ignored
comment masking a live drift	fail	✅ live drift caught
legitimate local const QString hex	clean	✅ no false failure
same token seeded twice	fail	✅ named as a duplicate
reindented scope block	clean	✅ still 87
renamed m_tokens receiver	clean	✅ still 87
root insert after scoped blocks	fail	✅ no misattribution
repair a known drift	fail	✅ still fails as stale

Clean tree unchanged: 87 compared, 9 known, 0 new, exit 0. The Theme seed vs bundled JSON job is green on 3dc5d0c3.

And then stop

Worth saying plainly since this is round four: a regex parser for C++ has no fixed point. Every round has found a new way to fool it, and another round would find another. I'd merge this and put the remaining energy into #3184 item 2 — once the seed is generated, this checker's job collapses to "is the generated file fresh," and every failure mode above stops existing because there's no hand-written C++ left to parse. Your two questions are answered on #3184 (JSON wins; commit-time generation with a freshness gate), so that path is unblocked.

Thanks for the checker — the drift it found was real, and the KNOWN_SEED_DRIFT shape is what makes the generator landable against a floor.

— Claude (AI dev partner)