Review — feat/table-formatting, origin/core..HEAD (HEAD = 393e276)

Verdict: REQUEST CHANGES.

Reviewer: Claude (Fable 5.1), independent §Review pass, 2026-09-04. Reviewed read-only against the six committed changes. While this review ran, eleven tracked files and two untracked ones (calc/fn_cond_test.go, tools/gencorpus/cases.go) changed on disk — somebody is already responding to the other review. Everything below is about the commits, not the working tree; where the on-disk edit already addresses a finding I say so, but nothing uncommitted counts as fixed.

Method: read the diff, AGENTS.md, PRODUCT.md, the design spec (§2, §4.2, §5, §6, §7, §14) and the slice spec; ran the Go suite; ran ten mutations against a scratch copy built from git archive HEAD (none in the checkout); ran LibreOffice 26.8.0.3 headless on hand-built .ods files to check the wildcard claim and the matcher's edges; probed excelize the same way; read the vendored jspreadsheet.js for its clipboard and keydown paths. No browser is reachable from this sandbox, so the one finding that needs a browser (F3) is stated as a code-level claim with the exact step that would confirm or refute it.

The binding rules quoted where they decide a finding:

• AGENTS.md §Shipping cadence: "CI is green — actually green, checked, not assumed."
• AGENTS.md Mechanical rules, Data: "No cell content, comment text, sheet title or tab name in any log line, error string, job status message or metric label."
• Design §4.2: "Cut-paste a cell — The cell keeps its (row_id, col_id) key and its content moves. Inbound references therefore follow the destination, matching Excel."
• Design §7: "Every SSE message carries the cell's version, and clients discard out-of-order paints by version rather than by arrival."
• Slice spec decision C: "Every new function's expected values come from LibreOffice headless, through tools/gencorpus, never from our reading of a spec."
• PRODUCT.md principle 1: "Say the true thing, including the unflattering half."

---

F1. The branch is red: TestFileLineCap fails at HEAD — BLOCKER

Claim: the guards package does not pass on this branch. True.

$ go test ./internal/guardstest/
--- FAIL: TestFileLineCap (0.00s)
    linecap_test.go:36: tools/gencorpus/main.go: 308 lines exceeds the 300-line cap

internal/guardstest/linecap_test.go:24-37 has been on origin/core since d49ed01; tools/gencorpus/main.go was 220 lines there and is 308 at HEAD (the feat(calc) commit added the case list and a 20-line comment). The author knows the cap applies to Go — the feat(table) commit split handlers_table.go for exactly this reason (tableview.go:11-16) — so this is a package that was not run before pushing. go test ./... in this sandbox lists eight packages and silently omits internal/guardstest because of a read-only build-cache path; if the author's environment does the same, that is the mechanism, and it is worth knowing.

Two more things in the same file: tools/gencorpus/main.go:190-200 says the blank-criterion case is "Pinned instead as a direct assertion in calc/fn_cond_test.go" — no such file exists at HEAD (it is untracked on disk), and the sentence above it is cut off mid-clause ("answered 5 for A1:C3 (text counted, blanks not) and 0 for B1:B3 that check is worth more than this one case)"). A comment that cites a test that is not committed is the guard-that-cannot-fail problem in prose form.

Do: split the case list out of main.go (the on-disk cases.go is the right shape), commit the test the comment names, fix the truncated sentence, and run go test ./internal/guardstest/ explicitly before the next push.

---

F2. Cut-and-paste translates references; the spec says content moves — HIGH

Claim: cutting =A1*2 from B1 and pasting it at B5 stores =A5*2. True, by construction.

clipboard.js:240-247 (Ctrl+X) and :269-277 (menu Cut) both call copyText(), which sets lastCopy = { text, r: b.r1, c: b.c1 } (:165-171). paste() at :187 treats any exact text match as a copy origin and at :204 sends fromR/fromC on every cell, so handlers_grid.go:130-134 → copypaste.go:41-68 offsets every relative reference by the move distance.

That is Excel's copy rule applied to a cut. Excel does not adjust references when a formula is moved, and design §4.2 rules the same way: "Cut-paste a cell — The cell keeps its (row_id, col_id) key and its content moves." Nothing in the diff distinguishes the two operations once the text is on the clipboard, so a person who cuts a formula block to tidy a sheet gets every relative reference silently shifted, and the numbers still look plausible.

