oficina / docs Public

Whole-branch review by Fable, and what it changed — 0bab498

Done per AGENTS.md's highest-capability rule, read as one diff. It found one blocking problem, and it was not in this branch's code — which is exactly the class of thing a whole-branch review exists to catch and branch CI structurally cannot.

BLOCKING — the branch was stale, and the collision was semantic

origin/main had moved to cafb696 ("Impeccable pass") four seconds after this branch's previous merge point. cafb696 adds internal/docs/assets_test.go, whose TestExactlyOneEditorBundleIsEmbedded asserts static/ holds exactly one entry. This branch commits a second bundle.

Squash-merging as it stood would have put a red build on main — and no amount of green CI on this branch could have warned, because the failing test only exists on main. That is the argument for reading a branch against where it will land rather than where it started, stated better by an example than by the rule.

Merged, and the test reconciled rather than deleted. Its real job is not the number one: it is that static/ holds only the bundle build's output, because the tab icon was written there once and silently deleted by the gate's own cleanup — which is why it is a test and not a note. It is now "exactly one file per name in bundleKinds, and nothing present that no kind accounts for", which keeps the stray-file protection assets.go's registry cannot provide: the registry looks up names it knows and would never notice a third file nobody named. Verified by planting a stray file and watching it fail.

assets.go's conflict was both sides adding to the same two places — cafb696's icon serving and this branch's bundle registry. Both kept.

Fixed while in the files

• The palette hijacked IME composition keys. Mid-composition in Japanese, Chinese or Korean, Enter commits a conversion and Escape cancels it — so the palette navigated away while somebody was still typing the word they meant to search for, and closed when they tried to take it back. This app declares twelve base locales; that is not an edge case, it is most of them.
• A palette closed on "No documents match" reopened still saying it, over an empty query — and being a live region, announced the stale answer again.
• A catalog key nobody read (docs.list.search.results). Twelve locales would have translated it for nothing.

One claim withdrawn

The plan's D4 said a browser-drive check holds the palette's keyboard contract. It does not. The checks exist and pass — seventeen of them — but in a scratch driver deliberately not committed, because the harness they belong in is on the other branch. What protects that behaviour today is one person having run it once, which is precisely what this repo does not count as a gate. The plan now says so.

Two comments citing an AGENTS.md entry that lives on the other branch now state their reasoning inline instead, so they hold whichever order the branches land in.

What the review checked and did not find

Scoping holds — owned() fails closed, the query string never reaches SQL, and the tests cover the store, the JSON route and the /?q= page route separately. Escaping holds on both surfaces. The ranking is deterministic. The assets.go invariant was generalised, not weakened. §6.4's spirit is respected: one field, one button, ungrouped ranked results, no filter chrome, and the no-JS path is primary.

Still yours

Tasks 20–22: the copy review, accent folding (cafe does not match café — a product decision, not a technical one), and folding the palette's checks into the shared harness.

Three non-blocking findings I have left alone deliberately, since each is a judgement rather than a defect: the 50-result cap truncates with no "more matched" signal; there is no NFC normalisation, so precomposed and decomposed forms of the same text do not match (rare, and distinct from the accent-folding decision); and an expired session makes the palette say "unavailable" rather than "sign in again".

— claude~paul@keymail.dev, session "checklist formatting and export"

Merged to main. Paul approved after reading the review outcomes; both branches met the cadence's three conditions — done, reviewed by the highest available model and approved, CI green.

• formatting-and-export → eefa1a4
• search → 81f64d4
• main's own CI: passed at 81f64d4

Both were stale first and both had main merged in before landing, which is where the real work was. formatting-and-export had missed live sync and the Impeccable pass; search had then missed formatting-and-export. Conflicts were additive on every side — two route registrations, two sets of showPage fields, two imports in editor-entry.js, and two rules appended to the same paragraph of AGENTS.md — with one genuine adaptation: ApplyUpdate now returns the accepted row's seq alongside its error, so the export test's call site takes both.

The thing worth recording is what was verified rather than what merged. views/show.html now carries the formatting bar and the jump-to palette at once, from two separate bundles reaching for the same keyboard on the same page — so both drives were run against the merged tree rather than against either side of it: the committed browser drive at 22/22, including the three keyboard checks that are exactly where a second bundle could tread on the first, and the palette's own 17/17, with Control-K still opening from inside a document that now has a toolbar above it. "Both merged cleanly" and "both still work" are different claims and only the second one matters.

Decisions that merged as built

These were flagged for Paul and merged with his go-ahead rather than settled first. All are reversible, which is why they were not worth blocking on:

• Liberation instead of §11.1's Carlito and Caladea, and TTF instead of woff2. Documented deviations with their reasoning in the plan (D1); Carlito and Caladea have a job when DOCX import exists to name Calibri, and nothing here forecloses adding them.
• Accent folding: cafe does not match café. A one-line change with a test asserting the current behaviour, so reversing it is a deliberate act rather than a regression.
• The copy on both branches is unreviewed — docs.editor.toolbar.*, docs.export.*, docs.list.search.*, docs.jumpto.*. Catalog strings, changeable without touching code.
• Control-K as the palette's shortcut.
• scripts/browser-drive.sh is still not in CI. It needs the sandbox off, not merely a Chrome on the runner — that is a decision about what CI may do, and it stays open.

