# REQUEST CHANGES

Reviewing model: Codex, GPT-5.

## Findings

### 1. §5 asks Paul to rule on row and column insertion semantics that the design already settles

**Claim:** §5.1 says insertion needs a new choice between rewriting A1 text and binding references to stable ids.

**Verdict:** False. This relitigates §4.2. The binding rule says, “Where it disagrees with the design spec, the spec wins and this file is wrong,” and also says, “If one proves wrong, **stop and flag it** rather than quietly redesigning around it.” The design already rules: “**Insert / delete row or col | No rewrite. Ids are stable**” (`docs/superpowers/specs/2026-08-30-sheets-design.md:181-184`). It separately requires translation only for copy and drag-fill (`:184`). The current engine has not reached that design: `refKey` still maps positions to synthetic ids, and its comment explicitly leaves stable-id binding to later work (`internal/sheets/calc/eval.go:60-65`). That implementation gap is not an open product decision.

**Do:** Remove item 1 from “Paul's to rule.” Specify the already-settled stable-id binder and graph rebuild as prerequisite subsystem work, consistent with design §4.2 and §14.

### 2. Tabs is a subsystem wearing a feature name, and the stated cost omits most first-tab assumptions

**Claim:** §3.3 calls tabs “moderate” because history, comments, sort/filter, and live state are already keyed by tab id; it prices the work as routes, two tab strips, and sealed-name handling.

**Verdict:** Misleading. Persistence is tab-keyed, but the reachable product surfaces resolve only the first tab. `SheetSummary.TabID` is explicitly the first tab (`internal/sheets/sheetsvc.go:51-58`), `summarize` always selects the first by position (`internal/sheets/sheetsvc.go:236-249`), and `openTabFor` opens that id (`internal/sheets/handlers_table.go:215-220`). Table read/write (`internal/sheets/handlers_table.go:79-89,143-181`), comments (`internal/sheets/handlers_comments.go:47-54,126`), history/restore (`internal/sheets/handlers_history.go:47,206`), export (`internal/sheets/handlers_export.go:35-49`), import (`internal/sheets/handlers_import.go:167`), saved views (`internal/sheets/handlers_view.go:13-59`), warnings on the grid page (`internal/sheets/handlers_sheets.go:151-155`), and link view (`internal/sheets/handlers_link.go:65`) all take that first-tab path. The router has no tab-addressed HTML route at all (`internal/sheets/app.go:199-236`). This is a cross-cutting page, navigation, redirect, export, history, comments, and authorization refactor, not merely a strip plus a route.

**Do:** Rewrite §3 with every first-tab-only surface named and an explicit decision about which become tab-addressed in this slice. Either scope add/name/switch honestly to grid and table and state the resulting limits, or price the full subsystem. Do not call it moderate based only on storage keys.

### 3. Decision E is achievable, but the spec has not designed the required request and response shape

**Claim:** §4.2 says `POST .../format` is scriptless-ready and that a table form can post to it; §4.3 describes the work as form decoding beside JSON decoding.

**Verdict:** False as written. The handler accepts only a JSON body through `json.Decoder` (`internal/sheets/handlers_format.go:49-55`) and always returns a JSON grid payload (`:75-82`). A normal HTML form sends `application/x-www-form-urlencoded` or multipart data and needs a 303 redirect back to the table page. The patch also depends on pointer presence: omitted means “leave unchanged,” while an explicit `false` means “clear bold/italic” (`internal/sheets/formatpatch.go:9-39,60-63`). Unchecked checkboxes are omitted, so a naive form cannot express false. The route is tab-addressed (`internal/sheets/app.go:234`), while the current table page and redirects are sheet/first-tab-addressed (`internal/sheets/app.go:204-205`; `internal/sheets/handlers_table.go:211-212`).

**Do:** Keep one route and one validation/application path, but specify a content-type dispatcher and a form-to-`formatPatch` adapter. Define explicit set/clear fields for booleans, rectangle fields, how “no colour” and default alignment are encoded, and a safe return target derived by the server. JSON callers keep the JSON response; form callers receive a 303 to a server-built tab/page URL. Apply `validate`, `normalize`, and `maxFormatCells` only after both decoders converge on the same patch.

### 4. §5.1's sort/filter question does not need the ruling it claims

**Claim:** §5.3 says Paul must choose between making the grid paint the saved view and making the view personal and table-only.

**Verdict:** False. The current model is already shared: one `TabView` per tab (`internal/sheets/models.go:182-196`), the write handler says it changes what colleagues see (`internal/sheets/handlers_view.go:8-12`), and the table tells users it “changes what everybody sees on this tab” (`internal/sheets/templates/table.html:57-60`). Making it personal would require a new per-person storage model and would contradict the shipped contract. Also, the factual subclaim is true only for production call sites: `loadView` is called once in non-test code, by `handleTable` (`internal/sheets/handlers_table.go:127`; `internal/sheets/viewspec.go:80-103`). It also has three test call sites (`internal/sheets/viewspec_test.go:200,218,233`), so “from nowhere else” is literally false.

