amadan

oficina / memoria Public

fix: a fifth review, and the fix that made things worse

Fable reviewed spec draft 3 against the built code. 22 findings, one
blocker, and the blocker was mine: adding person_ref to S3 in draft 3
broke the path that fills the corpus.

There is no person on the ingest path — memoria pulls on its own wake,
and the warm-up, rebuild and regeneration have no asker either. Worse, a
per-person S3 succeeds for individually-granted rows, because a sibling's
grants join returns them, which would pull into memoria exactly the
content §6.4 forbids and §3.3 claims it cannot hold.

The error was collapsing two rules decision 3 keeps apart. Custody bounds
what memoria may hold, team-visible, no person. Authorization decides what
a person may read, per asker, at render time. Draft 2's S3 had the right
signature and a false comment; the fix was to delete the comment, not to
add a person to the call. S3 is person-less again and S2 is the only
per-person gate.

In the code:

external_id was a hash of the body, so every edit made a SECOND document
with the first still indexed under the same title, and Revision — which
the design leans on for staleness — could only move for a byte-identical
re-upload. The key is now the filename or title, with a keyed body_print
as the change detector. Re-seeding after editing a file updates one
document instead of accumulating stale copies.

chunkCount returned 0 on error, so a failed count rendered as "0 passages
indexed" and the no-match page added "so this is a real absence rather
than an empty index" — asserting the absence was real when the app had
failed to look. It returns -1 now and the pages say so.

AGENTS.md was still the scaffold's, and told future sessions to use
scope.Owned, which sheets-core forbids on team-shared tables and which
memoria's documents are. Rewritten for this app, with the two rules this
branch has already broken once each at the top.
Reviewed-by: Fable
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SiWVaR69Vb2Lzc9wgfit1v
Paul Campbell pushed by paul@keymail.dev 1d8d279b43ea84cafac77edbbe897a94846c6144 parent 226f062
12 files changed, +312 −110
  • AGENTS.md +126 −73
  • docs/superpowers/specs/2026-09-03-memoria-design.md +29 −9
  • internal/memoria/handlers.go +18 −4
  • internal/memoria/migrations/0002_alter_documents.sql +12 −0
  • internal/memoria/migrations/schema.sql +1 −1
  • internal/memoria/models.go +12 −3
  • internal/memoria/store.go +31 −8
  • internal/memoria/templates/index.html +7 −2
  • internal/memoria/templates/layout.html +1 −1
  • internal/memoriatest/app_test.go +50 −8
  • internal/memoriatest/harness_test.go +22 −0
  • tools/seed/main.go +3 −1
