oficina / docs Public

do we need to do anything?

<task-notification> <task-id>aeffd0d50354caa23</task-id> <tool-use-id>toolu_01PMiZJcrymtXuqB9HUvq9SK</tool-use-id> <output-file>/home/paulca/.cache/tmp/claude-1001/-home-paulca-amadan-net-oficina-docs/48ea084f-0296-4b0a-9104-dba9bbd5928c/tasks/aeffd0d50354caa23.output</output-file> <status>completed</status> <summary>Agent "Fable whole-branch review" 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># Whole-branch review: `worktree-impeccable-init` (fd1d21a..ffebf47) Read as one diff per AGENTS.md. Verified by running things: `go build ./...` clean, `go test ./internal/... ./cmd/...` all pass, `gofmt -l internal/docs/` clean, and I ran the app via `./scripts/run-dev.sh` and curled rendered pages. I did not run `.amadan/ci.d/10-gate` (this branch touches no Rust/wasm/bundle; Go build+tests green; the amadan CI status check remains the merge gate regardless). ## BLOCKING **B1. The "whole row clickable" fix is a functional no-op, and its measurement narrative — written into comments, a test, and the commit message — is false.** rastrillo v0.22.0's `tokens.css`, which this app serves live via `ui.TokensCSS()` (`internal/docs/render.go:271`; confirmed byte-for-byte in the running app's `/static/tokens.&lt;hash&gt;.css`), already ships all three mechanisms this fix claims to add: - `[rst-row] { position: relative; }` — tokens.css:500–507 - `[rst-row]:hover { background: var(--rst-surface-2); }` — tokens.css:515, **un-gated** - `[rst-row-main] &gt; a::after { content:""; inset:0; position:absolute; }` — tokens.css:558, under the framework's own comment *"The name link's overlay makes the whole row clickable without nesting one anchor inside another."* The pre-branch markup (`&lt;span rst-row-main&gt;&lt;a href=…&gt;`) already matched that selector, and the row was already the anchor's containing block. So before this branch, the whole row was already clickable, already hover-tinted, and already cleared SC 2.5.8 without leaning on the Spacing exception. The branch's three rules (`internal/docs/views/list.html:88–104`) restate identical property:value pairs on the same elements — `[docs-row-link]::after` and `[rst-row-main] &gt; a::after` are literally the *same* pseudo-element. The `@media (hover: hover)` gate achieves nothing: the framework's un-gated rule still applies on touch, so the sticky-tap tint the comment claims to prevent still happens. The comment's claim — "a 990×61 row whose only target was a 268×16 link — 7% of it … the thumbnail … did nothing at all" — is what you get from measuring the anchor's own bounding box, which cannot see a pseudo-element's hit area. That is AGENTS.md's named failure shape ("a verification that cannot distinguish 'no' from 'I can't see it'"), here about to be merged as a permanent false record, in a repo whose spec preamble says a document that quietly gets its facts wrong "teaches the next reader nothing." The commit message claims five defects; this one was not a defect. Fix before merge: delete the three CSS rules, the `docs-row`/`docs-row-link` attributes, `TestListRowsAreClickableAcrossTheWholeRow`, and the false comments (or rewrite them to record that rastrillo's row already stretches the link — that's worth a note). If a hover-capability gate is genuinely wanted, it must *override* the framework's rule, not duplicate it. **B2. PRODUCT.md asserts a security property, in the present tense, that does not exist.** Positioning item 3 (`PRODUCT.md:66–71`): "Content **is** encrypted in SQLite under a DEK held by the running instance; a test greps raw SQLite bytes and bucket objects for fixture plaintext and fails if it finds any (§2)." And Product Principles item 2 calls the byte-grep seal test "a shipped habit, not an aspiration" ("the seal is a byte-grep test"). Both are false today: `internal/docs/store.go:386–389` says outright "Sealing (spec §2 …) is out of scope for this plan; this plaintext replay is not a decision that content stays unsealed," and no plaintext-grep test exists anywhere in the repo (I searched). The file's own preamble promises anything that isn't citable fact is marked `[inferred — confirm]` — a false "content is encrypted at rest" claim in the product record is exactly what someone deploying an instance will trust. Rewrite both passages as the spec commitment they are (sealed at rest is designed and ruled, not shipped), or mark them. ## SHOULD FIX **S1 (before or after merge). `TestListRowsAreClickableAcrossTheWholeRow`'s third assertion is tautological** — independent of B1. It asserts `strings.Contains(body, "[docs-row-link]::after")`, and that exact string also appears in the CSS *comment* at `views/list.html:67`, which is served to the client. Delete the rule and the test still passes. Moot if B1 removes the test entirely; noted so the pattern isn't replicated. **S2 (after merge is fine). `TestIconIsServedAndLinkedConsistently` checks the template file's bytes, not rendered output** (`assets_test.go:170–177`). If `newRenderer` ever stops parsing `views/head.html` into the base (the exact wiring `render.go:216` adds), the favicon link silently vanishes from every page while both halves of this test stay green — the file still contains the href, the icon still serves. Assert the rendered sign-in page (no session needed) contains `href="/static/icon.svg"` instead; that subsumes the file check. I verified the current wiring works end-to-end — the rendered `&lt;head&gt;` does carry the link on all pages — so this is a test-strength gap, not a live bug. **S3 (before merge, one line). PRODUCT.md: "Passkeys are ruled out on purpose, not deferred."** Overstated: §8.1/§8.1.1 rule out *instance-local* passkeys; the suite path deliberately plans passkeys at the home's single relying party. Say "instance-local passkeys are ruled out." ## NIT - `[docs-create-form]`'s `flex-wrap: nowrap` with `flex: none` on the button means a locale whose submit label alone exceeds ~270px would overflow at 320px. Only `en` is declared today; worth remembering when the eleven catalogs land. - `TestDocumentPageLinksBackToTheList` asserts the exact serialization `&lt;a docs-back href="/"&gt;` and `&lt;div rst-row docs-row&gt;` — attribute-order brittle, though it's the app's own markup two files away, consistent with house style. - PRODUCT.md quotes the empty-state title as "No documents yet." — the catalog value (`render.go:43`) has no period. Trivial. Also "the eleven non-English catalogs are machine-drafted" is spec-future stated as present (zero exist; `Locales = ["en"]`). ## GOOD (verified, not padded) - **The `head.html` mechanism is correct and verified end-to-end.** Parse order (shell → head → clones) does override the shell's `{{block "head"}}` — the rendered `&lt;head&gt;` carries the icon link on the live server; a page can still define its own `head` in its clone; `newRenderer`'s error handling is right; excluding it from `pageTemplates` is right (it defines no `content`). - **The `brand/` split is exactly right.** I read `.amadan/ci.d/10-gate:60–71`: the dirt check, `git checkout` and `git clean -fd` are all scoped to `internal/docs/static/`, so `brand/` cannot suffer the recorded failure; `build-editor.mjs` writes only into `static/`; `//go:embed brand` is correct; the new only-the-bundle assertion in `TestExactlyOneEditorBundleIsEmbedded` matches the gate's real mechanics. Serving the icon unauthenticated ahead of `RequireSession` is correct (the sign-in page needs it) and the day-cache-no-hash tradeoff is sound. `#2464e0` really is day's light `--rst-accent`. - **`[docs-back]` is a real fix done well**: the page genuinely had no in-app navigation; logical properties throughout; no glyph to mirror in RTL; the 24px floor stated outright with the token-drift lesson (`--rst-fs-sm` is 12.5px) recorded; no interference with the `[docs-editor]` mount or `[docs-save-status]` (editor-entry.js selects only those two attributes). The shared `docs.list.title` key is defensible — a breadcrumb naming its destination screen conventionally carries that screen's title — and the cross-page render-comparison test is genuinely clever: it holds the invariant in every locale and cleanly distinguishes "absent" from "present but different." - **`TestIconIsWellFormedXML`** decoding every token, with the double-hyphen-in-XML-comment lesson written down, is the house style at its best. - The sign-in measure and the create-form flex reasoning (`min-inline-size: 0`, wrap-breaks-before-shrinking) are correct CSS, correctly explained. ## Verdict **DO NOT MERGE** — one of the five claimed defects was never a defect: the whole-row-click fix duplicates rastrillo's own already-served stretched-link CSS while writing a false measurement into the permanent record (plus PRODUCT.md's false "content is encrypted at rest" claim); both fixes are cheap, and the other four fixes are sound and can merge once B1/B2 land.</result> <usage><subagent_tokens>133828</subagent_tokens><tool_uses>57</tool_uses><duration_ms>658266</duration_ms></usage> </task-notification>