oficina / docs Public

<task-notification> <task-id>aac896ffcecc70973</task-id> <tool-use-id>toolu_012wmP6DdiDGRqM8e7dscSkk</tool-use-id> <output-file>/home/paulca/.cache/tmp/claude-1001/-home-paulca-amadan-net-oficina-docs/4a30c557-129a-4e57-8aa3-0c7518c36cf1/tasks/aac896ffcecc70973.output</output-file> <status>completed</status> <summary>Agent "Fix the toolbar's redundant writes" finished</summary> <note>A task-notification fires each time this agent stops with no live background children of its own. The user can send it another message and resume it, so the same task-id may notify more than once.</note> <result>**Status:** Done. Menu bar fixed, helpers shared, guard extended to both bars. **Commits:** `47c9080` (toolbar), `fd8eb43` (menu + shared module) **Changes:** `setAttr`/`setProp` moved to new `fixtures/dom-writes.mjs`; both bars import it — no copy. `menu.mjs` routes 4 writes through `setProp` (the 2 sync writes that caused your 5, plus its 2 roving-tabindex writes, so the module has one discipline rather than a carve-out). `toolbar.test.mjs` now mounts **both** bars as `editor-entry.js` does — one plugin, menu on `onSync`, two roots — so zero-writes-on-no-op is held once across the chrome. Structural rule covers `toolbar.mjs` + `menu.mjs`, hardcoded pair with a comment saying add a third surface here. **Tests:** 3/3 pass. Failed first with menu unfixed: exactly 4 recorded writes, all `from === to`, **all menu, zero toolbar** — your first fix holding under an independent test. Mutations: unconditional `setAttribute` in menu sync fails both guards (structural names `menu.mjs:311`); removing `setProp`'s comparison fails both bars at once. **Gate:** exit 0, tree clean, bundle rebuilt (`4dc0a272… → 5dc0309c…`). Browser drive 44/44, including `the menu bar and the formatting bar are one tab stop each` — the check the tabIndex change could have broken. **Concerns:** `GUARDED` is a manual file list, so a third surface added to that plugin and not added to the list repeats this; I found no mechanical derivation that didn't sweep in modules which legitimately write DOM off the update path. `.open` writes and props outside the denylist (`ariaLabel`, `style.*`) remain unguarded structurally. Report appended at `.superpowers/flash-report.md`.</result> <usage><subagent_tokens>204884</subagent_tokens><tool_uses>53</tool_uses><duration_ms>827059</duration_ms></usage> </task-notification>

