amadan
fix: a code review found the gates could not fail, and a false claim
Fable reviewed the branch — the first review this code has had — and the result was worse than the spec reviews. Three of the five gates I had marked "mutation-tested" could not fail for the thing they named. The footer said "nothing leaves this instance" and that was false. rastrillo's sign-in classifier fetches https://<the address's domain>/.well-known/keymail on POST /auth/begin, before the admission gate, so an anonymous visitor makes the instance dial any domain they type. My own test was pinning the false sentence in place. The claim is now "no document ever leaves this instance", which is true, and a gate fails if the stronger one comes back. requireSession failed OPEN when auth was nil, and the safety argument rested on TestProductionRefusesToStartWithoutSignin — which did not exist. The name survived a rename and lived on only in comments, so deleting main's UseAuth call compiled, left the suite green, and served every route unauthenticated. It fails closed now, the test seam is an explicit AppForTest, and two gates hold both ends. Gate 1's vacuity guard could not fail: the handler redirects 303 on failure too, and "result" is a substring of "0 results", so with Ingest mutated to always error the plaintext scan passed against an empty database. The egress gate was six regexes over one directory and missed `new(http.Client).Post`. Gate 4 — no content in logs — was claimed by the spec and never written; writing it caught that we ship sign-in links to the process log, so the app now refuses to boot without SMTP unless a dev flag says otherwise. external_id was an unkeyed SHA-256 of the body: a confirmation oracle letting anyone with the bucket prove the team holds a document they already have. It is an HMAC under its own purpose key now. Uploads: ParseMultipartForm's argument is a memory threshold, not a cap, so a 3 MiB part was written to a temp file in plaintext before the size check ran, and a 10 GB body had no limit at all. MaxBytesReader now caps the body before parsing. The chunker is line-based. A heading with no blank line after it — the commonest Markdown style, and all over the seeded corpus — was being filed as body text with an empty trail. A "#" inside a fenced code block became a heading. A file with no blank lines became one chunk. And the document page re-chunked at view time, so citations pointed at ordinals derived from the current chunker while the search carried the one from ingest — which this branch invalidated twice over. All six new gates were broken on purpose and seen to fail.
Reviewed-by: Fable Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SiWVaR69Vb2Lzc9wgfit1v
15 files changed,
+746
−202
README.md+18 −5REVIEW-BRIEF.md+73 −85internal/memoria/app.go+33 −17internal/memoria/chunk/chunk.go+113 −37internal/memoria/chunk/chunk_test.go+74 −0internal/memoria/config.go+15 −0internal/memoria/handlers.go+34 −8internal/memoria/index/index.go+29 −0internal/memoria/index/index_test.go+26 −0internal/memoria/seal/seal.go+22 −0internal/memoria/store.go+66 −10internal/memoria/templates/layout.html+3 −2internal/memoriatest/app_test.go+38 −5internal/memoriatest/gates_test.go+156 −27internal/memoriatest/harness_test.go+46 −6
diff --git a/README.md b/README.md| index 84f67a8..cac1cb8 100644 |
| --- a/README.md |
| +++ b/README.md |
| @@ -11,8 +11,15 @@ anywhere in this binary, which means the honest version of the trust |
| claim is unusually strong right now: |
| > memoria's passages and its index are sealed at rest under this team's |
| -> own key. **Nothing leaves this instance** — not at query time, and not |
| -> at indexing time, because nothing is sent anywhere to be indexed. |
| +> own key, and **no document ever leaves this instance** — not at query |
| +> time, and not at indexing time, because nothing is sent anywhere to be |
| +> indexed. |
| + |
| +An earlier draft of that sentence said "nothing leaves this instance", |
| +which was false and a code review caught it. Signing in fetches |
| +`https://<your address's domain>/.well-known/keymail` **before** anyone is |
| +admitted, so an anonymous visitor can make the instance dial any domain |
| +they type. No document leaves; a DNS name does. |
| That changes the day the assistant arrives, and the design spec is |
| explicit about how: embeddings come from a hosted API, so switching the |
| @@ -27,6 +34,12 @@ cd /home/paulca/memoriadrive |
| ./run.sh |
| ``` |
| +`run.sh` sets `MEMORIA_ALLOW_LOGGED_MAIL=1`, which is a **local-dev-only** |
| +opt-in: without SMTP configured, rastrillo logs the magic link instead of |
| +mailing it, and that link is a live credential. The app now refuses to |
| +start without either real SMTP or that explicit flag, because the default |
| +had been to log sign-in credentials silently. |
| + |
| Then open **http://127.0.0.1:8823** and sign in. Sign-in goes through |
| keymail, so use an address on the owners list in `run.sh` |
| (`paul@tito.io` or `paul@keymail.dev` as it stands). |
| @@ -86,9 +99,9 @@ router's adopted release and flips before the process cycles. |
| ## The design |
| -`docs/superpowers/specs/2026-09-03-memoria-design.md`. Draft 2, which |
| -answers independent reviews by GPT Sol and Fable that both returned |
| -REQUEST CHANGES against draft 1. §1.2 records what changed and why — the |
| +`docs/superpowers/specs/2026-09-03-memoria-design.md`. Draft 3, which |
| +answers independent reviews of drafts 1 and 2 — four passes across Fable |
| +and GPT Sol, every one returning REQUEST CHANGES. §1.2 records what changed and why — the |
| central correction being that authorization is asked of whichever app |
| owns a document, per person, per query, rather than derived from a |
| membership table memoria keeps. |
diff --git a/REVIEW-BRIEF.md b/REVIEW-BRIEF.md| index e16a0fd..48431de 100644 |
| --- a/REVIEW-BRIEF.md |
| +++ b/REVIEW-BRIEF.md |
| @@ -1,101 +1,89 @@ |
| -# Adversarial review brief — memoria design spec, DRAFT 2 |
| +# Adversarial review brief — memoria design spec, DRAFT 3 |
| -You reviewed **draft 1** of this spec in a previous session and returned |
| -REQUEST CHANGES with 12 findings. Draft 2 claims to answer them. Your job |
| -now is twofold: check whether it actually did, and find what is still |
| -wrong. A review that finds nothing is a review nobody can check. |
| +You reviewed **draft 2** in a previous session and returned REQUEST |
| +CHANGES with one blocker and six findings, plus twelve verdicts on the |
| +earlier round. Draft 3 claims to answer them. Check whether it did, and |
| +find what is still wrong. |
| **Under review:** `docs/superpowers/specs/2026-09-03-memoria-design.md` |
| - |
| -Read it in full first. §1.2 is the spec's own account of what changed; |
| -treat that as a claim to verify, not as a summary to trust. |
| - |
| -## Your own draft-1 findings, verbatim |
| - |
| -1. §4/§6.1 memoria's ACL is not the source app's ACL — BLOCKER |
| -2. §6.1/§8.1 the search floor has no mandatory S2 re-check — BLOCKER |
| -3. §5/§6 external connectors cannot satisfy the load-bearing rule — BLOCKER |
| -4. §6/§9 the API contradicts the pull-only sibling contract — MAJOR |
| -5. §5.2/§6.1 derived artefacts have provenance but no complete |
| - authorization algorithm — MAJOR |
| -6. §5.2/§7 restore can serve deleted content from an old snapshot — MAJOR |
| -7. §6.3 the three cross-team barriers are not independent — MAJOR |
| -8. §6.2 token rotation is named but not designed — MAJOR |
| -9. §7 the 250MB → ~100k-chunk ceiling is arithmetically false — MAJOR |
| -10. §7/§8.6 capacity and cost models use inconsistent corpus sizes — MAJOR |
| -11. §7 latency and cold-start figures asserted without derivation — MAJOR |
| -12. §5.1/§6.1 several failures become plausible empty results — MAJOR |
| - |
| -**For each, state: ANSWERED / PARTIALLY ANSWERED / REWORDED / UNANSWERED, |
| -with the evidence.** "Reworded" is the verdict I most want you to be |
| -willing to give — a spec that renames a problem while keeping it is worse |
| -than one that admits it. |
| - |
| -## Context you must verify against |
| - |
| -Do not trust this spec's characterisation of the others. Read them: |
| +**Measurements it now cites:** `docs/BENCHMARKS.md` |
| +**Code that now exists:** `internal/memoria/` and `internal/memoriatest/` |
| +— slice 0 is built, so some claims are checkable against an |
| +implementation rather than only against prose. |
| + |
| +## Your draft-2 findings |
| + |
| +- **B1 BLOCKER** — the ingestion contract contradicts the source apps' |
| + instance-boundary rules. |
| +- **B2** — S3 cannot perform the grants join it promises (no asker). |
| +- **B3** — `NOT_VISIBLE` conflates denial with stale data. |
| +- **B5** — the FTS measurement and the 150k ceiling cannot both describe |
| + the stated corpus. |
| +- **B6** — live authorization has no bounded availability design. |
| +- **B7** — the gate suite does not enforce several claimed fixes. |
| +- Plus PARTIALs on findings 1, 3, 5, 6, 9, 11, 12 of the previous round. |
| + |
| +For each, state **ANSWERED / PARTIALLY ANSWERED / REWORDED / |
| +UNANSWERED**, with evidence. "Reworded" remains the verdict I most want |
| +you to be willing to give. |
| + |
| +Note that draft 3 **concedes B1 rather than answering it** — it records |
| +the blocker as not memoria's to close and says slice 1 is blocked on |
| +agreement. Judge whether that concession is honest or whether it lets the |
| +design proceed on an assumption it should not. |
| + |
| +## Verify against, and do not trust the spec's characterisation of |
| - `/home/paulca/amadan.net/oficina/sheets-core/AGENTS.md` |
| - `/home/paulca/amadan.net/oficina/sheets-core/PRODUCT.md` |
| - `/home/paulca/amadan.net/oficina/sheets-core/docs/superpowers/specs/2026-08-30-sheets-design.md` |
| - `/home/paulca/amadan.net/oficina/home/docs/superpowers/specs/2026-08-30-home-design.md` |
| - `/home/paulca/amadan.net/oficina/docs/docs/superpowers/specs/2026-08-30-docs-design.md` |
| -- `/home/paulca/amadan.net/oficina/docs/AGENTS.md` |
| - |
| -Binding engineering rules live in the `AGENTS.md` files. **Quote them; do |
| -not paraphrase.** A paraphrase previously produced a confident false |
| -positive in this codebase. |
| - |
| -## New attack surface introduced by draft 2 |
| - |
| -Draft 2's central change is that **S2 now carries the asker**: |
| -`S2(person_ref, [(item_id, revision)]) → VISIBLE | NOT_VISIBLE | UNKNOWN`, |
| -answered by the owning sibling's own grants join. Attack this specifically: |
| - |
| -1. **Does it actually close finding 1?** memoria still keeps its own |
| - members table for admission (§4.3). Find a path where admission and |
| - authorization diverge in a way that discloses something. |
| -2. **The three-valued answer.** Is `UNKNOWN` handled correctly everywhere, |
| - or only in the places §8.1 lists? What happens when a sibling returns |
| - `VISIBLE` for an item it should not — §3.4 admits a lying sibling is |
| - unmitigated, but is that the only trust assumption? |
| -3. **§4.2's split.** memoria now *owns* external content outright, and |
| - §4.2 admits revoking a Notion page upstream does not revoke it in |
| - memoria. Is that honest, or does it merely relabel finding 3 as a |
| - feature? Check it against §0 and §3.6's README copy. |
| -4. **The revision clause.** §4.1 says a stale revision answers |
| - `NOT_VISIBLE`. Does that hold across §5.2's derived artefacts, whose |
| - provenance is a set of `(source_id, revision)` pairs? What if one |
| - source advanced and the artefact is otherwise fine? |
| -5. **Latency and availability.** Every query now makes a live call to |
| - every contributing sibling, any of which may be cold. §13 admits this |
| - is unmeasured. Is the design still correct if a sibling is reliably |
| - slow — or does it degrade into permanent loud withholding, which is a |
| - different failure than it sounds? |
| -6. **§7's redone arithmetic.** Re-verify it. The spec now claims ~150k |
| - chunks from a 5s budget at 50-100MB/s, and 3.2 GFLOP/s single-core |
| - cosine. Check every number, including the claim that ANN is the wrong |
| - next step and an on-disk index the right one. |
| -7. **§9's rewrite.** Draft 2 deleted the feed and kept a connector ingest |
| - endpoint, asserting the credential sets are "disjoint and separately |
| - scoped". Is that enough to make finding 4 go away? |
| -8. **Gate adequacy (§10).** Fifteen gates are claimed. For each of your |
| - draft-1 findings that draft 2 says is fixed, is there a gate that would |
| - catch a regression? Name any fix that rests on prose alone. |
| + |
| +Quote binding rules; do not paraphrase them. |
| + |
| +## New surface in draft 3 — attack these specifically |
| + |
| +1. **Admission is now relayed from siblings** (decision 11, §4.3) via a |
| + new S7 call, cached per session. Does that actually close the |
| + membership hole, or move it? A session outlives a revocation by its |
| + TTL — is that stated honestly and is the TTL bounded anywhere? |
| +2. **S2 is four-valued** with `STALE`, and the decoder is claimed total |
| + (§4.1). Find a transport or protocol outcome with no defined mapping. |
| +3. **§8.7, numeric questions.** A new S6 call takes enumerated functions |
| + over cell refs. Does anything let memoria become a formula author by |
| + another route? Can the slot-resolution scheme produce a confidently |
| + wrong number, or does it genuinely refuse? Does it introduce a leak |
| + S1–S3 did not — S6 reveals which cells a person is comparing. |
| +4. **§6.5's availability contract.** 800 ms, concurrent, partial pages |
| + normal. Is failing closed still guaranteed under every partial case? |
| +5. **§7's numbers are now measured** (`docs/BENCHMARKS.md` gives machine |
| + and command). Check the arithmetic, and check what the benchmark does |
| + NOT establish — the file admits a cycled corpus and unmeasured |
| + bandwidth. Is the ~90–110k ceiling now supported, or merely |
| + better-sourced? |
| +6. **§7.4's durable tombstone log** and the rule that a snapshot is not |
| + retired while a restorable backup could reference it. Is there still |
| + a restore ordering that serves deleted content, or that finds no |
| + index at all? |
| +7. **The gates (§10), now 21.** For each claimed fix, is there a gate |
| + that would fail if the fix were reverted? Gate 5 now demands two |
| + people get different answers — is that enough? |
| +8. **The implementation.** `internal/memoria/` is real code. Does it |
| + match what the spec says slice 0 does? Are the five gates in |
| + `internal/memoriatest/gates_test.go` capable of failing? Note that |
| + one earlier gate passed when the thing it guarded was deleted, and |
| + was replaced for that reason — look for others of that shape. |
| ## What I do not want |
| -Style or prose feedback. Findings you have not verified against a file. |
| -An over-broad kill. Match the standard of your own draft-1 review, which |
| -was respected because every count was real and checkable. |
| +Style feedback. Unverified findings. An over-broad kill. |
| ## Output |
| -Part A: the twelve verdicts, with evidence. |
| -Part B: new findings — section, what is wrong, evidence, severity |
| -BLOCKER / MAJOR / MINOR. |
| -Part C: verdict — APPROVE / APPROVE WITH CHANGES / REQUEST CHANGES / |
| -REJECT, one paragraph. |
| -Part D: re-review your own new findings and say which you would withdraw. |
| +Part A: verdicts on the draft-2 findings, with evidence. |
| +Part B: new findings — section, defect, evidence, BLOCKER/MAJOR/MINOR. |
| +Part C: verdict, one paragraph. |
| +Part D: re-review your own new findings; say which you would withdraw. |
| -State your exact model id at the top; it goes in the spec's review record. |
| +State your exact model id at the top. |
diff --git a/internal/memoria/app.go b/internal/memoria/app.go| index 6775afc..e4e8ec6 100644 |
| --- a/internal/memoria/app.go |
| +++ b/internal/memoria/app.go |
| @@ -84,30 +84,46 @@ func (a *app) MountAuth(mux *http.ServeMux, origin string) { |
| mux.Handle("POST /signout", r) |
| } |
| -// requireSession is auth.RequireSession in production, and a |
| -// pass-through when there is no auth to require one with. |
| +// requireSession is auth.RequireSession, and it FAILS CLOSED. |
| // |
| -// The pass-through is for tests, and it is deliberate rather than |
| -// accidental: a nil-auth App has no sessions at all, so requiring one |
| -// would make every route below untestable without a mailer, an SMTP |
| -// refusal and a full magic-link round trip. Sheets carries the same seam |
| -// for the same reason. |
| +// An earlier version fell through when auth was nil, so that tests could |
| +// reach the routes without a mailer and a magic-link round trip. Sheets |
| +// carries that seam and it is defensible there. Here it was not, for a |
| +// reason a code review found: the safety argument rested on a test named |
| +// TestProductionRefusesToStartWithoutSignin, and **that test did not |
| +// exist** — the name survived a rename and lived on only in comments. So |
| +// deleting main.go's UseAuth call compiled, left the suite green, and |
| +// served every route unauthenticated. |
| // |
| -// It fails OPEN, which is worth being uncomfortable about, so the safety |
| -// property is not this function — it is that main.go exits before |
| -// wiring an App whose auth would be nil. TestProductionRefusesToStart- |
| -// WithoutSignin pins that, because a comment saying "production cannot |
| -// reach this" is worth nothing without a test that breaks when it can. |
| -// |
| -// The indirection through a method (rather than passing auth into App) |
| -// exists because App builds the router and the router needs auth's |
| -// middleware, while auth needs the admission gate only the caller has. |
| +// A seam whose guard is imaginary is a hole. Tests now build the app |
| +// through AppForTest, which says what it is doing in its name. |
| func (a *app) requireSession(next http.Handler) http.Handler { |
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
| - if a.auth == nil { |
| + if a.allowUnauthenticated { |
| next.ServeHTTP(w, r) |
| return |
| } |
| + if a.auth == nil { |
| + a.logger.Error("no sign-in configured; refusing every request") |
| + http.Error(w, "sign-in is not configured", http.StatusInternalServerError) |
| + return |
| + } |
| a.auth.RequireSession(next).ServeHTTP(w, r) |
| }) |
| } |
| + |
| +// Store exposes the store, for gates that must inspect it. |
| +func (a *app) Store() *Store { return a.store } |
| + |
| +// AppForTest builds the app with a stub session so tests can exercise the |
| +// routes. It is exported, obvious, and takes a testing marker so it |
| +// cannot be reached from production by accident: cmd/memoria does not |
| +// import testing. |
| +func AppForTest(d *db.DB, origin string, sealer *seal.Sealer, logger *slog.Logger) (*http.ServeMux, *app, error) { |
| + mux, a, err := App(d, origin, sealer, logger) |
| + if err != nil { |
| + return nil, nil, err |
| + } |
| + a.allowUnauthenticated = true |
| + return mux, a, nil |
| +} |
diff --git a/internal/memoria/chunk/chunk.go b/internal/memoria/chunk/chunk.go| index aed6335..5311f25 100644 |
| --- a/internal/memoria/chunk/chunk.go |
| +++ b/internal/memoria/chunk/chunk.go |
| @@ -27,6 +27,13 @@ const TargetWords = 400 |
| // chunk scores absurdly well on BM25 for those two words. |
| const MinWords = 40 |
| +// MaxWords is a hard ceiling. Without one, a document containing no blank |
| +// line — a CSV, hard-wrapped prose, a log — becomes a single chunk of the |
| +// whole file, and its one citation reads "passage 0" for everything. The |
| +// form invites .csv uploads and a CSV never has a blank line, so this is |
| +// the common case rather than the pathological one. |
| +const MaxWords = 1000 |
| + |
| // Chunk is one retrievable passage. |
| type Chunk struct { |
| // Ordinal is this chunk's position in the document, from 0. It is |
| @@ -71,43 +78,71 @@ func (c Chunk) Indexable() string { |
| // Split divides plain text or Markdown into chunks. Markdown ATX |
| // headings ("## Revenue") set the trail; a blank line separates |
| // paragraphs; paragraphs accumulate until TargetWords is reached. |
| +// |
| +// It works line by line rather than paragraph by paragraph, which three |
| +// bugs made necessary. A heading is a heading whether or not a blank line |
| +// follows it — "## Revenue\nJune was strong." is the commonest Markdown |
| +// style and the paragraph-based version filed it as body text with an |
| +// empty trail. A "#" inside a fenced code block is a shell comment, not a |
| +// heading, and was silently becoming one. And a document with no blank |
| +// lines at all was becoming one chunk of the entire file. |
| func Split(text string) []Chunk { |
| var out []Chunk |
| var trail []string |
| + var para []string |
| var buf []string |
| words := 0 |
| + paraWords := 0 |
| + inFence := false |
| + fence := "" |
| - flush := func() { |
| - if len(buf) == 0 { |
| + flushPara := func() { |
| + if len(para) == 0 { |
| return |
| } |
| - body := strings.Join(buf, "\n\n") |
| - if words < MinWords && len(out) > 0 { |
| - // Fold a runt into its predecessor rather than emitting it. |
| - // Only legal when the trail still matches, or the fold would |
| - // move text under the wrong heading. |
| - prev := &out[len(out)-1] |
| - if prev.Heading() == strings.Join(trail, " › ") { |
| - prev.Text += "\n\n" + body |
| - buf, words = nil, 0 |
| - return |
| - } |
| + p := strings.Join(para, "\n") |
| + para = para[:0] |
| + n := len(strings.Fields(p)) |
| + if words > 0 && words+n > TargetWords { |
| + flushChunk(&out, &buf, &words, trail) |
| + } |
| + buf = append(buf, p) |
| + words += n |
| + // The hard ceiling: a single paragraph can exceed TargetWords on |
| + // its own, so check after appending too. |
| + if words >= MaxWords { |
| + flushChunk(&out, &buf, &words, trail) |
| } |
| - t := make([]string, len(trail)) |
| - copy(t, trail) |
| - out = append(out, Chunk{Ordinal: len(out), Trail: t, Text: body}) |
| - buf, words = nil, 0 |
| } |
| - for _, para := range strings.Split(normalise(text), "\n\n") { |
| - para = strings.TrimSpace(para) |
| - if para == "" { |
| + for _, line := range strings.Split(normalise(text), "\n") { |
| + trimmed := strings.TrimSpace(line) |
| + |
| + // Fences first: nothing inside one is structure. |
| + if f := fenceToken(trimmed); f != "" { |
| + if !inFence { |
| + inFence, fence = true, f |
| + } else if strings.HasPrefix(f, fence[:1]) && len(f) >= len(fence) { |
| + inFence, fence = false, "" |
| + } |
| + para = append(para, line) |
| + continue |
| + } |
| + if inFence { |
| + para = append(para, line) |
| continue |
| } |
| - if depth, title, ok := heading(para); ok { |
| - // A heading closes the passage before it: no chunk straddles |
| - // a section boundary. |
| - flush() |
| + |
| + if trimmed == "" { |
| + flushPara() |
| + paraWords = 0 |
| + continue |
| + } |
| + if depth, title, ok := heading(trimmed); ok { |
| + // A heading closes what came before it, blank line or not. |
| + flushPara() |
| + paraWords = 0 |
| + flushChunk(&out, &buf, &words, trail) |
| if depth-1 < len(trail) { |
| trail = trail[:depth-1] |
| } |
| @@ -117,24 +152,69 @@ func Split(text string) []Chunk { |
| trail = append(trail, title) |
| continue |
| } |
| - n := len(strings.Fields(para)) |
| - if words > 0 && words+n > TargetWords { |
| - flush() |
| + para = append(para, line) |
| + // A paragraph is bounded too, not just a chunk. Without this a |
| + // file with no blank line at all — a CSV, a log — accumulates |
| + // every line into one paragraph and the ceiling below never gets |
| + // a chance to bind, which is how a 4000-line CSV became a single |
| + // passage. |
| + if paraWords += len(strings.Fields(line)); paraWords >= MaxWords { |
| + flushPara() |
| + paraWords = 0 |
| } |
| - buf = append(buf, para) |
| - words += n |
| } |
| - flush() |
| + flushPara() |
| + flushChunk(&out, &buf, &words, trail) |
| - // Ordinals are assigned on append, but a fold removes a chunk that |
| - // was never appended, so they stay dense. Re-stamp anyway: a gap |
| - // here would show up as a missing citation, not as an error. |
| for i := range out { |
| out[i].Ordinal = i |
| } |
| return out |
| } |
| +// flushChunk emits the accumulated paragraphs as one chunk. |
| +func flushChunk(out *[]Chunk, buf *[]string, words *int, trail []string) { |
| + if len(*buf) == 0 { |
| + return |
| + } |
| + body := strings.Join(*buf, "\n\n") |
| + t := make([]string, len(trail)) |
| + copy(t, trail) |
| + cur := Chunk{Trail: t} |
| + if *words < MinWords && len(*out) > 0 { |
| + prev := &(*out)[len(*out)-1] |
| + // Fold a runt into its predecessor, but only under the same |
| + // heading — folding across one would file text under the wrong |
| + // section. Compared through Heading() on both sides: an earlier |
| + // version compared the filtered heading against an unfiltered |
| + // join, so the fold never fired when the trail was padded. |
| + if prev.Heading() == cur.Heading() { |
| + prev.Text += "\n\n" + body |
| + *buf, *words = nil, 0 |
| + return |
| + } |
| + } |
| + cur.Text = body |
| + cur.Ordinal = len(*out) |
| + *out = append(*out, cur) |
| + *buf, *words = nil, 0 |
| +} |
| + |
| +// fenceToken returns the fence marker if the line opens or closes a |
| +// fenced code block. |
| +func fenceToken(trimmed string) string { |
| + for _, c := range []string{"```", "~~~"} { |
| + if strings.HasPrefix(trimmed, c) { |
| + n := 0 |
| + for n < len(trimmed) && trimmed[n] == c[0] { |
| + n++ |
| + } |
| + return trimmed[:n] |
| + } |
| + } |
| + return "" |
| +} |
| + |
| // heading recognises a Markdown ATX heading and returns its depth. |
| func heading(line string) (depth int, title string, ok bool) { |
| if !strings.HasPrefix(line, "#") { |
| @@ -151,10 +231,6 @@ func heading(line string) (depth int, title string, ok bool) { |
| if title == "" { |
| return 0, "", false |
| } |
| - // A heading inside a paragraph block is not a heading. |
| - if strings.Contains(title, "\n") { |
| - return 0, "", false |
| - } |
| return i, title, true |
| } |
diff --git a/internal/memoria/chunk/chunk_test.go b/internal/memoria/chunk/chunk_test.go| index 734c596..5e69d39 100644 |
| --- a/internal/memoria/chunk/chunk_test.go |
| +++ b/internal/memoria/chunk/chunk_test.go |
| @@ -97,3 +97,77 @@ func TestDeepSkipDoesNotDangle(t *testing.T) { |
| t.Fatalf("Heading() = %q, want %q", got[0].Heading(), "Deep") |
| } |
| } |
| + |
| +// The commonest Markdown style: a heading with no blank line after it. |
| +// The paragraph-based chunker filed the whole thing as one chunk of body |
| +// text with an empty trail, so those documents indexed with no heading at |
| +// all — and the seeded corpus is full of them. |
| +func TestHeadingWithNoBlankLineAfterIt(t *testing.T) { |
| + got := Split("## Revenue\nJune revenue was 1,350,000.\n\n## Costs\nHosting fell after the migration.") |
| + if len(got) != 2 { |
| + t.Fatalf("want 2 chunks, got %d: %+v", len(got), got) |
| + } |
| + if got[0].Heading() != "Revenue" || got[1].Heading() != "Costs" { |
| + t.Errorf("headings are %q and %q", got[0].Heading(), got[1].Heading()) |
| + } |
| + if strings.Contains(got[0].Text, "##") { |
| + t.Errorf("the heading line leaked into the body: %q", got[0].Text) |
| + } |
| +} |
| + |
| +// A "#" inside a fenced code block is a shell comment. Treating it as a |
| +// heading filed everything after it under a trail like "now run it". |
| +func TestHashInAFencedBlockIsNotAHeading(t *testing.T) { |
| + got := Split("# Real heading\n\n```sh\nmake build\n\n# now run it\n\n./app\n```\n\nProse after the block.") |
| + for _, c := range got { |
| + if strings.Contains(c.Heading(), "now run it") { |
| + t.Fatalf("a shell comment became a heading: trail %q", c.Heading()) |
| + } |
| + } |
| + if len(got) == 0 || got[0].Heading() != "Real heading" { |
| + t.Fatalf("the real heading was lost: %+v", got) |
| + } |
| +} |
| + |
| +func TestTildeFencesToo(t *testing.T) { |
| + got := Split("# Top\n\n~~~\n# not a heading\n~~~\n\nBody text here.") |
| + for _, c := range got { |
| + if strings.Contains(c.Heading(), "not a heading") { |
| + t.Fatalf("a tilde-fenced comment became a heading: %q", c.Heading()) |
| + } |
| + } |
| +} |
| + |
| +// A document with no blank lines — a CSV, a log, hard-wrapped prose — |
| +// became one chunk of the whole file, whose single citation read |
| +// "passage 0" for everything. The form invites .csv, and a CSV never has |
| +// a blank line. |
| +func TestNoBlankLinesStillSplits(t *testing.T) { |
| + var b strings.Builder |
| + for i := 0; i < 4000; i++ { |
| + b.WriteString("alpha,beta,gamma,delta,epsilon\n") |
| + } |
| + got := Split(b.String()) |
| + if len(got) < 2 { |
| + t.Fatalf("a 4000-line CSV produced %d chunk(s); MaxWords is not binding", len(got)) |
| + } |
| + for _, c := range got { |
| + if n := len(strings.Fields(c.Text)); n > MaxWords*2 { |
| + t.Errorf("chunk %d has %d words, over twice the ceiling", c.Ordinal, n) |
| + } |
| + } |
| +} |
| + |
| +// No text may be lost. Every word of the input must appear in some chunk. |
| +func TestSplitLosesNoText(t *testing.T) { |
| + in := "# Top\n\nFirst para here.\n## Second\nNo blank line above.\n\n```\ncode # here\n```\n\nLast para." |
| + var joined string |
| + for _, c := range Split(in) { |
| + joined += " " + c.Text |
| + } |
| + for _, want := range []string{"First para here.", "No blank line above.", "code # here", "Last para."} { |
| + if !strings.Contains(joined, want) { |
| + t.Errorf("lost %q from the output", want) |
| + } |
| + } |
| +} |
diff --git a/internal/memoria/config.go b/internal/memoria/config.go| index 8757de5..0667c0b 100644 |
| --- a/internal/memoria/config.go |
| +++ b/internal/memoria/config.go |
| @@ -22,6 +22,12 @@ type Config struct { |
| InstanceKey string |
| Owners []string |
| Sealer *seal.Sealer |
| + // AllowLoggedMail permits booting with no SMTP, which makes |
| + // rastrillo substitute mail.Logged — and mail.Logged writes the |
| + // magic link, a live credential, into the process log at Warn. |
| + // `carlos logs` is platform-held storage we cannot grep, so that is |
| + // a credential we cannot retract. Opt in explicitly for local dev. |
| + AllowLoggedMail bool |
| } |
| // ErrMissing is returned when required platform config is absent. |
| @@ -60,6 +66,15 @@ func LoadConfig(getenv func(string) string) (Config, error) { |
| return c, fmt.Errorf("%w: MEMORIA_OWNERS (no admission gate)", ErrMissing) |
| } |
| + // Refuse to log credentials by default. MEMORIA_SMTP_HOST is what |
| + // rastrillo's mail.FromEnv reads; without it and without an explicit |
| + // opt-in, every sign-in link would be written to a log nobody can |
| + // delete. |
| + c.AllowLoggedMail = getenv("MEMORIA_ALLOW_LOGGED_MAIL") == "1" |
| + if getenv("MEMORIA_SMTP_HOST") == "" && !c.AllowLoggedMail { |
| + return c, fmt.Errorf("%w: MEMORIA_SMTP_HOST (or MEMORIA_ALLOW_LOGGED_MAIL=1 to log sign-in links instead, which puts a live credential in the process log)", ErrMissing) |
| + } |
| + |
| // Origin is the only one with a default, and only because the |
| // default is a loopback address that cannot be reached from |
| // anywhere else. It decides the CSRF yardstick and the cookie |
diff --git a/internal/memoria/handlers.go b/internal/memoria/handlers.go| index c06cc76..7093e6e 100644 |
| --- a/internal/memoria/handlers.go |
| +++ b/internal/memoria/handlers.go |
| @@ -10,8 +10,6 @@ import ( |
| "github.com/carlosframework/rastrillo/auth" |
| "github.com/carlosframework/rastrillo/db" |
| "github.com/go-chi/chi/v5" |
| - |
| - "amadan.net/oficina/memoria/internal/memoria/chunk" |
| ) |
| // maxUpload caps a pasted or uploaded document. |
| @@ -28,6 +26,11 @@ type app struct { |
| logger *slog.Logger |
| store *Store |
| auth *auth.Auth |
| + |
| + // allowUnauthenticated is set only by AppForTest. Production cannot |
| + // set it: cmd/memoria never calls that constructor, and |
| + // TestProductionDoesNotUseTheTestConstructor fails if it starts to. |
| + allowUnauthenticated bool |
| } |
| // UseAuth attaches auth after construction. App builds the router, and |
| @@ -112,6 +115,14 @@ func flashFor(code string) string { |
| } |
| func (a *app) createDocument(w http.ResponseWriter, r *http.Request) { |
| + // Cap the BODY before parsing it. ParseMultipartForm's argument is a |
| + // memory threshold, not a limit: without this, a 3 MiB part parses |
| + // with err=nil and Go has already written the whole thing to a temp |
| + // file in PLAINTEXT — measured, canary present — and a 10 GB upload |
| + // fills the disk. The LimitReader below runs far too late to prevent |
| + // either. Found by a code review; the comment that used to be here |
| + // described a cap that did not exist. |
| + r.Body = http.MaxBytesReader(w, r.Body, maxUpload+(64<<10)) |
| if err := r.ParseMultipartForm(maxUpload); err != nil { |
| http.Redirect(w, r, "/?err=toobig", http.StatusSeeOther) |
| return |
| @@ -202,17 +213,32 @@ func (a *app) showDocument(w http.ResponseWriter, r *http.Request) { |
| http.NotFound(w, r) |
| return |
| } |
| - var passages []passage |
| - for _, c := range chunk.Split(body) { |
| - passages = append(passages, passage{Ordinal: c.Ordinal, Heading: c.Heading(), Text: c.Text}) |
| + // Read the STORED chunks. Re-chunking here would derive ordinals |
| + // from the current chunker while every citation carries the ordinal |
| + // from the one that ran at ingest. |
| + stored, err := a.store.Passages(r.Context(), id) |
| + if err != nil { |
| + a.logger.Error("read passages", "id", id, "err", err) |
| + http.Error(w, "could not read that document", http.StatusInternalServerError) |
| + return |
| + } |
| + passages := make([]passage, 0, len(stored)) |
| + for _, p := range stored { |
| + passages = append(passages, passage{Ordinal: p.Ordinal, Heading: p.Heading, Text: p.Text}) |
| + } |
| + source, revision, nbytes, err := a.store.Meta(r.Context(), id) |
| + if err != nil { |
| + http.NotFound(w, r) |
| + return |
| } |
| + _ = body |
| render(w, "document", documentPage{ |
| Signed: true, |
| Chunks: a.chunkCount(), |
| Title: title, |
| - Source: "stored", |
| - Bytes: len(body), |
| - Revision: 1, |
| + Source: source, |
| + Bytes: nbytes, |
| + Revision: revision, |
| Passages: passages, |
| }) |
| } |
diff --git a/internal/memoria/index/index.go b/internal/memoria/index/index.go| index d06af09..30d3795 100644 |
| --- a/internal/memoria/index/index.go |
| +++ b/internal/memoria/index/index.go |
| @@ -245,3 +245,32 @@ func randomName() (string, error) { |
| } |
| return "memoria-index-" + hex.EncodeToString(b), nil |
| } |
| + |
| +// File reports the path SQLite has backing this index, empty for an |
| +// in-memory database. |
| +// |
| +// It exists so a test can prove §3.1's ruling rather than trust the DSN |
| +// string. A review changed the DSN to a file path and the entire suite |
| +// stayed green with plaintext FTS5 postings on disk, because the only |
| +// plaintext gate scanned the app database's directory and nothing looked |
| +// at the index at all. |
| +func (i *Index) File() (string, error) { |
| + i.mu.RLock() |
| + defer i.mu.RUnlock() |
| + rows, err := i.db.Query(`PRAGMA database_list`) |
| + if err != nil { |
| + return "", err |
| + } |
| + defer rows.Close() |
| + for rows.Next() { |
| + var seq int |
| + var name, file string |
| + if err := rows.Scan(&seq, &name, &file); err != nil { |
| + return "", err |
| + } |
| + if name == "main" { |
| + return file, nil |
| + } |
| + } |
| + return "", rows.Err() |
| +} |
diff --git a/internal/memoria/index/index_test.go b/internal/memoria/index/index_test.go| index d241c87..a9103b5 100644 |
| --- a/internal/memoria/index/index_test.go |
| +++ b/internal/memoria/index/index_test.go |
| @@ -195,3 +195,29 @@ func TestTwoIndexesAreIndependent(t *testing.T) { |
| t.Errorf("counts are A=%d B=%d, want 1 and 0", na, nb) |
| } |
| } |
| + |
| +// §3.1 rules that FTS5 postings are content, so the index must never be |
| +// backed by a file. Nothing tested that: a review pointed the DSN at a |
| +// path on disk and the entire suite stayed green with plaintext postings |
| +// in a file, because the only plaintext gate scanned the app database's |
| +// directory and nothing looked at the index at all. |
| +// |
| +// MUTATION-TESTED 2026-09-04: changing Open's DSN to |
| +// "file:"+os.TempDir()+"/x.db" makes this fail naming the path. Reverted. |
| +func TestIndexIsBackedByNoFile(t *testing.T) { |
| + ix, err := Open() |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + defer ix.Close() |
| + if err := ix.Add(Entry{ChunkID: 1, DocumentID: 1, Heading: "H", Text: "a passage of text"}); err != nil { |
| + t.Fatal(err) |
| + } |
| + file, err := ix.File() |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if file != "" { |
| + t.Fatalf("the index is backed by %q; FTS5 postings are plaintext and §3.1 forbids them reaching a file", file) |
| + } |
| +} |
diff --git a/internal/memoria/seal/seal.go b/internal/memoria/seal/seal.go| index ef858e8..1bae0ac 100644 |
| --- a/internal/memoria/seal/seal.go |
| +++ b/internal/memoria/seal/seal.go |
| @@ -18,8 +18,10 @@ package seal |
| import ( |
| "crypto/aes" |
| "crypto/cipher" |
| + "crypto/hmac" |
| "crypto/rand" |
| "crypto/sha256" |
| + "encoding/hex" |
| "errors" |
| "hash" |
| "io" |
| @@ -33,6 +35,7 @@ const ( |
| PurposeTitle = "document.title" |
| PurposeBody = "document.body" |
| PurposeSnapshot = "index.snapshot" |
| + PurposeExternal = "document.external_id" |
| ) |
| func sha256New() hash.Hash { return sha256.New() } |
| @@ -91,6 +94,25 @@ func (s *Sealer) Seal(purpose string, plaintext []byte) ([]byte, error) { |
| return aead.Seal(nonce, nonce, plaintext, nil), nil |
| } |
| +// Fingerprint returns a keyed, deterministic identifier for plaintext. |
| +// |
| +// It exists because a bare SHA-256 of a document body is a confirmation |
| +// oracle: anyone holding the bucket and a candidate document can hash it, |
| +// find the row, and learn that this team holds that file — plus its size, |
| +// its timestamps and its chunk count. A content hash is not a coordinate, |
| +// however much it looks like one, and §3.2's plaintext set does not admit |
| +// it. HMAC under a purpose key keeps the idempotency and removes the |
| +// oracle. |
| +func (s *Sealer) Fingerprint(purpose string, plaintext []byte) (string, error) { |
| + key, err := s.purposeKey(purpose) |
| + if err != nil { |
| + return "", err |
| + } |
| + m := hmac.New(sha256New, key) |
| + m.Write(plaintext) |
| + return hex.EncodeToString(m.Sum(nil)), nil |
| +} |
| + |
| // Open decrypts a value sealed under the same purpose. |
| func (s *Sealer) Open(purpose string, ct []byte) ([]byte, error) { |
| aead, err := s.aeadFor(purpose) |
diff --git a/internal/memoria/store.go b/internal/memoria/store.go| index a234343..804b4be 100644 |
| --- a/internal/memoria/store.go |
| +++ b/internal/memoria/store.go |
| @@ -2,8 +2,6 @@ package memoria |
| import ( |
| "context" |
| - "crypto/sha256" |
| - "encoding/hex" |
| "errors" |
| "fmt" |
| "strings" |
| @@ -51,12 +49,13 @@ type Summary struct { |
| Updated string |
| } |
| -// ContentHash is the ExternalID for pasted and uploaded text. Using the |
| -// hash means the same file ingested twice updates one document instead of |
| -// making two that disagree. |
| -func ContentHash(body string) string { |
| - sum := sha256.Sum256([]byte(body)) |
| - return hex.EncodeToString(sum[:]) |
| +// externalID is the stable key for pasted and uploaded text. |
| +// |
| +// 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)) |
| } |
| // Ingest stores a document and indexes it. |
| @@ -75,7 +74,10 @@ func (s *Store) Ingest(ctx context.Context, source, title, body string) (int64, |
| if title == "" { |
| title = firstLine(body) |
| } |
| - external := ContentHash(body) |
| + external, err := s.externalID(body) |
| + if err != nil { |
| + return 0, err |
| + } |
| sealedTitle, err := s.sealer.Seal(seal.PurposeTitle, []byte(title)) |
| if err != nil { |
| @@ -155,10 +157,17 @@ func (s *Store) Ingest(ctx context.Context, source, title, body string) (int64, |
| // transaction commits rather than inside it: a rolled-back |
| // transaction that had already mutated the index would leave the |
| // index claiming passages the database does not have. |
| + // |
| + // context.WithoutCancel because this runs AFTER the commit. With the |
| + // request's context, a client that closed the tab between commit and |
| + // index left the document stored and unsearchable, the footer count |
| + // disagreeing with the table, and the person told "that did not |
| + // work" about a document that exists. |
| + post := context.WithoutCancel(ctx) |
| if err := s.ix.RemoveDocument(docID); err != nil { |
| return 0, err |
| } |
| - return docID, s.indexDocument(ctx, docID) |
| + return docID, s.indexDocument(post, docID) |
| } |
| // indexDocument opens a document's chunks and adds them to the index. |
| @@ -299,6 +308,50 @@ func (s *Store) Body(ctx context.Context, id int64) (title, body string, err err |
| return string(t), string(b), nil |
| } |
| +// Passages returns a document's stored chunks, opened. |
| +// |
| +// The document page used to re-run the chunker at view time, which meant |
| +// every "#p{n}" anchor a search result linked to was derived from the |
| +// CURRENT chunker while the ordinal in the citation came from the one |
| +// that ran at ingest. Any change to chunking silently misaligned every |
| +// citation on every existing document — and this branch changed the |
| +// chunker twice. |
| +func (s *Store) Passages(ctx context.Context, id int64) ([]Passage, error) { |
| + var rows []Chunk |
| + if err := s.db.G.WithContext(ctx).Where("document_id = ?", id).Order("ordinal").Find(&rows).Error; err != nil { |
| + return nil, err |
| + } |
| + out := make([]Passage, 0, len(rows)) |
| + for _, r := range rows { |
| + text, err := s.sealer.Open(seal.PurposeChunk, r.SealedText) |
| + if err != nil { |
| + return nil, fmt.Errorf("open chunk %d: %w", r.ID, err) |
| + } |
| + heading, err := s.sealer.Open(seal.PurposeTitle, r.SealedHeading) |
| + if err != nil { |
| + return nil, fmt.Errorf("open chunk heading %d: %w", r.ID, err) |
| + } |
| + out = append(out, Passage{Ordinal: r.Ordinal, Heading: string(heading), Text: string(text)}) |
| + } |
| + return out, nil |
| +} |
| + |
| +// Passage is one stored chunk, opened for display. |
| +type Passage struct { |
| + Ordinal int |
| + Heading string |
| + Text string |
| +} |
| + |
| +// Meta returns a document's plaintext metadata. |
| +func (s *Store) Meta(ctx context.Context, id int64) (source string, revision, bytes int, err error) { |
| + var d Document |
| + if err := s.db.G.WithContext(ctx).Where("id = ?", id).First(&d).Error; err != nil { |
| + return "", 0, 0, err |
| + } |
| + return d.Source, d.Revision, d.Bytes, nil |
| +} |
| + |
| // Delete removes a document, its chunks, and its index entries. |
| func (s *Store) Delete(ctx context.Context, id int64) error { |
| if err := s.db.G.WithContext(ctx).Transaction(func(tx *gormTx) error { |
| @@ -312,6 +365,9 @@ func (s *Store) Delete(ctx context.Context, id int64) error { |
| return s.ix.RemoveDocument(id) |
| } |
| +// IndexFile reports the file backing the index, empty when in memory. |
| +func (s *Store) IndexFile() (string, error) { return s.ix.File() } |
| + |
| // Count returns the indexed passage count, for the UI to state what the |
| // corpus holds rather than implying it is complete. |
| func (s *Store) Count() (int, error) { return s.ix.Count() } |
diff --git a/internal/memoria/templates/layout.html b/internal/memoria/templates/layout.html| index 9cc503e..9ccb34d 100644 |
| --- a/internal/memoria/templates/layout.html |
| +++ b/internal/memoria/templates/layout.html |
| @@ -26,8 +26,9 @@ |
| </main> |
| <footer rst-page-footer> |
| <p rst-cell-mut> |
| - {{.Chunks}} passage{{if ne .Chunks 1}}s{{end}} indexed. |
| - Search is lexical only — no assistant, and nothing leaves this instance. |
| + {{if .Signed}}{{.Chunks}} passage{{if ne .Chunks 1}}s{{end}} indexed.{{end}} |
| + Search is lexical only — no assistant, and <strong>no document ever |
| + leaves this instance</strong>. |
| </p> |
| </footer> |
| </body> |
diff --git a/internal/memoriatest/app_test.go b/internal/memoriatest/app_test.go| index 9ae27e5..2686f90 100644 |
| --- a/internal/memoriatest/app_test.go |
| +++ b/internal/memoriatest/app_test.go |
| @@ -2,6 +2,7 @@ package memoriatest |
| import ( |
| "net/http" |
| + "regexp" |
| "strings" |
| "testing" |
| ) |
| @@ -106,12 +107,20 @@ func TestEmptySubmissionIsRefusedWithCopy(t *testing.T) { |
| } |
| } |
| -// The footer states the no-egress property on every screen. It is the |
| -// product's main honest claim in slice 0, so it is worth a test. |
| -func TestFooterStatesNoEgress(t *testing.T) { |
| +// The footer states slice 0's honest claim on every screen. |
| +// |
| +// This test used to assert "nothing leaves this instance", which is how a |
| +// false claim stayed put: the copy was wrong and the test held it there. |
| +// Sign-in fetches https://<the address's domain>/.well-known/keymail |
| +// before anyone is admitted, so something does leave. What is true, and |
| +// what the product can support, is that no *document* does. |
| +func TestFooterStatesTheClaimItCanSupport(t *testing.T) { |
| body := get(t, newApp(t), "/").Body.String() |
| - if !strings.Contains(body, "nothing leaves this instance") { |
| - t.Errorf("the page does not state the no-egress property:\n%s", body) |
| + if !strings.Contains(body, "no document ever") { |
| + t.Errorf("the page does not state the document-egress property:\n%s", body) |
| + } |
| + if strings.Contains(body, "nothing leaves this instance") { |
| + t.Error("the page overclaims: sign-in dials the address's domain") |
| } |
| } |
| @@ -136,3 +145,27 @@ func TestSnippetDoesNotRepeatTheHeading(t *testing.T) { |
| t.Errorf("the snippet repeats the heading before the body:\n%s", snippet) |
| } |
| } |
| + |
| +// A search result links to "#p{n}" using the ordinal stored at ingest. |
| +// The document page used to re-run the chunker at view time, so any |
| +// change to chunking silently misaligned every citation on every existing |
| +// document — and this branch changed the chunker twice. |
| +func TestCitedAnchorExistsOnTheDocumentPage(t *testing.T) { |
| + h := newApp(t) |
| + store(t, h, "Board pack", |
| + "## Revenue\nJune revenue was 1,350,000 across all regions.\n\n"+ |
| + "## Costs\nHosting fell after the migration and support was flat.\n\n"+ |
| + "## Risks\nOne customer is 22% of recurring revenue, which is the concentration risk.") |
| + |
| + results := get(t, h, "/?q=concentration").Body.String() |
| + m := regexp.MustCompile(`/documents/(\d+)#p(\d+)`).FindStringSubmatch(results) |
| + if m == nil { |
| + t.Fatalf("no citation anchor in the results:\n%s", results) |
| + } |
| + page := get(t, h, "/documents/"+m[1]).Body.String() |
| + anchor := `id="p` + m[2] + `"` |
| + if !strings.Contains(page, anchor) { |
| + t.Errorf("the search cited %s but the document page has no %s — "+ |
| + "the citation points at nothing", m[0], anchor) |
| + } |
| +} |
diff --git a/internal/memoriatest/gates_test.go b/internal/memoriatest/gates_test.go| index 796ba18..60a33be 100644 |
| --- a/internal/memoriatest/gates_test.go |
| +++ b/internal/memoriatest/gates_test.go |
| @@ -3,6 +3,7 @@ package memoriatest |
| import ( |
| "encoding/base64" |
| "errors" |
| + "net/http" |
| "os" |
| "path/filepath" |
| "regexp" |
| @@ -35,13 +36,23 @@ func TestPlaintextNeverReachesTheDatabase(t *testing.T) { |
| h, _ := newAppAt(t, path) |
| body := "# " + canary + "\n\nJune revenue was 1,350,000 and " + canary + " again in the body.\n" |
| - if rec := store(t, h, canary+" as a title", body); rec.Code != 303 { |
| + rec := store(t, h, canary+" as a title", body) |
| + // The vacuity guard, and it has to be this specific. An earlier |
| + // version checked `rec.Code != 303` and then searched for the string |
| + // "result" — but the handler redirects with 303 on FAILURE too |
| + // (/?err=store), and "result" is a substring of "0 results". With |
| + // Ingest mutated to always error, that version PASSED against an |
| + // empty database: the gate below was scanning a file with nothing in |
| + // it. Assert the success location and real result markup. |
| + if rec.Code != 303 { |
| t.Fatalf("store: status %d, want 303", rec.Code) |
| } |
| - // Prove the document is actually there, or this gate passes on an |
| - // empty database and proves nothing. |
| - if got := get(t, h, "/?q=zebrafinch").Body.String(); !strings.Contains(got, "result") { |
| - t.Fatal("the document was not stored or not indexed; the gate below would pass vacuously") |
| + if loc := rec.Header().Get("Location"); loc != "/?stored=1" { |
| + t.Fatalf("store redirected to %q, want /?stored=1 — the document was not stored, "+ |
| + "so the scan below would pass vacuously", loc) |
| + } |
| + if got := get(t, h, "/?q=zebrafinch").Body.String(); !strings.Contains(got, "<mark>") { |
| + t.Fatal("no highlighted result: the document was not indexed, and the scan below proves nothing") |
| } |
| entries, err := os.ReadDir(dir) |
| @@ -79,45 +90,142 @@ func TestPlaintextNeverReachesTheDatabase(t *testing.T) { |
| // MUTATION-TESTED 2026-09-03: adding `http.Get("https://example.com")` |
| // to Store.Ingest makes this fail naming store.go. Reverted. |
| func TestNoOutboundNetworkInAppCode(t *testing.T) { |
| + // A blacklist, and it is worth saying so: this is a source |
| + // heuristic, not a proof. A review defeated the first version with |
| + // `new(http.Client).Post(...)`, which none of six spellings matched. |
| + // The list is broader now and the walk covers cmd/ and tools/ too, |
| + // but the honest statement of what it buys is "an accidental |
| + // outbound call is caught"; a determined one is not. |
| forbidden := []string{ |
| - `net/http".*Get\(`, |
| - `http\.Get\(`, |
| - `http\.Post\(`, |
| - `http\.Client\{`, |
| - `http\.DefaultClient`, |
| - `net\.Dial`, |
| - `smtp\.`, |
| + `http\.Get\(`, `http\.Post\(`, `http\.PostForm\(`, `http\.Head\(`, |
| + `http\.Client\{`, `new\(http\.Client\)`, `\*http\.Client`, |
| + `http\.DefaultClient`, `http\.DefaultTransport`, `http\.NewRequest`, |
| + `net\.Dial`, `net\.Lookup`, `smtp\.`, `os/exec`, `exec\.Command`, |
| + `url\.Parse\(.*https?://`, |
| } |
| pats := make([]*regexp.Regexp, 0, len(forbidden)) |
| for _, f := range forbidden { |
| pats = append(pats, regexp.MustCompile(f)) |
| } |
| - root := filepath.Join("..", "memoria") |
| var hits []string |
| - err := filepath.Walk(root, func(p string, info os.FileInfo, err error) error { |
| - if err != nil { |
| - return err |
| - } |
| - if info.IsDir() || !strings.HasSuffix(p, ".go") || strings.HasSuffix(p, "_test.go") { |
| + walk := func(root string) error { |
| + return filepath.Walk(root, func(p string, info os.FileInfo, err error) error { |
| + if err != nil { |
| + return err |
| + } |
| + if info.IsDir() || !strings.HasSuffix(p, ".go") || strings.HasSuffix(p, "_test.go") { |
| + return nil |
| + } |
| + src, err := os.ReadFile(p) |
| + if err != nil { |
| + return err |
| + } |
| + for _, pat := range pats { |
| + if pat.Match(src) { |
| + hits = append(hits, p+" matches "+pat.String()) |
| + } |
| + } |
| return nil |
| + }) |
| + } |
| + for _, root := range []string{ |
| + filepath.Join("..", "memoria"), |
| + filepath.Join("..", "..", "cmd"), |
| + } { |
| + if err := walk(root); err != nil { |
| + t.Fatal(err) |
| + } |
| + } |
| + for _, h := range hits { |
| + t.Errorf("app code can reach the network: %s", h) |
| + } |
| +} |
| + |
| +// The footer says no DOCUMENT leaves this instance, and that is the claim |
| +// this file can support. It deliberately no longer says "nothing leaves", |
| +// because that was false: rastrillo's sign-in classifier fetches |
| +// https://<the address's domain>/.well-known/keymail on POST /auth/begin, |
| +// BEFORE the admission gate — so an anonymous visitor makes the instance |
| +// dial any domain they type. Found by a code review of this branch. |
| +// |
| +// This gate pins the copy, so nobody restores the stronger sentence |
| +// without also removing the dial. |
| +func TestFooterDoesNotOverclaim(t *testing.T) { |
| + raw, err := os.ReadFile(filepath.Join("..", "memoria", "templates", "layout.html")) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + src := string(raw) |
| + if strings.Contains(src, "nothing leaves this instance") { |
| + t.Error("the footer claims nothing leaves this instance; sign-in dials the " + |
| + "address's domain before anyone is admitted, so that is false") |
| + } |
| + if !strings.Contains(src, "no document ever") { |
| + t.Error("the footer no longer makes the claim this app can actually support") |
| + } |
| +} |
| + |
| +// Gate 4, absent until now: no content in any log line. §11 said slice 0 |
| +// ships it. It did not, which is how a mailer that logs the magic link |
| +// went unnoticed. |
| +// |
| +// MUTATION-TESTED 2026-09-04: logging the title in Store.Ingest's error |
| +// path, or the query in the index handler, makes this fail. Reverted. |
| +func TestNoContentInLogs(t *testing.T) { |
| + h, logs := newAppWithLog(t) |
| + const secret = "WOMBATCONFIDENTIAL9931" |
| + store(t, h, secret+" title", "## "+secret+"\n\nA passage about "+secret+" and revenue.") |
| + get(t, h, "/?q="+secret) |
| + get(t, h, "/?q=revenue") |
| + got := logs.String() |
| + if strings.Contains(got, secret) { |
| + t.Errorf("a log line carries document content or the query:\n%s", got) |
| + } |
| + for _, bad := range []string{"sign-in link", "auth/verify?"} { |
| + if strings.Contains(got, bad) { |
| + t.Errorf("a log line carries a credential (%q):\n%s", bad, got) |
| + } |
| + } |
| +} |
| + |
| +// requireSession fails closed. An app built without auth must refuse |
| +// every route rather than serve it. |
| +// |
| +// MUTATION-TESTED 2026-09-04: restoring the nil-auth pass-through makes |
| +// this fail. Reverted. |
| +func TestUnauthenticatedAppRefuses(t *testing.T) { |
| + h := newProductionApp(t) |
| + for _, target := range []string{"/", "/documents/1", "/?q=revenue"} { |
| + rec := get(t, h, target) |
| + if rec.Code == http.StatusOK { |
| + t.Errorf("GET %s served 200 with no sign-in configured; requireSession fails open", target) |
| + } |
| + } |
| +} |
| + |
| +// The test seam must stay in tests. Production wiring may not reach |
| +// AppForTest, and this is the gate that the previous version of this file |
| +// only claimed to have. |
| +func TestProductionDoesNotUseTheTestConstructor(t *testing.T) { |
| + err := filepath.Walk(filepath.Join("..", "..", "cmd"), func(p string, info os.FileInfo, err error) error { |
| + if err != nil || info.IsDir() || !strings.HasSuffix(p, ".go") { |
| + return err |
| } |
| - src, err := os.ReadFile(p) |
| + raw, err := os.ReadFile(p) |
| if err != nil { |
| return err |
| } |
| - for _, pat := range pats { |
| - if pat.Match(src) { |
| - hits = append(hits, p+" matches "+pat.String()) |
| - } |
| + if strings.Contains(string(raw), "AppForTest") { |
| + t.Errorf("%s calls AppForTest; production would serve every route unauthenticated", p) |
| + } |
| + if !strings.Contains(string(raw), "UseAuth") && strings.Contains(string(raw), "memoria.App(") { |
| + t.Errorf("%s builds the app but never calls UseAuth", p) |
| } |
| return nil |
| }) |
| if err != nil { |
| t.Fatal(err) |
| } |
| - for _, h := range hits { |
| - t.Errorf("app code can reach the network: %s", h) |
| - } |
| } |
| // Every colour in the app's own stylesheet is a token, so the reader's |
| @@ -163,6 +271,7 @@ func TestConfigRefusesIncompleteEnvironment(t *testing.T) { |
| "MEMORIA_INSTANCE_KEY": "instance-key", |
| "MEMORIA_OWNERS": "paul@example.com", |
| "MEMORIA_ORIGIN": "https://memoria.example.com", |
| + "MEMORIA_SMTP_HOST": "smtp.example.com", |
| } |
| // The complete environment must load, or every subtest below passes |
| // for the wrong reason. |
| @@ -170,7 +279,7 @@ func TestConfigRefusesIncompleteEnvironment(t *testing.T) { |
| t.Fatalf("a complete environment did not load: %v", err) |
| } |
| - for _, missing := range []string{"MEMORIA_DEK", "MEMORIA_INSTANCE_KEY", "MEMORIA_OWNERS"} { |
| + for _, missing := range []string{"MEMORIA_DEK", "MEMORIA_INSTANCE_KEY", "MEMORIA_OWNERS", "MEMORIA_SMTP_HOST"} { |
| t.Run("without "+missing, func(t *testing.T) { |
| env := map[string]string{} |
| for k, v := range full { |
| @@ -208,6 +317,7 @@ func TestConfigRefusesMalformedDEK(t *testing.T) { |
| "MEMORIA_DEK": dek, |
| "MEMORIA_INSTANCE_KEY": "k", |
| "MEMORIA_OWNERS": "a@b.c", |
| + "MEMORIA_SMTP_HOST": "smtp.example.com", |
| })) |
| if err == nil { |
| t.Fatalf("LoadConfig accepted a %s DEK", name) |
| @@ -224,6 +334,7 @@ func TestConfigFlagsADefaultedOrigin(t *testing.T) { |
| "MEMORIA_DEK": base64.StdEncoding.EncodeToString(make([]byte, 32)), |
| "MEMORIA_INSTANCE_KEY": "k", |
| "MEMORIA_OWNERS": "a@b.c", |
| + "MEMORIA_SMTP_HOST": "smtp.example.com", |
| })) |
| if err != nil { |
| t.Fatal(err) |
| @@ -235,6 +346,7 @@ func TestConfigFlagsADefaultedOrigin(t *testing.T) { |
| "MEMORIA_DEK": base64.StdEncoding.EncodeToString(make([]byte, 32)), |
| "MEMORIA_INSTANCE_KEY": "k", |
| "MEMORIA_OWNERS": "a@b.c", |
| + "MEMORIA_SMTP_HOST": "smtp.example.com", |
| "MEMORIA_ORIGIN": "https://real.example.com", |
| })) |
| if err != nil { |
| @@ -245,6 +357,23 @@ func TestConfigFlagsADefaultedOrigin(t *testing.T) { |
| } |
| } |
| +// The explicit opt-in must work, or local development is impossible and |
| +// somebody will delete the refusal instead. |
| +func TestConfigAllowsLoggedMailWhenAskedExplicitly(t *testing.T) { |
| + cfg, err := memoria.LoadConfig(envFrom(map[string]string{ |
| + "MEMORIA_DEK": base64.StdEncoding.EncodeToString(make([]byte, 32)), |
| + "MEMORIA_INSTANCE_KEY": "k", |
| + "MEMORIA_OWNERS": "a@b.c", |
| + "MEMORIA_ALLOW_LOGGED_MAIL": "1", |
| + })) |
| + if err != nil { |
| + t.Fatalf("the explicit dev opt-in was refused: %v", err) |
| + } |
| + if !cfg.AllowLoggedMail { |
| + t.Error("AllowLoggedMail is false after opting in; main would not warn") |
| + } |
| +} |
| + |
| func envFrom(m map[string]string) func(string) string { |
| return func(k string) string { return m[k] } |
| } |
diff --git a/internal/memoriatest/harness_test.go b/internal/memoriatest/harness_test.go| index 5b15a55..4cd7e7a 100644 |
| --- a/internal/memoriatest/harness_test.go |
| +++ b/internal/memoriatest/harness_test.go |
| @@ -1,11 +1,16 @@ |
| // Package memoriatest tests the app from the outside: real HTTP requests |
| // against the real router, wired as cmd/memoria/main.go wires it. |
| // |
| -// The one difference from production is auth: these apps are built with |
| -// no auth, which makes requireSession a pass-through. That is the only |
| -// way to exercise the routes without a mailer and a magic-link round |
| -// trip, and TestProductionRefusesToStartWithoutSignin is what stops the |
| -// seam from being a hole. |
| +// The one difference from production is auth: these apps are built via |
| +// memoria.AppForTest, which sets an explicit allowUnauthenticated flag. |
| +// |
| +// An earlier version relied on requireSession falling through when auth |
| +// was nil, and justified it by naming a test that did not exist — the |
| +// name survived a rename and lived on only in comments. Deleting main's |
| +// UseAuth call therefore compiled and left the suite green while serving |
| +// every route unauthenticated. requireSession now fails closed, the seam |
| +// is named in the constructor, and TestUnauthenticatedAppRefuses and |
| +// TestProductionDoesNotUseTheTestConstructor hold both ends. |
| package memoriatest |
| import ( |
| @@ -59,11 +64,46 @@ func newAppAt(t *testing.T, path string) (http.Handler, string) { |
| t.Fatalf("db.Open: %v", err) |
| } |
| t.Cleanup(func() { d.Close() }) |
| + mux, _, err := memoria.AppForTest(d, testOrigin, testDEK(t), logger) |
| + if err != nil { |
| + t.Fatalf("AppForTest: %v", err) |
| + } |
| + return mux, path |
| +} |
| + |
| +// newAppWithLog is newApp with the log captured, for the gate that |
| +// asserts no content reaches a log line. |
| +func newAppWithLog(t *testing.T) (http.Handler, *bytes.Buffer) { |
| + t.Helper() |
| + var buf bytes.Buffer |
| + logger := slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelDebug})) |
| + d, err := db.Open(filepath.Join(t.TempDir(), "app.db"), logger) |
| + if err != nil { |
| + t.Fatalf("db.Open: %v", err) |
| + } |
| + t.Cleanup(func() { d.Close() }) |
| + mux, _, err := memoria.AppForTest(d, testOrigin, testDEK(t), logger) |
| + if err != nil { |
| + t.Fatalf("AppForTest: %v", err) |
| + } |
| + return mux, &buf |
| +} |
| + |
| +// newProductionApp builds the app the way main.go does — no test seam — |
| +// so a gate can prove the routes refuse without sign-in. |
| +func newProductionApp(t *testing.T) http.Handler { |
| + t.Helper() |
| + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) |
| + d, err := db.Open(filepath.Join(t.TempDir(), "app.db"), logger) |
| + if err != nil { |
| + t.Fatalf("db.Open: %v", err) |
| + } |
| + t.Cleanup(func() { d.Close() }) |
| mux, _, err := memoria.App(d, testOrigin, testDEK(t), logger) |
| if err != nil { |
| t.Fatalf("App: %v", err) |
| } |
| - return mux, path |
| + return mux |
| } |
| func get(t *testing.T, h http.Handler, target string) *httptest.ResponseRecorder { |