feat(#343): adopt Lume icon + state-as-glyph specs (additive, gate-re… - #725
Conversation
|
Prepped for merge — Why it needed a refreshThe 9/9 green on this branch was measured against Verified, not assumedThe ADR says the
13/13 as documented. The Test state
Two notes for the reviewer1. The The risk is the next PR. Whoever adds icon emission must build the handshake gate in the same change — otherwise a Tier-1 renderer without icon support rejects the surface wholesale, because the whitelist is 2. Unrelated repair worth naming. No blockers from my side. |
|
Refreshed again — Re-verified on the new head:
One thing that is genuinely better than last time: #752 fixed the Still no blockers from me. The two review notes from before stand — the |
…lders audit row (#820) Two findings from the same review wave, one branch because both are small and sharp. #764 — CI never ran the workspace packages' own test suites. `npm run test` globs only `middleware/test/**`; canvas-core (vitest), conductor-core (vitest) and plugin-api (node:test) ran nowhere. Not hypothetical twice over: #759 broke two conductor-core tests while every required check stayed green, and #725's 15 canvas-core validator tests were only ever run by hand during review. New CI step runs all three. `@omadia/plugin-ui-helpers` is deliberately absent: it declares a test script but contains zero test files, and papering over that with --passWithNoTests would create a permanently green no-op — the exact failure family this repo keeps finding (#640, #752). If it gains tests, add it to the step. #775 — the `conductor.role_holders_change` audit entry never landed: the index.ts closure passed the session sub (an EMAIL under local auth) as `actor.id`, and `admin_audit.actor_id` is a uuid column, so every insert threw. Loud in the log, empty in the audit trail — the entire point of #759. Fix: the conductor entry additionally threads the session's `omadia_user_id` (a real uuid), and the mapping lives in an exported `roleChangeAuditEntry` (adminAuditLog.ts): uuid to `actor_id` only when the session carries one, the sub always to the free-text `actor_email` — the same treatment the adminUsers routes give it, and for the `'operator'` fallback the only place the actor survives at all. Tests: 3 mapper units; the existing route test now proves the uuid threading end-to-end (harness session carries omadia_user_id, deep-equal asserts it); and a pg-gated suite runs the mapper output through the real `AdminAuditLog.record` against the REAL migration DDL (read from the migration file, not copied — a hand-duplicated schema could drift and green-light what production rejects). Its second case pins the regression permanently: the OLD mapping must keep failing on the real column with `invalid input syntax for type uuid`. Mutation checks: removing the uuid threading turns the route test red; reverting the closure to the old inline mapping reproduces exactly the insert the pg test asserts the database refuses. Verified against an ephemeral postgres:16-alpine (2/2). Full suite: 7048 tests, 1 pre-existing-shape failure fixed by extending the existing expectation (the new field), then green; typecheck:test ratchet held with no regressions; lint clean.
2 participants
What
Adopt the Lume icon + state-as-glyph specs (#343) into core as an additive, gate-ready change. Adds the
icon/iconStatetrait and chartvariant:"spark"to the canvas-tree whitelist, plus the "show state, don't narrate it" authoring rule. ADR-0010 records the decision.Why
The upstream Lume specs (omadia-ui#41/#42) proposed an additive canvas-protocol §12 icon affordance and a spark chart variant; none had landed in core. This lands the vocabulary + validator contract so the desktop/mobile renderers have one thing to build against — without turning emission on. Nothing in core emits the new props yet; emission stays behind the
iconResolver@1handshake gate, because the Tier-1 renderer rejects unknown props wholesale (strictunevaluatedProperties:falsewhitelist), so a half-shipped prop would brick the whole surface rather than degrade.Test plan
npm run gen && npx vitest run test/validator.test.ts test/fixtures.test.tsincanvas-core→ 41 passedapp:/lib:icon +iconState, chartvariant:"spark",_gallery.jsonfixture read-backgen:prefix,iconStatewithouticon(dependentRequired), unknowniconState/chart varianticonResolver@1support (byte5ai/omadia-ui) — out of scope, tracked as follow-upRisk / blast radius
commonTraits+p_chart; no migration, no existing tree invalidated.$idstaysprotocol/1.0/…(additive vocabulary in the 1.0 whitelist — see ADR-0010 version story).gen:prefix is rejected by the schema (the last gate before a renderer) until the upstream §6 sanitised-SVG + consent review lands; pattern widens to include it in that same PR.app:/lib:only today.tone— state is never signalled by colour alone.Naming-decisions still pending
iconGenerator@1pinned togenerateAssetscope{kind:"icon"}(not a new capability enum) — decided in ADR-0010, no churn.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.