# REQUEST CHANGES

Reviewed by Codex / GPT-5.6-sol.

## 1. A batch refetch can overwrite a newer paint with an older snapshot

**Claim:** Six concurrent write lanes followed by one refetch leave the grid at the newest stored state.

**False.** `writeAll` starts the refetch after all six lanes have settled, but does not wait for it or associate it with a version (`internal/sheets/static/clipboard.js:112-127`). While that GET is in flight, a later edit can arrive over SSE and advance the client version. When the older GET resolves, `refetch` resets `version` to zero before applying it (`internal/sheets/static/grid.js:113-126`). That defeats the binding rule: **“Every SSE message carries the cell's version, and clients discard out-of-order paints by version rather than by arrival.”** The stale snapshot is then accepted and can paint over the later edit. The database remains correct, but the active grid is wrong until another update or reload.

There is a second refetch defect in the same code. It clears `meta` and styles but never clears worksheet values before applying a sparse snapshot. A cell absent from the snapshot therefore remains visibly present in Jspreadsheet (`internal/sheets/static/grid.js:118-125`; sparse-snapshot contract at `internal/sheets/grid.go:160-164`). This matters on failed paste/clear writes because `post` treats refetch as the correction path (`internal/sheets/static/grid.js:142-155`).

**Do:** Make snapshot application monotonic. Do not zero the version; reject a snapshot older than the newest applied event, or fetch/apply under an explicit snapshot generation. Replace the worksheet contents (including absent cells) when applying a snapshot. Add a browser test that delays a refetch response, applies a newer SSE event first, and proves the newer cell wins; add a failed-write test proving an optimistic value absent from the snapshot is cleared.

## 2. The copy translation check is incomplete and writes the wrong `#REF!` shape

**Claim:** `TranslateChecked` completely detects off-grid references and preserves Excel copy behavior for every AST node, including ranges.

**False.** The walker does cover every current node type: `Num`, `Str`, `Bool`, `Ref`, `Range`, `Binary`, `Unary`, and `Call` (`internal/sheets/calc/ast.go:4-43`; `internal/sheets/calc/translate.go:12-42,74-93`). Its bounds check, however, tests only `RowPos >= 0 && ColPos >= 0` (`internal/sheets/calc/translate.go:76-77`). It accepts references beyond Excel's last row and column. For example, copying `=XFD1` one column right renders `=XFE1`; Excel produces `=#REF!`. The parser itself accepts arbitrary column labels and row numbers (`internal/sheets/calc/token.go:199-227`), so no later layer repairs this.

If one endpoint of a range becomes invalid and the other remains valid, `translateCopied` replaces the entire formula with the literal text `#REF!` (`internal/sheets/copypaste.go:52-68`). Excel preserves the rest of the formula and the valid endpoint, for example `=SUM(#REF!:$A2)+C1`; this branch instead discards `SUM`, `$A2`, and `+C1`. It also stores a text cell, not an error value. The comment acknowledges the latter semantic difference (`internal/sheets/copypaste.go:54-65`), but the settled decision is exact: **“Copy / drag-fill — Translation pass at paste time. Relative refs are re-resolved by offset from the paste anchor; absolute (`$A$1`) refs resolve to the same id.”** Visible wrongness is not enough.

**Do:** Represent `#REF!` in the formula AST, allow it in a reference or range endpoint, render it in place, and evaluate it as `ErrRef`. Check both lower and upper spreadsheet bounds for every translated reference. Add mixed-absolute range cases and last-row/last-column cases. Recheck `maxRawBytes` after rendering because translation can lengthen the stored formula; the handler currently checks the source before translation only (`internal/sheets/handlers_grid.go:105-117`).

## 3. The conditional aggregates return plausible but wrong values

**Claim:** The six conditional aggregates implement Excel's criteria and range-shape rules.

**False in several untested cases.** These are silent calculation errors.

- A numeric `<>` criterion matches blanks and all text because every non-number returns `c.op == "<>"` (`internal/sheets/calc/fn_cond.go:126-130`). Excel's `COUNTIF(range,"<>20")` does not count an unused blank cell. The corpus tests only `A1:A3`, where all three cells contain numbers, so they cannot detect this (`internal/sheets/calc/testdata/corpus/formulas.json:68`).
- An empty `<>` criterion treats a formula result of `""` as non-empty because it checks only `KindEmpty` (`internal/sheets/calc/fn_cond.go:148-155`). Excel criteria distinguish blank-like empty strings in this family; this needs oracle cases rather than an assertion in a comment.
- `SUMIFS`/`COUNTIFS`/`AVERAGEIFS` require corresponding ranges with the same dimensions. `readPairs` checks only total cell count (`internal/sheets/calc/fn_cond_apply.go:65-73`), so a 1x3 range and a 3x1 range are accepted and silently reshaped by `offsetRect` (`internal/sheets/calc/fn_cond_apply.go:22-33`). Excel returns `#VALUE!` for mismatched dimensions.
- Excel's criteria grammar uses `~` to escape `*`, `?`, and `~`. `wildcardMatch` implements only `*` and `?` (`internal/sheets/calc/fn_cond.go:176-213`), so a criterion such as `"~*"` does not match a literal asterisk as Excel does.