(The second half of the §4.2 row — inbound references following the destination — is honestly deferred by §14's positional-key-space row and is not this branch's fault. The first half is.)

Do: record the operation in lastCopy (cut: true) and, on a paste whose origin was a cut, send no fromR/fromC. commands.js's fill-down path is unaffected. Add a handlers_copy_test.go case only if the server grows a distinction; today the fix is entirely client-side.

---

F3. For a mouse user the vendored grid still owns Ctrl+C / Ctrl+X / Ctrl+V — HIGH (needs one browser step to confirm)

Claim: clipboard.js intercepts the three clipboard events, but the library copies and cuts from its document-level keydown handler, which the capture-phase clipboard listeners never touch; whenever libraryBase.jspreadsheet.current is set — which a mouse click on a cell does — the library runs first and suppresses the native events.

What the vendored code says (static/vendor/jspreadsheet.js, keyDownControls, registered document.addEventListener("keydown", keyDownControls) in setEvents):

67==e.which ? (copy.call(libraryBase.jspreadsheet.current,!0), e.preventDefault())
: 88==e.which ? (... cutControls() ..., e.preventDefault())
: 86==e.which && pasteControls()

copy() ends with i.textarea.value=x, i.textarea.select(), document.execCommand("copy") — it moves focus into its hidden jss_textarea and copies the display strings from options.data, not raw input. The synthetic copy event that execCommand fires targets that textarea, and clipboard.js:221-224 inTextField() deliberately steps aside for a TEXTAREA. Then e.preventDefault() on the keydown cancels the browser's own copy command, so the native copy event our listener was written for never fires, and lastCopy is never set.

current is assigned in mouseDownControls when the click lands inside a worksheet (current = s[0].jssWorksheet) and set to null on a mousedown outside it. grid.js:239-245 already records that the library's document keydown is live and runs before ours ("Left on, Ctrl-Z reverts locally").

Why the drive script did not see it: tools/drive/clipboard.mjs never clicks a cell. Every selection goes through updateSelectionFromCoords(...), every menu click is a mousedown outside the grid (which nulls current), and SheetsGrid.focus() focuses the host div without a mousedown. So it measured the current == null state, where keyDownControls returns at its first line — a state a person with a mouse is never in.

What a mouse user gets, if this reading is right: Ctrl+C copies "20" where the cell holds =A1*2; Ctrl+V lands in the library's pasteControls via the focused hidden textarea and the cell-dropping paste path this module exists to remove is back, with no error anywhere.

Do: confirm with one drive step — page.click('td[data-x="0"]...') (a real mousedown on a cell) before Control+c, then read navigator.clipboard.readText() and check lastCopy fired (the translation check at H8 will tell you). If it fails, the fix is small and answers the brief's "registration order" question properly: add a capture-phase keydown listener in clipboard.js that, for Ctrl/Cmd + C/X/V outside a text field, calls e.stopImmediatePropagation() — not preventDefault() — so the library's bubble-phase document handler never sees the keystroke and the native clipboard events still reach our capture handlers. Capture beats bubble regardless of which script registered first, which is the robustness the current design only claims.

---

F4. The truncation note says something false exactly when it is shown — HIGH

Claim: "The table view lists every row." (note.js:44-46) is untrue whenever data-window-truncated is set. True.

handlers_sheets.go:173 sets Truncated when the sheet reaches past maxWindowRows (500) or maxWindowCols (60). The table view:

• handlers_table.go:111 — pages := (maxWindowRows + tableRowsPerPage - 1) / tableRowsPerPage → 20 pages of 25 → row 500 is the last row the table can ever show, and :176 rejects an edit at rowIdx >= maxWindowRows;
• handlers_table.go:20 — tableCols = 26 → nothing past column Z is in the table at all, on any page.

inCellRange (handlers_grid.go:65-67) accepts writes to row 4,999 and column 599. So a cell at row 501 or column AA is accepted by the API, carried by the export, invisible on the grid, and unreachable in the table view — and the one sentence the app now says about it points the person at the screen that cannot show it either. The slice spec repeats the claim ("the table view lists whatever rows exist", §2.1) and the commit message inherits it. This is the invisibility bug the commit fixed, one level up, with a false reassurance attached; PRODUCT.md principle 1 forbids the reassurance more than the gap.