**Do:** Remove this from “Paul's to rule.” Treat the missing grid application of the existing shared view as a bug or implementation task. If the product is to become personal, propose that as an explicit change to the existing contract rather than presenting both states as undecided.

### 5. §4.1 overstates the table formatting gap

**Claim:** The table view has “Nothing” from formatting and “only edits a cell's value.”

**Verdict:** False in presentation, true in controls. The table computes each display string with the cell's stored number format (`internal/sheets/handlers_table.go:281-286`), right-aligns numeric computed values through `Number` (`:283-286`; `internal/sheets/templates/table.html:81`), and surfaces imported fill/ink contrast failures (`internal/sheets/handlers_table.go:132`; `internal/sheets/templates/table.html:63`). It does not expose controls for bold, italic, alignment, number format, fill, ink, row height, or column width; the only per-cell form edits `raw` (`internal/sheets/templates/table.html:82-99`). It also does not paint bold, italic, fill, ink, or explicit alignment.

**Do:** Replace “Nothing” with the exact gap: stored number formats already affect displayed values and warnings already surface formatting risk, but the table neither paints most formats nor lets users change any format or size. State whether this slice adds controls only or also visual parity; PRODUCT.md's principle points to both.

### 6. The function priority is not supported by the corpus the spec invokes

**Claim:** Decision A says coverage is measured against imported files; §2.2 says the listed families are what ordinary business workbooks contain; §2.5 says conditional aggregates are the largest cause of `#NAME?` in imported workbooks.

**Verdict:** Unsupported. `tools/gencorpus` has a hand-maintained `cases` list (`tools/gencorpus/main.go:71-98`), and the checked-in corpus contains generated oracle answers for those formulas (`internal/sheets/calc/testdata/corpus/formulas.json:1-64`). Neither is an inventory of formulas observed in imported workbooks. No repository evidence ranks conditional aggregates as the largest cause. Leaving the exact list to implementation (§2.2) also violates intent-before-code: the implementation would decide scope without the promised imported-file evidence.

**Do:** Add a deterministic, content-safe inventory step that extracts function names from an identified workbook corpus without logging formulas or cell content, commit the aggregate counts or fixtures, and select the function list in the spec before implementation. Otherwise describe this as a judgment-based starter set and remove the measured-coverage and “largest cause” claims.

### 7. §6 contains one false implementation claim and weak triggers

**Claim:** Cross-tab references can wait because “Row and column ids are globally unique, so an id-keyed graph is already tab-agnostic.”

**Verdict:** False. Database row and column ids are globally allocated (`internal/sheets/store/models.go:74-85`), but the calc graph is not keyed by those ids. The code maps positions to `CellKey{row+1,col+1}` (`internal/sheets/calc/eval.go:60-65`), matching design §14's warning that the graph is keyed on position (`docs/superpowers/specs/2026-08-30-sheets-design.md:1058`). The proposed trigger—rebuild on stable ids—is checkable, but its rationale must not claim the rebuild already exists.

The per-tab-permissions trigger, “somebody asks,” is observable but not a sufficient design trigger: one request would force a new permission model whether or not there is a validated access-control need. The `TEXT` trigger is circular: “a corpus run covering the patterns an imported workbook actually uses” does not name the workbook corpus, coverage threshold, or collection method. Multi-worksheet import's trigger is checkable and fires when the second tab route ships.

**Do:** Correct the graph claim. Define the `TEXT` corpus and coverage threshold. Replace the permissions wish with a checkable need, such as a confirmed case requiring different access within one sheet. Keep the multi-worksheet trigger.

### 8. “No migration” is only narrowly true and is unsafe as an unqualified claim

**Claim:** §3.3 says no migration is needed because the tabs table and its columns already exist.

**Verdict:** The column claim is true: `tabs` has `id`, `sheet_id`, sealed name, position, and frozen-row/column fields (`internal/sheets/store/models.go:53-61`; `internal/sheets/migrations/0001_create_sheets.sql:5`). Add, rename, and switch can write those columns. But the schema has only a plain index on `sheet_id` and no uniqueness constraint on `(sheet_id, position)` (`internal/sheets/migrations/0001_create_sheets.sql:5-7`). Once add and reorder exist, duplicate positions can make “first tab” and strip order depend on id as a tie-breaker (`internal/sheets/sheetsvc.go:242-249`) instead of enforcing the document order invariant.

**Do:** Either specify a serialized allocation algorithm and accept duplicate-position recovery explicitly, or add an additive unique index/migration for `(sheet_id, position)` and handle allocation conflicts. Until that choice is made, do not promise “no migration.”

### 9. The cell API authorization claim is true; the stream is the only bypass and is not vulnerable