**Do:** Generate oracle vectors containing blank cells, `""` formula results, mixed text/number/error cells, escaped wildcards, and equal-cell-count ranges with different dimensions. Fix the matcher and compare width and height, not area. Do not approve this function slice until those vectors pass against an Excel-compatible oracle.

## 4. The new date functions expose a broken 1900 date system

**Claim:** `DATE`, `YEAR`, `MONTH`, `DAY`, and `EOMONTH` follow Excel's roll behavior and year rules.

**False around Excel's 1900 epoch.** The engine defines day zero as 1899-12-30 and applies ordinary Gregorian arithmetic (`internal/sheets/calc/fn_date.go:10-22`; `internal/sheets/calc/fn_date_parts.go:38-40`). Consequently `DATE(1900,1,1)` returns serial 2 instead of Excel's 1, and serial 1 decodes as 1899-12-31. It cannot represent Excel's fictitious 1900-02-29 at serial 60: `DAY(60)` returns 28, and `DATE(1900,2,29)` normalizes to March 1 and returns 61 (`internal/sheets/calc/fn_date_parts.go:85-113`). `EOMONTH` inherits the same defect (`internal/sheets/calc/fn_date_parts.go:123-149`). Years above Excel's supported range are also allowed by `time.Date` instead of returning `#NUM!`.

The committed LibreOffice cases all round-trip a modern `DATE` through `YEAR`/`MONTH`/`DAY`, so the same offset error cancels itself and the oracle stays green (`internal/sheets/calc/testdata/corpus/formulas.json:80-89`).

**Do:** Implement the Excel 1900 serial discontinuity explicitly, including serial 60, enforce Excel's year bounds, and add direct serial assertions for 0, 1, 59, 60, and 61 plus `DATE(1900,1,1)`, `DATE(1900,2,29)`, and boundary years. Keep modern roll tests, but do not use only self-cancelling round trips.

## 5. Clipboard provenance and event ownership are heuristic, not safe

**Claim:** Exact clipboard-text equality means a paste came from the remembered copy, and capture plus `stopImmediatePropagation` prevents the vendor from handling the event.

**False as a general guarantee.** `lastCopy` records only text and an origin, then translates any later paste with identical text (`internal/sheets/static/clipboard.js:52-59,129-137,155-169`). If the user subsequently copies the same text from another application, or intends that text as literal external input, this page still assigns the stale internal origin and rewrites formulas. Exact equality reduces false provenance; it does not establish provenance.

Capture order is also registration order when listeners share `document`. If a future vendored build registers a capture listener before this module, its handler runs before `stopImmediatePropagation` (`internal/sheets/static/clipboard.js:199-237`). Based on the failure documented in this module, the result is not merely a visible duplicate action: the vendor can paint cells through a path that never reaches the server, so they disappear after reload (`internal/sheets/static/clipboard.js:1-18`). The current integration relies on the vendor using a later phase or registering later, and no test pins that fact.

**Do:** Carry internal provenance in a custom clipboard MIME type where the browser permits it, and treat plain text as external. If compatibility requires the text fallback, expire or invalidate `lastCopy` on clipboard/focus changes and document the remaining ambiguity. Add a real-browser regression test that instruments vendor listener order and proves exactly one server write per pasted cell. Re-run that test on every vendor upgrade.

## 6. The truncation notice makes a false claim about access to rows

**Claim:** When the grid reaches its 500-row ceiling, “The table view lists every row.”

**False.** The table view fixes its page count from `maxWindowRows`, so it renders only rows 0-499 (`internal/sheets/handlers_table.go:107-114,125-147`). Its write route also rejects row 500 and beyond (`internal/sheets/handlers_table.go:175-183`). The new window test deliberately writes row 701 (`internal/sheets/handlers_window_test.go:97-109`), while the new notice tells the user the table lists it (`internal/sheets/static/note.js:41-47`). It does not. Export may retain the cell, but neither editing surface reaches it.

This branch does not newly truncate storage: the old grid stopped at 100 and the cell route already accepted rows through 4,999. It does newly publish an incorrect recovery path for the truncation.