<task-notification> <task-id>a9737ce20fc29988e</task-id> <tool-use-id>toolu_01VEsVR4pJaWErN8dK3jWqW9</tool-use-id> <output-file>/home/paulca/.cache/tmp/claude-1001/-home-paulca-amadan-net-oficina-docs/4a30c557-129a-4e57-8aa3-0c7518c36cf1/tasks/a9737ce20fc29988e.output</output-file> <status>completed</status> <summary>Agent "Fable review of the flash fix" finished</summary> <note>A task-notification fires each time this agent stops with no live background children of its own. The user can send it another message and resume it, so the same task-id may notify more than once.</note> <result>The review is complete — every claim checked against the real branch, all three failing-first mutations reproduced in a scratch copy, the browser drive and gate re-run by me. Here is the review. --- # Whole-branch review: `worktree-toolbar-flash` (47c9080, fd8eb43) ## Verdict: approve with changes — one small, test-only change The product fix is correct and merges as-is: every routed call site is safe to skip, the tabIndex routing cannot break roving focus, and the tests genuinely fail against the defect they guard (I reproduced all three failing-first claims independently). The one genuine defect is in the guard, not the shipped code: the report's central safety claim about the structural check — "false-positive-prone but never false-passing" — is provably wrong, and I constructed a one-line regression on the every-keystroke path that passes all three tests. Closing it is about ten lines in the test file. ## Findings, most severe first ### 1. Genuine defect (guard only): a complete false pass exists, contradicting the claimed failure direction **Concrete failure scenario, verified by running it:** insert into `menu.mjs`'s sync loop, on the every-state-update path: ```js if (!name.includes('https://')) el.title = 'seen' ``` All three tests pass (I ran it: 3/3 green). This is exactly the defect class the branch fixes — `title` is an attribute-reflected property, so the identical-value write invalidates style and records a mutation on every keystroke — and it ships invisibly. Two independent blindnesses overlap: - `stripComments` in `fixtures/toolbar.test.mjs` (~line 414 of the file) does not parse string literals, so the `//` inside `'https://'` blanks the rest of the line, hiding the write from the `WRITE` regex. This is a **false pass**, not the advertised false positive — anything written after a `//`-carrying string on the same line vanishes from the structural check. - The recording fake (`makeDOM`'s `element()`, ~line 118) logs only `setAttribute`/`removeAttribute` and the three defined accessors (`disabled`, `tabIndex`, `value`). An assignment to `.title`, `.textContent`, `.className`, `.checked`, `.hidden`, or `.innerHTML` lands silently on the plain object — behaviorally invisible even for controls the fake drives. (Verified separately: `el.title = 'seen'` with no string trick fails only the structural test, never the behavioral one.) Each net's hole is exactly the other's blind spot for these shapes. **Smallest fix:** make the fake log every property set — wrap each element in `new Proxy(node, { set(t, k, v) { log.push({ id, name: k, from: t[k], to: v }); t[k] = v; return true } })`, keeping the defined accessors for the select-value semantics. That catches my probe behaviorally (and, as a bonus, catches `el['disabled']`, `el.ariaPressed`, and compound assignments like `||=`, which the regex's bare-`=` shape also misses), making the "false-positive only" claim true of the combined guard for any driven control. Fixing the stripper to skip strings/regexes is the alternative but is harder to get right than the proxy. ### 2. The structural guard's direction, judged (question 2) With finding 1 recorded, the honest statement is: the structural check alone is false-positive-prone **and** false-passable (via the stripper); the behavioral check alone is blind to non-logged properties and to controls the fake doesn't carry; the union today covers the denylisted shapes on driven controls except the overlap in finding 1. **A third surface added to the same plugin gets caught by neither** — not in `GUARDED`, not mounted by the fake — and the defect recurs a third time, exactly as menu.mjs reproduced it. The only defense is the comment in `toolbar.test.mjs` ("Add a third surface to that plugin and add it to this list"), which is placed where the person adding one would look. I looked for a mechanical derivation and agree with the implementer that a rule over "every module that writes DOM" sweeps in `pages.mjs`, `wordcount.mjs`, `jump-to.mjs`, which legitimately write outside the update path. A check that greps `editor-entry.js` for modules wired into the toolbar's `onSync` and requires each in `GUARDED` would be the mechanical version; worth considering, not worth blocking on. Record it. ### 3. Question 1 — the DOM is genuinely identical at every routed call site; skipping loses nothing Checked each one: - `disabled` / `tabIndex` (buttons, selects, combo inputs, menu items, titles): property getters reflect the live state; equal means equal, skip is a no-op. - `aria-pressed`, `aria-label` via `setAttr`: string-vs-`getAttribute` comparison is exact. - `input.value` (comboboxes): the dangerous direction is the old code's — assigning even an identical string resets the caret to the end in Chromium (the well-known `el.value = el.value` trick). Skipping is strictly safer, and `showInCombo` already declines while the input has focus, so the caret case is doubly covered. Programmatic value sets fire no events either way, so no listener is starved. - `select.value`: "equal for a select is not string equality" fails only in two ways, and both are handled. Duplicate option values (set-to-value selects the first duplicate) — none of the three selects has duplicates (block/font/size lists are all unique, checked against `show.html`). Value not on the option list — the select reports `""` back, so the guard never quiets and the write fires on every update: loud, and the fake reproduces a real select's refusal semantics exactly (`makeDOM`'s value setter), so test 1 would fail rather than hide it. No enabled path reaches it today; a size ladder drifting from `export.go`'s would, and would be caught. ### 4. Question 5 — tabIndex through setProp cannot break roving focus, with the mechanism All toolbar controls are server-rendered with explicit `tabindex="-1"` (`show.html` lines 1407–1498), so property reflection is exact there. The one real divergence is the menu `&lt;summary docs-menu-title&gt;` (line 1364), rendered with **no** tabindex attribute: a details' summary's IDL default is 0, so `setProp(t, 'tabIndex', 0)` at mount now skips, and the first title never receives a `tabindex="0"` attribute. That is behaviorally identical — natively focusable, same tab-order position — and nothing in the repo reads the attribute (no `[tabindex` selectors, no `getAttribute('tabindex')` anywhere in fixtures or views). If any browser's getter reported non-zero, setProp would write, restoring old behavior; the skip only happens when native behavior already provides it. Confirmed empirically: **I re-ran `scripts/browser-drive.sh` myself — 44/44**, including "the menu bar and the formatting bar are one tab stop each" and the arrow-key checks. One note for the record: that drive check is an upper bound (`reachedAfter &lt;= 4`, lower bound only `&gt; 0`), so it would not catch the menu bar dropping out of the tab order entirely; the reflection argument above is what actually closes that case. ### 5. Question 3 — the recording-stub test supports every failing-first claim; I reproduced them In a scratch copy (worktree untouched): reverting `menu.mjs`'s two sync writes to unconditional fails test 1 with **exactly 4 writes, all `from === to`, all `menu-` ids, zero toolbar** — precisely the reported failure — plus the structural test. Dropping `setProp`'s comparison in `dom-writes.mjs` fails test 1 through both bars at once. A new unconditional `setAttribute` in either loop fails both guards. The design is sound where its net reaches: the plugin's `update()` calls `sync` unconditionally (no `docChanged` gate — `toolbar.mjs` lines 363–374), so the no-op path driven is the real one; test 1 cannot pass vacuously (asserts the mount wrote, asserts end states on both bars); test 2 is a genuine counterweight with literal expected values. The caveat is finding 1: a new unconditional write to a property the fake doesn't log is invisible to it. ### 6. Question 4 — the `globalThis.document` stub is acceptable The module's two global reaches (`document.activeElement` in `showInCombo`, the click-away listener in menu) are documented in both modules and in the test, and both are read only inside functions, so import hoisting above the assignment is harmless; `node --test` isolates per file. Injecting `document` would change a browser-behavior line for a test's convenience — the worse trade, as the implementer says. Not a hidden coupling; a named one. ### 7. Question 6 — no ninth unfailable test I hunted for the repo's recurring shape (asserting a value against itself or its derivative) in the new assertions: test 2's expectations are all literals (`'true'`, `'heading-1'`, `'mono'`, `'24'`, `'Heading 1'`); the fake logs `from` by reading before writing but no assertion compares log fields to each other; the structural test compares against `[]`. The nearest miss is the import check (`code.includes("from './dom-writes.mjs'")`), which proves the import exists but not that writes route through it — harmless, since the regex denylist does the actual work, and finding 1's fix is what matters. ## Worth recording (not defects, not blocking) - `toolbar-roving.mjs`'s `setStop` still writes `tabIndex` on all ~14 controls unconditionally whenever it runs (arrow key, focusin, or a sync that disables the current stop). Off the no-op path — `ensureReachable` early-returns — so the zero-writes property holds; the implementer flagged it accurately. - Toolbar sync adds `aria-pressed="false"` to undo/redo at first sync although `show.html` deliberately omits it there; pre-existing, unchanged by this branch, but declaring undo a toggle button is an ARIA wart someone may want to remove. - `dataset.*`, `style.*`, and `classList` writes are on neither net even after the proxy fix; the denylist comment already owns this class of gap. ## Verification performed - Full diff read (`.superpowers/flash-review.diff`); files checked at the branch tip, which is this worktree's checkout (fd8eb43). - `node --test toolbar.test.mjs menu.test.mjs`: 8/8 pass. - Three failing-first mutations reproduced in `$TMPDIR`, matching the report's outputs exactly; plus the new false-pass probe (finding 1). - `./.amadan/ci.d/10-gate`: `gate: ok`, exit 0, tree clean, bundle matches a fresh build. - `./scripts/browser-drive.sh` (sandbox disabled for that one command — Chromium cannot start inside it, per the script's own header): 44/44. The fix itself is sound, measured before and after, and honestly reported — the implementer's own concerns section called two of the three soft spots. Merge it; land the ten-line proxy change in `toolbar.test.mjs` so the guard's advertised direction becomes true.</result> <usage><subagent_tokens>96738</subagent_tokens><tool_uses>30</tool_uses><duration_ms>648577</duration_ms></usage> </task-notification>