From e98423cd9f4baf081f9cfd4bee4c5d631a8b9131 Mon Sep 17 00:00:00 2001 From: Richardson Gunde Date: Sat, 1 Aug 2026 17:48:49 +0530 Subject: [PATCH 1/3] feat(hub): the approval gate is a state machine, not a colour (#544) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Last child of epic #499, built zero-dep instead of on the Rive runtime. The advantage asked for is the GLYPH — a gate whose states are a machine, so the approval lifecycle reads as a mechanism. The objections raised against the Rive child all attach to the RUNTIME rather than that visual: an unreviewable .riv binary, a state machine holding its own copy of governance state, and a canvas nothing in the suite can assert against. Authoring the same machine in SVG + CSS keeps the advantage and loses all three. The shackle carries the meaning: pending shut the gate is closed, awaiting a human approved OPENS + check passage granted consumed SHUTS AGAIN the one attempt was spent; blocking resumed rejected shut + cross refused The consumed row is why this was worth building. A spent one-shot override is back to blocking, and nothing on the card said so as a mechanism — the copy explained it in a sentence and the colour just faded. A shackle that visibly re-closes states it in the way a reader absorbs without reading. Every part is present in every state, undrawn or transparent rather than absent: a part rendered only for its own state would be a new node at the flip, and a new node cannot transition (the #541 Proof Rail lesson). And transition rather than animation means a card that ARRIVES approved is still, while only a real flip animates — motion never claims an event that did not happen. Transform, opacity and stroke-dash only; a browser test asserts the card neither resizes nor moves when the gate opens. aria-hidden, because the pill beside it already states the status in words. Co-Authored-By: Claude Fable 5 --- .../dashboard/ui/pages/approvals.js | 28 ++- src/observability/dashboard/ui/styles.js | 45 +++++ .../browser/dashboard-544-gate-glyph.test.js | 179 ++++++++++++++++++ tests/hub-gate-glyph-544.test.js | 143 ++++++++++++++ 4 files changed, 394 insertions(+), 1 deletion(-) create mode 100644 tests/browser/dashboard-544-gate-glyph.test.js create mode 100644 tests/hub-gate-glyph-544.test.js diff --git a/src/observability/dashboard/ui/pages/approvals.js b/src/observability/dashboard/ui/pages/approvals.js index 641c856c..721e5f25 100644 --- a/src/observability/dashboard/ui/pages/approvals.js +++ b/src/observability/dashboard/ui/pages/approvals.js @@ -50,6 +50,31 @@ function renderOpsAuditRejections(s) { setHTML('ops-audit-list', items.map(opsAuditRejectionHtml).join('') || emptyHtml('No audit rejections', 'Every approval record read in the recent event window passed the consistency audit.')); } +// #544: the gate as a state machine rather than a colour. +// +// Every part is present in EVERY state — undrawn or transparent, never +// absent. A part rendered only for its own state would be a new node at the +// flip, and a new node cannot transition, so the change would never animate +// on the one occasion it means something (the #541 Proof Rail lesson). +// +// The shackle carries the meaning: shut while a human is awaited, open once +// passage is granted, and shut AGAIN once a one-shot override is spent — +// which is the thing the card's colour alone could never say. +// +// pathLength="1" keeps the dash resolution-independent (the #525 idiom). +// aria-hidden because the pill beside it already states the status in words; +// a second announcement would be noise, not access. +function gateGlyphHtml(status) { + return ''; +} + function approvalHtml(item, canAct) { var status = item.status || 'pending'; // CONSUMED lifecycle (#156): the server cross-references the run-level @@ -65,7 +90,8 @@ function approvalHtml(item, canAct) { : isOverride ? '
🛡 One-shot override: approving grants exactly one more attempt for this task, then the override is consumed and further attempts block again.
' : ''; - return '
' + esc(item.title || item.type || 'Approval required') + '
' + esc(item.detail || item.reason || '') + '
' + overrideNote + '
' + esc(shortName(item.projectRoot)) + '' + esc((item.runId || '').slice(-16)) + '' + timeHtml(item.ts) + '
' + pill(status, status) + '
' + + return '
' + esc(item.title || item.type || 'Approval required') + '
' + esc(item.detail || item.reason || '') + '
' + overrideNote + '
' + esc(shortName(item.projectRoot)) + '' + esc((item.runId || '').slice(-16)) + '' + timeHtml(item.ts) + '
' + + '
' + gateGlyphHtml(status) + pill(status, status) + '
' + (canAct ? '
' : '') + '
'; } diff --git a/src/observability/dashboard/ui/styles.js b/src/observability/dashboard/ui/styles.js index bd08eea8..769bb850 100644 --- a/src/observability/dashboard/ui/styles.js +++ b/src/observability/dashboard/ui/styles.js @@ -1162,6 +1162,51 @@ tr.clickable:hover td { background: #f8fbff; } entirely while its pill stayed styled. It is a SPENT credential, not a standing approval — muted, and distinct from the green of a live one. */ .approval-card.consumed { border-left: 4px solid var(--line-strong); opacity: .72; background: var(--soft); } + +/* ── #544: the gate glyph — a state machine, not four pictures ───────────── + Every part is always in the DOM; state only moves and reveals it, so the + nodes survive the morph and their transitions are the animation. The + shackle is the sentence: shut / open / shut again. */ +.approval-state { display: flex; align-items: center; gap: 8px; } +.gate-glyph { display: inline-flex; flex: 0 0 auto; width: 26px; height: 26px; } +.gate-glyph svg { width: 100%; height: 100%; overflow: visible; } +.gate-shackle { + fill: none; stroke: var(--muted); stroke-width: 2.4; stroke-linecap: round; + transform-origin: 10px 14px; + transition: transform var(--motion-base) var(--motion-ease-emphatic), stroke var(--motion-base) var(--motion-ease); +} +.gate-body { fill: var(--panel); stroke: var(--muted); stroke-width: 2.2; transition: stroke var(--motion-base) var(--motion-ease), fill var(--motion-base) var(--motion-ease); } +.gate-check { + fill: none; stroke: var(--green); stroke-width: 2.6; stroke-linecap: round; stroke-linejoin: round; + stroke-dasharray: 1; stroke-dashoffset: 1; opacity: 0; + transition: stroke-dashoffset var(--motion-slow) var(--motion-ease-emphatic), opacity var(--motion-fast) var(--motion-ease); +} +.gate-cross { + fill: none; stroke: var(--red); stroke-width: 2.6; stroke-linecap: round; + stroke-dasharray: 1; stroke-dashoffset: 1; opacity: 0; + transition: stroke-dashoffset var(--motion-base) var(--motion-ease-emphatic), opacity var(--motion-fast) var(--motion-ease); +} +/* The spent mark: what is left of a grant that has been used up. */ +.gate-bar { fill: none; stroke: var(--muted); stroke-width: 2.4; stroke-linecap: round; opacity: 0; transition: opacity var(--motion-base) var(--motion-ease); } + +.approval-card.pending .gate-shackle { stroke: var(--amber); } +.approval-card.pending .gate-body { stroke: var(--amber); } + +/* Passage granted: the shackle lifts and swings clear. */ +.approval-card.approved .gate-shackle { transform: translateY(-3px) rotate(-24deg); stroke: var(--green); } +.approval-card.approved .gate-body { stroke: var(--green); } +.approval-card.approved .gate-check { stroke-dashoffset: 0; opacity: 1; } + +.approval-card.rejected .gate-shackle { stroke: var(--red); } +.approval-card.rejected .gate-body { stroke: var(--red); } +.approval-card.rejected .gate-cross { stroke-dashoffset: 0; opacity: 1; } + +/* Spent: the one granted attempt was used, so the gate is shut again. The + check goes with it — the grant is no longer in force. */ +.approval-card.consumed .gate-shackle { transform: translateY(0) rotate(0deg); stroke: var(--muted); } +.approval-card.consumed .gate-body { stroke: var(--muted); fill: var(--soft); } +.approval-card.consumed .gate-check { opacity: 0; } +.approval-card.consumed .gate-bar { opacity: 1; } .approval-actions { display: flex; gap: 8px; margin-top: 10px; } .btn { border: 1px solid var(--line); diff --git a/tests/browser/dashboard-544-gate-glyph.test.js b/tests/browser/dashboard-544-gate-glyph.test.js new file mode 100644 index 00000000..7f625821 --- /dev/null +++ b/tests/browser/dashboard-544-gate-glyph.test.js @@ -0,0 +1,179 @@ +/** + * #544 — the approval gate glyph in a real browser. + * + * The claim being tested is behavioural, not cosmetic: the shackle is OPEN + * only while passage is actually granted, and shut again once the grant is + * spent. Plus the #541 pair — a flip animates on the surviving node, an + * arrival does not. + * + * owner: RStack developed by Richardson Gunde + */ + +/* global document, window, getComputedStyle */ + +import assert from 'node:assert/strict'; +import { createRequire } from 'node:module'; + +import { fixtureBlockedRun, fixtureReadyRun } from '../helpers/dashboard-fixtures.js'; +import { + setupBrowserSuite, gotoDashboard as harnessGoto, navigate, + PAGE_VISIBLE_TIMEOUT_MS, +} from './helpers/harness.js'; + +const require = createRequire(import.meta.url); +let AXE_PATH = null; +try { AXE_PATH = require.resolve('axe-core/axe.min.js'); } catch { /* dep absent */ } +const AXE_KNOWN_DEBT = new Set(['color-contrast']); +const AXE_BLOCKING_IMPACTS = new Set(['serious', 'critical']); + +// Past --motion-slow (.4s) with room to spare. +const SETTLE_MS = 900; +const REDUCED_MOTION_SETTLE_MS = 150; + +const seedRuns = async (root) => { await fixtureBlockedRun(root); await fixtureReadyRun(root); }; + +const suite = setupBrowserSuite(); +const glyphTest = (name, body, options = {}) => suite.browserTest( + name, + { seed: seedRuns, tmpPrefix: 'rstack-browser-544-', ...options }, + body, +); + +async function openApprovals(page, server) { + await harnessGoto(page, server, { waitForState: true }); + await navigate(page, 'approvals', { assertVisible: false }); + await page.waitForFunction(() => document.querySelector('#page-approvals .gate-glyph') != null, + null, { timeout: PAGE_VISIBLE_TIMEOUT_MS }); +} + +// Read each state on its OWN fresh node: re-classing one node and reading +// immediately samples mid-transition (the #541 lesson). +const readStates = (states) => page => page.evaluate((wanted) => { + const card = document.querySelector('#page-approvals .approval-card'); + const out = {}; + for (const state of wanted) { + const clone = card.cloneNode(true); + clone.className = 'approval-card ' + state; + card.parentNode.appendChild(clone); + const shackle = clone.querySelector('.gate-shackle'); + const style = getComputedStyle(shackle); + out[state] = { + transform: style.transform, + shackleStroke: style.stroke, + check: getComputedStyle(clone.querySelector('.gate-check')).opacity, + cross: getComputedStyle(clone.querySelector('.gate-cross')).opacity, + bar: getComputedStyle(clone.querySelector('.gate-bar')).opacity, + }; + clone.remove(); + } + return out; +}, states); + +glyphTest('the shackle is open only while passage is granted (#544)', async ({ page, server }) => { + await openApprovals(page, server); + const seen = await readStates(['pending', 'approved', 'rejected', 'consumed'])(page); + + const closed = (state) => seen[state].transform === 'none' || /matrix\(1,\s*0,\s*0,\s*1,\s*0,\s*0\)/.test(seen[state].transform); + assert.equal(closed('pending'), true, `pending keeps the gate shut — got ${seen.pending.transform}`); + assert.equal(closed('approved'), false, `approved opens it — got ${seen.approved.transform}`); + assert.equal(closed('rejected'), true, `rejected keeps it shut — got ${seen.rejected.transform}`); + // The whole point of the child: a spent override is BACK TO BLOCKING. + assert.equal(closed('consumed'), true, + `a spent one-shot override shuts the gate again — got ${seen.consumed.transform}`); +}); + +glyphTest('each state shows its own mark and only its own (#544)', async ({ page, server }) => { + await openApprovals(page, server); + const seen = await readStates(['pending', 'approved', 'rejected', 'consumed'])(page); + + assert.deepEqual( + { check: seen.approved.check, cross: seen.approved.cross, bar: seen.approved.bar }, + { check: '1', cross: '0', bar: '0' }, 'approved shows the check alone'); + assert.deepEqual( + { check: seen.rejected.check, cross: seen.rejected.cross, bar: seen.rejected.bar }, + { check: '0', cross: '1', bar: '0' }, 'rejected shows the cross alone'); + assert.deepEqual( + { check: seen.consumed.check, cross: seen.consumed.cross, bar: seen.consumed.bar }, + { check: '0', cross: '0', bar: '1' }, 'consumed shows the spent bar, and the check is gone'); + assert.deepEqual( + { check: seen.pending.check, cross: seen.pending.cross, bar: seen.pending.bar }, + { check: '0', cross: '0', bar: '0' }, 'pending makes no claim at all'); +}); + +glyphTest('a real approval animates the glyph on the surviving node (#544)', async ({ page, server }) => { + await openApprovals(page, server); + + const flip = await page.evaluate(() => { + window.handleGlobalSnapshot = function () {}; // hold live ticks + const card = document.querySelector('#page-approvals .approval-card'); + card.querySelector('.gate-check').__rs544 = 'original'; + card.className = 'approval-card approved'; + const check = card.querySelector('.gate-check'); + return { + sameNode: check.__rs544 === 'original', + offsetAtFlip: parseFloat(getComputedStyle(check).strokeDashoffset), + }; + }); + assert.equal(flip.sameNode, true, 'the check survived — a replaced node could never transition'); + assert.ok(flip.offsetAtFlip > 0, `and starts undrawn, so it animates — got ${flip.offsetAtFlip}`); + + await page.waitForTimeout(SETTLE_MS); + const settled = await page.evaluate(() => + parseFloat(getComputedStyle(document.querySelector('#page-approvals .approval-card .gate-check')).strokeDashoffset)); + assert.ok(settled < 0.02, `and finishes drawn — settled at ${settled}`); +}); + +glyphTest('a card that ARRIVES approved is already drawn, never replayed (#544)', async ({ page, server }) => { + await openApprovals(page, server); + const arrival = await page.evaluate(() => { + const card = document.querySelector('#page-approvals .approval-card'); + // A brand-new node: nothing was observed to change, so nothing may animate. + const fresh = card.cloneNode(true); + fresh.className = 'approval-card approved'; + card.parentNode.appendChild(fresh); + return parseFloat(getComputedStyle(fresh.querySelector('.gate-check')).strokeDashoffset); + }); + assert.ok(arrival < 0.02, + `an arriving approval shows its check without drawing it — got ${arrival}. Motion here would claim an approval nobody watched happen.`); +}); + +glyphTest('reduced motion lands on the final state at once (#544)', async ({ page, server }) => { + await openApprovals(page, server); + await page.evaluate(() => { + window.handleGlobalSnapshot = function () {}; + document.querySelector('#page-approvals .approval-card').className = 'approval-card approved'; + }); + await page.waitForTimeout(REDUCED_MOTION_SETTLE_MS); + const offset = await page.evaluate(() => + parseFloat(getComputedStyle(document.querySelector('#page-approvals .approval-card .gate-check')).strokeDashoffset)); + assert.ok(offset < 0.02, `no draw under reduced motion — got ${offset}`); +}, { contextOptions: { reducedMotion: 'reduce' } }); + +glyphTest('the glyph never changes the card geometry (#544)', async ({ page, server }) => { + await openApprovals(page, server); + const box = await page.evaluate(() => { + const card = document.querySelector('#page-approvals .approval-card'); + const before = card.getBoundingClientRect(); + card.className = 'approval-card approved'; + const after = card.getBoundingClientRect(); + const glyph = card.querySelector('.gate-glyph').getBoundingClientRect(); + return { + shifted: Math.abs(after.height - before.height) + Math.abs(after.top - before.top), + glyphW: Math.round(glyph.width), glyphH: Math.round(glyph.height), + }; + }); + assert.equal(box.shifted, 0, 'the card does not resize or move when the gate opens'); + assert.ok(box.glyphW >= 20 && box.glyphH >= 20, `the glyph is large enough to read — ${box.glyphW}x${box.glyphH}`); +}); + +glyphTest('no serious/critical axe violations with the glyph present (#544)', async ({ page, server, t }) => { + if (!AXE_PATH) { t.skip('axe-core not resolvable'); return; } + await openApprovals(page, server); + await page.addScriptTag({ path: AXE_PATH }); + const violations = await page.evaluate(async () => { + const result = await window.axe.run(document, { resultTypes: ['violations'] }); + return result.violations.map((v) => ({ id: v.id, impact: v.impact, nodes: v.nodes.length })); + }); + const blocking = violations.filter((v) => AXE_BLOCKING_IMPACTS.has(v.impact) && !AXE_KNOWN_DEBT.has(v.id)); + assert.deepEqual(blocking, [], `approvals stays axe-clean: ${JSON.stringify(blocking)}`); +}); diff --git a/tests/hub-gate-glyph-544.test.js b/tests/hub-gate-glyph-544.test.js new file mode 100644 index 00000000..e1d3dba9 --- /dev/null +++ b/tests/hub-gate-glyph-544.test.js @@ -0,0 +1,143 @@ +/** + * #544 — the approval gate glyph (epic #499, last child). + * + * A state machine for the gate, authored in SVG + CSS rather than pulled in + * as a Rive runtime. The shackle carries the meaning: closed while a human + * is awaited, open once passage is granted, and RE-CLOSED once a one-shot + * override has been spent — which is the state the card's colour alone + * could never express. + * + * Same rule as #541 throughout: transition, never animation, and every part + * present in every state so the nodes survive the morph. + * + * owner: RStack developed by Richardson Gunde + */ + +import test from 'node:test'; +import assert from 'node:assert/strict'; + +import { libScript } from '../src/observability/dashboard/ui/lib.js'; +import { approvalsScript } from '../src/observability/dashboard/ui/pages/approvals.js'; +import { styles } from '../src/observability/dashboard/ui/styles.js'; + +function renderer() { + const document = { + getElementById: () => null, + createElement: () => ({ innerHTML: '', style: {}, appendChild() {}, setAttribute() {}, className: '' }), + querySelector: () => null, + querySelectorAll: () => [], + addEventListener() {}, + }; + return new Function('document', `${libScript}\n${approvalsScript}\nreturn { gateGlyphHtml, approvalHtml };`)(document); +} + +function ruleFor(selector) { + const escaped = selector.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + const match = styles.match(new RegExp(`(^|[\\n}])\\s*${escaped}\\s*\\{([^}]*)\\}`)); + return match ? match[2].replace(/\s+/g, ' ').trim() : null; +} + +const STATES = ['pending', 'approved', 'rejected', 'consumed']; +// Every moving part of the glyph. All must exist in every state. +const PARTS = ['gate-shackle', 'gate-body', 'gate-check', 'gate-bar', 'gate-cross']; + +// ── the glyph is one machine, not four pictures ──────────────────────────── + +test('every part of the glyph exists in every state (#544)', () => { + const glyph = renderer().gateGlyphHtml; + for (const state of STATES) { + const html = glyph(state); + for (const part of PARTS) { + assert.match(html, new RegExp(`class="${part}"`), + `${state} must still carry .${part} — a part drawn only for one state is a NEW node at the flip, and a new node cannot transition`); + } + } +}); + +test('the glyph is decorative — the card already says the state in words (#544)', () => { + const html = renderer().gateGlyphHtml('pending'); + assert.match(html, /aria-hidden="true"/, 'hidden from assistive tech'); + assert.doesNotMatch(html, /aria-label|/, 'and it never duplicates the pill copy as a second announcement'); +}); + +test('the card renders the glyph beside its pill (#544)', () => { + const html = renderer().approvalHtml({ id: 'a1', status: 'pending', title: 'Override guardrail' }, true); + assert.match(html, /gate-glyph/, 'the card carries the glyph'); + assert.match(html, /class="pill pending"/, 'the textual state stays exactly as it was'); +}); + +// ── the state machine itself ─────────────────────────────────────────────── + +test('the shackle opens on APPROVED and closes again on CONSUMED (#544)', () => { + // The whole reason this is worth building: a spent one-shot override is + // BACK TO BLOCKING, and no colour change says that. A shackle that visibly + // re-closes does. + const open = ruleFor('.approval-card.approved .gate-shackle'); + assert.ok(open, 'approved moves the shackle'); + assert.match(open, /transform:/, 'by transform, so nothing reflows'); + + const consumed = ruleFor('.approval-card.consumed .gate-shackle'); + assert.ok(consumed, 'consumed has its own shackle rule'); + assert.match(consumed, /transform:\s*none|translate\(0|translateY\(0/, + 'and it returns to the closed position'); + + // Pending colours the shackle but must never MOVE it: closed is the base + // position of a gate, and the open position has to mean one thing only. + const pending = ruleFor('.approval-card.pending .gate-shackle') || ''; + assert.doesNotMatch(pending, /transform:/, + 'pending leaves the shackle where it starts — shut'); +}); + +test('each state draws its own mark and hides the others (#544)', () => { + assert.match(ruleFor('.approval-card.approved .gate-check'), /stroke-dashoffset:\s*0/, + 'approved draws the check'); + assert.match(ruleFor('.approval-card.rejected .gate-cross'), /stroke-dashoffset:\s*0/, + 'rejected draws the cross'); + assert.match(ruleFor('.approval-card.consumed .gate-bar'), /opacity:\s*1/, + 'consumed shows the spent bar'); + assert.match(ruleFor('.approval-card.consumed .gate-check'), /opacity:\s*0/, + 'and takes the check away — the grant is no longer in force'); +}); + +test('marks start hidden so a state change is what reveals them (#544)', () => { + for (const part of ['.gate-check', '.gate-cross']) { + const base = ruleFor(part); + assert.ok(base, `${part} has a base rule`); + assert.match(base, /stroke-dashoffset:\s*1/, `${part} starts undrawn`); + assert.match(base, /transition:/, 'and reaches its drawn state by transition'); + } + assert.match(ruleFor('.gate-bar'), /opacity:\s*0/, 'the spent bar starts hidden'); +}); + +test('nothing in the glyph animates on its own or moves the layout (#544)', () => { + // Vacuity guard: every part below is skipped when it has no rule, so + // without this the whole test passes on an unstyled glyph. + const styled = PARTS.filter((part) => ruleFor(`.${part}`)); + assert.equal(styled.length, PARTS.length, + `every part must be styled for this rule to mean anything — unstyled: ${PARTS.filter((p) => !ruleFor(`.${p}`))}`); + for (const part of PARTS) { + const rule = ruleFor(`.${part}`); + if (!rule) continue; + assert.doesNotMatch(rule, /animation:/, + `.${part} must not keyframe — an approved card rendered on page load would replay its approval`); + assert.doesNotMatch(rule, /(^|[; ])(width|height|margin|padding|top|left|font-size):/, + `.${part} must not touch layout — got: ${rule}`); + } +}); + +test('the glyph transitions only transform, opacity and the stroke dash (#544)', () => { + // Live-updating surface: anything else risks a reflow on every repaint. + const allowed = /^(transform|opacity|stroke-dashoffset|stroke|fill)$/; + const transitioning = PARTS.filter((part) => /transition:/.test(ruleFor(`.${part}`) || '')); + assert.ok(transitioning.length >= 3, + `the glyph must actually declare transitions for this to check anything — found ${JSON.stringify(transitioning)}`); + for (const part of PARTS) { + const rule = ruleFor(`.${part}`) || ''; + const declared = rule.match(/transition:([^;]*)/); + if (!declared) continue; + for (const clause of declared[1].split(',')) { + const property = clause.trim().split(/\s+/)[0]; + assert.match(property, allowed, `.${part} transitions ${property}, which is not transform/opacity/stroke`); + } + } +}); From bf6d1e4af7ca00a02370821ac633cf2a0904011c Mon Sep 17 00:00:00 2001 From: Richardson Gunde <rgunde@evoketechnologies.com> Date: Sun, 2 Aug 2026 08:19:15 +0530 Subject: [PATCH 2/3] review: pin the live-tick override, guard the DOM reads (#544) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit asked whether `window.handleGlobalSnapshot = ...` actually holds live ticks. Verified in a real browser rather than reasoned about: the client bundle is a classic <script>, so its top-level function declarations become window properties, and assigning over one rebinds the identifier the WS handler calls. Probe returned {onWindow: true, rebound: true} — the override works and the comment was accurate. But three suites depended on that non-obvious fact silently. A switch to <script type="module"> would scope the declaration and turn every one of those overrides into a no-op, with the tests still passing for the wrong reason. Extracted `holdLiveTicks(page)` into the shared harness, which performs the override AND asserts the rebind took effect, and routed all three suites through it — the #515 lesson that copies drift, applied to a line whose correctness nobody could see. Qodo: unguarded querySelector dereferences inside page.evaluate turned any selector or fixture regression into a TypeError instead of a readable failure. Named lookups now throw with the selector and the likely cause. 1973/1973 core, 59/59 browser, lint 0 errors. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --- tests/browser/dashboard-516-micro.test.js | 8 ++--- tests/browser/dashboard-541-motion.test.js | 6 ++-- .../browser/dashboard-544-gate-glyph.test.js | 29 +++++++++++++------ tests/browser/helpers/harness.js | 25 ++++++++++++++++ 4 files changed, 51 insertions(+), 17 deletions(-) diff --git a/tests/browser/dashboard-516-micro.test.js b/tests/browser/dashboard-516-micro.test.js index 1b3387ce..ae31410e 100644 --- a/tests/browser/dashboard-516-micro.test.js +++ b/tests/browser/dashboard-516-micro.test.js @@ -9,7 +9,7 @@ import assert from 'node:assert/strict'; import { fixtureReadyRun, fixtureBlockedRun } from '../helpers/dashboard-fixtures.js'; -import { setupBrowserSuite, gotoDashboard as harnessGoto } from './helpers/harness.js'; +import { setupBrowserSuite, gotoDashboard as harnessGoto, holdLiveTicks } from './helpers/harness.js'; // Shared harness (#515): server boot, browser lifecycle, per-test teardown. const suite = setupBrowserSuite(); @@ -40,8 +40,8 @@ suite.browserTest('pills carry token transitions and feed rows keep keyed identi suite.browserTest('a genuinely-new feed event arrives as a new node; existing rows persist (#516)', { seed: seedRichProject, tmpPrefix: 'rstack-browser-516-' }, async ({ page, server }) => { await gotoDashboard(page, server); + await holdLiveTicks(page); // asserts on injected state const result = await page.evaluate(() => { - window.handleGlobalSnapshot = function () {}; // hold live ticks: this test asserts on injected state const rows = [...document.querySelectorAll('#command-feed .feed-row')]; rows.forEach((row) => { row.__rs516 = 'existing'; }); const next = structuredClone(window.STATE); @@ -66,8 +66,8 @@ suite.browserTest('a genuinely-new feed event arrives as a new node; existing ro suite.browserTest('KPI count-up lands on the exact value and snaps instantly under reduced motion (#516)', { seed: seedRichProject, contextOptions: { reducedMotion: 'reduce' }, tmpPrefix: 'rstack-browser-516-' }, async ({ page, server }) => { await gotoDashboard(page, server); + await holdLiveTicks(page); // asserts on injected state const snap = await page.evaluate(() => { - window.handleGlobalSnapshot = function () {}; // hold live ticks: this test asserts on injected state const next = structuredClone(window.STATE); next.totalRuns = 42; window.applyState(next, {}); @@ -79,8 +79,8 @@ suite.browserTest('KPI count-up lands on the exact value and snaps instantly und suite.browserTest('KPI count-up reaches the target value with motion enabled (#516)', { seed: seedRichProject, tmpPrefix: 'rstack-browser-516-' }, async ({ page, server }) => { await gotoDashboard(page, server); + await holdLiveTicks(page); // asserts on injected state await page.evaluate(() => { - window.handleGlobalSnapshot = function () {}; // hold live ticks: this test asserts on injected state const next = structuredClone(window.STATE); next.totalRuns = 37; window.applyState(next, {}); diff --git a/tests/browser/dashboard-541-motion.test.js b/tests/browser/dashboard-541-motion.test.js index 589be079..6776514a 100644 --- a/tests/browser/dashboard-541-motion.test.js +++ b/tests/browser/dashboard-541-motion.test.js @@ -21,6 +21,7 @@ import { fixtureReadyRun, fixtureBlockedRun } from '../helpers/dashboard-fixture import { setupBrowserSuite, gotoDashboard as harnessGoto, navigate, STATE_LOAD_TIMEOUT_MS, PAGE_VISIBLE_TIMEOUT_MS, + holdLiveTicks, } from './helpers/harness.js'; const require = createRequire(import.meta.url); @@ -46,6 +47,7 @@ const motionTest = (name, body, options = {}) => suite.browserTest( async function open(page, server) { await harnessGoto(page, server, { waitForState: true }); + await holdLiveTicks(page); // every test here asserts on injected state await page.waitForFunction(() => document.querySelectorAll('.overview-proof-step').length > 0, null, { timeout: STATE_LOAD_TIMEOUT_MS }); } @@ -56,7 +58,6 @@ motionTest('a stage that flips to PASS draws its check on the surviving node (#5 await open(page, server); const flipped = await page.evaluate(() => { - window.handleGlobalSnapshot = function () {}; // hold live ticks; this asserts on injected state const next = structuredClone(window.STATE); const stages = (next.overview && next.overview.stages) || []; const target = stages.find((stage) => stage.state !== 'passed'); @@ -98,7 +99,6 @@ motionTest('a stage that ARRIVES passed is drawn instantly, never replayed (#541 await open(page, server); const arrival = await page.evaluate(() => { - window.handleGlobalSnapshot = function () {}; const next = structuredClone(window.STATE); const stages = (next.overview && next.overview.stages) || []; // A stage id that was never on screen: its <li> is created fresh, so the @@ -127,7 +127,6 @@ motionTest('a stage that ARRIVES passed is drawn instantly, never replayed (#541 motionTest('the check never animates under reduced motion (#541)', async ({ page, server }) => { await open(page, server); const index = await page.evaluate(() => { - window.handleGlobalSnapshot = function () {}; const next = structuredClone(window.STATE); const stages = (next.overview && next.overview.stages) || []; const target = stages.find((stage) => stage.state !== 'passed'); @@ -160,7 +159,6 @@ motionTest('a consumed override is visually distinct from a live approval (#541) null, { timeout: PAGE_VISIBLE_TIMEOUT_MS }); const styles = await page.evaluate(() => { - window.handleGlobalSnapshot = function () {}; const card = document.querySelector('#page-approvals .approval-card'); // Each state is read on its OWN fresh node. Re-classing one node and // reading immediately samples the value mid-transition — the very diff --git a/tests/browser/dashboard-544-gate-glyph.test.js b/tests/browser/dashboard-544-gate-glyph.test.js index 7f625821..c0e3442d 100644 --- a/tests/browser/dashboard-544-gate-glyph.test.js +++ b/tests/browser/dashboard-544-gate-glyph.test.js @@ -18,6 +18,7 @@ import { fixtureBlockedRun, fixtureReadyRun } from '../helpers/dashboard-fixture import { setupBrowserSuite, gotoDashboard as harnessGoto, navigate, PAGE_VISIBLE_TIMEOUT_MS, + holdLiveTicks, } from './helpers/harness.js'; const require = createRequire(import.meta.url); @@ -41,6 +42,7 @@ const glyphTest = (name, body, options = {}) => suite.browserTest( async function openApprovals(page, server) { await harnessGoto(page, server, { waitForState: true }); + await holdLiveTicks(page); // every test here asserts on injected state await navigate(page, 'approvals', { assertVisible: false }); await page.waitForFunction(() => document.querySelector('#page-approvals .gate-glyph') != null, null, { timeout: PAGE_VISIBLE_TIMEOUT_MS }); @@ -49,20 +51,26 @@ async function openApprovals(page, server) { // Read each state on its OWN fresh node: re-classing one node and reading // immediately samples mid-transition (the #541 lesson). const readStates = (states) => page => page.evaluate((wanted) => { - const card = document.querySelector('#page-approvals .approval-card'); + // Named lookups: a selector or fixture regression should surface as a + // readable assertion, not a TypeError from dereferencing null (Qodo, #545). + const need = (root, selector) => { + const found = root.querySelector(selector); + if (!found) throw new Error(`expected ${selector} to exist — the glyph markup or the fixture changed`); + return found; + }; + const card = need(document, '#page-approvals .approval-card'); const out = {}; for (const state of wanted) { const clone = card.cloneNode(true); clone.className = 'approval-card ' + state; card.parentNode.appendChild(clone); - const shackle = clone.querySelector('.gate-shackle'); - const style = getComputedStyle(shackle); + const style = getComputedStyle(need(clone, '.gate-shackle')); out[state] = { transform: style.transform, shackleStroke: style.stroke, - check: getComputedStyle(clone.querySelector('.gate-check')).opacity, - cross: getComputedStyle(clone.querySelector('.gate-cross')).opacity, - bar: getComputedStyle(clone.querySelector('.gate-bar')).opacity, + check: getComputedStyle(need(clone, '.gate-check')).opacity, + cross: getComputedStyle(need(clone, '.gate-cross')).opacity, + bar: getComputedStyle(need(clone, '.gate-bar')).opacity, }; clone.remove(); } @@ -104,8 +112,8 @@ glyphTest('a real approval animates the glyph on the surviving node (#544)', asy await openApprovals(page, server); const flip = await page.evaluate(() => { - window.handleGlobalSnapshot = function () {}; // hold live ticks const card = document.querySelector('#page-approvals .approval-card'); + if (!card) throw new Error('no approval card rendered — the fixture changed'); card.querySelector('.gate-check').__rs544 = 'original'; card.className = 'approval-card approved'; const check = card.querySelector('.gate-check'); @@ -127,6 +135,7 @@ glyphTest('a card that ARRIVES approved is already drawn, never replayed (#544)' await openApprovals(page, server); const arrival = await page.evaluate(() => { const card = document.querySelector('#page-approvals .approval-card'); + if (!card) throw new Error('no approval card rendered — the fixture changed'); // A brand-new node: nothing was observed to change, so nothing may animate. const fresh = card.cloneNode(true); fresh.className = 'approval-card approved'; @@ -140,8 +149,9 @@ glyphTest('a card that ARRIVES approved is already drawn, never replayed (#544)' glyphTest('reduced motion lands on the final state at once (#544)', async ({ page, server }) => { await openApprovals(page, server); await page.evaluate(() => { - window.handleGlobalSnapshot = function () {}; - document.querySelector('#page-approvals .approval-card').className = 'approval-card approved'; + const card = document.querySelector('#page-approvals .approval-card'); + if (!card) throw new Error('no approval card rendered — the fixture changed'); + card.className = 'approval-card approved'; }); await page.waitForTimeout(REDUCED_MOTION_SETTLE_MS); const offset = await page.evaluate(() => @@ -153,6 +163,7 @@ glyphTest('the glyph never changes the card geometry (#544)', async ({ page, ser await openApprovals(page, server); const box = await page.evaluate(() => { const card = document.querySelector('#page-approvals .approval-card'); + if (!card) throw new Error('no approval card rendered — the fixture changed'); const before = card.getBoundingClientRect(); card.className = 'approval-card approved'; const after = card.getBoundingClientRect(); diff --git a/tests/browser/helpers/harness.js b/tests/browser/helpers/harness.js index 0945bea2..70636fd8 100644 --- a/tests/browser/helpers/harness.js +++ b/tests/browser/helpers/harness.js @@ -170,3 +170,28 @@ export async function navigate(page, pageId, { assertVisible = true } = {}) { if (assertVisible) await waited; else await waited.catch(() => { /* assertions in the caller report the real state */ }); } + +/** + * Stop live WS snapshots from overwriting state a test injected. + * + * Three suites did this with a bare `window.handleGlobalSnapshot = ...`, + * which only works because the client bundle is a classic <script>: its + * top-level function declarations become window properties, so assigning + * over one rebinds the identifier the WS handler calls. That is true today + * and verified here — but it is silent if it ever stops being true (a switch + * to <script type="module"> would scope the declaration and make every one + * of those overrides a no-op, with the tests still passing for the wrong + * reason). CodeRabbit asked the question on PR #545; this is the answer, + * pinned in one place instead of assumed in three. + */ +export async function holdLiveTicks(page) { + const rebound = await page.evaluate(() => { + if (typeof window.handleGlobalSnapshot !== 'function') return 'absent'; + window.handleGlobalSnapshot = function () {}; + // Resolve the BARE identifier the WS handler uses, not the window + // property we just set — that is the whole question being asked. + return new Function('return handleGlobalSnapshot')() === window.handleGlobalSnapshot; + }); + assert.equal(rebound, true, + 'overriding window.handleGlobalSnapshot must actually rebind the identifier the WS handler calls — otherwise injected state is silently overwritten by the next tick'); +} From 3f61431b73bb0f4da6123e25eb0a9d8dfc9ad668 Mon Sep 17 00:00:00 2001 From: Richardson Gunde <rgunde@evoketechnologies.com> Date: Sun, 2 Aug 2026 08:24:53 +0530 Subject: [PATCH 3/3] review: compare the rebind against the replacement, not against itself (#544) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit, and it was right at Major. My check read window.handleGlobalSnapshot and the bare identifier and compared them TO EACH OTHER. If the assignment silently failed — a non-writable or getter-only property — both reads return the original, match, and the check passes while the override does nothing. Both reads are now compared against the replacement function itself, and the failure message names which side did not take. Proved rather than asserted. On a global defined non-writable: old check => true (would have passed, blind) new check => correctly failed A guard that cannot fail is not a guard, and this one is meant to protect three suites from silently asserting on state a live tick already replaced. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --- tests/browser/helpers/harness.js | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/tests/browser/helpers/harness.js b/tests/browser/helpers/harness.js index 70636fd8..c68df21a 100644 --- a/tests/browser/helpers/harness.js +++ b/tests/browser/helpers/harness.js @@ -186,11 +186,17 @@ export async function navigate(page, pageId, { assertVisible = true } = {}) { */ export async function holdLiveTicks(page) { const rebound = await page.evaluate(() => { - if (typeof window.handleGlobalSnapshot !== 'function') return 'absent'; - window.handleGlobalSnapshot = function () {}; - // Resolve the BARE identifier the WS handler uses, not the window - // property we just set — that is the whole question being asked. - return new Function('return handleGlobalSnapshot')() === window.handleGlobalSnapshot; + if (typeof window.handleGlobalSnapshot !== 'function') return 'no window.handleGlobalSnapshot to override'; + // Both reads are compared against THIS function, never against each + // other: if the assignment silently fails (a non-writable or getter-only + // property), both reads return the original and would happily match + // (CodeRabbit, PR #545). + const replacement = function rstackHeldTick() {}; + window.handleGlobalSnapshot = replacement; + const onWindow = window.handleGlobalSnapshot === replacement; + // The BARE identifier is the one the WS handler calls. + const bare = new Function('return handleGlobalSnapshot')() === replacement; + return onWindow && bare ? true : `assignment did not take (window=${onWindow}, identifier=${bare})`; }); assert.equal(rebound, true, 'overriding window.handleGlobalSnapshot must actually rebind the identifier the WS handler calls — otherwise injected state is silently overwritten by the next tick');