**Do:** Either paginate the table through the actual extent and permit edits across the accepted coordinate range, or change the notice to name the route that genuinely contains the data. Add a test that follows the notice's promised route and finds the row beyond 500.

## 7. The form decoder admits unbounded multipart bodies

**Claim:** Content-Type dispatch gives form and JSON callers equivalent bounded handling before shared validation.

**Partly false.** Both decoders do converge before `validate`, `normalize`, and `maxFormatCells`, so the 4,000-cell cap applies to both (`internal/sheets/handlers_format.go:51-82`). JSON is wrapped in an 8 KiB `MaxBytesReader`; form decoding is not (`internal/sheets/handlers_format.go:56-68`). `isFormPost` accepts multipart bodies (`internal/sheets/handlers_format.go:117-121`), and `formatFromForm` calls `FormValue`, which can parse a much larger multipart request before the shared cell-count bound is reached (`internal/sheets/formatform.go:27-68`). The rendered form uses URL encoding, so multipart support adds attack surface without serving this screen.

The caller cannot create an open redirect. `tableReturn` parses the requested page as an integer, bounds it to the server's fixed page count, and constructs a root-relative path from the numeric sheet id (`internal/sheets/handlers_format.go:123-137`).

**Do:** Put a request-body cap around both form encodings before parsing, or accept only `application/x-www-form-urlencoded` and cap it. Add oversized URL-encoded and multipart tests.

## 8. The dropdown guard does not enforce the branch it describes

**Claim:** `TestEveryDropdownInsetOverrideHasAnAnchorBranch` proves each fallback inset override has the required anchor-positioning branch.

**Only partly true.** The guard can fail, its parser mutation test is live, and deleting the paired `inset: auto` makes it fail (`internal/guardstest/dropdownanchor_test.go:92-180`). It does not check that the matching rule sets `position-area`; it only checks that some enclosing `@supports` condition mentions `position-area` and the rule body contains `inset: auto` (`internal/guardstest/dropdownanchor_test.go:73-89`). Removing the rule's `position-area: block-end span-inline-end` leaves the guard green while removing the menubar-specific anchor placement that the diagnostic tells the developer to add (`internal/guardstest/dropdownanchor_test.go:112-128`).

**Do:** Parse and require the expected `position-area` declaration in the matching selector, then add that deletion to the mutation test.

## Verified claims and gates

- `safeHex` is sufficient for the imported-colour interpolation. It accepts exactly six or eight ASCII hex digits, discards the two ARGB alpha digits, and prepends `#` (`internal/sheets/previewsvg.go:28-47`). `paintOf` concatenates only that value after a fixed custom-property name and semicolon (`internal/sheets/tablepaint.go:82-100`). `template.CSS` therefore cannot carry an attacker-controlled delimiter or declaration.
- I found no added cell value, sheet title, tab name, comment, or number-format pattern in a log line, metric label, URL, redirect, or error response. The new errors are fixed text. `note.js` reports only fixed window dimensions. This satisfies **“No cell content, comment text, sheet title or tab name in any log line, error string, job status message or metric label.”**
- The reviewed diff adds no migration, no `scope.Owned`/`scope.OwnedBy`, no `sessions.UserID`, no request-body binding onto a GORM model, and no new transaction that scopes the outer handle.
- `sheets.css` adds no colour literal. The new template uses the required attribute grammar; `rst-sr-only` remains a permitted utility class. Authored JavaScript stays at or below the binding **“300-line JS module cap, test-enforced, ratchet-down only”**: `clipboard.js` is 292 lines and `note.js` is 60. Clipboard actions have menu equivalents.
- The wildcard oracle is not vacuous: it requires a non-empty case list, fails when excelize cannot answer, and includes a mutation check whose expected result cannot be zero (`internal/sheets/xlsx/oracle_wildcard_test.go:43-92`). Moving the wildcard cases out of the LibreOffice corpus is justified by an engine/profile semantic difference, not by deleting the oracle; excelize independently evaluates all seven cases. It still needs the missing escaped-wildcard cases in finding 3.
- The window tests and table-format tests exercise their features and fail if those features are removed. The table tests cover write, paint, tri-state clear/keep, shared validation, the range cap, redirect shape, and JSON response preservation (`internal/sheets/handlers_table_format_test.go:55-258`). They do not cover findings 6 and 7.
- `go test ./internal/guardstest ./internal/sheets/calc ./internal/sheets/xlsx` passes. `git diff origin/core..HEAD --check` passes. The full `internal/sheets` package reached `TestStreamIsAStreamed200NotAnUpgrade` and then failed because this sandbox forbids `httptest` from binding `[::1]:0`; that is an environment failure, not a passing full-suite result.