| 1 | # Review — feat/table-formatting, `origin/core..HEAD` (HEAD = `393e276`) |
| 2 | |
| 3 | **Verdict: REQUEST CHANGES.** |
| 4 | |
| 5 | Reviewer: Claude (Fable 5.1), independent §Review pass, 2026-09-04. Reviewed |
| 6 | read-only against the six committed changes. While this review ran, eleven |
| 7 | tracked files and two untracked ones (`calc/fn_cond_test.go`, |
| 8 | `tools/gencorpus/cases.go`) changed on disk — somebody is already responding |
| 9 | to the other review. Everything below is about the commits, not the working |
| 10 | tree; where the on-disk edit already addresses a finding I say so, but |
| 11 | nothing uncommitted counts as fixed. |
| 12 | |
| 13 | Method: read the diff, `AGENTS.md`, `PRODUCT.md`, the design spec (§2, §4.2, |
| 14 | §5, §6, §7, §14) and the slice spec; ran the Go suite; ran ten mutations |
| 15 | against a scratch copy built from `git archive HEAD` (none in the checkout); |
| 16 | ran LibreOffice 26.8.0.3 headless on hand-built `.ods` files to check the |
| 17 | wildcard claim and the matcher's edges; probed excelize the same way; read |
| 18 | the vendored `jspreadsheet.js` for its clipboard and keydown paths. No |
| 19 | browser is reachable from this sandbox, so the one finding that needs a |
| 20 | browser (F3) is stated as a code-level claim with the exact step that |
| 21 | would confirm or refute it. |
| 22 | |
| 23 | The binding rules quoted where they decide a finding: |
| 24 | |
| 25 | - `AGENTS.md` §Shipping cadence: "**CI is green** — actually green, checked, |
| 26 | not assumed." |
| 27 | - `AGENTS.md` Mechanical rules, Data: "**No cell content, comment text, |
| 28 | sheet title or tab name in any log line, error string, job status message |
| 29 | or metric label.**" |
| 30 | - Design §4.2: "**Cut-paste a cell** — The cell keeps its `(row_id, |
| 31 | col_id)` key and its *content moves*. Inbound references therefore follow |
| 32 | the destination, matching Excel." |
| 33 | - Design §7: "**Every SSE message carries the cell's version**, and clients |
| 34 | discard out-of-order paints by version rather than by arrival." |
| 35 | - Slice spec decision C: "**Every new function's expected values come from |
| 36 | LibreOffice headless**, through `tools/gencorpus`, never from our reading |
| 37 | of a spec." |
| 38 | - `PRODUCT.md` principle 1: "**Say the true thing, including the |
| 39 | unflattering half.**" |
| 40 | |
| 41 | --- |
| 42 | |
| 43 | ## F1. The branch is red: `TestFileLineCap` fails at HEAD — BLOCKER |
| 44 | |
| 45 | **Claim:** the guards package does not pass on this branch. |
| 46 | **True.** |
| 47 | |
| 48 | ``` |
| 49 | $ go test ./internal/guardstest/ |
| 50 | --- FAIL: TestFileLineCap (0.00s) |
| 51 | linecap_test.go:36: tools/gencorpus/main.go: 308 lines exceeds the 300-line cap |
| 52 | ``` |
| 53 | |
| 54 | `internal/guardstest/linecap_test.go:24-37` has been on `origin/core` since |
| 55 | `d49ed01`; `tools/gencorpus/main.go` was 220 lines there and is 308 at HEAD |
| 56 | (the `feat(calc)` commit added the case list and a 20-line comment). The |
| 57 | author knows the cap applies to Go — the `feat(table)` commit split |
| 58 | `handlers_table.go` for exactly this reason (`tableview.go:11-16`) — so this |
| 59 | is a package that was not run before pushing. `go test ./...` in this |
| 60 | sandbox lists eight packages and silently omits `internal/guardstest` |
| 61 | because of a read-only build-cache path; if the author's environment does |
| 62 | the same, that is the mechanism, and it is worth knowing. |
| 63 | |
| 64 | Two more things in the same file: `tools/gencorpus/main.go:190-200` says the |
| 65 | blank-criterion case is "Pinned instead as a direct assertion in |
| 66 | `calc/fn_cond_test.go`" — **no such file exists at HEAD** (it is untracked |
| 67 | on disk), and the sentence above it is cut off mid-clause ("answered 5 for |
| 68 | A1:C3 (text counted, blanks not) and 0 for B1:B3 that check is worth more |
| 69 | than this one case)"). A comment that cites a test that is not committed is |
| 70 | the guard-that-cannot-fail problem in prose form. |
| 71 | |
| 72 | **Do:** split the case list out of `main.go` (the on-disk `cases.go` is |
| 73 | the right shape), commit the test the comment names, fix the truncated |
| 74 | sentence, and run `go test ./internal/guardstest/` explicitly before the |
| 75 | next push. |
| 76 | |
| 77 | --- |
| 78 | |
| 79 | ## F2. Cut-and-paste translates references; the spec says content moves — HIGH |
| 80 | |
| 81 | **Claim:** cutting `=A1*2` from B1 and pasting it at B5 stores `=A5*2`. |
| 82 | **True, by construction.** |
| 83 | |
| 84 | `clipboard.js:240-247` (Ctrl+X) and `:269-277` (menu Cut) both call |
| 85 | `copyText()`, which sets `lastCopy = { text, r: b.r1, c: b.c1 }` |
| 86 | (`:165-171`). `paste()` at `:187` treats any exact text match as a copy |
| 87 | origin and at `:204` sends `fromR/fromC` on every cell, so |
| 88 | `handlers_grid.go:130-134` → `copypaste.go:41-68` offsets every relative |
| 89 | reference by the move distance. |
| 90 | |
| 91 | That is Excel's *copy* rule applied to a *cut*. Excel does not adjust |
| 92 | references when a formula is moved, and design §4.2 rules the same way: |
| 93 | "Cut-paste a cell — The cell keeps its `(row_id, col_id)` key and its |
| 94 | *content moves*." Nothing in the diff distinguishes the two operations once |
| 95 | the text is on the clipboard, so a person who cuts a formula block to tidy |
| 96 | a sheet gets every relative reference silently shifted, and the numbers |
| 97 | still look plausible. |
| 98 | |
| 99 | (The second half of the §4.2 row — inbound references following the |
| 100 | destination — is honestly deferred by §14's positional-key-space row and is |
| 101 | not this branch's fault. The first half is.) |
| 102 | |
| 103 | **Do:** record the operation in `lastCopy` (`cut: true`) and, on a paste |
| 104 | whose origin was a cut, send no `fromR/fromC`. `commands.js`'s fill-down |
| 105 | path is unaffected. Add a `handlers_copy_test.go` case only if the server |
| 106 | grows a distinction; today the fix is entirely client-side. |
| 107 | |
| 108 | --- |
| 109 | |
| 110 | ## F3. For a mouse user the vendored grid still owns Ctrl+C / Ctrl+X / Ctrl+V — HIGH (needs one browser step to confirm) |
| 111 | |
| 112 | **Claim:** `clipboard.js` intercepts the three clipboard *events*, but the |
| 113 | library copies and cuts from its document-level *keydown* handler, which |
| 114 | the capture-phase clipboard listeners never touch; whenever |
| 115 | `libraryBase.jspreadsheet.current` is set — which a mouse click on a cell |
| 116 | does — the library runs first and suppresses the native events. |
| 117 | |
| 118 | **What the vendored code says** (`static/vendor/jspreadsheet.js`, |
| 119 | `keyDownControls`, registered `document.addEventListener("keydown", |
| 120 | keyDownControls)` in `setEvents`): |
| 121 | |
| 122 | ``` |
| 123 | 67==e.which ? (copy.call(libraryBase.jspreadsheet.current,!0), e.preventDefault()) |
| 124 | : 88==e.which ? (... cutControls() ..., e.preventDefault()) |
| 125 | : 86==e.which && pasteControls() |
| 126 | ``` |
| 127 | |
| 128 | `copy()` ends with `i.textarea.value=x, i.textarea.select(), |
| 129 | document.execCommand("copy")` — it moves focus into its hidden |
| 130 | `jss_textarea` and copies the **display strings** from `options.data`, not |
| 131 | raw input. The synthetic `copy` event that `execCommand` fires targets that |
| 132 | textarea, and `clipboard.js:221-224` `inTextField()` deliberately steps |
| 133 | aside for a `TEXTAREA`. Then `e.preventDefault()` on the keydown cancels |
| 134 | the browser's own copy command, so the native `copy` event our listener |
| 135 | was written for never fires, and `lastCopy` is never set. |
| 136 | |
| 137 | `current` is assigned in `mouseDownControls` when the click lands inside a |
| 138 | worksheet (`current = s[0].jssWorksheet`) and set to `null` on a mousedown |
| 139 | outside it. `grid.js:239-245` already records that the library's document |
| 140 | keydown is live and runs before ours ("Left on, Ctrl-Z reverts locally"). |
| 141 | |
| 142 | **Why the drive script did not see it:** `tools/drive/clipboard.mjs` never |
| 143 | clicks a cell. Every selection goes through |
| 144 | `updateSelectionFromCoords(...)`, every menu click is a mousedown *outside* |
| 145 | the grid (which nulls `current`), and `SheetsGrid.focus()` focuses the |
| 146 | host `div` without a mousedown. So it measured the `current == null` state, |
| 147 | where `keyDownControls` returns at its first line — a state a person with |
| 148 | a mouse is never in. |
| 149 | |
| 150 | **What a mouse user gets, if this reading is right:** Ctrl+C copies "20" |
| 151 | where the cell holds `=A1*2`; Ctrl+V lands in the library's `pasteControls` |
| 152 | via the focused hidden textarea and the cell-dropping paste path this |
| 153 | module exists to remove is back, with no error anywhere. |
| 154 | |
| 155 | **Do:** confirm with one drive step — `page.click('td[data-x="0"]...')` |
| 156 | (a real mousedown on a cell) before `Control+c`, then read |
| 157 | `navigator.clipboard.readText()` and check `lastCopy` fired (the |
| 158 | translation check at H8 will tell you). If it fails, the fix is small and |
| 159 | answers the brief's "registration order" question properly: add a |
| 160 | capture-phase `keydown` listener in `clipboard.js` that, for Ctrl/Cmd + |
| 161 | C/X/V outside a text field, calls `e.stopImmediatePropagation()` — **not** |
| 162 | `preventDefault()` — so the library's bubble-phase document handler never |
| 163 | sees the keystroke and the native clipboard events still reach our capture |
| 164 | handlers. Capture beats bubble regardless of which script registered |
| 165 | first, which is the robustness the current design only claims. |
| 166 | |
| 167 | --- |
| 168 | |
| 169 | ## F4. The truncation note says something false exactly when it is shown — HIGH |
| 170 | |
| 171 | **Claim:** "The table view lists every row." (`note.js:44-46`) is untrue |
| 172 | whenever `data-window-truncated` is set. |
| 173 | **True.** |
| 174 | |
| 175 | `handlers_sheets.go:173` sets `Truncated` when the sheet reaches past |
| 176 | `maxWindowRows` (500) or `maxWindowCols` (60). The table view: |
| 177 | |
| 178 | - `handlers_table.go:111` — `pages := (maxWindowRows + tableRowsPerPage - 1) |
| 179 | / tableRowsPerPage` → 20 pages of 25 → **row 500 is the last row the |
| 180 | table can ever show**, and `:176` rejects an edit at `rowIdx >= |
| 181 | maxWindowRows`; |
| 182 | - `handlers_table.go:20` — `tableCols = 26` → **nothing past column Z is |
| 183 | in the table at all**, on any page. |
| 184 | |
| 185 | `inCellRange` (`handlers_grid.go:65-67`) accepts writes to row 4,999 and |
| 186 | column 599. So a cell at row 501 or column AA is accepted by the API, |
| 187 | carried by the export, invisible on the grid, and unreachable in the table |
| 188 | view — and the one sentence the app now says about it points the person at |
| 189 | the screen that cannot show it either. The slice spec repeats the claim |
| 190 | ("the table view lists whatever rows exist", §2.1) and the commit message |
| 191 | inherits it. This is the invisibility bug the commit fixed, one level up, |
| 192 | with a false reassurance attached; `PRODUCT.md` principle 1 forbids the |
| 193 | reassurance more than the gap. |
| 194 | |
| 195 | Related and new: the window is now *sized per page load*, so two tabs on |
| 196 | one sheet can hold different windows. A paste in tab A past tab B's |
| 197 | `data-rows` is dropped by B's `applyBatch` (`grid.js:101`) with no note, |
| 198 | until B reloads. That was latent under the constant window; the sizing |
| 199 | makes it ordinary. |
| 200 | |
| 201 | **Do:** either make the sentence true (the table view paginates to the |
| 202 | sheet's own extent and shows every column the sheet has — it is |
| 203 | scriptless HTML, the cost is a wider `<table>`), or change the sentence |
| 204 | to what is true today ("Rows past 500 and columns past Z are in the |
| 205 | .xlsx export only"). Fix the slice spec's §2.1 in the same commit. For |
| 206 | the two-tab case, set `resync` on a batch that carries a cell outside |
| 207 | the receiving window, or have `note.js` say so. |
| 208 | |
| 209 | --- |
| 210 | |
| 211 | ## F5. `SUMIF`/`AVERAGEIF`/`SUMIFS` swallow an error in the sum range; `SUM` propagates it — HIGH |
| 212 | |
| 213 | **Claim:** an error cell inside a selected row of the sum range is skipped |
| 214 | and a number is returned. |
| 215 | **True, and it contradicts the function's own stated invariant.** |
| 216 | |
| 217 | `fn_cond_apply.go:83-89` (`total`): `if v.Kind != KindNum { return }` — |
| 218 | the comment says this is "exactly as SUM skips them … a conditional sum |
| 219 | must not disagree with an unconditional one about what a number is." But |
| 220 | `SUM` does not skip errors: `fn_math.go:79-86` `numericValues` returns the |
| 221 | first `KindErr` it meets. Measured, with C3 = `=1/0`: |
| 222 | |
| 223 | | expression | ours | LibreOffice 26.8 | Excel | |
| 224 | |---|---|---|---| |
| 225 | | `SUM(C1:C3)` | `#DIV/0!` | `#DIV/0!` | `#DIV/0!` | |
| 226 | | `SUMIF(A1:A3,">5",C1:C3)` | **0** | `#DIV/0!` | `#DIV/0!` | |
| 227 | | `SUMIF(A1:A3,">25",C1:C3)` | **0** | `#DIV/0!` | `#DIV/0!` | |
| 228 | |
| 229 | A zero where the sheet has a broken cell is the quiet wrong number |
| 230 | `PRODUCT.md` forbids. The corpus cannot see it: the support table has no |
| 231 | error cell. Mutation M6 (letting errors through `matches`) stayed green |
| 232 | across `calc` and `xlsx` for the same reason. |
| 233 | |
| 234 | **Do:** in `total`, return the error value when `v.Kind == KindErr`; add an |
| 235 | error cell to gencorpus's support data (an `of:=1/0` cell in column C is |
| 236 | one line in `ods.go`) and generate the two cases above. |
| 237 | |
| 238 | --- |
| 239 | |
| 240 | ## F6. The wildcard cases did not need to leave the corpus; the "profile" claim is wrong — MEDIUM |
| 241 | |
| 242 | **Claim:** "LibreOffice's default profile does not read * and ? as |
| 243 | wildcards" (`xlsx/oracle_wildcard_test.go:16-21`, `gencorpus/main.go:165-178`), |
| 244 | so the five cases had to move to excelize. |
| 245 | **The measurement is right; the diagnosis and the conclusion are wrong.** |
| 246 | |
| 247 | Wildcard versus regex matching is a **per-document calculation setting**, |
| 248 | not a profile setting, and `gencorpus/ods.go:137-140` writes an `.ods` with |
| 249 | no `<table:calculation-settings>` element at all, so LibreOffice falls back |
| 250 | to ODF's legacy default (regular expressions on, wildcards off), where |
| 251 | `"t*"` is the regex `t*` and matches nothing in "ten". I rebuilt the same |
| 252 | document with one added element and ran the same binary: |
| 253 | |
| 254 | ``` |
| 255 | <table:calculation-settings table:use-wildcards="true" table:use-regular-expressions="false"/> |
| 256 | ``` |
| 257 | |
| 258 | | case | as gencorpus writes it | with the element | excelize / ours | |
| 259 | |---|---|---|---| |
| 260 | | `COUNTIF(B1:B3;"t*")` | 0 | **3** | 3 | |
| 261 | | `COUNTIF(B1:B3;"t??")` | 0 | **1** | 1 | |
| 262 | | `SUMIF(B1:B3;"t*";A1:A3)` | 0 | **60** | 60 | |
| 263 | | `COUNTIF(B1:B3;"t?n")` | 0 | **1** | 1 | |
| 264 | | `COUNTIF(B1:B3;"*y")` | 0 | **2** | 2 | |
| 265 | | `SUMIFS(A1:A3;B1:B3;"t*")` | 0 | **60** | 60 | |
| 266 | | `COUNTIFS(A1:A3;">=20";B1:B3;"t*")` | 0 | **2** | 2 | |
| 267 | |
| 268 | So decision C ("expected values come from LibreOffice headless … never |
| 269 | from our reading") was set aside for these five when a one-line change to |
| 270 | the generator keeps it. That matters because the substitute oracle is |
| 271 | weak: in this session excelize returned `0` for `SUM(C1:C3)` over an error |
| 272 | cell and **panicked** (`calc.go:8230`, index out of range) on |
| 273 | `COUNTIFS(A1:A3,">5",B1:C1,"<>")`. `TestWildcardCriteriaAgainstExcelize` |
| 274 | itself is sound — mutation M9 (`?` widened to `*`) fails it — but it is |
| 275 | sound against a second-choice oracle that was never needed. |
| 276 | |
| 277 | **Do:** add the element to `contentXML`, move the seven wildcard cases back |
| 278 | into `cases`, regenerate, update `corpusSHA256`, and reduce |
| 279 | `oracle_wildcard_test.go` to a cross-check or delete it. Note in the |
| 280 | divergences file that this is a *document* setting so the next person |
| 281 | does not re-measure the "profile". |
| 282 | |
| 283 | --- |
| 284 | |
| 285 | ## F7. The criteria matcher diverges from Excel on the edges the corpus never reaches — MEDIUM |
| 286 | |
| 287 | All measured against LibreOffice 26.8 on a hand-built document (C1 blank, |
| 288 | C2 = `=""`, C3 = `=1/0`, D1 = text `"10"`, D2 = number 10, D3 blank). Where |
| 289 | I am confident Excel agrees with LibreOffice I say so; where I am not, the |
| 290 | row says only what was measured. |
| 291 | |
| 292 | | expression | ours | LibreOffice | note | |
| 293 | |---|---|---|---| |
| 294 | | `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. | |
| 295 | | `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 `<>`. | |
| 296 | | `COUNTIF(A1:C3,"<>20")` | 7 | **8** | Same cause — the error cell. | |
| 297 | | `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. | |
| 298 | | `COUNTIF(D1:D2,"<>10")` | 1 | **0** | Mirror of the row above. | |
| 299 | | `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. | |
| 300 | | `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. | |
| 301 | |
| 302 | Two more from reading: `readPairs` (`fn_cond_apply.go:65-73`) compares |
| 303 | `cells()` and not shape, so a 1×6 criteria range against a 6×1 one passes |
| 304 | and is silently reshaped — the exact "mistyped range reads the wrong |
| 305 | cells and still returns a number" its own comment says it refuses. And |
| 306 | mutation M5 (blank/text never matching a numeric `<>`) stayed green across |
| 307 | `calc` and `xlsx`: at HEAD no test distinguishes any row in this table. |
| 308 | |
| 309 | **Do:** treat `""`-valued cells as blank for `""`, count error cells for |
| 310 | `<>`, match text criteria against text cells before numeric coercion, |
| 311 | make an empty criterion match nothing, check shape not count, and refuse |
| 312 | a SUMIFS/AVERAGEIFS sum range of a different shape. Add the support cells |
| 313 | (an `=""` cell, an error cell, a text `"10"`) to gencorpus and generate |
| 314 | every row above; they are the semantic fault lines the earlier memory note |
| 315 | said the corpus lacked. |
| 316 | |
| 317 | --- |
| 318 | |
| 319 | ## F8. `writeAll`'s trailing `refetch` can paint stale over a concurrent edit; the root cause is batch-level versioning — MEDIUM |
| 320 | |
| 321 | **Claim:** there is an interleaving where a cell is painted stale. |
| 322 | **True.** |
| 323 | |
| 324 | `clipboard.js:154-156` calls `g.refetch()` once every lane is idle. |
| 325 | `grid.js:113-128` `refetch()` does `version = 0; meta = …; applyBatch(payload)` |
| 326 | in its `.then`. Sequence: the snapshot request leaves at tab version V; |
| 327 | another tab's edit lands at V+1 and its SSE batch paints here (version = |
| 328 | V+1); the snapshot (taken at V) arrives, `version` is zeroed, and |
| 329 | `applyBatch` paints the V-state value over the V+1 cell. No later message |
| 330 | repairs it because the edit already happened. |
| 331 | |
| 332 | The reason the refetch was added — six concurrent responses, the last to |
| 333 | arrive being the oldest, five of six painted — is the same defect from the |
| 334 | other side: `applyBatch` (`grid.js:80-83`) discards a **whole batch** by |
| 335 | the **tab's** version, while design §7 says "Every SSE message carries the |
| 336 | **cell's** version, and clients discard out-of-order paints by version". |
| 337 | A response to your own write is authoritative for the cells it names; it |
| 338 | should never be discarded because a different cell moved the tab's |
| 339 | version on. The on-disk edit goes serial (`LANES = 1`, no refetch), which |
| 340 | is correct and slow — 500 cells at one round trip each. The real fix is |
| 341 | per-cell versions on the wire, which also removes the `applyBatch` |
| 342 | special-casing for format writes. |
| 343 | |
| 344 | Also at HEAD: a keyboard cut's `clearBox` writes and an immediately |
| 345 | following paste's writes run in two independent pumps; if the regions |
| 346 | overlap, a late clear can land after the paste and blank a pasted cell. |
| 347 | |
| 348 | **Do:** short term, keep serial writes and say so in `MAX_CELLS`'s comment; |
| 349 | medium term, carry a per-cell version and let `applyBatch` compare per |
| 350 | cell. Chain `clearBox` and the next paste, or make cut clear only after |
| 351 | the paste completes. |
| 352 | |
| 353 | --- |
| 354 | |
| 355 | ## F9. `own()` prevents the default before it knows there is anything to copy, and a paste past the window is silently dropped — MEDIUM |
| 356 | |
| 357 | - `clipboard.js:226-232`: `preventDefault()` and `stopImmediatePropagation()` |
| 358 | run before `copyText()` learns that `box()` is `null`. With no grid |
| 359 | selection yet (fresh page, or after the library nulled it) Ctrl+C |
| 360 | anywhere outside a text field — text highlighted in the help dialog, the |
| 361 | contrast notice, a menu label — copies **nothing** and the browser's own |
| 362 | copy of the highlighted text is suppressed too. Same shape at `:240-242`: |
| 363 | on a read-only grid Ctrl+X swallows the event and does not even copy. |
| 364 | - `clipboard.js:198`: `if (r >= g.rows || c >= g.cols) continue;` — a paste |
| 365 | whose block runs past the window drops those cells without a word. The |
| 366 | server would accept them (`inCellRange` to row 4,999) and the next page |
| 367 | load would widen the window to show them. The module's header says "a |
| 368 | paste that silently drops a cell is the failure this whole change exists |
| 369 | to remove." |
| 370 | - `:188`: `count` uses `rows[0].length`; a ragged paste under-counts against |
| 371 | `MAX_CELLS`. Minor. |
| 372 | |
| 373 | **Do:** check `box()` (and, for cut/paste, `mayWrite`) before calling |
| 374 | `preventDefault()`, and let the event fall through otherwise; send the |
| 375 | off-window cells (the server is the bound) and `say()` that the grid will |
| 376 | show them after a reload, or `say()` that N cells were not pasted. |
| 377 | |
| 378 | --- |
| 379 | |
| 380 | ## F10. Copy provenance is per tab: a copy in one tab pastes untranslated in another — MEDIUM |
| 381 | |
| 382 | `lastCopy` (`clipboard.js:69`) lives in the page. Copy `=A1*2` in tab A, |
| 383 | paste it three rows down in tab B of the same sheet (or a different sheet |
| 384 | on this instance), and B has no origin, so `=A1*2` is stored verbatim — |
| 385 | where Excel, and this app one tab over, would store `=A4*2`. It is the |
| 386 | comment's "pasted verbatim, which is right" applied to text that *does* |
| 387 | have references to translate, and the numbers look fine. The clipboard |
| 388 | can carry provenance: `setData` with a second flavour (`text/html` with a |
| 389 | `data-oficina-origin` attribute, or a custom type — Chromium round-trips |
| 390 | custom types within a profile) and `getData` on paste; fall back to the |
| 391 | exact-text match when the flavour is absent. |
| 392 | |
| 393 | --- |
| 394 | |
| 395 | ## F11. Guards that stay green with the feature removed — LOW |
| 396 | |
| 397 | Ran against a scratch copy of HEAD; the check is whether each guard goes |
| 398 | red when the thing it guards is broken. |
| 399 | |
| 400 | | mutation | result | |
| 401 | |---|---| |
| 402 | | `view.Truncated = false` | **red** (`TestTheWindowStopsAtTheMaximum`) | |
| 403 | | `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 | |
| 404 | | table.html: remove `data-b` | **red** (`TestTheTableViewPaintsWhatItSets`) | |
| 405 | | `TranslateChecked` returns `true` | **red** (both off-grid tests) | |
| 406 | | `formatFromForm`: drop the `keep` branch | **red** (`TestKeepLeavesAFieldAlone`) | |
| 407 | | `globMatch`: `?` behaves as `*` | **red** (`TestWildcardCriteriaAgainstExcelize`) | |
| 408 | | sheets.css: delete the menubar `@supports` block | **red** (anchor guard) | |
| 409 | | 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. | |
| 410 | | `matches`: blank/text never satisfy numeric `<>` | **green** (F7) | |
| 411 | | `matches`: errors allowed through | **green** (F5, F7) | |
| 412 | |
| 413 | `TestTheAnchorGuardReadsTheStylesheetItThinksItDoes` refers to a |
| 414 | `stripBlocks` that does not exist (`:44-46`); the parser works because the |
| 415 | regex backtracks past the outer brace, which is luck rather than design. |
| 416 | |
| 417 | --- |
| 418 | |
| 419 | ## F12. Translation is bounded below but not above — LOW |
| 420 | |
| 421 | `onGrid` (`translate.go:73-92`) checks `>= 0` only. The lexer's |
| 422 | `refPattern` (`token.go:46`) is `^\$?[A-Za-z]{1,3}\$?[0-9]+$`, so column |
| 423 | ZZZ (18,277) is the last one that parses. A formula holding a 3-letter |
| 424 | column reference offset by up to `maxWindowCols*10` renders a 4-letter |
| 425 | label that `lexWord` (`token.go:163-175`) reads as an identifier; the |
| 426 | store's `contentNode` then refuses the write and the pasted cell is |
| 427 | skipped — the outcome `TestAnUntranslatableFormulaIsStillWritten` exists |
| 428 | to forbid, reached by the other edge. Excel gives `#REF!` past its last |
| 429 | column, which is what `ok=false` already produces. Also |
| 430 | `handlers_grid.go:121` checks `maxRawBytes` *before* `:134` translates, so |
| 431 | the stored raw can exceed the cap the route claims. |
| 432 | |
| 433 | **Do:** `ColPos > 18277` (or a named `maxColPos`) is off-grid in `onGrid`; |
| 434 | run the length check after translation. |
| 435 | |
| 436 | --- |
| 437 | |
| 438 | ## F13. `#REF!` is written as text — LOW, documented, still a divergence |
| 439 | |
| 440 | `copypaste.go:54-67` stores the literal `#REF!`; `handle_codec.go:41-50` |
| 441 | makes it a `Str`. Consequences the comment does not list: `=ISERROR(B2)` |
| 442 | is `FALSE`, `=B2+1` is `#VALUE!` rather than `#REF!`, and the .xlsx export |
| 443 | writes a string cell that Excel shows left-aligned as text. Visible, |
| 444 | as the comment says, but a wrong error code is a different kind of |
| 445 | visible. An error literal in the grammar is the fix; if it stays deferred, |
| 446 | put a row in §14 with that trigger. |
| 447 | |
| 448 | --- |
| 449 | |
| 450 | ## F14. Form decoder details — LOW |
| 451 | |
| 452 | - `handlers_format.go:58-63` reads the form with no `MaxBytesReader`; |
| 453 | the JSON path (`:64`) caps at 8 KiB. `r.FormValue` parses up to 10 MiB |
| 454 | urlencoded or 32 MiB multipart into memory first. |
| 455 | - A scriptless submit with every control on "keep" is an empty patch → |
| 456 | `:70-72` → a bare `400 That formatting could not be applied.` text page, |
| 457 | not a redirect back. `TestAFormPostStillObeysEveryBound` asserts this as |
| 458 | correct; for the form it is the ordinary "pressed Apply by accident" case. |
| 459 | - `tableReturn` (`:130-137`) drops `edit=` and the view's sort/filter, so |
| 460 | after Apply the cell's form closes and a filtered page reverts. Both are |
| 461 | derivable from validated inputs (`r1,c1` and the stored view). |
| 462 | |
| 463 | --- |
| 464 | |
| 465 | ## F15. Date serials below 61 — informational |
| 466 | |
| 467 | `serialTime` (`fn_date_parts.go:38-40`) counts from 1899-12-30 with no |
| 468 | 1900 leap-day. `DAY(1)` is 31, `MONTH(1)` 12, `YEAR(1)` 1899, |
| 469 | `DATE(1900,1,1)` is 2; Excel says 1, 1, 1900, 1. LibreOffice agrees with |
| 470 | us, so the corpus **cannot** arbitrate this by construction. Nothing to |
| 471 | change — every serial from 61 up agrees — but it belongs in |
| 472 | `oracle_divergences_test.go` as a recorded, oracle-blind divergence rather |
| 473 | than an unknown. |
| 474 | |
| 475 | --- |
| 476 | |
| 477 | ## Checked and clean |
| 478 | |
| 479 | - **Sealing.** No new log line. Every added `http.Error` string is fixed |
| 480 | text (`handlers_grid.go:131`, `handlers_format.go:61,71,76,80`). The 303 |
| 481 | target is `"/sheets/" + id + "/table?page=" + n` from a parsed, bounded |
| 482 | integer (`:130-137`; `TestTheFormRedirectIsBuiltByTheServer` covers the |
| 483 | hostile inputs). `note.js`'s only message interpolates `data-rows` and |
| 484 | `data-cols`, which are numbers. `tablepaint.go:71-105` builds |
| 485 | `template.CSS` only from `strconv`-formatted floats and `safeHex`, whose |
| 486 | regex (`previewsvg.go:37`) is `^[0-9A-Fa-f]{6}$|^[0-9A-Fa-f]{8}$`, so the |
| 487 | attribute can only ever hold `--name:#hhhhhh;` and numbers; fills and |
| 488 | number-format codes are §2.2 plaintext by design. Calc's new error paths |
| 489 | return codes. |
| 490 | - **CSRF on the form path.** `app.go:135-145` applies rastrillo's |
| 491 | origin-checking `csrf.Protect` to every state-changing method, so |
| 492 | accepting `application/x-www-form-urlencoded` on the API route does not |
| 493 | open a cross-site form post. |
| 494 | - **Content-type dispatch.** `isFormPost` keys on the body's own encoding; |
| 495 | a JSON body under a form content-type fails `Atoi` → 400, and the |
| 496 | reverse fails the JSON decoder → 400. `maxFormatCells` and `normalize` |
| 497 | run after both decoders converge (`:70-82`); `TestAHugeRectangleFromAFormIsRefused` |
| 498 | and `TestJSONStillAnswersJSON` hold. |
| 499 | - **`origin()` / `inCellRange`.** Origin is bounded to the same box as the |
| 500 | destination, so the offset is at most ±4,999 rows / ±599 columns and the |
| 501 | parse cost is the input's, not the offset's (F12 is the residual). |
| 502 | - **House rules.** No migration, no `scope.Owned`, no GORM binding, no |
| 503 | `sessions.UserID`. `TestOurStylesheetHasNoColourLiterals` is green; |
| 504 | every new `sheets.css` rule uses tokens. `clipboard.js` is 292 lines |
| 505 | against a 300 cap — no headroom for F3's listener without a split. |
| 506 | Paste has a menu item and a shortcut; fill has both. |
| 507 | - **Copy tests** are at the HTTP boundary and each fails under the |
| 508 | matching mutation. |
| 509 | |
| 510 | --- |
| 511 | |
| 512 | ## Summary of what has to change before merge |
| 513 | |
| 514 | 1. Make `internal/guardstest` green (F1) and commit the test the comment cites. |
| 515 | 2. Stop translating on cut (F2). |
| 516 | 3. Prove Ctrl+C/X/V with a real mousedown on a cell, and take the keystroke |
| 517 | from the library at capture phase if they fail (F3). |
| 518 | 4. Make the truncation sentence true, and fix the slice spec's claim (F4). |
| 519 | 5. Propagate errors from the sum range (F5). |
| 520 | 6. Put the wildcard cases back in the LibreOffice corpus with the one-line |
| 521 | `.ods` change (F6), and generate the matcher edges with the extra |
| 522 | support cells (F7). |
| 523 | |
| 524 | F8-F15 are worth doing and can follow; F8's serial-write stopgap is already |
| 525 | on disk. |
| 526 | |
amadan