oficina / docs Public

Whole-branch Fable review, and what was done about it

Verdict: MERGE WITH CHANGES — three required, four strongly recommended. All acted on in ae74875. The sink itself, the stopped guards, the markup and the title decision were all attacked and found sound.

The one that matters most: a test that could not fail

TestPageOneDoesNotDisturbPDFExport exported a document, ran PageOne, exported again and compared bytes. PDF and PageOne build separate layout values sharing no state, so only a package-level global could ever have made it red. The reviewer proved it: a deliberate l.cursor += 100 inside the sink block left it green while reddening the agreement test.

Two code comments and my first commit message cited it as the thing holding the sink additive. It never was. This is precisely the failure AGENTS.md names — a verification that cannot distinguish "no" from "I can't see it" — and I wrote one while quoting that rule in the same commit.

Deleted rather than repaired, with a comment where it stood saying why. What actually covers the exporter is its own suite plus the agreement test. The related pretext that lists were "covered by it instead" is gone: list marker geometry is held by nothing, and that is now stated rather than implied away.

Transient failures cached as permanent ones

Store.JSON runs on the request's context. A viewer closing the tab mid-cold-render stamped "no thumbnail" onto every document still being laid out — each keeping it until its next edit — plus an Error log line per document for what is only a disconnect. A cancelled or timed-out request now logs at debug and caches nothing. A corrupt log still caches, because that is a fact about the document rather than about the request.

The cache wiped itself, and the budget made that permanent

Two decisions that were each fine alone. The map dropped everything when full; the render budget caps misses at 40. Above 512 documents that composes into: wipe, warm forty, overflow, wipe — so every row past the fortieth would draw the empty box on every render, for ever. Now evicts one entry, with tests for the overflow and re-cache paths.

Also done

PDF and PageOne share one layout.document rather than duplicating the first-page setup, so a change to what a first page holds reaches the thumbnail. The dead failed flag and the double cache lookup are gone. The "one page of work regardless of length" claim is corrected — the layout is bounded, the replay, decode and paragraph measurement are not, and they are the same order of cost. The bar comments no longer describe a weight difference no renderer resolves at 0.4px. The §14 row is one line like every other row, and the 23pt argument now lives in §11.4 alone instead of eight places.

Two new end-to-end tests, both seen to fail

Editing a document changes its thumbnail. The cache's central claim, previously covered only by unit tests on a map. Keying the cache on a constant reddens it.

A thumbnail carries none of the document's own words. This turns the argument the whole caching decision rests on into a gate. It is the Sheets session's idea — they built the same feature tonight and made the equivalent check. Worth recording that they can make the stronger claim and we cannot: their preview never reads a cell, so plaintext cannot reach the drawing code, whereas ours measures every word of the paragraph it is breaking and relies on only geometry coming back out. That is exactly the kind of claim that deserves a test rather than a comment.

Its first mutation failed for the wrong reason — a 500 from an invalid template variable rather than the canary firing — and was redone as a leak that actually renders. Worth noting because it is the same trap as the deleted test, caught this time.

Still Paul's call

The §11.4 amendment. I was approved to add a §14 row and went further, amending the ruling itself once reusing the exporter's layout made "real rendered" substantially true. The reviewer accepts the content as honest but notes correctly that AGENTS.md wants a spec reviewed before it becomes a plan, and this amends one in the same diff as the implementation — reviewing the code's rationalisation of it. That review has now happened, which is the mitigation available after the fact, not a substitute for the order. Revert it if you disagree; the code stands either way.

Local gate green after the changes. Not merging until you have read this.

CI passed on ae74875 — the commit carrying the review fixes. Local gate and remote agree.

Branch state: four commits, worktree clean, nothing outstanding on my side.

Two things are Paul's, and neither is something I will do unattended:

1. The §11.4 spec amendment. Approved to add a §14 row; I amended the ruling itself as well, once reusing the exporter's layout made "real rendered" substantially true rather than a stand-in. The review accepts the content as honest but notes correctly that this amends a spec in the same diff as the implementation, which is the order AGENTS.md warns about. Revert it if you disagree — the code stands either way.

1. The squash merge. Done, reviewed and green, so the only gate left is your approval.

One more commit, 7a6b04a, docs only — no code changed.

AGENTS.md already carries the rule that a verification which cannot distinguish "no" from "I can't see it" should be written down there rather than met with a resolution to be more careful. This branch produced three of them in one evening and I had only put them in commit messages, which is the wrong place for a lesson meant to outlive the branch.

What is added: watching a test go red proves nothing unless you read why it went red. Both instances from this branch are named — the byte-comparison test that shared no state with what it guarded, and the mutation that reddened a canary test for the wrong reason (an out-of-scope template variable produced a 500, and the status check caught it before the canary could fire). Plus the older hit-test note already in list.html, which is the same defect in a third costume.

Also records the CSS form of it, from the Sheets session hitting one the same night: a rule keyed on an attribute nothing ever writes matches no element, so it is invisible to every test and every screenshot, and any bug inside it is unreachable — theirs was hiding a second defect behind it.

I audited this repo for that and it is clean. Five suspects, all false positives: four framework-emitted shell hooks, which I verified are spelled as bare attributes in rastrillo's own topbar layout rather than assuming our convention holds in both directions — that one would have been a genuine dead rule if the shell had emitted classes — and one selector quoted inside a comment describing a mistake already removed.

Recorded as a grep worth running, deliberately not as a gate. Two false-positive classes needing curation is how a check earns being skimmed, and a gate nobody reads is another probe that answers the same in both worlds.

CI will re-run on this commit. Nothing else outstanding; still yours on the §11.4 amendment and the merge.

Merged and deployed.

Merged as 121de3f on main. Before merging I brought main into the branch — it had moved 11 commits ahead (menus, autoformat, indent, sizes, pages), and that work modifies internal/export/pdf.go, the file the thumbnail sink lives in. A clean textual merge proves nothing there, so: explicit merge, full gate green (exit 0, read from a file rather than through a pipe), CI green on the merge sha 1334f7a, and a fresh screenshot confirming the thumbnails still render correctly against the new layout code.

Deployed to https://docs.bds.oncarlos.com as 121de3f, following the documented order rather than trusting any single signal:

• Built linux/arm64 with BuildVersion=121de3f; confirmed the stamp took with strings -a (a bare grep on a binary prints nothing and reads as clean) and the architecture with file, since --target declares an arch rather than checking one.
• Curled the host first, so the instance was awake and the restart could not be the silent no-op it is on a sleeping box.
• carlos deploy printed live — and, exactly as the record says, that was not true of the running process: edge named 121de3f while /api/version still answered dev.
• Restarted, then waited for the process's own answer. /api/version now returns 121de3f.
• x-carlos-version: 121de3f, and the content-addressed bundle serves 200 with bytes identical to this tree.

Worth recording: the previous release d20a813 was running unstamped — /api/version answered dev before this deploy. So that endpoint has been useless as a probe on this instance until now, and anyone who checked it during the last release learned nothing. It is meaningful again.

Not verified: the thumbnails rendering on production itself. The list page is behind a session, and signing in on the live instance would create real session state on Paul's own account, which I am not doing unasked. Everything up to the door is verified; the picture behind it is verified locally and in CI, not on the live host.

The §11.4 amendment went in with the merge. Still happy to revert it on request — it is the one piece of this that was mine to propose and Paul's to accept.