**Claim:** §3.1 says the cell API is tab-scoped and `openTab` checks that a tab belongs to its sheet on every request.

**Verdict:** True for the cell GET/POST routes. Both are under `/api/sheets/{sheetID}/tabs/{tabID}/cells` (`internal/sheets/app.go:232-233`) and both call `openTab` (`internal/sheets/handlers_grid.go:69-80,97-108`). `openTab` checks the pair before checking the sheet grant (`internal/sheets/handlers_grid.go:188-225`). Format and size do the same (`internal/sheets/handlers_format.go:36-47`; `internal/sheets/handlers_size.go:34-43`). The SSE stream bypasses `openTab`, but repeats the pair check and read authorization itself (`internal/sheets/handlers_stream.go:50-68`), so it is not a live authorization bug. No API route that exposes tab cells skips both checks.

**Do:** Narrow “on every request” to the cell/format/size routes, and name the stream's equivalent manual check. Add a shared authorization helper or a route-coverage test if the spec wants “every request” to remain mechanically true.

### 10. The 100×26 bug claim is true and belongs in the top three

**Claim:** A row-150 cell is reachable in the table view and invisible in the grid.

**Verdict:** True. The grid page hard-codes 100 rows and 26 columns (`internal/sheets/sheetsvc.go:37-42`; `internal/sheets/handlers_sheets.go:141-145`). The client creates only that matrix, ignores cells outside it, and refetches the same dimensions (`internal/sheets/static/grid.js:30-32,61-67,100-105,113-125,166-170,210-223`). The table paginates 500 rows at 25 per page (`internal/sheets/handlers_table.go:12-22,98-116`) and accepts edits below row 500 (`:160-169`). The JSON write route accepts much farther—below 5,000 rows and 600 columns—because it uses ten times the maximum request window (`internal/sheets/handlers_grid.go:62-67`; `internal/sheets/sheetsvc.go:40-41`).

**Do:** Promote this bug ahead of function expansion. Provide viewport growth or pagination for the grid and make read/write bounds consistent and visible. Copy alone would disclose the loss but would not make the grid a usable editor for already-reachable cells.

### 11. The sealing rule is preserved by the proposed tab shape, but the spec needs explicit negative requirements

**Claim:** Tab names remain sealed and a tab can have a shareable URL.

**Verdict:** Achievable without leaking the name. The current model stores only `NameSealed` (`internal/sheets/store/models.go:53-61`), creation seals `"Sheet 1"` with purpose `tab.name` (`internal/sheets/sheetsvc.go:107-127`), and the route scheme uses numeric ids (`internal/sheets/app.go:232-236`). A tab-addressed HTML URL can likewise use a numeric tab id. Nothing in the slice requires a name in a URL, log, error, job message, or metric label. This matches the binding rule: “**No cell content, comment text, sheet title or tab name in any log line, error string, job status message or metric label.**”

**Do:** State that add and rename errors are fixed text or codes and never wrap an error containing the submitted name; URLs contain only ids. Add raw-SQLite/WAL coverage for renamed and additional tab names, plus tests that rejected names do not appear in error or log output.

## Verified factual claims

- The engine registers 32 callable names, counting all three dispatch tables: 22 ordinary registrations (`internal/sheets/calc/fn_logic.go:6-11`, `fn_math.go:13`, `fn_math_round.go:12-18`, `fn_text.go:6-13`), two clock registrations (`fn_date.go:6-7`), and eight node registrations (`fn_lookup.go:12-14`, `fn_math.go:10-15`). The 32 names listed in §2.1 match those registrations. Duplicate registration panics (`internal/sheets/calc/registry.go:16-20,45-49,81-85`).
- Every proposed function in §2.2 is genuinely absent from those registration sites. Import preserves formula input and then writes it through the app engine (`internal/sheets/xlsx/import.go:98-103,119-145`), so an unknown function reaches the calc engine and resolves as `#NAME?`; the examples `SUMIF`, `YEAR`, and `ISBLANK` are missing. What is not verified is their frequency or priority.
- The tabs table and all fields needed for basic add/name/switch exist, and names are sealed. The missing schema invariant is ordered-position uniqueness.
- `loadView` has exactly one non-test caller, not exactly one call site in the repository.
- The fixed-window invisibility bug is real, and cells beyond row 100 are reachable from both the table UI and the JSON write API.

## What I would cut

Cut **function coverage** from these three. The grid currently hides reachable data, while tabs and table formatting fire written §14/PRODUCT.md obligations. Function coverage has no imported-workbook inventory to justify its chosen families or its “largest cause” claim. Replace it with the 100×26 reachability fix. Function work should return after the content-safe workbook corpus exists and the spec names the measured list.

Do not cut tabs, but rewrite it as the cross-cutting subsystem it is. Do not cut table formatting; its trigger has fired, though its scope must include visual parity and a real form protocol.