diff --git a/AGENTS.md b/AGENTS.md
index c4bfb70..6cf9924 100644
--- a/AGENTS.md
+++ b/AGENTS.md
@@ -1,73 +1,126 @@
-# memoria
-
-A [rastrillo](https://github.com/rastrilloorg/rastrillo) app. Read the
-framework's own SKILL.md (repo root, or in the module cache) before
-writing app code — it is the whole app story in ~15KB. The conventions
-below are load-bearing; the framework enforces most of them
-mechanically.
-
-## Layout
-
-- `internal/<app>/` — the app: `models.go` (plain GORM structs
- + Models), `migrations.go` + `migrations/` (Schema, BootSchema),
- `app.go` (migrate.Apply + chi router), `handlers.go`,
- `render.go` + `templates/`. This is SKILL.md's five-file shape.
-- `manifest/` — the declarative path, optional and equal: TOML or .go
- Resource manifests, each generating a full screen set under `gen/`
- (see manifest/README.md for the mounting recipe). A hand-written
- action file at a generated path (carrying `//go:build rastrillo_actions`)
- takes that one screen over, by existence.
-- `gen/` — generated when manifests are used; never edit. Regenerate
- with `rastrillo generate`.
-
-## Rules the family holds hard
-
-- Every query touching user-owned rows goes through `scope.Owned` —
- reads AND writes, transactions included. A row that isn't yours 404s,
- never 403s.
-- Never bind a request body onto a GORM model: explicit
- `map[string]any` + `.Select` allowlist.
-- Migrations apply at boot, once each, and are never re-run or reversed.
-- Schema changes: edit models.go, then run `rastrillo migration generate`
- and read the SQL it writes — it may be a full table rebuild, not just
- an `ADD COLUMN`. Never edit a migration that has shipped.
- Renames are hand-written (`rastrillo migration new rename_x`) because
- a rename is indistinguishable from a drop plus an add.
-- Money is integer cents. A float never touches a value a person will
- be held to.
-- Screens work with JavaScript disabled; destructive actions get their
- own confirm-page URL.
-- The gate is `make ci` (vet + gofmt + tests) — the same definition
- CI runs. Run it before every push. `CGO_ENABLED=0` throughout: the
- stack is cgo-free by design.
-
-## Serving
-
-`rastrillo.Resolve` + `Serve` speak the CARLOS activation contract
-(socket/addr/db argv, LISTEN_FDS, $STATE_DIRECTORY, `sidecar run`) —
-the golden deployment target, not a requirement: `./app -addr :8080`
-serves anywhere. `GET /healthz` and `GET /api/version` are answered
-by the framework, outside app middleware.
-
-## UX conventions (seeded from profile: considered)
-
-Accessible by construction, progressively enhanced, and never dependent on JavaScript to be usable.
-
-These are rastrillo's own conventions, and the profile is named for what it does rather than after anyone else's work. For wider reading on interface quality, impeccable.style, the WAI-ARIA Authoring Practices and Inclusive Components are all worth your time — none of them is endorsed by, or authoritative for, this framework.
-
-- [x] Icons — lucide, inline — `internal/<app>/icons`
-- [x] Fields — label, hint, help and error in one envelope, wired with aria-describedby — `ui/field, ui/field-select, ui/field-text`
-- [x] Destructive actions — their own confirm-page URL, never a modal — `ui/confirm-form`
-- [x] State — never colour alone — always a text label too — `ui/status-pill, ui/badge`
-- [ ] Dates — relative, with the absolute date in a title attribute
-- [ ] Errors — server-rendered beside the field, in plain language
-- [x] Empty states — say what the screen is for, not just that it is empty — `ui/empty-state`
-
-`[x]` is enforced by a vendored component: use it, do not hand-roll an
-equivalent. `[ ]` is guidance to apply by hand. A convention absent from
-this list is one rastrillo has no opinion about.
-
-Edit any line. **This file is the source of truth, not the profile name.**
-The profile only seeded this list at scaffold time; nothing re-reads it, so
-changing a line here changes what the app does. A convention deleted from
-this file is not a convention to reinstate.
+# 🤖 memoria
+
+The Oficina suite's memory. One instance per team.
+
+This file is binding. Where it disagrees with a general skill, this file
+wins. Where it disagrees with the design spec, the spec wins and this file
+is wrong — fix it in the same commit.
+
+**The design:** `docs/superpowers/specs/2026-09-03-memoria-design.md`. Its
+§1 is the settled-decisions table, §1.2–§1.4 record what five independent
+reviews changed. Don't relitigate a settled decision in a drive-by.
+
+---
+
+## The one rule
+
+> **memoria never speaks a passage without asking whoever owns it whether
+> this person may see it, right now.**
+
+Sibling content: the sibling decides, per person, per query (§4.1).
+Content with no other owner — Notion, Drive, mail, uploads — memoria owns
+outright and says so (§4.2).
+
+The second rule is Sheets': **the bucket never holds a readable passage.**
+Chunks, titles and headings are sealed. So is the index, which is why it
+lives in memory and nowhere else: FTS5 keeps term postings in plaintext.
+
+---
+
+## Two rules this app has already broken once each
+
+**A guard you have never seen fail is a guard written to pass.** Three of
+the first five gates here could not fail for the thing they named — one
+grepped `main.go` for `os.Exit` near a variable and a neighbouring error
+path supplied it; one checked for the string `"result"`, which is a
+substring of `"0 results"`; one scanned a directory the index was not in.
+Two more named a test that had been renamed away. **After writing any
+gate, break the thing it guards, watch it go red, revert.** Do it per
+path, not per file, and say in the test comment what you mutated and when.
+
+**A count that fails is not a count of zero.** `chunkCount` returned 0 on
+error, and the page then told the reader "so this is a real absence rather
+than an empty index" — the codebase's own worst failure shape, asserted on
+the line written to prevent it. Anything that cannot be determined says
+so; it never renders as a confident zero.
+
+---
+
+## Mechanical rules
+
+**Data**
+- **Never `scope.Owned` or `scope.OwnedBy`.** Both are single-column
+ equality on `user_id`, and memoria's documents and chunks are
+ team-shared. Applied to them a list returns only the creator's rows and
+ fails as a plausible empty list, not as an error. (The scaffold's
+ AGENTS.md said the opposite; it was wrong for this app.)
+- A row that isn't yours **404s, never 403s**.
+- Scope `tx`, never the outer handle, inside transactions.
+- Never bind a request body onto a GORM model. Explicit `map[string]any`
+ plus a `.Select` allowlist.
+- Migrations are additive-only. New code over an old DB is always safe.
+- **No document content, title, heading, snippet or query text in any log
+ line, error string or metric label.** `carlos logs` is platform-held
+ storage we cannot grep. Gate 4 enforces it; it was missing once, and
+ what it found when written was that we were logging sign-in links.
+- **Derived state is refreshed after the transaction commits, under
+ `context.WithoutCancel`.** A client closing the tab must not leave the
+ index disagreeing with the database.
+
+**Sealing**
+- `internal/memoria/seal` is the only encryption surface. One HKDF purpose
+ per kind of thing sealed.
+- **A content hash is not a coordinate.** `external_id` was a bare
+ SHA-256 of the body, which let anyone with the bucket confirm the team
+ held a document they already had. Identifiers derived from plaintext are
+ keyed (`seal.Fingerprint`) or they are content.
+
+**Frontend**
+- No build step. Every affordance works scriptless.
+- **Colours are tokens, never literals**, so light and dark resolve
+ through `theme.css`. Gate: `TestNoColourLiteralsInAppCSS`.
+- **`layout.html` must link `theme.css`.** `tokens.css` defines eleven
+ tokens, none of them colours; without the theme every colour resolves to
+ `initial` and whole shorthand declarations go invalid — rules vanish
+ rather than the page going grey. The rastrillo scaffold omits this link.
+- The attribute grammar (`rst-btn="primary compact"`), never the class
+ spelling. Utilities stay classes.
+- **Look at the page.** `tools/drive` serves the app with sign-in off so a
+ browser can see it. It found three bugs in its first run that the whole
+ Go suite passed over — a heading trail rendering as "› Revenue", a
+ snippet repeating its own heading, and a submit button floating over the
+ textarea. A template test proves markup exists, not that it renders.
+
+**Auth**
+- `requireSession` **fails closed**. The test seam is `AppForTest`, named
+ so it cannot be reached by accident, and two gates hold both ends. An
+ earlier version failed open and justified it with a test that did not
+ exist.
+- The app refuses to start without a DEK, an instance key, owners, and
+ either SMTP or an explicit `MEMORIA_ALLOW_LOGGED_MAIL=1` — because
+ without SMTP rastrillo logs the magic link, and that link is a live
+ credential in storage we cannot delete.
+
+**Deploys**
+- One binary, `CGO_ENABLED=0`, cgo-free SQLite.
+- `carlos deploy`, never `carlos ship`. Flags before the binary path.
+- `make release`, never a bare `go build`, or `/api/version` reports `dev`
+ forever. Poll the process, not `X-Carlos-Version`.
+
+---
+
+## Review
+
+Every spec and every branch is reviewed by a model that did not write it,
+before it is approved or merged, and the reviewing model is recorded.
+Five passes so far: Fable and GPT Sol on spec drafts 1–3, Fable on the
+code. Every one returned REQUEST CHANGES, and the code review was the most
+useful of them.
+
+---
+
+## Authorship
+
+🤖 / 👨 markers, `Co-Authored-By` trailers. AI is a guest with a name tag:
+opt-in, disclosed, and every path works with it switched off — which in
+slice 0 means the whole product does.
diff --git a/docs/superpowers/specs/2026-09-03-memoria-design.md b/docs/superpowers/specs/2026-09-03-memoria-design.md
index 930e8b5..d43af30 100644
--- a/docs/superpowers/specs/2026-09-03-memoria-design.md
+++ b/docs/superpowers/specs/2026-09-03-memoria-design.md
@@ -245,6 +245,8 @@ getting an answer grounded in the team's own documents is the gap.
- **Never acts as a person.** It holds no signing key; every assertion's
`aud` binds to "app + instance host" (home §5.3).
- **Never crosses a team label** (§6.3).
+- **Never holds an individually-granted sibling item.** S3 answers on
+ team-visibility alone, so such an item cannot enter the corpus.
- **Never trains or fine-tunes.** Weights are the one copy that cannot be
forgotten (decision 8).
- **Never recomputes a number.** A question with an arithmetic answer
@@ -689,7 +691,7 @@ memoria pulls on its own wake and on each question. It runs no timers.
|---|---|---|
| S1 | Change feed | Cursor is an opaque position in the sibling's event log, issued by the sibling, stored per consumer. A row is `(item_id, revision, gone, touched_at)` — **no title, no content, no visibility flag** — so it passes the plaintext-grep gate unaided. Retention is a **per-consumer TTL** (home §9.1: "any 'prune when all consumers have caught up' is unbounded in a hibernating fleet"). Past TTL, or a restore-rewound cursor, takes a full re-announce. |
| S2 | Authorization re-check | `(person_ref, [(item_id, revision)]) → VISIBLE / NOT_VISIBLE / STALE / UNKNOWN` per item. §4.1 specifies it, including the total decoder. Cap 256 pairs. |
-| S3 | Content fetch | **`(person_ref, id, revision)`** → plaintext body over TLS into memoria's RAM only. The sibling re-runs its grants join and **404s, never 403s**. |
+| S3 | Content fetch | **`(id, revision)`, no person** → plaintext body over TLS into memoria's RAM only. The sibling answers only for items that are **team-visible** — the custody predicate, not the grants join — and **404s, never 403s**. This is an ingest call and there is no asker on the ingest path. |
| S4 | Tombstone | *a feed row.* `gone` covers deletion and departure from team-visibility, indistinguishably. |
| S5 | Consumer expiry | *no call.* TTL expiry is the decommission verb. |
| S6 | Aggregate | `(person_ref, fn, [(sheet, tab, row, col, revision)…]) → {value, display, format_code}` or `STALE(cell)` / `NOT_VISIBLE` / `UNKNOWN` / `NON_NUMERIC(cell)` / `MIXED_UNITS`. §8.7. Cap 64 cells. Never cached. |
@@ -709,14 +711,32 @@ failure it was supposed to prevent. It holds now only because S2 carries
the revision **and** answers `STALE` distinctly. All three clauses travel
together or the sentence is false.
-**S3 carries the person, and draft 2's did not.** Draft 2's S3 took
-`(id, revision)` and promised the sibling "re-runs its grants join at
-fetch time" — with no asker in the call, which the draft-2 review named as
-objectively impossible. The alternative considered was an S2-minted
-capability that S3 consumes; rejected as a second credential type to
-issue, bind and expire, where passing the person ref costs nothing and
-keeps one authority. **Every sibling call that touches content carries the
-person it is for.**
+**S3 takes no person, and the reasoning went wrong twice before landing
+here.**
+
+Draft 2's S3 took `(id, revision)` and *claimed* the sibling "re-runs its
+grants join at fetch time". The draft-2 review correctly called that
+impossible: no asker, no grants join.
+
+Draft 3 fixed it by adding `person_ref`. That was the wrong fix, and a
+review of the built code found two consequences. **There is no person on
+the ingest path** — memoria pulls on its own wake, and the warm-up, the
+rebuild and derived-artefact regeneration have no asker either — so the
+call that fills the corpus could not be made. And a per-person S3
+*succeeds* for individually-granted rows, because Sheets §4.5's join is
+"rows this team can see, **plus rows granted to me individually**" — which
+would pull into memoria's store exactly the content §6.4 forbids and §3.3
+claims it cannot hold.
+
+**The error was collapsing two rules decision 3 keeps apart.** Custody
+bounds what memoria may *hold*: team-visible only, no person involved,
+and that is S3's predicate. Authorization decides what a person may
+*read*: per asker, per query, and that is S2's, at render time. Draft 2's
+S3 had the right signature and a false comment; the fix was to delete the
+comment, not to add a person to the call.
+
+So: **S3 is custody-scoped and person-less. S2 is the only per-person
+gate, and nothing is spoken without it.**
### 6.2 Authentication, and rotation
diff --git a/internal/memoria/handlers.go b/internal/memoria/handlers.go
index 7093e6e..54758ea 100644
--- a/internal/memoria/handlers.go
+++ b/internal/memoria/handlers.go
@@ -52,11 +52,18 @@ type page struct {
Documents []Summary
}
+// chunkCount returns the indexed passage count, and -1 when it cannot be
+// determined.
+//
+// Not 0. Returning 0 on error made the footer say "0 passages indexed"
+// and the no-match page add "so this is a real absence rather than an
+// empty index" — asserting, on the line that exists to prevent exactly
+// this, that an absence was real when the app had failed to look.
func (a *app) chunkCount() int {
n, err := a.store.Count()
if err != nil {
a.logger.Error("count passages", "err", err)
- return 0
+ return -1
}
return n
}
@@ -130,6 +137,7 @@ func (a *app) createDocument(w http.ResponseWriter, r *http.Request) {
title := strings.TrimSpace(r.FormValue("title"))
body := r.FormValue("body")
source := "paste"
+ key := ""
if f, hdr, err := r.FormFile("file"); err == nil {
defer f.Close()
@@ -152,8 +160,14 @@ func (a *app) createDocument(w http.ResponseWriter, r *http.Request) {
}
body = string(raw)
source = "upload"
- if title == "" && hdr != nil {
- title = strings.TrimSuffix(hdr.Filename, filepathExt(hdr.Filename))
+ if hdr != nil {
+ // The filename is the stable key: re-uploading an edited
+ // file updates one document instead of leaving its old self
+ // indexed under the same title.
+ key = hdr.Filename
+ if title == "" {
+ title = strings.TrimSuffix(hdr.Filename, filepathExt(hdr.Filename))
+ }
}
}
@@ -166,7 +180,7 @@ func (a *app) createDocument(w http.ResponseWriter, r *http.Request) {
return
}
- if _, err := a.store.Ingest(r.Context(), source, title, body); err != nil {
+ if _, err := a.store.Ingest(r.Context(), source, key, title, body); err != nil {
// No document content in the log line — AGENTS.md's rule, and
// the reason this carries no title either.
a.logger.Error("ingest failed", "source", source, "bytes", len(body), "err", err)
diff --git a/internal/memoria/migrations/0002_alter_documents.sql b/internal/memoria/migrations/0002_alter_documents.sql
new file mode 100644
index 0000000..4c0a547
--- /dev/null
+++ b/internal/memoria/migrations/0002_alter_documents.sql
@@ -0,0 +1,12 @@
+-- body_print is the change detector for a document's body, added when
+-- external_id stopped being a hash of that body. Existing rows get "",
+-- which is not a real fingerprint, so the next ingest under the same key
+-- sees a mismatch and re-chunks — the right outcome, since those rows
+-- were keyed by body hash under the old scheme and their chunking
+-- predates the line-based chunker anyway.
+--
+-- The DEFAULT is required rather than stylistic: SQLite refuses to add a
+-- NOT NULL column to a table with rows without one, so this migration
+-- would fail on every database that has ever been used.
+ALTER TABLE `documents` ADD `body_print` text NOT NULL DEFAULT "";
+
diff --git a/internal/memoria/migrations/schema.sql b/internal/memoria/migrations/schema.sql
index 150e004..8338ecc 100644
--- a/internal/memoria/migrations/schema.sql
+++ b/internal/memoria/migrations/schema.sql
@@ -3,7 +3,7 @@
CREATE TABLE `chunks` (`id` integer PRIMARY KEY AUTOINCREMENT,`document_id` integer NOT NULL,`ordinal` integer NOT NULL,`sealed_heading` blob,`sealed_text` blob NOT NULL,`created_at` datetime,CONSTRAINT `fk_documents_chunks` FOREIGN KEY (`document_id`) REFERENCES `documents`(`id`) ON DELETE CASCADE);
-CREATE TABLE `documents` (`id` integer PRIMARY KEY AUTOINCREMENT,`source` text NOT NULL,`external_id` text NOT NULL,`revision` integer NOT NULL DEFAULT 1,`sealed_title` blob NOT NULL,`sealed_body` blob NOT NULL,`bytes` integer,`created_at` datetime,`updated_at` datetime);
+CREATE TABLE `documents` (`id` integer PRIMARY KEY AUTOINCREMENT,`source` text NOT NULL,`external_id` text NOT NULL,`revision` integer NOT NULL DEFAULT 1,`sealed_title` blob NOT NULL,`sealed_body` blob NOT NULL,`bytes` integer,`created_at` datetime,`updated_at` datetime, `body_print` text NOT NULL DEFAULT "");
CREATE INDEX `idx_chunks_document_id` ON `chunks`(`document_id`);
diff --git a/internal/memoria/models.go b/internal/memoria/models.go
index b870120..fc5f1ee 100644
--- a/internal/memoria/models.go
+++ b/internal/memoria/models.go
@@ -26,10 +26,19 @@ type Document struct {
// Source names where this came from — "upload" or "paste" in slice 0.
// Sibling sources arrive in slice 1 and carry the app's name.
Source string `gorm:"index;not null"`
- // ExternalID is the id in the source system, stable across edits.
- // For an upload it is the content hash, which is what makes a
- // re-upload of the same file an update rather than a duplicate.
+ // ExternalID is the id in the source system, STABLE ACROSS EDITS —
+ // a file path, a Notion page id, a sibling's item id. Keyed
+ // (seal.Fingerprint) so it is not a plaintext-derived oracle.
+ //
+ // It used to be a hash of the body, which made every edit a new
+ // document: the old one stayed indexed under the same title, and
+ // Revision could only ever increment for a byte-identical re-upload.
+ // The staleness story the spec leans on was unreachable for the case
+ // it names. Found by a code review of this branch.
ExternalID string `gorm:"index;not null"`
+ // BodyPrint is a keyed fingerprint of the body — the change
+ // detector. Keyed for the same reason ExternalID is.
+ BodyPrint string `gorm:"not null;default:''"`
// Revision increments on every change to the body. Slice 1's
// authorization re-check is keyed on (id, revision), so a stale
// passage can be refused rather than spoken.
diff --git a/internal/memoria/store.go b/internal/memoria/store.go
index 804b4be..fefde48 100644
--- a/internal/memoria/store.go
+++ b/internal/memoria/store.go
@@ -49,13 +49,13 @@ type Summary struct {
Updated string
}
-// externalID is the stable key for pasted and uploaded text.
+// externalID keys a document by its stable source identifier.
//
-// Keyed, not a bare hash: an unkeyed SHA-256 of the body lets anyone with
-// the bucket confirm the team holds a document they already have a copy
-// of. See seal.Fingerprint.
-func (s *Store) externalID(body string) (string, error) {
- return s.sealer.Fingerprint(seal.PurposeExternal, []byte(body))
+// Keyed, not a bare hash: an unkeyed digest of anything plaintext-derived
+// lets someone with the bucket confirm what the team holds. See
+// seal.Fingerprint.
+func (s *Store) externalID(key string) (string, error) {
+ return s.sealer.Fingerprint(seal.PurposeExternal, []byte("key\x00"+key))
}
// Ingest stores a document and indexes it.
@@ -65,7 +65,9 @@ func (s *Store) externalID(body string) (string, error) {
// database before the new ones land, so a passage that the new revision
// removed cannot survive as a citation to text that is no longer there —
// which is the stale-passage failure the spec's §4.1 refuses to allow.
-func (s *Store) Ingest(ctx context.Context, source, title, body string) (int64, error) {
+// key is the STABLE identifier in the source: a file path, a page id, a
+// pasted document's title. Not the body — see Document.ExternalID.
+func (s *Store) Ingest(ctx context.Context, source, key, title, body string) (int64, error) {
title = strings.TrimSpace(title)
body = strings.TrimSpace(body)
if body == "" {
@@ -74,7 +76,14 @@ func (s *Store) Ingest(ctx context.Context, source, title, body string) (int64,
if title == "" {
title = firstLine(body)
}
- external, err := s.externalID(body)
+ if strings.TrimSpace(key) == "" {
+ key = title
+ }
+ external, err := s.externalID(key)
+ if err != nil {
+ return 0, err
+ }
+ bodyPrint, err := s.sealer.Fingerprint(seal.PurposeExternal, []byte("body\x00"+body))
if err != nil {
return 0, err
}
@@ -94,12 +103,21 @@ func (s *Store) Ingest(ctx context.Context, source, title, body string) (int64,
}
var docID int64
+ var unchanged bool
err = s.db.G.WithContext(ctx).Transaction(func(tx *gormTx) error {
var existing Document
res := tx.Where("source = ? AND external_id = ?", source, external).First(&existing)
switch {
case res.Error == nil:
docID = existing.ID
+ if existing.BodyPrint == bodyPrint {
+ // Same key, same body: nothing changed. Do not bump the
+ // revision — a revision that moves without the content
+ // moving would make every cached authorization stale for
+ // no reason.
+ unchanged = true
+ return nil
+ }
if err := tx.Where("document_id = ?", docID).Delete(&Chunk{}).Error; err != nil {
return err
}
@@ -107,6 +125,7 @@ func (s *Store) Ingest(ctx context.Context, source, title, body string) (int64,
"revision": existing.Revision + 1,
"sealed_title": sealedTitle,
"sealed_body": sealedBody,
+ "body_print": bodyPrint,
"bytes": len(body),
}).Error; err != nil {
return err
@@ -115,6 +134,7 @@ func (s *Store) Ingest(ctx context.Context, source, title, body string) (int64,
d := Document{
Source: source,
ExternalID: external,
+ BodyPrint: bodyPrint,
Revision: 1,
SealedTitle: sealedTitle,
SealedBody: sealedBody,
@@ -152,6 +172,9 @@ func (s *Store) Ingest(ctx context.Context, source, title, body string) (int64,
if err != nil {
return 0, err
}
+ if unchanged {
+ return docID, nil
+ }
// The index is derived state, so it is refreshed after the
// transaction commits rather than inside it: a rolled-back
diff --git a/internal/memoria/templates/index.html b/internal/memoria/templates/index.html
index 8cf1ecb..699173b 100644
--- a/internal/memoria/templates/index.html
+++ b/internal/memoria/templates/index.html
@@ -12,8 +12,13 @@
{{if not .Results}}
<div rst-callout="quiet">
<p rst-callout-body>
- Nothing matched. There {{if eq .Chunks 1}}is{{else}}are{{end}} {{.Chunks}} passage{{if ne .Chunks 1}}s{{end}}
- indexed, so this is a real absence rather than an empty index.
+ {{if lt .Chunks 0}}
+ Nothing matched — but the passage count could not be read, so
+ this may be an empty index rather than a real absence.
+ {{else}}
+ Nothing matched. There {{if eq .Chunks 1}}is{{else}}are{{end}} {{.Chunks}} passage{{if ne .Chunks 1}}s{{end}}
+ indexed, so this is a real absence rather than an empty index.
+ {{end}}
</p>
</div>
{{end}}
diff --git a/internal/memoria/templates/layout.html b/internal/memoria/templates/layout.html
index 9ccb34d..459d8fa 100644
--- a/internal/memoria/templates/layout.html
+++ b/internal/memoria/templates/layout.html
@@ -26,7 +26,7 @@
</main>
<footer rst-page-footer>
<p rst-cell-mut>
- {{if .Signed}}{{.Chunks}} passage{{if ne .Chunks 1}}s{{end}} indexed.{{end}}
+ {{if .Signed}}{{if lt .Chunks 0}}The passage count is unavailable.{{else}}{{.Chunks}} passage{{if ne .Chunks 1}}s{{end}} indexed.{{end}}{{end}}
Search is lexical only — no assistant, and <strong>no document ever
leaves this instance</strong>.
</p>
diff --git a/internal/memoriatest/app_test.go b/internal/memoriatest/app_test.go
index 2686f90..3b64fb2 100644
--- a/internal/memoriatest/app_test.go
+++ b/internal/memoriatest/app_test.go
@@ -58,17 +58,35 @@ func TestStoredMarkupIsEscaped(t *testing.T) {
}
}
-// Re-storing the same text updates one document rather than making two
-// that disagree. The ExternalID is the content hash, so this is what
-// makes a re-upload idempotent.
-func TestReStoringSameTextDoesNotDuplicate(t *testing.T) {
+// Re-storing under the same key is idempotent. The key is the filename
+// for an upload and the title for a paste, so pasting the same text twice
+// under the same title updates one document.
+func TestReStoringUnderTheSameKeyDoesNotDuplicate(t *testing.T) {
h := newApp(t)
const text = "## Revenue\n\nJune revenue was 1,350,000 across every region we serve."
- store(t, h, "First", text)
- store(t, h, "Second", text)
+ store(t, h, "Board pack", text)
+ store(t, h, "Board pack", text)
+ if body := get(t, h, "/").Body.String(); !strings.Contains(body, "1 document") {
+ t.Errorf("re-storing under one key made more than one document:\n%s", body)
+ }
+}
+
+// Two different documents that happen to share text are two documents.
+// Under the old scheme — external_id as a hash of the body — they
+// collided into one, and the second silently overwrote the first's title.
+func TestSameTextUnderDifferentKeysIsTwoDocuments(t *testing.T) {
+ h := newApp(t)
+ const text = "## Revenue\n\nJune revenue was 1,350,000 across every region we serve."
+ store(t, h, "Board pack", text)
+ store(t, h, "Minutes", text)
body := get(t, h, "/").Body.String()
- if !strings.Contains(body, "1 document") {
- t.Errorf("re-storing identical text made more than one document:\n%s", body)
+ if !strings.Contains(body, "2 documents") {
+ t.Errorf("two documents sharing text collapsed into one:\n%s", body)
+ }
+ for _, want := range []string{"Board pack", "Minutes"} {
+ if !strings.Contains(body, want) {
+ t.Errorf("%q is missing from the list", want)
+ }
}
}
@@ -169,3 +187,27 @@ func TestCitedAnchorExistsOnTheDocumentPage(t *testing.T) {
"the citation points at nothing", m[0], anchor)
}
}
+
+// A document edited and re-ingested under the same key must UPDATE, not
+// accumulate. external_id used to be a hash of the body, so every edit
+// produced a second document with the first still indexed under the same
+// title — and Revision, which the design leans on for staleness, could
+// only ever move for a byte-identical re-upload. Found by a code review.
+func TestEditingUnderTheSameKeyUpdatesOneDocument(t *testing.T) {
+ h := newApp(t)
+ const key = "board-pack.md"
+ upload(t, h, key, "## Revenue\nThe zebrafinch figure was 100 for the quarter just gone.")
+ upload(t, h, key, "## Revenue\nThe wildebeest figure was 200 for the quarter just gone.")
+
+ list := get(t, h, "/").Body.String()
+ if !strings.Contains(list, "1 document") {
+ t.Errorf("editing under the same key made more than one document:\n%s", list)
+ }
+ // The old passage must be gone, not merely outranked.
+ if got := get(t, h, "/?q=zebrafinch").Body.String(); strings.Contains(got, "<mark>") {
+ t.Error("the superseded passage is still searchable; a citation would point at text that is no longer there")
+ }
+ if got := get(t, h, "/?q=wildebeest").Body.String(); !strings.Contains(got, "<mark>") {
+ t.Error("the new passage was not indexed")
+ }
+}
diff --git a/internal/memoriatest/harness_test.go b/internal/memoriatest/harness_test.go
index 4cd7e7a..373458e 100644
--- a/internal/memoriatest/harness_test.go
+++ b/internal/memoriatest/harness_test.go
@@ -143,3 +143,25 @@ func store(t *testing.T, h http.Handler, title, body string) *httptest.ResponseR
h.ServeHTTP(rec, req)
return rec
}
+
+// upload posts a document as a file, so it carries a filename — the
+// stable key that decides whether a re-ingest updates or duplicates.
+func upload(t *testing.T, h http.Handler, filename, body string) *httptest.ResponseRecorder {
+ t.Helper()
+ var buf bytes.Buffer
+ mw := multipart.NewWriter(&buf)
+ w, err := mw.CreateFormFile("file", filename)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if _, err := w.Write([]byte(body)); err != nil {
+ t.Fatal(err)
+ }
+ mw.Close()
+ req := httptest.NewRequest(http.MethodPost, "/documents", &buf)
+ req.Header.Set("Content-Type", mw.FormDataContentType())
+ req.Header.Set("Origin", testOrigin)
+ rec := httptest.NewRecorder()
+ h.ServeHTTP(rec, req)
+ return rec
+}
diff --git a/tools/seed/main.go b/tools/seed/main.go
index 974a33a..007655a 100644
--- a/tools/seed/main.go
+++ b/tools/seed/main.go
@@ -110,7 +110,9 @@ func main() {
}
rel, _ := filepath.Rel(*root, p)
title := rel
- if _, err := store.Ingest(ctx, *source, title, string(raw)); err != nil {
+ // The relative path is the stable key, so re-seeding after an
+ // edit updates the document rather than adding a second copy.
+ if _, err := store.Ingest(ctx, *source, rel, title, string(raw)); err != nil {
// Fatal, not a skip. An earlier version counted these and
// carried on, so a missing table produced 59 skips and a
// cheerful "0 documents seeded" summary.