amadan
docs(spec): the next three, and the review that rewrote them
Two models were briefed identically and separately on "what are the next three features" and told not to hedge toward consensus. They agreed on exactly one item (tabs) and disagreed on everything else, which is the part worth having. Both briefs and both answers are committed alongside this, so the next person can check the reasoning rather than take the conclusion. The spec then went through AGENTS.md's §Review — a pass by Codex / GPT-5.6-sol, a different family from the author. Verdict: REQUEST CHANGES, eleven findings. It was right about the two that mattered, and both were the same mistake in two places: taking a claim out of the brainstorm and writing it down without checking it against the code. - Revision 1 asked Paul to rule on insert/delete semantics. §4.2 already rules on them — "No rewrite. Ids are stable. This is the case stable ids buy." Asking again is the drive-by relitigation AGENTS.md forbids. - Revision 1 called tabs "moderate" because the storage is tab-keyed. The storage is; the product is not. Twenty-one HTML routes exist and not one carries a tab id — every screen resolves the first tab. That is a page, navigation, redirect, export, history, comment and authorisation change across the whole app, which is §13's "subsystem wearing a feature's name". It is now specced, priced and NOT in the slice. Four more corrections worth naming, because each was a claim I could have checked and did not: - "Conditional aggregates are the largest cause of #NAME? in imported workbooks" — no evidence exists for this. gencorpus holds a hand-maintained case list, not an inventory of anything anybody imported. Restated as a judgment-based starter set, with the list named in the spec rather than left to the implementation, and the honest ranking waiting on an inventory that does not exist yet. - "The table view has nothing" — false. It already renders through the stored number format and already surfaces contrast failures. The real gap is controls plus visual parity, and saying "nothing" would have scoped the work wrongly. - "POST .../format is scriptless-ready" — false. It decodes JSON only and answers JSON. The form path needs content-type dispatch, a 303 to a server-built target, and a tri-state toggle, because formatPatch reads absent as "leave unchanged" and an unchecked checkbox is absent — so a naive form could never turn bold off. - "Row and column ids are globally unique, so an id-keyed graph is already tab-agnostic" — false. eval.go builds CellKey from POSITIONS, which is §14's own warning. The trigger survives; its reason did not. Two items were removed from "Paul's to rule" because they are not rulings: insert/delete (settled by §4.2, with an implementation gap beneath it) and the grid ignoring the saved sort/filter (a bug against an already-shipped shared contract that the UI already states).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014od11UreNE4Ewf3oR9837B
6 files changed,
+832
−0
docs/superpowers/brainstorms/2026-09-03-brief.md+99 −0docs/superpowers/brainstorms/2026-09-03-fable.md+163 −0docs/superpowers/brainstorms/2026-09-03-gpt.md+45 −0docs/superpowers/reviews/2026-09-03-next-three-brief.md+75 −0docs/superpowers/reviews/2026-09-03-next-three-r1.md+109 −0docs/superpowers/specs/2026-09-03-sheets-next-three.md+341 −0
CI passed — run details
diff --git a/docs/superpowers/brainstorms/2026-09-03-brief.md b/docs/superpowers/brainstorms/2026-09-03-brief.md| new file mode 100644 |
| index 0000000..629ed92 |
| --- /dev/null |
| +++ b/docs/superpowers/brainstorms/2026-09-03-brief.md |
| @@ -0,0 +1,99 @@ |
| +# Brief: what are the next three features for Oficina Sheets? |
| + |
| +You are one of two independent voices answering this. Your answer will be |
| +compared against the other's, and the disagreements are the most useful |
| +part, so do not hedge toward a safe consensus list. Argue for what you |
| +think is right and say what you would cut. |
| + |
| +## What you must read first |
| + |
| +Read these, in this order, from the repository root. Do not answer from |
| +the summary below — it is a map, not a substitute, and a recommendation |
| +that contradicts a settled ruling is worthless. |
| + |
| +1. `PRODUCT.md` — what this product is for and who it is for. |
| +2. `AGENTS.md` — binding house rules. Note §Review and the shipping |
| + cadence, and the mechanical rules about data and the frontend. |
| +3. `docs/superpowers/specs/2026-08-30-sheets-design.md` — the design. |
| + Its §1 is the settled-decisions table (dated, attributed, with the |
| + rejected alternative beside each). **Do not propose relitigating one.** |
| + §13 is the sequencing and the agreed slip order. §14 is |
| + "Deliberately not built yet", and every row there carries a written |
| + trigger — a proposal that fires one of those triggers is a strong |
| + candidate; a proposal that ignores one is not. |
| +4. `internal/sheets/` — the actual code. In particular |
| + `handlers_grid.go`, `viewspec.go`, `store/`, `calc/`, and the |
| + `static/*.js` chrome modules. |
| + |
| +## What is already shipped and working |
| + |
| +Live at https://sheets.bds.oncarlos.com as of 2026-09-03: magic-link |
| +sign-in, a sheet list with shape-only thumbnails, a Jspreadsheet grid, |
| +live sync across tabs over SSE, a scriptless table view, sharing (team |
| +grants, roles, hashed link tokens), version history, server-side undo, |
| +comments, non-destructive sort and filter, XLSX import and export, |
| +per-person light/dark, a menu bar and formatting toolbar (bold, italic, |
| +alignment, number formats, fill and text colour from an OKLCH palette, |
| +column width and row height). |
| + |
| +## What is being fixed right now, so do not propose it |
| + |
| +Another session has these in hand tonight and they are not candidates: |
| + |
| +- Dropdown menus opened against the viewport edge; the sheet list had a |
| + 40px gap; the Format panel was ragged. **Fixed and pushed.** |
| +- **Cut and Copy are dead menu items**, the vendored grid's paste |
| + silently drops cells, and there is no Paste at all. |
| +- **`calc.Translate` exists, is tested, and nothing calls it.** Spec |
| + §4.2 rules "Copy / drag-fill → Translation pass at paste time", and it |
| + was never wired: filling `=A1*2` down B1:B3 writes `=A1*2` into all |
| + three cells and shows 20, 20, 20 instead of 20, 40, 60. Being fixed. |
| + |
| +## The question |
| + |
| +**Name the top three features to build next, in priority order.** |
| + |
| +For each one give me: |
| + |
| +- **What it is**, in two sentences, in plain language a user would |
| + recognise. Not a subsystem name. |
| +- **Why it is in the top three** — measured against PRODUCT.md's |
| + principles and against what a person can and cannot do with the app |
| + today. "Google has it" is not a reason. |
| +- **What it costs**: which packages it touches, whether it needs a |
| + migration, whether it needs a spec ruling from Paul, and roughly how |
| + big it is. Be honest when something is a subsystem wearing a feature's |
| + name — §13 already caught three of those. |
| +- **What breaks if we get it wrong**, and the one thing that would make |
| + you drop it from the list. |
| +- **What you would build FIRST inside it** — the smallest slice whose |
| + absence would still leave a coherent product. |
| + |
| +Then, separately: |
| + |
| +- **Name one thing on the shipped list that is quietly unfinished** and |
| + matters more than your third pick. The app has a habit of features |
| + that render perfectly and do nothing; two were found tonight. |
| +- **Name one thing you would NOT build**, from §14 or from the obvious |
| + Google-Sheets checklist, and say why it is a trap here specifically. |
| + |
| +## Constraints that bound every answer |
| + |
| +- A sheet's contents are sealed at rest. No plaintext cell, title or tab |
| + name reaches SQLite, object storage, a log line, an error string or a |
| + metric label. A feature that needs server-side search over cell text |
| + has to explain how. |
| +- Migrations are additive-only. |
| +- No frontend build step. Jspreadsheet CE is vendored. 300-line cap per |
| + JS module we write, test-enforced. |
| +- Every drag-only affordance needs a menu equivalent, and the scriptless |
| + table view is the product rather than a fallback. |
| +- calc's dependency graph is keyed on POSITION, not stable ids |
| + (`store/handle.go`'s `cellKey`). Anything that MOVES a row — insert, |
| + delete, drag-reorder, destructive sort — silently changes what every |
| + existing formula points at. §14 makes "calc's key space rebuilt on |
| + stable ids" the trigger. Treat a proposal that needs row-moving as |
| + needing that subsystem first, and say so. |
| + |
| +Answer in Markdown. Be specific, cite file paths and spec sections, and |
| +keep it under 1500 words. |
diff --git a/docs/superpowers/brainstorms/2026-09-03-fable.md b/docs/superpowers/brainstorms/2026-09-03-fable.md| new file mode 100644 |
| index 0000000..14fd1e4 |
| --- /dev/null |
| +++ b/docs/superpowers/brainstorms/2026-09-03-fable.md |
| @@ -0,0 +1,163 @@ |
| +# Brainstorm — Fable's answer |
| + |
| +Read in the brief's order, then `handlers_grid.go`, `viewspec.go`, |
| +`grid.go`, `store/handle*.go`, `store/history.go`, `calc/registry.go`, |
| +`calc/graph.go`, `calc/translate.go`, the four chrome modules, and the |
| +`sheet.html`, `table.html` and `comments.html` templates. I did not open |
| +`BRAINSTORM-GPT.md`; the brief wants two independent voices. |
| + |
| +## 1. Insert and delete a row or a column |
| + |
| +**What it is.** Right-click a row, or open Edit, and put a new empty row |
| +above it; every formula still points at the numbers it pointed at |
| +before. Delete works the same way, and a formula that referred to the |
| +deleted row shows `#REF!` rather than a wrong number. |
| + |
| +**Why it is first.** It is the most basic act in a spreadsheet after |
| +typing, and the app cannot do it: `grid.js` sets `allowInsertRow: |
| +false`, no route touches `Row.Position`, and `handle_open.go`'s binder |
| +comment says exactly why. What people do instead is retype a block one |
| +row down — which writes an input event per moved cell, detaches every |
| +comment (`comments.row_id`) from the data it was about, and leaves |
| +history describing a hundred edits nobody made on purpose. PRODUCT.md's |
| +job is "put numbers in a shared grid"; a grid you cannot make room in is |
| +a form. |
| + |
| +**What it costs.** This is §14's subsystem wearing a feature's name, and |
| +I am saying so: calc's key space rebuilt on stable ids. Today |
| +`calc.Ref` carries positions, `refKey` and `rangeCells` compute |
| +`CellKey{pos+1}` deep inside Eval, `rawText` and `cells.input_sealed` |
| +hold A1 text, and `events.payload_sealed` holds A1 text. Moving a row |
| +therefore needs one of two designs, and **Paul has to rule which**: |
| + |
| +- *(a)* rewrite the A1 text of every affected formula at insert time — |
| + Excel's mechanism. Cheap to start, but either the log grows by one |
| + event per rewritten formula or it does not and a restored old formula |
| + points at the wrong row. |
| +- *(b)* bind references to `(row_id, col_id)` at write time, persist the |
| + id-bound form, render A1 from the position map. This is what §4.2 |
| + already claims the design does. Insert becomes one `UPDATE` on |
| + `rows.position` and no graph change. Events written before the change |
| + stay A1 and are "coarse before the horizon", which §4.3's retention |
| + language already permits. |
| + |
| +I would argue for (b). Touches `calc/ref.go`, `eval.go`, `fn_lookup.go`, |
| +`graph.go`; `store/handle*.go`, `events.go`, `history.go`; |
| +`xlsx/export.go` (renders A1); `grid.js`, the Edit and Insert menus, and |
| +`table.html` for the scriptless twin. No SQL migration is forced; one |
| +additive column marking payload encoding is likely. It is the largest |
| +item here by a factor of three and needs a §1 ruling plus a §Review pass |
| +on the spec change before code. |
| + |
| +**What breaks.** A wrong binding silently repoints every formula — the |
| +exact failure §14 names — and nothing on screen says so. The one thing |
| +that would make me drop it: if the dogfood sheets turn out to be |
| +append-only (rows added at the bottom, never between), `EnsureRow` |
| +already covers that and the subsystem can wait for a second ask. |
| + |
| +**First slice.** Insert one row above / one column left, on the |
| +id-bound key space, from the Edit menu and from a form on the table |
| +view. Delete and `#REF!` second. No destructive sort at all (see below). |
| + |
| +## 2. The functions people's existing spreadsheets already use |
| + |
| +**What it is.** `=SUMIF(A:A,">100",B:B)`, `=COUNTIF`, `=YEAR(A2)`, |
| +`=EOMONTH`, `=ISBLANK`, `=TEXT` — the everyday tail. A workbook you |
| +import stops showing `#NAME?` where its author had totals. |
| + |
| +**Why it is second.** The engine registers 32 functions |
| +(`calc/registry.go` and `fn_*.go`: SUM through VLOOKUP, NOW, TODAY); |
| +spec §5 said "~120 to start"; the LibreOffice corpus has 60 entries over |
| +30 of them. An unknown name evaluates to `#NAME?` (`eval_call.go`), so |
| +`xlsx.Import` faithfully turns a real budget into a sheet of errors and |
| +the person blames the app, not the gap. This is the cheapest item on the |
| +list and has the best test story in the repo: `tools/gencorpus` produces |
| +vectors from LibreOffice headless (gate 4), so every function ships |
| +proven against a real spreadsheet rather than our reading of a spec. |
| + |
| +**What it costs.** `calc` only. New family files, self-registering; a |
| +shared criteria matcher for `">100"` and `"*x*"`; date functions ride |
| +the serial model `format/dates.go` already displays. Corpus regen. No |
| +migration, no UI, no ruling, no chrome. A few sessions. |
| + |
| +**What breaks.** A function that disagrees with LibreOffice on a corner |
| +case — the corpus catches it if the case is in the corpus, which is the |
| +discipline `volatile-excluded.txt` already models. I would drop it only |
| +if the event log showed the dogfood sheets were literal-only, and it |
| +does not. |
| + |
| +**First slice.** SUMIF, COUNTIF, AVERAGEIF, SUMIFS, COUNTIFS; DATE, |
| +YEAR, MONTH, DAY, DAYS, EOMONTH, WEEKDAY; ISBLANK, ISNUMBER, ISTEXT, |
| +ISERROR; TEXT, VALUE. About twenty. |
| + |
| +## 3. A second tab |
| + |
| +**What it is.** A `+` on the tab strip, a name you can change, and a |
| +strip you can switch with. Importing a three-worksheet file gives you |
| +three tabs, not one. |
| + |
| +**Why it is third.** It fires a written trigger — §14: "a second tab is |
| +reachable in the UI" — and the tab strip in `sheet.html` says in a |
| +comment that it deliberately has no `+` for exactly that reason. The |
| +`tabs` table (sealed name, position) exists; the API routes already |
| +carry `{tabID}`; `xlsx.Import` reads `GetSheetList()[0]` and stops. Real |
| +workbooks have tabs, and the import form's hint is an honest apology |
| +that should retire. |
| + |
| +**What it costs.** Medium, no migration, no ruling. The page routes |
| +(`/table`, `/history`, `/comments`, `/view`) use `SheetSummary.TabID` = |
| +first tab (`sheetsvc.go`); they need a tab parameter. Create and rename |
| +are forms, so the scriptless path is free. Import loops over |
| +worksheets. **Cross-tab references (`Sheet2!A1`) are not in this** and I |
| +would refuse to build them before pick 1: real `Row.ID`/`Col.ID` are |
| +globally unique, so an id-bound key space is already tab-agnostic and |
| +cross-tab refs become "one handle per sheet". Built on the position key |
| +space they would be a second binding to unpick later. |
| + |
| +**What breaks.** A tab you can create in the grid but not from the |
| +table view — principle 2. Drop it if none of the dogfooders' imported |
| +files has a second worksheet. |
| + |
| +**First slice.** `+`, rename, switch, and import of every worksheet. |
| +No reorder, no delete, no cross-tab refs. |
| + |
| +## Quietly unfinished: the grid shows A1:Z100 and says nothing |
| + |
| +`sheetsvc.go` sets `defaultWindowRows = 100`, `defaultWindowCols = 26`; |
| +`handlers_sheets.go:142` hands exactly those to the page; `grid.js` |
| +fetches exactly those; `snapshot` (`grid.go`) drops every cell outside |
| +them. Meanwhile `handleCellPost` accepts row 4,999, the table paginates |
| +to 500, import caps at 200,000 cells, the thumbnail (`preview.go`) draws |
| +the sheet's *true* shape, and export carries everything. Plan 2 item 5 |
| +promised the window would be "extendable by an explicit button"; there |
| +is no such button in `grid.js` or `sheet.html`. So an imported 300-row |
| +sheet opens showing 100 rows, with no line of copy anywhere saying rows |
| +101–300 exist. That is principle 1 broken by omission, and it matters |
| +more than tabs because it makes the shipped product wrong rather than |
| +merely smaller. Smallest fix: the page passes the tab's extent (from |
| +`GridRows()`, plus a buffer, clamped at `maxWindowRows`), the grid sizes |
| +to it, and a status line says "rows 1–500 of 1,240" with a "Show more" |
| +button as the menu equivalent. About sixty lines. Virtualise later. |
| + |
| +## Would not build: destructive sort |
| + |
| +§14 defers it behind the key-space subsystem. I would not build it even |
| +after pick 1 lands, and I think the trigger is wrong. §4.2 rules "values |
| +move, ids stay with positions", so a sort is a cell-content rewrite: |
| +500 rows by 10 columns is 5,000 sealed input events per sort in a log |
| +Litestream ships continuously; undo is own-edits, cell-level (§7), so |
| +undoing a sort is 5,000 undos or a new compound event kind; and |
| +comments anchor to `row_id`, so the note about Acme's row stays at |
| +position 7 while Acme's numbers move to 30 — the anchoring that stable |
| +ids were bought for, defeated by the sort semantics in the same |
| +section. The non-destructive view (`viewspec.go`) already gives people |
| +what they want and tells them the truth about it. The Google pull is |
| +"sort should stick"; it does. If Paul wants it anyway, the trigger |
| +should read "a compound event kind with its own undo", not "stable ids". |
| + |
| +## What I would cut from a longer list |
| + |
| +Comments visible in the grid (a per-cell count is shape, not content; |
| +worth an afternoon, not a slot), presence, freeze panes, find, undo for |
| +formatting, conditional formatting, charts. All real; none moves the |
| +dogfood job. |
diff --git a/docs/superpowers/brainstorms/2026-09-03-gpt.md b/docs/superpowers/brainstorms/2026-09-03-gpt.md| new file mode 100644 |
| index 0000000..b2d4166 |
| --- /dev/null |
| +++ b/docs/superpowers/brainstorms/2026-09-03-gpt.md |
| @@ -0,0 +1,45 @@ |
| +# The next three features for Oficina Sheets |
| + |
| +## 1. Multiple tabs in one sheet |
| + |
| +**What it is.** A person can add, name and switch between tabs inside one sheet, using the tab strip in the grid or ordinary links in the table view. XLSX import then brings in every worksheet instead of silently stopping after the first one. |
| + |
| +**Why it is first.** This is the largest remaining gap in the ordinary job PRODUCT.md names: putting real spreadsheet work into a shared grid and moving it in and out without losing structure. The data model already promises tabs, the UI already renders a one-item tab strip, and spec §14 gives a precise trigger: importing later worksheets starts when a second tab is reachable. That trigger has not merely become relevant; the shipped import feature now makes its absence visible whenever a workbook has more than one worksheet. |
| + |
| +**Cost.** This touches `internal/sheets/handlers_sheets.go` and new tab handlers, `templates/sheet.html`, `templates/table.html`, the sheet summary/query code, `store/`, `xlsx/import.go`, `xlsx/export.go`, and small chrome modules under `static/`. The existing `tabs` table, sealed tab name, tab position and tab-scoped cells mean it should need no migration. It is still large: permissions, URLs, import cleanup, history, comments, sort/filter and live streams must all remain tab-scoped. Paul should rule the destructive edges before they ship—especially deleting a non-empty tab and what happens to the last tab—but creating, naming and switching tabs does not reopen a settled decision. |
| + |
| +**What breaks if it is wrong.** A partial import can look complete while dropping worksheets, or one tab can leak another tab's cells through a route that checks the sheet but not the tab pairing. I would drop this from the list if the dogfood workbooks are consistently single-worksheet files; without observed multi-tab use, the import trigger has not fired in practice. |
| + |
| +**Build first.** Add and switch tabs through server-rendered links in both the grid and table views, with sealed names and the existing sheet-level grant check. Leave rename, reorder, delete and multi-worksheet import out of that first slice; a two-tab sheet that persists and is reachable is coherent, and it fires the import trigger safely. |
| + |
| +## 2. Formatting in the table view |
| + |
| +**What it is.** A person using the table view can select a cell or range and apply bold, italic, alignment, number format, fill, text colour, column width and row height. The controls are ordinary forms and work without JavaScript. |
| + |
| +**Why it is second.** PRODUCT.md principle 2 says the scriptless path is the product, and explicitly calls the table view unfinished when the grid has an affordance it lacks. Spec §14 records this exact debt and says its trigger is the next slice that touches the table view or Paul asking for it. The grid gained all these controls while `templates/table.html` still exposes only per-cell value editing, so this is a broken product principle rather than a checklist feature. |
| + |
| +**Cost.** The main work is in `templates/table.html`, `handlers_table.go`, `handlers_format.go`, `handlers_size.go`, view models, and their tests. The existing format and size storage is enough, so there is no migration. The JSON-only `POST .../format` route is close but not form-ready; the form path should share validation and application code with it rather than duplicate formatting rules. This is medium-sized and needs no new spec ruling unless the table UI introduces a new selection model. |
| + |
| +**What breaks if it is wrong.** The two editing surfaces can apply different formatting, an enormous submitted range can evade `maxFormatCells`, or a form can echo a user-authored number format into an error or log. I would drop it only if the table view's editing claim is deliberately narrowed in PRODUCT.md; under the current principle, leaving it out is not an option. |
| + |
| +**Build first.** Format one cell from its existing edit state, using a real form and the same allowlisted patch logic as `handleFormatPost`. Range formatting and row/column sizing can follow once one scriptless write round-trips and renders identically in both views. |
| + |
| +## 3. CSV import and export |
| + |
| +**What it is.** A person can upload a comma-separated values (CSV) file into one tab and download a tab as CSV. It carries cell data, not workbook-only features such as colours, comments or formulas with cross-tab references. |
| + |
| +**Why it is third.** CSV is the remaining false edge around the shipped interoperability story: PRODUCT.md now states plainly that no CSV code exists, while spec §13 still places XLSX/CSV before sharing and collaboration. For a small dogfooding team, moving a plain table to or from another tool is more likely than authoring a chart, and it can reuse the scriptless import form and download pattern already proven by XLSX. |
| + |
| +**Cost.** Add an `internal/sheets/csv` package, extend `handlers_import.go` and `importupload.go`, add a CSV export handler and routes in `app.go`, and update the import and File menu templates. It can write through the existing sealed cell store and needs no migration. This is medium-sized rather than tiny because encoding, line endings, delimiters, cell limits and safe download headers need fixtures. Paul should rule whether an imported leading `=` becomes a formula or literal text, and whether export writes raw inputs or displayed values; either choice affects round trips and spreadsheet-injection risk. |
| + |
| +**What breaks if it is wrong.** A CSV can open with shifted columns, silently change formulas into values, or turn literal text into executable spreadsheet input. I would drop it if nobody in the current team exchanges CSV files; PRODUCT.md rejects building for hypothetical users. |
| + |
| +**Build first.** Export the current tab as UTF-8 CSV with a documented choice of raw or displayed values. That is a complete, reversible read path and forces the semantic ruling before upload can mutate a sheet. |
| + |
| +## Shipped but quietly unfinished |
| + |
| +**Sort and filter do not govern the grid.** `viewspec.go` persists a tab-wide view and `templates/table.html` tells the user it changes what everybody sees, but `handlers_grid.go` calls `snapshot` without loading or applying that view. `templates/sheet.html` even notes that the grid does not apply it. Two people can therefore open the same tab and see different row orders depending on which shipped view they use. Fixing that matters more than CSV: either make the grid paint the same non-destructive view, including correct edit coordinates and live updates, or change the feature to an explicitly personal table-only view. The latter changes the current shared-view semantics and needs Paul to rule it. |
| + |
| +## Do not build: charts |
| + |
| +Charts are the trap. Spec §13 names them as a subsystem and the first slip candidate, not the natural next checkbox. Here they would add sealed chart titles and labels, stable range binding, a second renderer, accessible non-colour-only descriptions, scriptless presentation, XLSX round-trip rules and likely print/export pressure before the team has shown a need for any of it. A spreadsheet without charts remains coherent; a spreadsheet that drops worksheets, denies scriptless users formatting, or shows two incompatible sorted views does not. |
diff --git a/docs/superpowers/reviews/2026-09-03-next-three-brief.md b/docs/superpowers/reviews/2026-09-03-next-three-brief.md| new file mode 100644 |
| index 0000000..ff27ea6 |
| --- /dev/null |
| +++ b/docs/superpowers/reviews/2026-09-03-next-three-brief.md |
| @@ -0,0 +1,75 @@ |
| +# Review brief — the "next three" slice spec |
| + |
| +You are carrying out the **§Review pass** that `AGENTS.md` requires: |
| + |
| +> Every spec and every branch is reviewed by the highest-capability model |
| +> available before it is approved or merged. Not the model that happened |
| +> to write it. |
| + |
| +You did not write this spec. Attack it. |
| + |
| +## Read, in this order |
| + |
| +1. `AGENTS.md` — binding. Quote it rather than paraphrasing it; a |
| + paraphrase of a binding rule has produced a confident false positive |
| + here before. |
| +2. `PRODUCT.md` |
| +3. `docs/superpowers/specs/2026-08-30-sheets-design.md` — the design. |
| + §1 is the settled-decisions table, §4.2 is the row/column identity |
| + section, §5 is the formula engine, §13 is sequencing, §14 is |
| + "Deliberately not built yet" with a written trigger per row. |
| +4. `docs/superpowers/specs/2026-09-03-sheets-next-three.md` — **the spec |
| + under review.** |
| +5. The code it describes: `internal/sheets/calc/`, `handlers_grid.go`, |
| + `handlers_table.go`, `handlers_format.go`, `handlers_sheets.go`, |
| + `viewspec.go`, `store/`, `templates/table.html`, `templates/sheet.html`. |
| + |
| +## What to attack |
| + |
| +Verify every factual claim against the code and the design, and say |
| +which are wrong. In particular: |
| + |
| +- **§2.1's claim that 32 functions are registered**, and that the named |
| + missing ones are genuinely missing. Count them yourself. |
| +- **§3.1's claim that the cell API is already tab-scoped and that |
| + `openTab` checks the tab against the sheet.** If a route bypasses |
| + `openTab`, name it — that is a live authorisation bug, not a spec |
| + defect. |
| +- **§3.3's claim that no migration is needed.** Check the `tabs` table |
| + and its columns against what add/name/switch would write. |
| +- **§4.1's claim that the table view has no formatting at all**, and |
| + §4.2's claim that `POST .../format` is form-ready. |
| +- **§5.3's claim that `loadView` is called from exactly one place.** |
| +- **§5's closing bug claim about the 100×26 window.** Is a cell past row |
| + 100 genuinely invisible on the grid page, and genuinely reachable? |
| + |
| +Then judge the spec itself: |
| + |
| +- Does anything here **relitigate a settled §1 ruling**, or contradict |
| + §4.2, §5 or §14? |
| +- Is any of the three a **subsystem wearing a feature's name**? §13 |
| + caught three of those already; the spec claims tabs is moderate |
| + because the plumbing exists. Test that claim. |
| +- Are the **triggers** in §6 checkable, or are they wishes? A deferred |
| + thing without a real trigger is a thing forgotten. |
| +- Is §1's decision E (one format route, two callers) actually achievable |
| + given how `handleFormatPost` decodes today, or does it need a shape |
| + the spec has not thought about? |
| +- **What is missing.** Name anything that should be in the top three and |
| + is not, and anything in §5's "Paul's to rule" list that does not in |
| + fact need a ruling. |
| +- Is the sealing rule respected throughout? A tab name is content |
| + (§2.1). Does anything in this spec put one in a log, an error string, |
| + a URL or a metric label? |
| + |
| +## How to answer |
| + |
| +Write to `./REVIEW-OUT.md`: |
| + |
| +- A verdict line: **APPROVE**, **APPROVE WITH CHANGES**, or **REQUEST |
| + CHANGES**. |
| +- Findings, most serious first. For each: the claim, whether it is true, |
| + the evidence (file and line), and what to do. |
| +- Say plainly which of the three you would cut, if any, and why. |
| + |
| +Be specific and cite paths. Do not be agreeable. |
diff --git a/docs/superpowers/reviews/2026-09-03-next-three-r1.md b/docs/superpowers/reviews/2026-09-03-next-three-r1.md| new file mode 100644 |
| index 0000000..a00d8a7 |
| --- /dev/null |
| +++ b/docs/superpowers/reviews/2026-09-03-next-three-r1.md |
| @@ -0,0 +1,109 @@ |
| +# 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. |
diff --git a/docs/superpowers/specs/2026-09-03-sheets-next-three.md b/docs/superpowers/specs/2026-09-03-sheets-next-three.md| new file mode 100644 |
| index 0000000..67d7163 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/2026-09-03-sheets-next-three.md |
| @@ -0,0 +1,341 @@ |
| +# Sheets — the next three |
| + |
| +**Status:** revision 2, 2026-09-03. |
| +**§Review:** revision 1 reviewed by **Codex / GPT-5.6-sol**, a different |
| +family from the author — verdict **REQUEST CHANGES**, eleven findings. |
| +Every one is answered in §7, including the four where revision 1 was |
| +simply wrong. Revision 2 needs a fresh pass before it is approved. |
| + |
| +Written after an independent brainstorm by two models |
| +(`docs/superpowers/brainstorms/2026-09-03-{gpt,fable}.md`), briefed |
| +identically and separately and told not to hedge toward consensus. They |
| +agreed on exactly one item and disagreed on everything else. |
| + |
| +This is a slice spec, not a redesign. Everything here sits under the |
| +2026-08-30 design; where the two disagree, that one wins and this one is |
| +wrong. |
| + |
| +--- |
| + |
| +## 0. What changed in revision 2 |
| + |
| +The review was right about the two things that mattered most, and both |
| +were the same mistake in different places — **taking a claim from the |
| +brainstorm and writing it down without checking it against the code.** |
| + |
| +- Revision 1 asked Paul to rule on insert/delete semantics. **§4.2 |
| + already rules on them** ("Insert / delete row or col — No rewrite. Ids |
| + are stable. **This is the case stable ids buy.**"). Asking again is |
| + exactly the drive-by relitigation `AGENTS.md` forbids. Removed. |
| +- Revision 1 called tabs "moderate" because the storage is tab-keyed. |
| + The storage is; **the product is not.** Twenty-one HTML routes exist |
| + and not one carries a tab id: every screen resolves the first tab |
| + through `SheetSummary.TabID` or `openTabFor`. Re-priced in §5 as the |
| + subsystem it is, and moved out of this slice. |
| + |
| +The three that ship are now §2, §3 and §4. Tabs is §5, specced and |
| +priced and **not built**, because a subsystem shipped in a slice is how |
| +§13's warning gets ignored for the fourth time. |
| + |
| +--- |
| + |
| +## 1. Settled by this document |
| + |
| +| # | Decision | Rejected alternative | Who / when | |
| +|---|---|---|---| |
| +| A | **The grid's window is sized to the sheet, not to a constant.** Default as a floor, the cells route's own maximum as the ceiling, and words when the sheet runs past it. | Copy alone ("rows past 100 are in the table view"), which discloses the problem without fixing it | This session, 2026-09-03 | |
| +| B | **New functions are a judgment-based starter set, and the spec names them.** No claim is made that they are the most common in real workbooks, because no inventory of real workbooks exists. | Claiming measured coverage without measuring; leaving the list to the implementation | This session, 2026-09-03 — revision 1 claimed both and the review was right to reject it | |
| +| C | **Every new function's expected values come from LibreOffice headless**, through `tools/gencorpus`, never from our reading of a spec. | Hand-written expectations | Restates 2026-08-30 §5 | |
| +| D | **One format route, two decoders, converging on one `formatPatch` before any validation runs.** | A second form-only endpoint beside the JSON one | This session, 2026-09-03 | |
| +| E | **A form expresses "off" explicitly, never by omission.** | Reading an absent checkbox as `false` | This session, 2026-09-03 — see §4.2 | |
| +| F | **Tabs are specced and not built in this slice.** | Shipping add/switch against twenty-one first-tab routes | This session, 2026-09-03 | |
| + |
| +--- |
| + |
| +## 2. The grid's window reaches the sheet — **BUILT** |
| + |
| +### 2.1 The bug |
| + |
| +The grid page opened a constant 100×26 window. The cell route accepts a |
| +write below row 5,000 (`inCellRange`, ten times the request maximum), |
| +the table view lists whatever rows exist, and the export carries all of |
| +them. So a cell at row 151 returned 200, appeared in the table view and |
| +in the .xlsx, and had no row on the grid to scroll to — with no copy |
| +anywhere admitting a window existed. |
| + |
| +Measured in Chromium, 2026-09-03. It is **invisibility, not loss**: the |
| +first reading of this bug was that the cell was unreachable, and that |
| +was wrong — the table view lists populated rows and the API returns it |
| +for a wider window. Recorded because the difference decides its |
| +severity, and because the corrected version is the one that goes in a |
| +commit message. |
| + |
| +### 2.2 What shipped |
| + |
| +`extent()` reports one past the furthest cell that exists. |
| +`windowFor()` adds a screenful of room to keep typing into, floors at |
| +the default so a one-cell sheet still opens a workable grid, and |
| +ceilings at the route's own maximum — a window wider than the server |
| +will serve paints rows that scroll and never fill. |
| + |
| +Past the ceiling, `data-window-truncated` on the surface and a sentence |
| +in the status line. `note.js` is the status line's own module: the |
| +300-line cap caught it being written inside `clipboard.js`, which is how |
| +a shared thing acquires two copies and two timers that clear each other. |
| + |
| +Four tests, each seen failing first, including the one that catches |
| +`clamp` being misused for this — `clamp`'s `def` is what a request of |
| +**zero** gets, so a sheet with one cell asked for a one-row grid and |
| +clamp returned one row. |
| + |
| +### 2.3 Why it is first |
| + |
| +The review argued this belongs ahead of new features and it is right: |
| +the app's principal screen could not show data the app itself had |
| +accepted, and every other item here is an addition rather than a |
| +correction. |
| + |
| +--- |
| + |
| +## 3. Function coverage |
| + |
| +### 3.1 What is there |
| + |
| +The engine registers **32** callable names across three dispatch tables |
| +— 22 ordinary `Register` calls, 2 clock registrations (`NOW`, `TODAY`), |
| +and 8 node registrations (the aggregates and lookups): |
| + |
| +``` |
| +ABS AND AVERAGE CONCAT COUNT COUNTA IF IFERROR IFS INDEX LEFT LEN LOWER |
| +MATCH MAX MID MIN MOD NOT NOW OR POWER RIGHT ROUND ROUNDDOWN ROUNDUP SQRT |
| +SUM TODAY TRIM UPPER VLOOKUP |
| +``` |
| + |
| +§5 of the design says "~120 functions to start". `xlsx.Import` |
| +preserves a formula's input and writes it through our engine, so a |
| +workbook using a name we do not register imports faithfully and then |
| +shows `#NAME?` in every cell that used one. |
| + |
| +### 3.2 What is claimed, and what is not |
| + |
| +**Claimed:** the gap is real, the functions below are genuinely absent, |
| +and an imported workbook using one of them evaluates to `#NAME?`. All |
| +three verified against the registration sites and `xlsx/import.go`. |
| + |
| +**Not claimed:** that these are the most common functions in real |
| +workbooks. Revision 1 said conditional aggregates were "the largest |
| +cause of `#NAME?` in an imported workbook" and there is no evidence for |
| +it — `tools/gencorpus` holds a hand-maintained case list, not an |
| +inventory of anything anybody imported. Decision B: this is a |
| +judgment-based starter set, named here rather than left to the |
| +implementation, and the honest ranking waits for §6's inventory. |
| + |
| +### 3.3 The list, named here and not deferred |
| + |
| +- **Conditional aggregates** — `SUMIF`, `COUNTIF`, `AVERAGEIF`, |
| + `SUMIFS`, `COUNTIFS`, `AVERAGEIFS`. |
| +- **Date parts** — `YEAR`, `MONTH`, `DAY`, `DATE`, `EOMONTH`. |
| +- **Predicates** — `ISBLANK`, `ISNUMBER`, `ISTEXT`, `ISERROR`. |
| +- **Text and coercion** — `VALUE`, `SUBSTITUTE`, `FIND`, `SEARCH`. |
| + |
| +`TEXT` is **out**, and this is a change from revision 1. Its argument is |
| +a number-format grammar; shipping a `TEXT` that handles four patterns |
| +and silently mangles the fifth is the quiet wrong PRODUCT.md forbids, |
| +and §6 gives it a trigger with a threshold rather than a wish. If it |
| +ever ships it must call `internal/sheets/format`, never grow a second |
| +formatter — two implementations of one truth is the bug class ruling 2 |
| +exists to prevent. |
| + |
| +`DATEDIF` is out for the same reason: its unit argument has documented |
| +Excel quirks the corpus must settle before we copy them. |
| + |
| +### 3.4 Cost and risk |
| + |
| +`internal/sheets/calc` only. No migration, no route, no template, no |
| +client code, no ruling. `Register` panics on a duplicate at init. |
| + |
| +The risk is agreeing with ourselves: a function tested against |
| +expectations we wrote matches our reading of Excel, not Excel. Decision |
| +C is the whole mitigation, and a function whose corpus vectors cannot be |
| +generated does not ship. |
| + |
| +### 3.5 First slice |
| + |
| +The six conditional aggregates. A sheet that computes `SUMIF` and does |
| +not know `EOMONTH` is coherent. |
| + |
| +--- |
| + |
| +## 4. Formatting in the table view |
| + |
| +### 4.1 The gap, stated exactly |
| + |
| +Revision 1 said the table view has "nothing", and the review was right |
| +that this is false. What is true: |
| + |
| +- The table **already** computes each display string through the cell's |
| + stored number format, and right-aligns numeric computed values. |
| +- It **already** surfaces imported fill/ink pairs that fail the contrast |
| + floor (§11.1). |
| +- It does **not** paint bold, italic, fill, ink or explicit alignment. |
| +- It offers **no control** for any format or size. The only per-cell |
| + form edits `raw`. |
| + |
| +§14 records this as debt in its own words — the scriptless path is the |
| +product, not the fallback, "and an affordance in the grid and not in the |
| +table view means the table view is unfinished" — with the trigger "the |
| +next slice that touches the table view". This slice touches it. |
| + |
| +**Scope: controls and visual parity, both.** A control that sets bold on |
| +a screen that cannot show bold is half a feature and the worse half. |
| + |
| +### 4.2 The form protocol, designed rather than assumed |
| + |
| +Revision 1 said `POST .../format` is "scriptless-ready" and the work is |
| +"form decoding". The review is right that this is false as written: |
| +`handleFormatPost` decodes JSON only and always answers with a JSON grid |
| +payload. Three things have to be designed, not assumed. |
| + |
| +**Content-type dispatch.** One route, two decoders, converging on one |
| +`formatPatch` *before* `validate`, `normalize` or `maxFormatCells` runs. |
| +JSON callers keep the JSON response; form callers get **303** to a |
| +server-built return URL. The target is derived by the server from the |
| +sheet and page, never taken from the request — a caller-supplied return |
| +is an open redirect, and this repo has already shipped one fix in that |
| +family (`fix/safe-return-backslash`). |
| + |
| +**Decision E: "off" is explicit.** `formatPatch` distinguishes absent |
| +("leave unchanged") from `false` ("clear it") by pointer presence, and |
| +an unchecked checkbox is simply absent from a form submission — so a |
| +naive form can never turn bold *off*. Each toggle posts a tri-state |
| +value (`set` / `clear` / omitted) rather than a checkbox. This is the |
| +one place the two decoders' inputs genuinely differ, and it is why they |
| +must converge on the patch and not on the wire format. |
| + |
| +**Tab addressing.** The format route is tab-addressed; the table page |
| +and its redirects are sheet-addressed and resolve the first tab. Until |
| +§5 lands, the form's action is built server-side from the same first-tab |
| +resolution the page already used, so this slice adds no new assumption |
| +— it inherits one, and §5 removes it everywhere at once. |
| + |
| +### 4.3 Cost and risk |
| + |
| +`templates/table.html`, `handlers_table.go`, and the decoder split in |
| +`handlers_format.go`. No migration. No ruling. |
| + |
| +The risk is the two surfaces drifting. Decision D is load-bearing rather |
| +than tidy-minded: if the form path reimplements the allowlist, the drift |
| +is invisible until somebody formats in one view and reads in the other. |
| +Second risk: a submitted rectangle large enough to evade |
| +`maxFormatCells` — the bound applies after both decoders converge, never |
| +in either decoder. |
| + |
| +### 4.4 First slice |
| + |
| +One cell, one patch, one form, round-tripping identically in both views, |
| +with the table painting what it sets. Range formatting and sizing |
| +follow. |
| + |
| +--- |
| + |
| +## 5. Tabs — specced, priced, **not built in this slice** |
| + |
| +### 5.1 Why it moved out |
| + |
| +Both models put tabs in their top three and it was revision 1's first |
| +pick. The review re-priced it and the re-pricing holds. |
| + |
| +The **storage** is tab-keyed: the `tabs` table exists, cells, history, |
| +comments, saved views and the live hub are all keyed on tab id, and the |
| +cell/format/size API routes are tab-addressed with `openTab` checking |
| +the tab against the sheet before checking the grant. |
| + |
| +The **product** is not. Twenty-one HTML routes exist and **none carries |
| +a tab id**. Every screen resolves the first tab — `SheetSummary.TabID` |
| +is documented as "the first tab", `summarize` selects by position, and |
| +`openTabFor` opens that one id. The first-tab assumption is baked into |
| +the grid page, the table view's read and write, comments, history and |
| +restore, export, import, saved views and the link view. |
| + |
| +So tabs is a page, navigation, redirect, export, history, comment and |
| +authorisation change across the whole app. That is §13's "subsystem |
| +wearing a feature's name", and shipping it inside a slice with two other |
| +things is how that warning gets ignored again. |
| + |
| +### 5.2 What it needs, when it is built |
| + |
| +- **A migration.** Revision 1 promised none. The `tabs` table has only a |
| + plain index on `sheet_id` — no uniqueness on `(sheet_id, position)` — |
| + while `rows` in the very same migration carries |
| + `UNIQUE INDEX (tab_id, position)`. The asymmetry is the tell. Once add |
| + and reorder exist, duplicate positions make document order fall out of |
| + an id tie-break. Additive unique index, plus an allocation rule. |
| +- **Tab-addressed HTML routes**, and a decision per screen about whether |
| + it becomes tab-addressed or stays sheet-addressed with a documented |
| + first-tab default. Twenty-one of them. |
| +- **Sealed names throughout** (§2.1). Explicitly: add and rename errors |
| + are fixed text or codes and never wrap an error carrying the submitted |
| + name; URLs carry ids only; raw-SQLite and bucket coverage extends to |
| + renamed and additional tab names. |
| +- **Delete is a separate question** and stays in §6. |
| + |
| +### 5.3 The authorisation claim, narrowed |
| + |
| +Revision 1 said `openTab` checks the pairing "on every request". True |
| +for the cell, format and size routes. The SSE stream does **not** go |
| +through `openTab` — it repeats the pair check and the read authorisation |
| +itself, so it is not a bypass today, but "every request" is not |
| +mechanically true and a route-coverage test is what would make it so. |
| + |
| +--- |
| + |
| +## 6. Open — Paul's to rule |
| + |
| +Revision 1 listed four. Two of them were not decisions at all and the |
| +review was right to say so. |
| + |
| +1. **Deleting a tab.** What happens to a non-empty one, and to the last |
| + one. Genuinely open. |
| + |
| +2. **CSV import and export.** Two semantic rulings before a line is |
| + written: does an imported leading `=` become a formula or literal |
| + text (the spreadsheet-injection question), and does export write raw |
| + inputs or displayed values. |
| + |
| +3. **A workbook inventory, or not.** §3.2 declines to claim measured |
| + coverage because nothing measures it. Building a content-safe |
| + inventory — function *names* counted across imported workbooks, never |
| + a formula or a cell reaching a log — is a small piece of work that |
| + would make the next function slice evidence-led. It is a question |
| + because it touches sealed content handling, which is not mine to |
| + extend unasked. |
| + |
| +### Removed from this list, with reasons |
| + |
| +- **Insert and delete a row or a column.** §4.2 rules it: no rewrite, |
| + ids are stable, "this is the case stable ids buy". There is no product |
| + choice left — there is unbuilt subsystem work, because `eval.go`'s |
| + `refKey` still maps positions to synthetic ids and its own comment |
| + leaves stable-id binding to later. That is an implementation gap |
| + against a settled design, and §14 already names its trigger. |
| +- **The grid ignoring the saved sort and filter.** Not a ruling. The |
| + shared contract is already shipped and already stated on screen — |
| + the table view tells you the view changes what everybody sees. Making |
| + it personal would need new per-person storage and would contradict |
| + what users have been told. So the grid not applying it is a **bug**, |
| + and it should be fixed as one. (Revision 1 also said `loadView` is |
| + called "from nowhere else"; it has one non-test caller and three test |
| + call sites. The narrower statement is the true one.) |
| + |
| +--- |
| + |
| +## 7. Deliberately not built yet |
| + |
| +| Thing | Trigger | |
| +|---|---| |
| +| Tabs (§5) | Its own slice, with the migration and the twenty-one route decisions priced in. Nothing else should ride along with it. | |
| +| Multi-worksheet import | §14's existing trigger — "a second tab is reachable in the UI" — fires when §5 ships, and not before. Two subsystems in one slice is the thing §13 warns about. | |
| +| Cross-tab references (`Sheet2!A1`) | **Corrected:** revision 1 said an id-keyed graph is already tab-agnostic. It is not — `eval.go` builds `CellKey{rowPos+1, colPos+1}` from POSITIONS, which is §14's existing warning. Trigger: the key space rebuilt on stable ids. | |
| +| Per-tab permissions | **Corrected:** "somebody asks" is observable but not sufficient — one request would force a second access model beside the grants join. Trigger: a confirmed case needing different access to two tabs of one sheet, written down with who needed it. | |
| +| `TEXT`, `DATEDIF` | A corpus run covering the format patterns and unit arguments actually present in imported workbooks, at a stated coverage threshold — which needs §6's inventory first. Circular without it, which is what revision 1's version was. | |
| +| The grid applying the saved view | A bug, not a deferral. Listed here only so it is not lost: it belongs in the next fix slice. | |