Open follow-ups live on tasks 23 and 24: hard_break vanishing in all three renderers, a newline in a run injecting Markdown block syntax, the search cap truncating silently, no NFC normalisation, and an expired session reading as an outage in the palette.

Nothing is deployed. carlos deploy, or promote-then-restart, is a separate step — and carlos channels cannot be trusted to say a release is live (AGENTS.md's own entry now, from the live-sync session's deploy).

— claude~paul@keymail.dev, session "checklist formatting and export"

Overnight triage: task 22 done, task 24 half done, and two of your checks could not fail

Paul asked for a triage-and-fix pass on everything open. The work is on worktree-discussions-triage, discussion #26; this is what it means for this thread.

Task 22 — the palette's checks are folded in, and the fold found two false passes.

The withdrawn D4 claim on this thread was right and is now retired properly: what protected the palette's keyboard contract was one person having run a scratch script once. Twenty checks are in fixtures/browser-drive.mjs now, inside the committed harness, and the drive is at 44.

Two of the seventeen were checks that could not fail, which is worth more to this thread than the fold itself:

• "focus is no longer held in the field" passed with the focus-return call deleted — 44/44, green. Closing the palette hides the input, so focus leaves it whether or not anything put it back. It now focuses a named element, opens from there, and asserts Escape returns focus to that exact element. Broken, it reports "focus is on BODY".
• "Up moves back to the first" passed with the arrow handler gutted, because the first option is selected both when Up walked back to it and when nothing moved at all. It is guarded on Down having actually moved now, and says so when it did not.

Both are this thread's own recurring shape, in the checks written to hold the contract this thread was most careful about. That is not a criticism of the scratch driver — it is the argument for the deliberate-break step, which is the only thing that surfaced them.

Three checks are new, one being a regression check for the stale-empty-message bug Fable found here and fixed without one. And every query is derived from the titles actually on the list rather than typed in as "s" and "waterworks" — your own warning, taken.

Task 24 — one of three fixed; the other two need copy.

No NFC normalisation — fixed. "café" as U+00E9 and "café" as e + combining acute compared as bytes and did not match, so a title typed on a machine producing decomposed text (macOS's filesystem, several IMEs) was unfindable from one producing precomposed text: an empty result for a title you are looking straight at. NFC is applied inside normaliseForSearch so the title and the query go through the same code — normalising one side fixes one direction and looks fixed from whichever side you tested.

Your caution was exactly right and I have honoured it: this does not touch accent folding. cafe still does not match café, TestSearchDoesNotFoldAccents still passes, and the new test asserts the non-folding directly so nobody later reads this as having settled task 21.

The 50-result cap truncating silently and an expired session reading as an outage — not done, deliberately. Both are one small change plus a user-visible string, and every user-visible string here is a catalog key across twelve locales. I am not writing copy Paul has not reviewed and then having twelve catalogs translate it; they go in the same pass as task 20.

NFC and the cap were also easy to conflate in one commit — they are separate commits.

Tasks 20 and 21 remain yours, unchanged. Task 21 in particular is now the only thing standing between this thread and "search is finished": accent folding is a one-line change with a test asserting the current behaviour, so reversing it is a deliberate act either way.

Leaving this thread open for 20, 21, and the two copy-blocked halves of 24.

— claude~paul@keymail.dev, session "discussions triage overnight"

Tasks 22 and the NFC third of 24 are done and deployed — merged to main as 1b6d392, live and confirmed running that version.

Task 22's fold is in, and it is worth repeating what folding it found: of the checks that came across, four could not fail, two found by me and two by the Fable review. Focus-return passed with the focus-return call deleted; "Up moves back" passed with the arrow handler gutted; "a query matching nothing says so" passed when the search was broken, because it only asked whether some text appeared and the status element carries the failure copy too; and the 320px check I added the same night never asserted the viewport was actually 320. All four are rewritten and each is now confirmed red against a deliberate break.

That is the argument for this thread's own D2 restated: one ranker, one harness, and checks that have been seen to fail.

Tasks 20, 21 and the other two-thirds of 24 remain open, all for the same reason — they need copy you have not reviewed:

• 21, accent folding is a genuine product decision, not a copy one, and is now the only thing between this thread and finished. cafe still does not match café. One line either way, with a test asserting whichever you choose.
• 24's remaining halves — the 50-result cap needs a "more matched" signal and an expired session needs "sign in again" rather than "unavailable". Each is one small change plus one catalog key across twelve locales, which is why they wait for task 20 rather than shipping in English.

To be explicit about what NFC did and did not settle: it fixed "café" (U+00E9) not matching "café" (e + combining acute) — the same string in two encodings. It did not touch folding. TestSearchDoesNotFoldAccents still passes and the new test asserts the non-folding directly, so task 21 is exactly as open as it was.

Leaving open for 20, 21 and 24.

— claude~paul@keymail.dev, session "discussions triage overnight"