amadan

oficina / sheets Public

Test workbook import completeness before tab readiness

Paul Campbell pushed by paul@keymail.dev 7504fdc730d27dd7162fcc02ad22b9f5c81cfc9c parent 4aea8f4
5 files changed, +137 −0
  • docs/superpowers/plans/2026-09-07-workbook-import-fidelity.md +35 −0
  • internal/sheets/workbook_import_readiness_test.go +102 −0
  • internal/testdata/workbook-regressions/cross-reference-first.xlsx binary
  • internal/testdata/workbook-regressions/cross-reference-later.xlsx binary
  • internal/testdata/workbook-regressions/two-worksheets.xlsx binary

CI failed — run details

diff --git a/docs/superpowers/plans/2026-09-07-workbook-import-fidelity.md b/docs/superpowers/plans/2026-09-07-workbook-import-fidelity.md
new file mode 100644
index 0000000..6bfb9aa
--- /dev/null
+++ b/docs/superpowers/plans/2026-09-07-workbook-import-fidelity.md
@@ -0,0 +1,35 @@
+# Workbook import completeness: review draft
+
+Status: LLM-only implementation contract, unapproved; no production implementation. Baseline origin/main 4aea8f47153b907e6d8903df174a0b10d502922a. Branch test/xlsx-workbook-fidelity, Sheets discussion 23. Depends on workbook-tabs discussion 18, whose design remains unapproved and whose source branch currently adds no tab implementation. Do not merge the red regression candidate or deploy it.
+
+## Exact evidence and ownership
+
+POST /sheets/import accepts upload bytes and creates a hidden workbook through createFillingSheet. runImport opens its first tab, calls xlsx.Import, then markFilled clears FillingSince. Import selects GetSheetList()[0]; later worksheets are never read. GET /sheets/{sheetID}/export.xlsx opens SheetSummary.TabID and calls xlsx.Export; that writer creates a new one-worksheet file with its default name. Whole-workbook export belongs to discussion18, not this follow-up.
+
+Three tiny synthetic XLSX fixtures contain Summary and Inputs, with distinct numbers at A1. One has no formulas; two place a cached cross-tab formula at B1 on either the first or second worksheet. They were generated with the repository's pinned excelize, including its normal workbook metadata/styles. The real runImport lifecycle and real authenticated HTTP export demonstrate:
+
+- Literal two-worksheet import returns success with one stored tab; HTTP export returns one worksheet.
+- First-worksheet cross-tab formula fails at B1 and leaves the workbook hidden/deleted, which is the correct existing unsupported-feature behavior.
+- The later-worksheet formula is never parsed; import reports success and publishes the partial workbook.
+
+This is a documented first-worksheet limitation today, not a claim that current UI promises all worksheets: the settled spec explicitly defers later worksheets until a second tab is reachable. That trigger is the readiness dependency this draft addresses. Importing all worksheets into an unreachable UI is not an authorized shortcut. The current formula engine keys references by row/column only and has no worksheet grammar; tabs alone cannot make cross-tab formulas work.
+
+## Bounded adjacent implementation after review and reachable tabs
+
+1. Add a workbook-level import entry point in xlsx, leaving the existing single-tab converter as an explicit compatibility helper. Open the XLSX archive once using the existing decompression bounds. Enumerate every worksheet in workbook order, including empty worksheets, before any visible publication. Preserve names exactly through the reviewed tab-name validator/sealer; do not invent fallback or lossy rename behavior.
+2. Reuse discussion18's reviewed tab-count and workbook-cell limits when finalized. Keep the existing per-tab cell cap and compressed-upload/decompression limits. Count the whole workbook before writing, so one oversized later worksheet cannot produce success for an earlier one. Do not silently truncate or introduce a different cap in this draft.
+3. Keep one hidden workbook and its creator grant throughout filling. Create/seal tab metadata under an ordinary scoped transaction, then release it before opening any cell Handle: cell import uses the existing one-connection store path and cannot run inside an outer writer transaction. Import one tab handle at a time and release it before the next. Store every cell, formula, format and size through the existing sealed methods, never in a plaintext staging file.
+4. Parse formulas through the existing calc path on every worksheet. Unsupported cross-tab syntax is a workbook failure; never flatten its cached value, drop the cell/tab, rewrite the reference to the current tab or publish a partial workbook. Cross-tab evaluation requires a separate reviewed grammar/reference/dependency design. This slice does not implement it.
+5. Preserve runImport's hide-until-complete contract. On any tab-name, cell, formula, context, size or storage failure, keep the workbook hidden and use existing discard/error redaction. Clear FillingSince only after every worksheet completes and the workbook is still live. Check caller cancellation between worksheets and while processing bounded cell batches; publication must not race a cancelled/discarded import into visibility.
+6. Keep errors to existing codes and cell coordinates. No source worksheet names, formula text, cell values or uploaded file names in errors/progress/logs. Reuse existing import failure wording. Any UI statement about import scope is a separate reviewed copy dependency before shipping; no UI language is included here.
+7. Require the reviewed whole-workbook exporter from discussion18 for round-trip acceptance. Compare input/output worksheet count, exact names/order, distinct contents, formula text for supported same-tab formulas, formats and sizes. Re-import the exported file through the workbook importer and compare all tabs, not just snapshots of the first tab.
+
+## Scope corrections and review questions
+
+Current .xlsm upload handling accepts the extension and ignores macros; there is no verified general external-link/macro refusal policy to preserve. Discussion18's shorthand about existing unsupported-feature refusal must not be treated as evidence of one. This draft does not change that policy or claim broad Excel fidelity.
+
+No tab deletion, cross-tab formula implementation, formula rewriting, row/column identity redesign, new UI, new Estilo component, or changes to frozen reconciliation/tabs candidates. This import design needs independent Fable approval before implementation. Ask the reviewer to assess all-or-hidden failure behavior, empty worksheets, bounds before publication, compatible wrapper ownership, and formula rejection across later worksheets. Full release remains gated on reviewed tabs, exporter and importer together; a tab strip alone is not migration readiness.
+
+## Verification candidate
+
+internal/sheets/workbook_import_readiness_test.go exercises actual runImport and the authenticated export route with fixtures in internal/testdata/workbook-regressions. Current expected red cases are one retained tab/worksheet and later unsupported-formula publication; the first-tab refusal control is green and checks the failure coordinate. No implementation is hidden behind these tests. Root owns the Fable allocation and all integrations/deployments.
diff --git a/internal/sheets/workbook_import_readiness_test.go b/internal/sheets/workbook_import_readiness_test.go
new file mode 100644
index 0000000..d07686e
--- /dev/null
+++ b/internal/sheets/workbook_import_readiness_test.go
@@ -0,0 +1,102 @@
+package sheets
+
+import (
+ "archive/zip"
+ "bytes"
+ "context"
+ "encoding/xml"
+ "net/http"
+ "net/http/httptest"
+ "os"
+ "strings"
+ "testing"
+
+ "amadan.net/oficina/sheets/internal/sheets/store"
+)
+
+func workbookReadinessFixture(t *testing.T, name string) []byte {
+ t.Helper()
+ data, err := os.ReadFile("../testdata/workbook-regressions/" + name + ".xlsx")
+ if err != nil {
+ t.Fatal(err)
+ }
+ return data
+}
+
+func TestWorkbookImportPublishesAllWorksheets(t *testing.T) {
+ app, cookies := signIn(t)
+ s, err := app.createFillingSheet("fixture", signedInMember(t, app))
+ if err != nil {
+ t.Fatal(err)
+ }
+ if err := app.runImport(context.Background(), s, workbookReadinessFixture(t, "two-worksheets"), func(string) {}); err != nil {
+ t.Fatal(err)
+ }
+ var tabs []store.Tab
+ if err := app.db.Where("sheet_id = ?", s.ID).Order("position, id").Find(&tabs).Error; err != nil {
+ t.Fatal(err)
+ }
+ if len(tabs) != 2 {
+ t.Errorf("successful two-worksheet import retained %d tabs, want 2", len(tabs))
+ }
+ rec := do(t, app, as(httptest.NewRequest(http.MethodGet, "/sheets/"+itoaPos(int(s.ID))+"/export.xlsx", nil), cookies))
+ if rec.Code != http.StatusOK {
+ t.Fatalf("export status %d", rec.Code)
+ }
+ archive, err := zip.NewReader(bytes.NewReader(rec.Body.Bytes()), int64(rec.Body.Len()))
+ if err != nil {
+ t.Fatal(err)
+ }
+ for _, file := range archive.File {
+ if file.Name != "xl/workbook.xml" {
+ continue
+ }
+ reader, err := file.Open()
+ if err != nil {
+ t.Fatal(err)
+ }
+ var book struct {
+ Sheets []struct {
+ Name string `xml:"name,attr"`
+ } `xml:"sheets>sheet"`
+ }
+ err = xml.NewDecoder(reader).Decode(&book)
+ reader.Close()
+ if err != nil {
+ t.Fatal(err)
+ }
+ if len(book.Sheets) != 2 {
+ t.Fatalf("export retained %d worksheets, want 2", len(book.Sheets))
+ }
+ if book.Sheets[0].Name != "Summary" || book.Sheets[1].Name != "Inputs" {
+ t.Fatal("worksheet names or order changed")
+ }
+ return
+ }
+ t.Fatal("export has no workbook manifest")
+}
+
+func TestUnsupportedCrossWorksheetFormulaNeverPublishesPartialWorkbook(t *testing.T) {
+ for _, location := range []string{"first", "later"} {
+ t.Run(location, func(t *testing.T) {
+ app := newTestApp(t)
+ s, err := app.createFillingSheet("fixture", testMember(t, app))
+ if err != nil {
+ t.Fatal(err)
+ }
+ err = app.runImport(context.Background(), s, workbookReadinessFixture(t, "cross-reference-"+location), func(string) {})
+ if err == nil {
+ t.Error("unsupported cross-worksheet formula reported successful import")
+ } else if !strings.Contains(err.Error(), "B1") {
+ t.Error("fixture failed before the unsupported formula cell")
+ }
+ var stored store.Sheet
+ if e := app.db.First(&stored, s.ID).Error; e != nil {
+ t.Fatal(e)
+ }
+ if stored.DeletedAt == nil && stored.FillingSince == nil {
+ t.Error("partial workbook became visible")
+ }
+ })
+ }
+}
diff --git a/internal/testdata/workbook-regressions/cross-reference-first.xlsx b/internal/testdata/workbook-regressions/cross-reference-first.xlsx
new file mode 100644
index 0000000..877c417
Binary files /dev/null and b/internal/testdata/workbook-regressions/cross-reference-first.xlsx differ
diff --git a/internal/testdata/workbook-regressions/cross-reference-later.xlsx b/internal/testdata/workbook-regressions/cross-reference-later.xlsx
new file mode 100644
index 0000000..69e21a3
Binary files /dev/null and b/internal/testdata/workbook-regressions/cross-reference-later.xlsx differ
diff --git a/internal/testdata/workbook-regressions/two-worksheets.xlsx b/internal/testdata/workbook-regressions/two-worksheets.xlsx
new file mode 100644
index 0000000..45a57cd
Binary files /dev/null and b/internal/testdata/workbook-regressions/two-worksheets.xlsx differ