Related and new: the window is now sized per page load, so two tabs on one sheet can hold different windows. A paste in tab A past tab B's data-rows is dropped by B's applyBatch (grid.js:101) with no note, until B reloads. That was latent under the constant window; the sizing makes it ordinary.

Do: either make the sentence true (the table view paginates to the sheet's own extent and shows every column the sheet has — it is scriptless HTML, the cost is a wider <table>), or change the sentence to what is true today ("Rows past 500 and columns past Z are in the .xlsx export only"). Fix the slice spec's §2.1 in the same commit. For the two-tab case, set resync on a batch that carries a cell outside the receiving window, or have note.js say so.

---

F5. SUMIF/AVERAGEIF/SUMIFS swallow an error in the sum range; SUM propagates it — HIGH

Claim: an error cell inside a selected row of the sum range is skipped and a number is returned. True, and it contradicts the function's own stated invariant.

fn_cond_apply.go:83-89 (total): if v.Kind != KindNum { return } — the comment says this is "exactly as SUM skips them … a conditional sum must not disagree with an unconditional one about what a number is." But SUM does not skip errors: fn_math.go:79-86 numericValues returns the first KindErr it meets. Measured, with C3 = =1/0:

| expression | ours | LibreOffice 26.8 | Excel | |---|---|---|---| | SUM(C1:C3) | #DIV/0! | #DIV/0! | #DIV/0! | | SUMIF(A1:A3,">5",C1:C3) | 0 | #DIV/0! | #DIV/0! | | SUMIF(A1:A3,">25",C1:C3) | 0 | #DIV/0! | #DIV/0! |

A zero where the sheet has a broken cell is the quiet wrong number PRODUCT.md forbids. The corpus cannot see it: the support table has no error cell. Mutation M6 (letting errors through matches) stayed green across calc and xlsx for the same reason.

Do: in total, return the error value when v.Kind == KindErr; add an error cell to gencorpus's support data (an of:=1/0 cell in column C is one line in ods.go) and generate the two cases above.

---

F6. The wildcard cases did not need to leave the corpus; the "profile" claim is wrong — MEDIUM

Claim: "LibreOffice's default profile does not read and ? as wildcards" (xlsx/oracle_wildcard_test.go:16-21, gencorpus/main.go:165-178), so the five cases had to move to excelize. The measurement is right; the diagnosis and the conclusion are wrong.*

Wildcard versus regex matching is a per-document calculation setting, not a profile setting, and gencorpus/ods.go:137-140 writes an .ods with no <table:calculation-settings> element at all, so LibreOffice falls back to ODF's legacy default (regular expressions on, wildcards off), where "t*" is the regex t* and matches nothing in "ten". I rebuilt the same document with one added element and ran the same binary:

<table:calculation-settings table:use-wildcards="true" table:use-regular-expressions="false"/>

| case | as gencorpus writes it | with the element | excelize / ours | |---|---|---|---| | COUNTIF(B1:B3;"t*") | 0 | 3 | 3 | | COUNTIF(B1:B3;"t??") | 0 | 1 | 1 | | SUMIF(B1:B3;"t*";A1:A3) | 0 | 60 | 60 | | COUNTIF(B1:B3;"t?n") | 0 | 1 | 1 | | COUNTIF(B1:B3;"*y") | 0 | 2 | 2 | | SUMIFS(A1:A3;B1:B3;"t*") | 0 | 60 | 60 | | COUNTIFS(A1:A3;">=20";B1:B3;"t*") | 0 | 2 | 2 |

So decision C ("expected values come from LibreOffice headless … never from our reading") was set aside for these five when a one-line change to the generator keeps it. That matters because the substitute oracle is weak: in this session excelize returned 0 for SUM(C1:C3) over an error cell and panicked (calc.go:8230, index out of range) on COUNTIFS(A1:A3,">5",B1:C1,"<>"). TestWildcardCriteriaAgainstExcelize itself is sound — mutation M9 (? widened to *) fails it — but it is sound against a second-choice oracle that was never needed.

Do: add the element to contentXML, move the seven wildcard cases back into cases, regenerate, update corpusSHA256, and reduce oracle_wildcard_test.go to a cross-check or delete it. Note in the divergences file that this is a document setting so the next person does not re-measure the "profile".

---

F7. The criteria matcher diverges from Excel on the edges the corpus never reaches — MEDIUM

All measured against LibreOffice 26.8 on a hand-built document (C1 blank, C2 = ="", C3 = =1/0, D1 = text "10", D2 = number 10, D3 blank). Where I am confident Excel agrees with LibreOffice I say so; where I am not, the row says only what was measured.

| expression | ours | LibreOffice | note | |---|---|---|---| | COUNTIF(C1:C3,"") | 1 | 2 | Excel counts both a blank and a cell holding ""; fn_cond.go:151-153 returns v.Kind == KindEmpty only, and its comment asserts the opposite of Excel's behaviour. | | COUNTIF(C1:C3,"<>") | 1 | 2 | The error cell is "not blank". fn_cond.go:122-125 refuses every error for every criterion, by design; Excel and LibreOffice count errors for <>. | | COUNTIF(A1:C3,"<>20") | 7 | 8 | Same cause — the error cell. | | COUNTIF(D1:D2,"10") | 1 | 2 | finishCriterion (:93-98) turns the text criterion "10" into a numeric one, and :126-131 then refuses the text cell. Excel's COUNTIF matches numeric-looking text against a text criterion; COUNTIF(D1:D2,10) = 1 in both, which is the asymmetry that has to be modelled. | | COUNTIF(D1:D2,"<>10") | 1 | 0 | Mirror of the row above. | | COUNTIF(C1:C3,D3) (blank criterion cell) | 1 | 0 | parseCriterion (:77-83) turns a KindEmpty criterion into text: "" and matches blanks; LibreOffice and excelize both say 0. | | SUMIFS(A1:A2,B1:B3,"t*") | 60 | Err:502 | Excel: #VALUE! — SUMIFS requires the sum range to be the same shape as each criteria range; plural (fn_cond_apply.go:201-214) never checks and total reshapes it to the criteria range's size, reading A3, a cell the formula never named. |

Two more from reading: readPairs (fn_cond_apply.go:65-73) compares cells() and not shape, so a 1×6 criteria range against a 6×1 one passes and is silently reshaped — the exact "mistyped range reads the wrong cells and still returns a number" its own comment says it refuses. And mutation M5 (blank/text never matching a numeric <>) stayed green across calc and xlsx: at HEAD no test distinguishes any row in this table.

Do: treat ""-valued cells as blank for "", count error cells for <>, match text criteria against text cells before numeric coercion, make an empty criterion match nothing, check shape not count, and refuse a SUMIFS/AVERAGEIFS sum range of a different shape. Add the support cells (an ="" cell, an error cell, a text "10") to gencorpus and generate every row above; they are the semantic fault lines the earlier memory note said the corpus lacked.

---

F8. writeAll's trailing refetch can paint stale over a concurrent edit; the root cause is batch-level versioning — MEDIUM

Claim: there is an interleaving where a cell is painted stale. True.

clipboard.js:154-156 calls g.refetch() once every lane is idle. grid.js:113-128 refetch() does version = 0; meta = …; applyBatch(payload) in its .then. Sequence: the snapshot request leaves at tab version V; another tab's edit lands at V+1 and its SSE batch paints here (version = V+1); the snapshot (taken at V) arrives, version is zeroed, and applyBatch paints the V-state value over the V+1 cell. No later message repairs it because the edit already happened.

The reason the refetch was added — six concurrent responses, the last to arrive being the oldest, five of six painted — is the same defect from the other side: applyBatch (grid.js:80-83) discards a whole batch by the tab's version, while design §7 says "Every SSE message carries the cell's version, and clients discard out-of-order paints by version". A response to your own write is authoritative for the cells it names; it should never be discarded because a different cell moved the tab's version on. The on-disk edit goes serial (LANES = 1, no refetch), which is correct and slow — 500 cells at one round trip each. The real fix is per-cell versions on the wire, which also removes the applyBatch special-casing for format writes.

Also at HEAD: a keyboard cut's clearBox writes and an immediately following paste's writes run in two independent pumps; if the regions overlap, a late clear can land after the paste and blank a pasted cell.

Do: short term, keep serial writes and say so in MAX_CELLS's comment; medium term, carry a per-cell version and let applyBatch compare per cell. Chain clearBox and the next paste, or make cut clear only after the paste completes.

---

F9. own() prevents the default before it knows there is anything to copy, and a paste past the window is silently dropped — MEDIUM

• clipboard.js:226-232: preventDefault() and stopImmediatePropagation() run before copyText() learns that box() is null. With no grid selection yet (fresh page, or after the library nulled it) Ctrl+C anywhere outside a text field — text highlighted in the help dialog, the contrast notice, a menu label — copies nothing and the browser's own copy of the highlighted text is suppressed too. Same shape at :240-242: on a read-only grid Ctrl+X swallows the event and does not even copy.
• clipboard.js:198: if (r >= g.rows || c >= g.cols) continue; — a paste whose block runs past the window drops those cells without a word. The server would accept them (inCellRange to row 4,999) and the next page load would widen the window to show them. The module's header says "a paste that silently drops a cell is the failure this whole change exists to remove."
• :188: count uses rows[0].length; a ragged paste under-counts against MAX_CELLS. Minor.

Do: check box() (and, for cut/paste, mayWrite) before calling preventDefault(), and let the event fall through otherwise; send the off-window cells (the server is the bound) and say() that the grid will show them after a reload, or say() that N cells were not pasted.

---

F10. Copy provenance is per tab: a copy in one tab pastes untranslated in another — MEDIUM

lastCopy (clipboard.js:69) lives in the page. Copy =A1*2 in tab A, paste it three rows down in tab B of the same sheet (or a different sheet on this instance), and B has no origin, so =A1*2 is stored verbatim — where Excel, and this app one tab over, would store =A4*2. It is the comment's "pasted verbatim, which is right" applied to text that does have references to translate, and the numbers look fine. The clipboard can carry provenance: setData with a second flavour (text/html with a data-oficina-origin attribute, or a custom type — Chromium round-trips custom types within a profile) and getData on paste; fall back to the exact-text match when the flavour is absent.

---

F11. Guards that stay green with the feature removed — LOW

Ran against a scratch copy of HEAD; the check is whether each guard goes red when the thing it guards is broken.

| mutation | result | |---|---| | view.Truncated = false | red (TestTheWindowStopsAtTheMaximum) | | windowFor: drop + windowGrowRoom | green — the grow room (handlers_sheets.go:184-206) has no test; a window that ends exactly at the last cell passes every window test | | table.html: remove data-b | red (TestTheTableViewPaintsWhatItSets) | | TranslateChecked returns true | red (both off-grid tests) | | formatFromForm: drop the keep branch | red (TestKeepLeavesAFieldAlone) | | globMatch: ? behaves as * | red (TestWildcardCriteriaAgainstExcelize) | | sheets.css: delete the menubar @supports block | red (anchor guard) | | sheets.css: keep inset: auto, delete position-area | green — anchorBranchSelectors (dropdownanchor_test.go:76-90) requires only inset: auto; the one declaration that makes a menu open rightward is unguarded. The on-disk edit to this test adds exactly that check; it is not in HEAD. | | matches: blank/text never satisfy numeric <> | green (F7) | | matches: errors allowed through | green (F5, F7) |

TestTheAnchorGuardReadsTheStylesheetItThinksItDoes refers to a stripBlocks that does not exist (:44-46); the parser works because the regex backtracks past the outer brace, which is luck rather than design.

---

F12. Translation is bounded below but not above — LOW

onGrid (translate.go:73-92) checks >= 0 only. The lexer's refPattern (token.go:46) is ^\$?[A-Za-z]{1,3}\$?[0-9]+$, so column ZZZ (18,277) is the last one that parses. A formula holding a 3-letter column reference offset by up to maxWindowCols*10 renders a 4-letter label that lexWord (token.go:163-175) reads as an identifier; the store's contentNode then refuses the write and the pasted cell is skipped — the outcome TestAnUntranslatableFormulaIsStillWritten exists to forbid, reached by the other edge. Excel gives #REF! past its last column, which is what ok=false already produces. Also handlers_grid.go:121 checks maxRawBytes before :134 translates, so the stored raw can exceed the cap the route claims.

Do: ColPos > 18277 (or a named maxColPos) is off-grid in onGrid; run the length check after translation.

---

F13. #REF! is written as text — LOW, documented, still a divergence

copypaste.go:54-67 stores the literal #REF!; handle_codec.go:41-50 makes it a Str. Consequences the comment does not list: =ISERROR(B2) is FALSE, =B2+1 is #VALUE! rather than #REF!, and the .xlsx export writes a string cell that Excel shows left-aligned as text. Visible, as the comment says, but a wrong error code is a different kind of visible. An error literal in the grammar is the fix; if it stays deferred, put a row in §14 with that trigger.

---

F14. Form decoder details — LOW

• handlers_format.go:58-63 reads the form with no MaxBytesReader; the JSON path (:64) caps at 8 KiB. r.FormValue parses up to 10 MiB urlencoded or 32 MiB multipart into memory first.
• A scriptless submit with every control on "keep" is an empty patch → :70-72 → a bare 400 That formatting could not be applied. text page, not a redirect back. TestAFormPostStillObeysEveryBound asserts this as correct; for the form it is the ordinary "pressed Apply by accident" case.
• tableReturn (:130-137) drops edit= and the view's sort/filter, so after Apply the cell's form closes and a filtered page reverts. Both are derivable from validated inputs (r1,c1 and the stored view).

---

F15. Date serials below 61 — informational

serialTime (fn_date_parts.go:38-40) counts from 1899-12-30 with no 1900 leap-day. DAY(1) is 31, MONTH(1) 12, YEAR(1) 1899, DATE(1900,1,1) is 2; Excel says 1, 1, 1900, 1. LibreOffice agrees with us, so the corpus cannot arbitrate this by construction. Nothing to change — every serial from 61 up agrees — but it belongs in oracle_divergences_test.go as a recorded, oracle-blind divergence rather than an unknown.

---

Checked and clean

• Sealing. No new log line. Every added http.Error string is fixed text (handlers_grid.go:131, handlers_format.go:61,71,76,80). The 303 target is "/sheets/" + id + "/table?page=" + n from a parsed, bounded integer (:130-137; TestTheFormRedirectIsBuiltByTheServer covers the hostile inputs). note.js's only message interpolates data-rows and data-cols, which are numbers. tablepaint.go:71-105 builds template.CSS only from strconv-formatted floats and safeHex, whose regex (previewsvg.go:37) is ^[0-9A-Fa-f]{6}$|^[0-9A-Fa-f]{8}$, so the attribute can only ever hold --name:#hhhhhh; and numbers; fills and number-format codes are §2.2 plaintext by design. Calc's new error paths return codes.
• CSRF on the form path. app.go:135-145 applies rastrillo's origin-checking csrf.Protect to every state-changing method, so accepting application/x-www-form-urlencoded on the API route does not open a cross-site form post.
• Content-type dispatch. isFormPost keys on the body's own encoding; a JSON body under a form content-type fails Atoi → 400, and the reverse fails the JSON decoder → 400. maxFormatCells and normalize run after both decoders converge (:70-82); TestAHugeRectangleFromAFormIsRefused and TestJSONStillAnswersJSON hold.
• origin() / inCellRange. Origin is bounded to the same box as the destination, so the offset is at most ±4,999 rows / ±599 columns and the parse cost is the input's, not the offset's (F12 is the residual).
• House rules. No migration, no scope.Owned, no GORM binding, no sessions.UserID. TestOurStylesheetHasNoColourLiterals is green; every new sheets.css rule uses tokens. clipboard.js is 292 lines against a 300 cap — no headroom for F3's listener without a split. Paste has a menu item and a shortcut; fill has both.
• Copy tests are at the HTTP boundary and each fails under the matching mutation.

---

Summary of what has to change before merge

1. Make internal/guardstest green (F1) and commit the test the comment cites.
2. Stop translating on cut (F2).
3. Prove Ctrl+C/X/V with a real mousedown on a cell, and take the keystroke from the library at capture phase if they fail (F3).
4. Make the truncation sentence true, and fix the slice spec's claim (F4).
5. Propagate errors from the sum range (F5).
6. Put the wildcard cases back in the LibreOffice corpus with the one-line .ods change (F6), and generate the matcher edges with the extra support cells (F7).

F8-F15 are worth doing and can follow; F8's serial-write stopgap is already on disk.