amadan
🤖 fix: the second review's twelve findings, five of them live defects
Fable 5.1 reviewed the roster, the poll loop and the warm-up. Twelve mutations, seven survived; ten probes, five of which found defects in shipped behaviour rather than gaps in gates. **The roster accepted addresses the transport refuses on every call — and the documented example was one.** Suite hosts are `<team>.<app>`, team first (§6.3). This file's own example, and every fixture, had the labels reversed, and nothing checked. An instance configured from the documentation boots, logs "siblings configured", and then has every S1, S2 and S3 refused *before dialling* — not even a timeout. The reader gets "Docs did not answer; N results withheld" on every query forever, naming an app that is working perfectly, with nothing in the logs but "sibling poll failed". That is exactly the failure commit 3fdfb9e claims to turn into a boot error, arriving through the one field nobody was checking. `LoadSiblings` now calls the module's own `CheckSiblingHost`. **A single empty item wedged a sibling's feed permanently.** `applyRow` passed `AllowTitleOnly: true` with no Title — S3 gives a body and nothing else — so Ingest refused with "neither a body nor a title", the row failed, the cursor stayed, and *every subsequent poll failed on the same row*. One stub page in Docs and nothing from Docs is ever ingested again. The comment claimed this was the case it prevented. Empty items are now skipped and counted. **A revision bump on an identical body left `SiblingRevision` stale, and the consequence is permanent.** Ingest's unchanged branch returned without recording it, so a sibling moving 7→8 on a body it did not change — a permissions edit, a re-save — left memoria at 7. Every query then asks S2 about `(item, 7)`, the sibling correctly answers STALE, and the passage is withheld loudly forever. Re-polling cannot fix it, because the body never changes. **`PolledAt` claimed currency after a partial poll.** It was written by `setCursor`, once per page, so a catch-up failing on page four left it at the last page it managed. A position is "how far I got"; a timestamp is "and there was nothing after it". Only the drained-feed branch may say the second. The old test only ever failed on page one, so deleting the write entirely stayed green — the absence gate had no presence half. **Error strings carried the cursor and the item id.** `*url.Error`'s message is `Get "<the whole URL>": <cause>`, and S1's URL holds the cursor, S3's the item id — both sealed at rest. Nothing logs them today, which is the reason to fix it now: the next caller to log one has no reason to suspect it. The cause survives; the URL does not. The rest: - **The wiring moved out of `main.go`.** The AST gate proves a call is *spelled*, not reached: a review wrapped `app.UseAuth(a)` in a closure nobody invoked, and put the sibling wiring behind `if len(apps) > 1` so a one-sibling instance ran with nil custody — both green. A cleverer walk is a race against however the next person writes it, so there is now one call to `memoria.WireSiblings`, which has tests. - Two siblings in one store keep their own cursor and their own documents — dropping either scope was green for want of a fixture. - A re-announced tombstone deletes nothing and says so; a 403 on S3 is a contract breach, not a missing item. - "Docs" and "docs" both read `MEMORIA_SIBLING_TOKEN_DOCS`, so one app received the other's credential. The duplicate check keys on the token variable now. - A malformed roster entry is named by position, never echoed: main logs that error, and the entry sits beside the token variables. - The sort fixture could not tell "by name" from "by host" — both sorted identically. The names and hosts now sort in opposite orders. - Three comments corrected that described what the code does not do, and **spec §6.5 amended**: it still stated gate 24 as "before the query handler serves its first request", the ordering §10's own gate 24 works through and rejects as unwritable. The code cites §10; the spec disagreed with itself. Eighteen mutations re-run against the fixes, all red on real test failures rather than compile errors, none surviving. `make ci` green, `-race -count=2` clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H3VKtXnq6fSmM2F11Bjcsy
13 files changed,
+925
−89
cmd/memoria/main.go+14 −29docs/superpowers/specs/2026-09-03-memoria-design.md+10 −1internal/memoria/config_siblings.go+59 −10internal/memoria/config_siblings_test.go+104 −13internal/memoria/custody.go+46 −0internal/memoria/models.go+8 −5internal/memoria/sibling/feed.go+28 −2internal/memoria/sibling/feed_test.go+57 −0internal/memoria/siblingsync.go+42 −14internal/memoria/siblingsync_test.go+402 −6internal/memoria/store.go+40 −6internal/memoria/warmup_test.go+100 −2internal/memoriatest/gates_test.go+15 −1
diff --git a/cmd/memoria/main.go b/cmd/memoria/main.go| index 4255a1c..01d4fe7 100644 |
| --- a/cmd/memoria/main.go |
| +++ b/cmd/memoria/main.go |
| @@ -3,7 +3,6 @@ |
| package main |
| import ( |
| - "context" |
| "log/slog" |
| "os" |
| @@ -89,38 +88,24 @@ func main() { |
| // The apps of record memoria may ask about passages (§4.1). |
| // |
| - // Fatal on error, like every other refusal in this file, and for the |
| - // characteristic reason: a half-configured roster does not fail |
| - // loudly at query time. A sibling with no token cannot be asked, a |
| - // sibling that cannot be asked withholds every passage it owns, and |
| - // what a reader sees is a permanently short page blaming an app that |
| - // is working fine. Boot is the only place that failure can be said |
| - // out loud. |
| + // One call, deliberately: the roster, the custody, the log line and |
| + // the wake poll all live in WireSiblings, where they have tests. This |
| + // file's guard is a gate over its own text, and a gate over text can |
| + // only see that a call is spelled — a review put the same wiring |
| + // behind `if len(apps) > 1` and inside an uninvoked closure, and both |
| + // read as present. Logic that matters does not live here. |
| // |
| - // An EMPTY roster is not an error and not a warning: slice 0 holds |
| - // only memoria-owned content (§4.2), which has no app of record. |
| - sibs, err := memoria.LoadSiblings(os.Getenv) |
| - if err != nil { |
| + // Fatal on error, like every other refusal in this file: a |
| + // half-configured roster does not fail loudly at query time, it |
| + // serves a permanently short page blaming an app that is working |
| + // fine. An EMPTY roster is not a failure. |
| + // |
| + // nil dial: the module's team-bound transport does its own host check |
| + // before dialling. The hook is for tests. |
| + if err := memoria.WireSiblings(app, os.Getenv, nil); err != nil { |
| logger.Error("sibling roster", "err", err) |
| os.Exit(1) |
| } |
| - if len(sibs.Apps) > 0 { |
| - // nil dial: the module's team-bound transport does its own host |
| - // check before dialling. The hook is for tests. |
| - custody, err := memoria.NewSiblings(sibs.Team, nil, sibs.Apps) |
| - if err != nil { |
| - logger.Error("sibling roster", "err", err) |
| - os.Exit(1) |
| - } |
| - app.UseSiblings(custody) |
| - // §6.5's wake poll, started here and not awaited: the point is to |
| - // spend a cold sibling's latency where nobody is waiting, and |
| - // waiting for it would put that latency back on the first reader. |
| - app.WarmSiblings(context.Background()) |
| - } |
| - // Names, never the roster itself: the structs carry consumer tokens, |
| - // and `carlos logs` is storage we cannot grep. |
| - logger.Info("siblings configured", "team", sibs.Team, "apps", sibs.Names()) |
| opts.Mux = mux |
| opts.DBPath = "" |
diff --git a/docs/superpowers/specs/2026-09-03-memoria-design.md b/docs/superpowers/specs/2026-09-03-memoria-design.md| index 5ce68e2..84a1e50 100644 |
| --- a/docs/superpowers/specs/2026-09-03-memoria-design.md |
| +++ b/docs/superpowers/specs/2026-09-03-memoria-design.md |
| @@ -1914,7 +1914,16 @@ because either alone passes on the other's failure: a sibling sleeping |
| past the deadline must not add its latency to the page, *and* three |
| siblings called at once must not cost three times one sibling's time. |
| Gate 24 holds the warm-up: every configured sibling receives its S1 poll |
| -before the query handler serves its first request. |
| +**unprompted, within a stated few seconds of the app coming up, with no |
| +query issued at all.** |
| + |
| +*This read "before the query handler serves its first request" until |
| +2026-09-07, which is the ordering §10's own gate 24 works through and |
| +rejects as unwritable either way — asynchronous it is a race between |
| +goroutines, synchronous it puts three sibling wakes in front of the first |
| +reader and undoes the warm-up's entire purpose. §10 had the corrected |
| +form; this sentence did not, and the code cites §10. Found by a review of |
| +the branch that built it.* |
| --- |
diff --git a/internal/memoria/config_siblings.go b/internal/memoria/config_siblings.go| index 24af43c..2390ede 100644 |
| --- a/internal/memoria/config_siblings.go |
| +++ b/internal/memoria/config_siblings.go |
| @@ -2,9 +2,12 @@ package memoria |
| import ( |
| "fmt" |
| + "net/url" |
| "sort" |
| "strings" |
| + "amadan.net/oficina/suite" |
| + |
| "amadan.net/oficina/memoria/internal/memoria/sibling" |
| ) |
| @@ -29,10 +32,15 @@ type SiblingConfig struct { |
| // LoadSiblings reads the roster. |
| // |
| // MEMORIA_TEAM=acme |
| -// MEMORIA_SIBLINGS=Docs=https://docs.acme.oficina.works,Sheets=https://sheets.acme.oficina.works |
| +// MEMORIA_SIBLINGS=Docs=https://acme.docs.oficina.works,Sheets=https://acme.sheets.oficina.works |
| // MEMORIA_SIBLING_TOKEN_DOCS=... |
| // MEMORIA_SIBLING_TOKEN_SHEETS=... |
| // |
| +// **The host is `<team>.<app>`, team first** (§6.3). This comment had the |
| +// labels the other way round, and so did every fixture — which meant the |
| +// roster accepted an address the transport refuses on every single call. |
| +// See the host check below for what that costs. |
| +// |
| // **No siblings is a valid configuration and not a warning.** Slice 0 |
| // holds only memoria-owned content (§4.2), which has no app of record to |
| // ask, so an empty roster is the ordinary state of an instance that has |
| @@ -57,8 +65,17 @@ func LoadSiblings(getenv func(string) string) (SiblingConfig, error) { |
| return c, nil |
| } |
| + // Checked before the loop, not after it. The per-sibling host check |
| + // below needs a team to compare against, so without this an instance |
| + // missing MEMORIA_TEAM fails with the module's "no team configured" |
| + // — accurate, and not the message that tells an operator which |
| + // variable to set. |
| + if c.Team == "" { |
| + return c, fmt.Errorf("%w: MEMORIA_TEAM, which binds the sibling transport to one team's hosts (§6.4). Without it there is nothing for the host guard to compare against", ErrMissing) |
| + } |
| + |
| seen := map[string]bool{} |
| - for _, entry := range strings.Split(raw, ",") { |
| + for i, entry := range strings.Split(raw, ",") { |
| entry = strings.TrimSpace(entry) |
| if entry == "" { |
| continue |
| @@ -66,19 +83,54 @@ func LoadSiblings(getenv func(string) string) (SiblingConfig, error) { |
| name, base, ok := strings.Cut(entry, "=") |
| name, base = strings.TrimSpace(name), strings.TrimSpace(base) |
| if !ok || name == "" || base == "" { |
| - return c, fmt.Errorf("%w: MEMORIA_SIBLINGS entry %q is not name=url", ErrMissing, entry) |
| + // **The entry is counted, not echoed.** main logs this error, |
| + // and MEMORIA_SIBLINGS sits beside the token variables: one |
| + // transposed assignment and a malformed "entry" is a bearer |
| + // token in `carlos logs`, which is storage we cannot delete. |
| + // The position is enough to find it in the variable. |
| + return c, fmt.Errorf("%w: MEMORIA_SIBLINGS entry %d is not name=url", ErrMissing, i+1) |
| } |
| - if seen[name] { |
| - return c, fmt.Errorf("memoria: MEMORIA_SIBLINGS lists %q twice; the name is the key a document's source resolves through", name) |
| + // **Keyed on the token variable, not on the name**, because that |
| + // is the collision that matters: "Docs" and "docs" are different |
| + // map keys and different Document.Source values, but they read |
| + // the same MEMORIA_SIBLING_TOKEN_DOCS — so one app silently |
| + // receives the other's credential. Keying on the name alone |
| + // missed it, and a review found it. |
| + if seen[tokenVar(name)] { |
| + return c, fmt.Errorf("memoria: MEMORIA_SIBLINGS lists two apps that resolve to %s; the name is the key a document's source resolves through, and the token variable is derived from it", tokenVar(name)) |
| } |
| - seen[name] = true |
| + seen[tokenVar(name)] = true |
| // https only, and said here rather than left to the transport. |
| // A consumer token on a cleartext hop is a credential handed to |
| // anyone on the path, and the roster is where the address is |
| // chosen. |
| if !strings.HasPrefix(base, "https://") { |
| - return c, fmt.Errorf("memoria: sibling %q is configured as %q; a consumer token may not travel over cleartext", name, base) |
| + return c, fmt.Errorf("memoria: sibling %q is not https", name) |
| + } |
| + |
| + // **The address must be one the transport will actually dial**, |
| + // checked here with the module's own predicate rather than with a |
| + // second implementation of it. |
| + // |
| + // This was missing, and the cost was the exact failure this |
| + // function exists to prevent. `CheckSiblingHost` requires |
| + // `<team>.<app>.oficina.works` — **team first** — and this file's |
| + // own documented example had the labels reversed. An instance |
| + // configured from it boots, logs "siblings configured", and then |
| + // has every S1, S2 and S3 refused before the transport dials. |
| + // What a reader sees is "Docs did not answer; N results withheld" |
| + // on every query forever, naming an app that is working |
| + // perfectly, and there is nothing in the logs but "sibling poll |
| + // failed". A boot failure is the whole point of the paragraph |
| + // above; without this check it was a boot failure that did not |
| + // happen. |
| + u, err := url.Parse(base) |
| + if err != nil { |
| + return c, fmt.Errorf("memoria: sibling %q has an unparseable address", name) |
| + } |
| + if err := suite.CheckSiblingHost(c.Team, u.Hostname()); err != nil { |
| + return c, fmt.Errorf("memoria: sibling %q: %w", name, err) |
| } |
| token := strings.TrimSpace(getenv(tokenVar(name))) |
| @@ -88,9 +140,6 @@ func LoadSiblings(getenv func(string) string) (SiblingConfig, error) { |
| c.Apps = append(c.Apps, sibling.Sibling{Name: name, BaseURL: strings.TrimRight(base, "/"), Token: token}) |
| } |
| - if len(c.Apps) > 0 && c.Team == "" { |
| - return c, fmt.Errorf("%w: MEMORIA_TEAM, which binds the sibling transport to one team's hosts (§6.4). Without it there is nothing for the guard to compare a host against", ErrMissing) |
| - } |
| sort.Slice(c.Apps, func(i, j int) bool { return c.Apps[i].Name < c.Apps[j].Name }) |
| return c, nil |
| } |
diff --git a/internal/memoria/config_siblings_test.go b/internal/memoria/config_siblings_test.go| index 08241dc..6897b0c 100644 |
| --- a/internal/memoria/config_siblings_test.go |
| +++ b/internal/memoria/config_siblings_test.go |
| @@ -36,6 +36,15 @@ func env(pairs map[string]string) func(string) string { |
| // -> the ordering assertion red |
| // require MEMORIA_TEAM even with no siblings |
| // -> the empty-roster case red, which is slice 0's whole config |
| +// |
| +// MUTATION-TESTED 2026-09-07, after a review found the host check missing: |
| +// |
| +// drop the CheckSiblingHost call |
| +// -> the swapped-label case red |
| +// key the duplicate check on the name instead of the token variable |
| +// -> the case-collision case red |
| +// echo the raw entry into the malformed-entry error |
| +// -> the no-echo case red |
| func TestLoadSiblings(t *testing.T) { |
| t.Run("no siblings is a configuration, not a warning", func(t *testing.T) { |
| got, err := LoadSiblings(env(nil)) |
| @@ -50,7 +59,7 @@ func TestLoadSiblings(t *testing.T) { |
| t.Run("a sibling with no token is refused", func(t *testing.T) { |
| _, err := LoadSiblings(env(map[string]string{ |
| "MEMORIA_TEAM": "acme", |
| - "MEMORIA_SIBLINGS": "Docs=https://docs.acme.oficina.works", |
| + "MEMORIA_SIBLINGS": "Docs=https://acme.docs.oficina.works", |
| })) |
| if !errors.Is(err, ErrMissing) { |
| t.Fatalf("err = %v, want ErrMissing", err) |
| @@ -63,7 +72,7 @@ func TestLoadSiblings(t *testing.T) { |
| t.Run("a sibling on cleartext is refused", func(t *testing.T) { |
| _, err := LoadSiblings(env(map[string]string{ |
| "MEMORIA_TEAM": "acme", |
| - "MEMORIA_SIBLINGS": "Docs=http://docs.acme.oficina.works", |
| + "MEMORIA_SIBLINGS": "Docs=http://acme.docs.oficina.works", |
| "MEMORIA_SIBLING_TOKEN_DOCS": "t", |
| "MEMORIA_SIBLING_TOKEN_SHEET": "t", |
| })) |
| @@ -75,7 +84,7 @@ func TestLoadSiblings(t *testing.T) { |
| t.Run("a duplicate name is refused", func(t *testing.T) { |
| _, err := LoadSiblings(env(map[string]string{ |
| "MEMORIA_TEAM": "acme", |
| - "MEMORIA_SIBLINGS": "Docs=https://a.acme.oficina.works,Docs=https://b.acme.oficina.works", |
| + "MEMORIA_SIBLINGS": "Docs=https://acme.docs.oficina.works,Docs=https://acme.sheets.oficina.works", |
| "MEMORIA_SIBLING_TOKEN_DOCS": "t", |
| })) |
| if err == nil { |
| @@ -87,7 +96,7 @@ func TestLoadSiblings(t *testing.T) { |
| t.Run("siblings without a team are refused", func(t *testing.T) { |
| _, err := LoadSiblings(env(map[string]string{ |
| - "MEMORIA_SIBLINGS": "Docs=https://docs.acme.oficina.works", |
| + "MEMORIA_SIBLINGS": "Docs=https://acme.docs.oficina.works", |
| "MEMORIA_SIBLING_TOKEN_DOCS": "t", |
| })) |
| if !errors.Is(err, ErrMissing) { |
| @@ -98,10 +107,15 @@ func TestLoadSiblings(t *testing.T) { |
| t.Run("a good roster parses, or none of the above proves anything", func(t *testing.T) { |
| got, err := LoadSiblings(env(map[string]string{ |
| - "MEMORIA_TEAM": "acme", |
| - "MEMORIA_SIBLINGS": " Sheets=https://sheets.acme.oficina.works/ , Docs=https://docs.acme.oficina.works ", |
| - "MEMORIA_SIBLING_TOKEN_DOCS": "docs-token", |
| - "MEMORIA_SIBLING_TOKEN_SHEETS": "sheets-token", |
| + "MEMORIA_TEAM": "acme", |
| + // **The names and the hosts sort in OPPOSITE orders**, on |
| + // purpose: with Docs/Sheets against acme.docs/acme.sheets the |
| + // two orderings are identical, so sorting by BaseURL passes — |
| + // a review's mutation did. Here "Archive" sorts first by name |
| + // and its host sorts last. |
| + "MEMORIA_SIBLINGS": " Sheets=https://acme.sheets.oficina.works/ , Archive=https://acme.zzz.oficina.works ", |
| + "MEMORIA_SIBLING_TOKEN_ARCHIVE": "archive-token", |
| + "MEMORIA_SIBLING_TOKEN_SHEETS": "sheets-token", |
| })) |
| if err != nil { |
| t.Fatalf("a good roster was refused: %v", err) |
| @@ -111,20 +125,97 @@ func TestLoadSiblings(t *testing.T) { |
| } |
| // Sorted, so a log line and a page's notices do not depend on the |
| // order somebody happened to type the variable in. |
| - if len(got.Apps) != 2 || got.Apps[0].Name != "Docs" || got.Apps[1].Name != "Sheets" { |
| - t.Fatalf("apps = %+v, want Docs then Sheets", got.Names()) |
| + if len(got.Apps) != 2 || got.Apps[0].Name != "Archive" || got.Apps[1].Name != "Sheets" { |
| + t.Fatalf("apps = %+v, want Archive then Sheets — sorted by NAME, "+ |
| + "which is what a reader sees; these hosts sort the other way", got.Names()) |
| } |
| - if got.Apps[0].Token != "docs-token" || got.Apps[1].Token != "sheets-token" { |
| + if got.Apps[0].Token != "archive-token" || got.Apps[1].Token != "sheets-token" { |
| t.Error("a token went to the wrong app, which would send one " + |
| "sibling's credential to another") |
| } |
| // The trailing slash is trimmed, or every route becomes |
| // `//suite/s1` — which some routers serve and some 404. |
| - if got.Apps[1].BaseURL != "https://sheets.acme.oficina.works" { |
| + if got.Apps[1].BaseURL != "https://acme.sheets.oficina.works" { |
| t.Errorf("base url = %q, want no trailing slash", got.Apps[1].BaseURL) |
| } |
| }) |
| + t.Run("an address the transport will refuse is refused at boot", func(t *testing.T) { |
| + // **`<team>.<app>`, team first** (§6.3). This file's own example |
| + // had the labels the other way round, and nothing checked, so the |
| + // roster accepted an address every S1, S2 and S3 would then be |
| + // refused for — before dialling, so not even a timeout. The |
| + // instance boots, logs "siblings configured", and answers every |
| + // query with "Docs did not answer; N results withheld" about an |
| + // app that is working perfectly. |
| + // |
| + // That is precisely the failure this whole function exists to |
| + // turn into a boot error, arriving through the one field nobody |
| + // was checking. Found by a review, 2026-09-07. |
| + _, err := LoadSiblings(env(map[string]string{ |
| + "MEMORIA_TEAM": "acme", |
| + "MEMORIA_SIBLINGS": "Docs=https://docs.acme.oficina.works", |
| + "MEMORIA_SIBLING_TOKEN_DOCS": "t", |
| + })) |
| + if err == nil { |
| + t.Error("a swapped <app>.<team> address was accepted; every call to " + |
| + "it would be refused before the transport dialled, forever, " + |
| + "and the page would blame the sibling") |
| + } |
| + |
| + for _, bad := range []string{ |
| + "https://acme.docs.example.com", // outside the suite domain |
| + "https://acme.docs.oficina.works.x", // suffix attack |
| + "https://elsewhere.docs.oficina.works", |
| + "https://acme.oficina.works", // no app label |
| + } { |
| + if _, err := LoadSiblings(env(map[string]string{ |
| + "MEMORIA_TEAM": "acme", |
| + "MEMORIA_SIBLINGS": "Docs=" + bad, |
| + "MEMORIA_SIBLING_TOKEN_DOCS": "t", |
| + })); err == nil { |
| + t.Errorf("accepted %q", bad) |
| + } |
| + } |
| + }) |
| + |
| + t.Run("two names that resolve to one token variable are refused", func(t *testing.T) { |
| + // "Docs" and "docs" are different map keys and different |
| + // Document.Source values, so the name check passed them — and |
| + // they read the SAME MEMORIA_SIBLING_TOKEN_DOCS, so one app |
| + // silently receives the other's credential. Found by a review. |
| + _, err := LoadSiblings(env(map[string]string{ |
| + "MEMORIA_TEAM": "acme", |
| + "MEMORIA_SIBLINGS": "Docs=https://acme.docs.oficina.works,docs=https://acme.sheets.oficina.works", |
| + "MEMORIA_SIBLING_TOKEN_DOCS": "t", |
| + })) |
| + if err == nil { |
| + t.Error("two apps resolving to one token variable were accepted; " + |
| + "one would be sent the other's consumer token") |
| + } |
| + }) |
| + |
| + t.Run("a malformed entry is named by position, never echoed", func(t *testing.T) { |
| + // main logs this error, and MEMORIA_SIBLINGS sits beside the |
| + // token variables in the same config. One transposed assignment |
| + // and the "malformed entry" is a bearer token in `carlos logs`, |
| + // which is storage we cannot delete. |
| + _, err := LoadSiblings(env(map[string]string{ |
| + "MEMORIA_TEAM": "acme", |
| + "MEMORIA_SIBLINGS": "Docs=https://acme.docs.oficina.works,sk-live-not-a-roster-entry", |
| + "MEMORIA_SIBLING_TOKEN_DOCS": "t", |
| + })) |
| + if err == nil { |
| + t.Fatal("a malformed entry was accepted") |
| + } |
| + if strings.Contains(err.Error(), "sk-live-not-a-roster-entry") { |
| + t.Errorf("the error echoed the entry: %v", err) |
| + } |
| + if !strings.Contains(err.Error(), "entry 2") { |
| + t.Errorf("the error does not say which entry: %v", err) |
| + } |
| + }) |
| + |
| t.Run("a display name maps to one token variable", func(t *testing.T) { |
| // One spelling, so an app cannot be configured two ways and the |
| // operator cannot set the variable that is never read. |
| @@ -150,7 +241,7 @@ func TestLoadSiblings(t *testing.T) { |
| func TestTheRosterCanBeLoggedWithoutItsTokens(t *testing.T) { |
| got, err := LoadSiblings(env(map[string]string{ |
| "MEMORIA_TEAM": "acme", |
| - "MEMORIA_SIBLINGS": "Docs=https://docs.acme.oficina.works", |
| + "MEMORIA_SIBLINGS": "Docs=https://acme.docs.oficina.works", |
| "MEMORIA_SIBLING_TOKEN_DOCS": "super-secret-token", |
| })) |
| if err != nil { |
diff --git a/internal/memoria/custody.go b/internal/memoria/custody.go| index 6d4305a..1dc1c2a 100644 |
| --- a/internal/memoria/custody.go |
| +++ b/internal/memoria/custody.go |
| @@ -128,3 +128,49 @@ func (s *Siblings) Warm(ctx context.Context, store *Store, logger *slog.Logger) |
| } |
| wg.Wait() |
| } |
| + |
| +// WireSiblings reads the roster and attaches it to the app, warm-up and |
| +// all. **One call, so main has one line and this has a test.** |
| +// |
| +// It exists because the gate over `cmd/` can only prove a call is |
| +// *spelled*, never that it is reached: a review wrapped `app.UseAuth(a)` |
| +// in a closure nobody invoked and the gate stayed green, and put the |
| +// sibling wiring behind `if len(sibs.Apps) > 1` so a one-sibling instance |
| +// ran with nil custody — also green. An AST walk over a file cannot see |
| +// control flow, and chasing that with a smarter walk is a race against |
| +// however the next person writes it. |
| +// |
| +// So the logic leaves main. What is left there is a call this function's |
| +// own tests cover, which is the rule main.go already states for |
| +// LoadConfig: *the refusals are a safety property and a safety property |
| +// needs a test.* |
| +// |
| +// **Every failure is fatal to boot, and an empty roster is not a |
| +// failure.** Slice 0 holds only memoria-owned content (§4.2), which has |
| +// no app of record to ask. |
| +func WireSiblings(a *app, getenv func(string) string, dial suite.DialFunc) error { |
| + cfg, err := LoadSiblings(getenv) |
| + if err != nil { |
| + return err |
| + } |
| + if len(cfg.Apps) == 0 { |
| + // Nothing configured. Custody stays nil, which withholds every |
| + // sibling passage — and there are none, because nothing was |
| + // ingested from a sibling either. |
| + a.logger.Info("no siblings configured") |
| + return nil |
| + } |
| + custody, err := NewSiblings(cfg.Team, dial, cfg.Apps) |
| + if err != nil { |
| + return err |
| + } |
| + a.UseSiblings(custody) |
| + // Names, never the roster itself: the structs carry consumer tokens, |
| + // and `carlos logs` is storage we cannot grep. |
| + a.logger.Info("siblings configured", "team", cfg.Team, "apps", cfg.Names()) |
| + // §6.5's wake poll, started and not awaited: the point is to spend a |
| + // cold sibling's latency where nobody is waiting, and waiting for it |
| + // would put that latency back on the first reader. |
| + a.WarmSiblings(context.Background()) |
| + return nil |
| +} |
diff --git a/internal/memoria/models.go b/internal/memoria/models.go| index 83eb137..013239d 100644 |
| --- a/internal/memoria/models.go |
| +++ b/internal/memoria/models.go |
| @@ -146,12 +146,15 @@ type SiblingFeed struct { |
| // it happened. Every failure path in Poll returns before this column |
| // is touched. |
| SealedCursor []byte |
| - // PolledAt is when the last poll COMPLETED, not when it started. |
| + // PolledAt is when the last poll DRAINED THE FEED — not when one |
| + // started, and not when one got partway. |
| // |
| - // It is what the sources page says the corpus is current as of, so a |
| - // poll that failed halfway must not move it — §8.1: "a failed or |
| - // stale S1 → the page states the corpus is current as of a time, |
| - // rather than implying freshness". |
| + // It is what the sources page WILL say the corpus is current as of — |
| + // the page is not built yet, and saying "is" would be claiming a |
| + // reader today sees something they do not. What is true today is the |
| + // column's rule: a poll that failed halfway, or stopped short, must |
| + // not move it. §8.1: "a failed or stale S1 → the page states the |
| + // corpus is current as of a time, rather than implying freshness". |
| PolledAt time.Time |
| // Reconciling marks a source whose sibling has been restored behind |
| // memoria (§4.1's post-restore rule 1): its chunks leave the ranking |
diff --git a/internal/memoria/sibling/feed.go b/internal/memoria/sibling/feed.go| index cb86044..a6589b5 100644 |
| --- a/internal/memoria/sibling/feed.go |
| +++ b/internal/memoria/sibling/feed.go |
| @@ -2,6 +2,7 @@ package sibling |
| import ( |
| "context" |
| + "errors" |
| "fmt" |
| "io" |
| "net/http" |
| @@ -54,7 +55,7 @@ func (c *Client) Feed(ctx context.Context, s Sibling, cursor string) (suite.Feed |
| res, err := c.http.Do(req) |
| if err != nil { |
| - return suite.Feed{}, fmt.Errorf("sibling %s: S1 unreachable: %w", s.Name, err) |
| + return suite.Feed{}, fmt.Errorf("sibling %s: S1 unreachable: %w", s.Name, bare(err)) |
| } |
| defer res.Body.Close() |
| @@ -117,7 +118,7 @@ func (c *Client) Content(ctx context.Context, s Sibling, itemID string, revision |
| res, err := c.http.Do(req) |
| if err != nil { |
| - return nil, fmt.Errorf("sibling %s: S3 unreachable: %w", s.Name, err) |
| + return nil, fmt.Errorf("sibling %s: S3 unreachable: %w", s.Name, bare(err)) |
| } |
| defer res.Body.Close() |
| @@ -137,3 +138,28 @@ func (c *Client) Content(ctx context.Context, s Sibling, itemID string, revision |
| } |
| return suite.DecodeS3(res.StatusCode, raw) |
| } |
| + |
| +// bare strips the URL out of a transport error. |
| +// |
| +// `http.Client.Do` returns a `*url.Error`, and its message is |
| +// `Get "<the whole URL>": <cause>`. On S1 that URL carries the cursor; on |
| +// S3 it carries the item id. **Both are sealed at rest** — the cursor |
| +// under seal.PurposeCursor, the item id under seal.PurposeItemID — and |
| +// AGENTS.md's rule about content in logs covers error strings, not just |
| +// log calls. |
| +// |
| +// Nothing logs these errors today, which is exactly why this is worth |
| +// fixing now: the next caller to log one would put a plaintext coordinate |
| +// into `carlos logs`, which is storage we cannot delete, and would have |
| +// no reason to suspect it. A review found it by unplugging a sibling and |
| +// reading the error. |
| +// |
| +// The cause survives — "connection refused", "context deadline exceeded" |
| +// — because that is the part an operator needs. |
| +func bare(err error) error { |
| + var ue *url.Error |
| + if errors.As(err, &ue) && ue.Err != nil { |
| + return ue.Err |
| + } |
| + return err |
| +} |
diff --git a/internal/memoria/sibling/feed_test.go b/internal/memoria/sibling/feed_test.go| index c5bbd8c..1dfa040 100644 |
| --- a/internal/memoria/sibling/feed_test.go |
| +++ b/internal/memoria/sibling/feed_test.go |
| @@ -7,6 +7,7 @@ import ( |
| "net/http" |
| "net/url" |
| "reflect" |
| + "strings" |
| "testing" |
| "time" |
| @@ -338,3 +339,59 @@ func assertNoPersonInHeaders(t *testing.T, call string, h http.Header) { |
| } |
| } |
| } |
| + |
| +// **A transport error must not carry the URL, because the URL carries a |
| +// coordinate.** |
| +// |
| +// `http.Client.Do` returns a `*url.Error` whose message is |
| +// `Get "<the whole URL>": <cause>`. On S1 that URL holds the cursor; on S3 |
| +// it holds the item id. Both are sealed at rest — the cursor under |
| +// seal.PurposeCursor, the item id under seal.PurposeItemID — and |
| +// AGENTS.md's rule about content covers error strings, not only log calls. |
| +// |
| +// Nothing logs these errors today, which is why it is worth fixing now: |
| +// the next caller to log one puts a plaintext coordinate into `carlos |
| +// logs`, which is storage nobody can delete, and has no reason to suspect |
| +// it. Found by a review that unplugged a sibling and read the error. |
| +// |
| +// PRESENCE FIRST: the canaries really are in the request, so the absence |
| +// check is an assertion. The cause must survive, because "connection |
| +// refused" is the part an operator needs. |
| +// |
| +// MUTATION-TESTED 2026-09-07, each verified red then reverted: |
| +// |
| +// drop bare() from the S1 path -> the cursor canary appears |
| +// drop bare() from the S3 path -> the item canary appears |
| +// bare() returns nil for a url.Error -> the cause assertion red |
| +func TestATransportErrorCarriesNoCoordinate(t *testing.T) { |
| + ctx := context.Background() |
| + // Port 1 is reserved; nothing listens, so Do fails at the transport |
| + // with the URL still in the error. |
| + dead := Sibling{Name: "Docs", BaseURL: "http://127.0.0.1:1", Token: "t"} |
| + |
| + t.Run("S1 does not leak the cursor", func(t *testing.T) { |
| + const canary = "CURSOR-CANARY-77" |
| + _, err := newTestClient().Feed(ctx, dead, canary) |
| + if err == nil { |
| + t.Fatal("a dead sibling answered") |
| + } |
| + if strings.Contains(err.Error(), canary) { |
| + t.Errorf("the cursor is in the error string: %v", err) |
| + } |
| + if !strings.Contains(err.Error(), "refused") && !strings.Contains(err.Error(), "connect") { |
| + t.Errorf("the cause did not survive: %v — an operator needs it, "+ |
| + "and stripping the whole error would be the other mistake", err) |
| + } |
| + }) |
| + |
| + t.Run("S3 does not leak the item id", func(t *testing.T) { |
| + const canary = "ITEM-CANARY-99" |
| + _, err := newTestClient().Content(ctx, dead, canary, 1) |
| + if err == nil { |
| + t.Fatal("a dead sibling answered") |
| + } |
| + if strings.Contains(err.Error(), canary) { |
| + t.Errorf("the item id is in the error string: %v", err) |
| + } |
| + }) |
| +} |
diff --git a/internal/memoria/siblingsync.go b/internal/memoria/siblingsync.go| index 636dda9..b402274 100644 |
| --- a/internal/memoria/siblingsync.go |
| +++ b/internal/memoria/siblingsync.go |
| @@ -4,6 +4,7 @@ import ( |
| "context" |
| "errors" |
| "fmt" |
| + "strings" |
| "time" |
| "amadan.net/oficina/suite" |
| @@ -36,6 +37,13 @@ type PollResult struct { |
| // feeds is §8.1's S3 breaker, which is the thing that notices when |
| // this happens wholesale. |
| Missing int |
| + // Empty counts items the sibling served with no body at all. |
| + // |
| + // Skipped, not stored: a stub page has no passages, so there is |
| + // nothing to retrieve and nothing to cite. Counted rather than passed |
| + // over in silence, because a source that is suddenly all stubs is a |
| + // fact about the sibling worth being able to see. |
| + Empty int |
| // More means the poll stopped before the sibling ran out. What |
| // memoria holds is consistent; it is not current, and the sources |
| // page must not say it is. |
| @@ -146,6 +154,26 @@ func (s *Store) applyRow(ctx context.Context, c *sibling.Client, sib sibling.Sib |
| return err |
| } |
| + // **An empty item is skipped and counted, not stored and not failed.** |
| + // |
| + // This said `AllowTitleOnly: true` and claimed it stopped one empty |
| + // document blocking a catch-up. It did the opposite: no Title is |
| + // passed here — a sibling's S3 gives a body and nothing else — so |
| + // Ingest refused with "neither a body nor a title", the row failed, |
| + // the cursor stayed put, and **every subsequent poll of that sibling |
| + // failed on the same row forever**. One stub page in Docs and nothing |
| + // from Docs is ever ingested again. A review found it. |
| + // |
| + // Skipping is right rather than a fallback title: a stub page has no |
| + // passages, so there is nothing to retrieve and nothing to cite, and |
| + // inventing a title from an empty body would put a document in the |
| + // corpus whose only content is its own name. The count is what keeps |
| + // it from being silent. |
| + if strings.TrimSpace(string(body)) == "" { |
| + res.Empty++ |
| + return nil |
| + } |
| + |
| // The item id is the stable key, so ExternalID's fingerprint is |
| // computed over it. A title would not be: a rename would make a second |
| // document and leave the first indexed under the old name. |
| @@ -155,12 +183,6 @@ func (s *Store) applyRow(ctx context.Context, c *sibling.Client, sib sibling.Sib |
| Body: string(body), |
| ItemID: row.ItemID, |
| SiblingRevision: row.Revision, |
| - // A sibling's item can legitimately be empty — a stub page, a |
| - // heading with nothing under it yet. Failing the row would let one |
| - // empty document stop a whole catch-up; skipping it would make the |
| - // page an absence with nothing to explain it. Same reasoning as |
| - // the connectors'. |
| - AllowTitleOnly: true, |
| }) |
| switch { |
| case errors.Is(err, ErrIndexLag): |
| @@ -202,20 +224,26 @@ func (s *Store) cursor(ctx context.Context, app string) (string, error) { |
| return string(raw), nil |
| } |
| -// setCursor stores a position and marks the poll current, in one write. |
| +// setCursor stores a position. **It does not touch PolledAt.** |
| // |
| -// The two move together on purpose: PolledAt is what the sources page |
| -// reports the corpus as current as of, and a position that advanced |
| -// without the timestamp — or the reverse — would make one of the two lie. |
| +// The two used to move together, and that was wrong in the direction that |
| +// matters. A multi-page catch-up calls this once per page, so a poll that |
| +// failed on page four — or one that stopped at the page bound with more to |
| +// come — left PolledAt at the moment of the last page it managed. The |
| +// sources page would then report the corpus current as of a time when it |
| +// demonstrably was not, which is §8.1's rule inverted: *"a failed or stale |
| +// S1 → the page states the corpus is current as of a time, rather than |
| +// implying freshness."* |
| +// |
| +// A position is "how far I got". A timestamp is "and there was nothing |
| +// after it". Only the second is a claim about completeness, and only the |
| +// drained-feed branch of Poll may make it. |
| func (s *Store) setCursor(ctx context.Context, app, cursor string) error { |
| sealed, err := s.sealer.Seal(seal.PurposeCursor, []byte(cursor)) |
| if err != nil { |
| return err |
| } |
| - return s.upsertFeed(ctx, app, map[string]any{ |
| - "sealed_cursor": sealed, |
| - "polled_at": time.Now().UTC(), |
| - }) |
| + return s.upsertFeed(ctx, app, map[string]any{"sealed_cursor": sealed}) |
| } |
| // markPolled records that a poll reached the end of the feed without |
diff --git a/internal/memoria/siblingsync_test.go b/internal/memoria/siblingsync_test.go| index 7cb5f17..aa25abd 100644 |
| --- a/internal/memoria/siblingsync_test.go |
| +++ b/internal/memoria/siblingsync_test.go |
| @@ -29,8 +29,11 @@ type feedServer struct { |
| // on, a 500 stops the page — and no fixture distinguished them until |
| // a mutation showed the cursor could advance mid-page unnoticed. |
| brokeS3 map[string]bool |
| - s1Calls int |
| - s3Calls int |
| + // forbidS3 answers 403, which is a CONTRACT BREACH and a third thing |
| + // again: the sibling is telling callers which ids exist. |
| + forbidS3 map[string]bool |
| + s1Calls int |
| + s3Calls int |
| } |
| func (f *feedServer) start(t *testing.T) (sibling.Sibling, *sibling.Client) { |
| @@ -57,6 +60,10 @@ func (f *feedServer) start(t *testing.T) (sibling.Sibling, *sibling.Client) { |
| case strings.HasPrefix(r.URL.Path, suite.RouteS3+"/"): |
| f.s3Calls++ |
| id := strings.TrimPrefix(r.URL.Path, suite.RouteS3+"/") |
| + if f.forbidS3[id] { |
| + w.WriteHeader(http.StatusForbidden) |
| + return |
| + } |
| if f.brokeS3[id] { |
| w.WriteHeader(http.StatusInternalServerError) |
| return |
| @@ -287,11 +294,11 @@ func TestAFailedPollAdvancesNothing(t *testing.T) { |
| f.pages[""] = suite.Feed{Cursor: "c2", Rows: []suite.FeedRow{row("a", 2, false)}} |
| f.pages["c2"] = suite.Feed{Cursor: "c2", Rows: []suite.FeedRow{}} |
| f.failS3 = map[string]bool{"a": true} |
| - s2 := storeForTest(t) |
| - if _, err := s2.Poll(ctx, client, sib); err != nil { |
| - t.Fatalf("reset poll: %v", err) |
| - } |
| + // The store under test is already at cursor "c1", so it is page |
| + // "c1" that must now carry the re-announced row — not page "". |
| + // Both are set: the first line above is what a sibling starting |
| + // fresh would serve, and this is what ours will actually ask for. |
| f.pages["c1"] = suite.Feed{Cursor: "c2", Rows: []suite.FeedRow{row("a", 2, false)}} |
| res, err := s.Poll(ctx, client, sib) |
| if err != nil { |
| @@ -480,3 +487,392 @@ func TestAFailedPollDoesNotClaimFreshness(t *testing.T) { |
| t.Fatalf("read back: %v", err) |
| } |
| } |
| + |
| +// **An empty item is skipped, and the feed keeps moving.** |
| +// |
| +// This was the opposite: `AllowTitleOnly` was passed with no Title, so |
| +// Ingest refused with "neither a body nor a title", the row failed, the |
| +// cursor stayed, and every subsequent poll of that sibling failed on the |
| +// same row. One stub page in Docs and nothing from Docs was ever ingested |
| +// again — a permanent, silent halt with only "sibling poll failed" to show |
| +// for it. A review found it by putting a stub first in a page. |
| +// |
| +// TWO-SIDED: the real document after the stub must arrive, or "refuse |
| +// everything" satisfies the count. |
| +// |
| +// MUTATION-TESTED 2026-09-07, each verified red then reverted: |
| +// |
| +// restore AllowTitleOnly and drop the empty check |
| +// -> red: the poll errors and the real document is not held |
| +// skip the empty row without counting it |
| +// -> red on Empty |
| +// treat whitespace as a body |
| +// -> red: Ingest refuses " " the same way |
| +func TestAnEmptyItemIsSkippedNotWedged(t *testing.T) { |
| + ctx := context.Background() |
| + f := &feedServer{ |
| + t: t, |
| + pages: map[string]suite.Feed{ |
| + "": {Cursor: "c1", Rows: []suite.FeedRow{ |
| + row("stub", 1, false), |
| + row("blank", 1, false), |
| + row("real", 1, false), |
| + }}, |
| + "c1": {Cursor: "c1", Rows: []suite.FeedRow{}}, |
| + }, |
| + bodies: map[string]string{ |
| + "stub": "", |
| + "blank": " \n\t ", |
| + "real": "The quarterly planning process runs in March.", |
| + }, |
| + } |
| + sib, client := f.start(t) |
| + s := storeForTest(t) |
| + |
| + res, err := s.Poll(ctx, client, sib) |
| + if err != nil { |
| + t.Fatalf("a stub page stopped the poll: %v", err) |
| + } |
| + if res.Empty != 2 { |
| + t.Errorf("result = %+v, want 2 empty", res) |
| + } |
| + if res.Stored != 1 { |
| + t.Errorf("result = %+v, want 1 stored", res) |
| + } |
| + // The document after the stubs arrived, which is the whole point. |
| + assertHeld(t, s, "Docs", "real", 1) |
| + if heldCount(t, s, "Docs") != 1 { |
| + t.Errorf("%d documents held; an empty item has no passages to "+ |
| + "retrieve and must not become one", heldCount(t, s, "Docs")) |
| + } |
| + // And the feed moved on, so the next poll does not re-read this page. |
| + if got := cursorOf(t, s, "Docs"); got != "c1" { |
| + t.Errorf("cursor = %q, want c1", got) |
| + } |
| +} |
| + |
| +// **A revision bump with an identical body must still be recorded.** |
| +// |
| +// The two revisions answer different questions. memoria's counts its own |
| +// edits; the sibling's is the token S2 is asked about, and a sibling |
| +// issues a new one for reasons memoria cannot see — a permissions change, |
| +// a metadata edit, a re-save that touched no bytes. |
| +// |
| +// Ingest's unchanged branch returned without touching it, and the failure |
| +// was permanent and invisible: the sibling announces revision 8 of a body |
| +// identical to 7, memoria keeps 7, and every query thereafter asks S2 |
| +// about `(item, 7)`. The sibling answers STALE — correctly, it has moved |
| +// on — so the passage is withheld loudly on every page forever, and |
| +// re-polling cannot fix it because the body never changes. Found by a |
| +// review, 2026-09-07. |
| +// |
| +// MUTATION-TESTED 2026-09-07, each verified red then reverted: |
| +// |
| +// restore the bare `unchanged = true; return nil` |
| +// -> red, the row stays at 7 |
| +// update sibling_revision but also bump Revision |
| +// -> red: memoria's own counter must not move for a body that did |
| +// not, or §5.2's provenance invalidates artefacts for nothing |
| +func TestARevisionBumpOnAnIdenticalBodyIsRecorded(t *testing.T) { |
| + ctx := context.Background() |
| + const body = "The quarterly planning process runs in March." |
| + f := &feedServer{ |
| + t: t, |
| + pages: map[string]suite.Feed{ |
| + "": {Cursor: "c1", Rows: []suite.FeedRow{row("a", 7, false)}}, |
| + "c1": {Cursor: "c1", Rows: []suite.FeedRow{}}, |
| + }, |
| + bodies: map[string]string{"a": body}, |
| + } |
| + sib, client := f.start(t) |
| + s := storeForTest(t) |
| + if _, err := s.Poll(ctx, client, sib); err != nil { |
| + t.Fatalf("first poll: %v", err) |
| + } |
| + assertHeld(t, s, "Docs", "a", 7) |
| + before := documentOf(t, s, "Docs", "a") |
| + |
| + // The sibling moves to 8. Same bytes. |
| + f.pages["c1"] = suite.Feed{Cursor: "c2", Rows: []suite.FeedRow{row("a", 8, false)}} |
| + f.pages["c2"] = suite.Feed{Cursor: "c2", Rows: []suite.FeedRow{}} |
| + if _, err := s.Poll(ctx, client, sib); err != nil { |
| + t.Fatalf("second poll: %v", err) |
| + } |
| + |
| + after := documentOf(t, s, "Docs", "a") |
| + if after.SiblingRevision != 8 { |
| + t.Errorf("held at sibling revision %d after the sibling announced 8: "+ |
| + "S2 is asked about %d, the sibling answers STALE, and the passage "+ |
| + "is withheld on every query forever — re-polling cannot fix it "+ |
| + "because the body never changes", |
| + after.SiblingRevision, after.SiblingRevision) |
| + } |
| + if after.Revision != before.Revision { |
| + t.Errorf("memoria's own revision moved from %d to %d for a body that "+ |
| + "did not change; §5.2 would invalidate every artefact built from "+ |
| + "it for nothing", before.Revision, after.Revision) |
| + } |
| +} |
| + |
| +// PolledAt is a claim about COMPLETENESS, and only a drained feed may make |
| +// it. |
| +// |
| +// A position is "how far I got"; a timestamp is "and there was nothing |
| +// after it". These were written together, once per page, so a multi-page |
| +// catch-up that failed on page four — or stopped at the page bound with |
| +// more to come — left PolledAt at the moment of the last page it managed. |
| +// The sources page would then report the corpus current as of a time it |
| +// demonstrably was not. §8.1: *a failed or stale S1 states what the corpus |
| +// is current as of, rather than implying freshness.* |
| +// |
| +// **The presence half is the one that was missing.** The existing test |
| +// only ever failed on page one, so "PolledAt was not set" passed against a |
| +// Poll that never set it at all — and a mutation deleting the write |
| +// entirely stayed green. This asserts a good poll DOES set it first. |
| +// |
| +// MUTATION-TESTED 2026-09-07, each verified red then reverted: |
| +// |
| +// setCursor writes polled_at again |
| +// -> the second-page failure case red, and the More case red |
| +// the drained branch stops writing polled_at |
| +// -> the presence case red |
| +func TestPolledAtMeansTheFeedWasDrained(t *testing.T) { |
| + ctx := context.Background() |
| + |
| + t.Run("a completed poll records when", func(t *testing.T) { |
| + f := &feedServer{ |
| + t: t, |
| + pages: map[string]suite.Feed{ |
| + "": {Cursor: "c1", Rows: []suite.FeedRow{row("a", 1, false)}}, |
| + "c1": {Cursor: "c1", Rows: []suite.FeedRow{}}, |
| + }, |
| + bodies: map[string]string{"a": "Held."}, |
| + } |
| + sib, client := f.start(t) |
| + s := storeForTest(t) |
| + if _, err := s.Poll(ctx, client, sib); err != nil { |
| + t.Fatalf("poll: %v", err) |
| + } |
| + if polledAt(t, s, "Docs").IsZero() { |
| + t.Error("a poll that drained the feed recorded no time; the sources " + |
| + "page has nothing to say the corpus is current as of") |
| + } |
| + }) |
| + |
| + t.Run("a poll failing on a later page claims nothing", func(t *testing.T) { |
| + f := &feedServer{ |
| + t: t, |
| + pages: map[string]suite.Feed{ |
| + "": {Cursor: "c1", Rows: []suite.FeedRow{row("a", 1, false)}}, |
| + "c1": {Cursor: "c2", Rows: []suite.FeedRow{row("b", 1, false)}}, |
| + }, |
| + bodies: map[string]string{"a": "First.", "b": "Second."}, |
| + failS1: map[string]bool{"c1": true}, |
| + } |
| + sib, client := f.start(t) |
| + s := storeForTest(t) |
| + if _, err := s.Poll(ctx, client, sib); err == nil { |
| + t.Fatal("a 500 on page two came back as a completed poll") |
| + } |
| + // Page one WAS applied and the cursor moved — so the row exists, |
| + // and this is an assertion about the column rather than about the |
| + // row's absence. |
| + if got := cursorOf(t, s, "Docs"); got != "c1" { |
| + t.Fatalf("cursor = %q, want c1", got) |
| + } |
| + if at := polledAt(t, s, "Docs"); !at.IsZero() { |
| + t.Errorf("a poll that failed on page two recorded PolledAt=%v: the "+ |
| + "sources page would call the corpus current as of a moment "+ |
| + "there was demonstrably more to read", at) |
| + } |
| + }) |
| + |
| + t.Run("a poll that stopped short claims nothing", func(t *testing.T) { |
| + f := &feedServer{ |
| + t: t, |
| + pages: map[string]suite.Feed{ |
| + "": {Cursor: "same", Rows: []suite.FeedRow{row("a", 1, false)}}, |
| + "same": {Cursor: "same", Rows: []suite.FeedRow{row("a", 1, false)}}, |
| + }, |
| + bodies: map[string]string{"a": "Going nowhere."}, |
| + } |
| + sib, client := f.start(t) |
| + s := storeForTest(t) |
| + res, err := s.Poll(ctx, client, sib) |
| + if err != nil { |
| + t.Fatalf("poll: %v", err) |
| + } |
| + if !res.More { |
| + t.Fatal("the fixture did not produce a short poll") |
| + } |
| + if at := polledAt(t, s, "Docs"); !at.IsZero() { |
| + t.Errorf("a poll reporting More recorded PolledAt=%v", at) |
| + } |
| + }) |
| +} |
| + |
| +// Two siblings in one store keep their own position and their own |
| +// documents. |
| +// |
| +// Neither scoping had a fixture: a review dropped `source = ?` from the |
| +// tombstone lookup and `app = ?` from the cursor read, and both stayed |
| +// green. In production the first means Docs' tombstone for item "42" |
| +// deletes Sheets' item "42"; the second means Sheets polls from Docs' |
| +// cursor and silently skips everything before it. |
| +// |
| +// MUTATION-TESTED 2026-09-07, each verified red then reverted: |
| +// |
| +// deleteBySiblingItem without `source = ?` |
| +// -> red, the other sibling's document goes |
| +// cursor()/upsertFeed without `app = ?` |
| +// -> red, the positions collide |
| +func TestTwoSiblingsKeepTheirOwnPositionAndDocuments(t *testing.T) { |
| + ctx := context.Background() |
| + s := storeForTest(t) |
| + |
| + // The same item id in both apps, which is the collision that matters: |
| + // item ids are the siblings' own and nothing makes them unique across |
| + // apps. |
| + docs := &feedServer{ |
| + t: t, |
| + pages: map[string]suite.Feed{ |
| + "": {Cursor: "docs-1", Rows: []suite.FeedRow{row("42", 1, false)}}, |
| + "docs-1": {Cursor: "docs-1", Rows: []suite.FeedRow{}}, |
| + }, |
| + bodies: map[string]string{"42": "Docs' document forty-two."}, |
| + } |
| + sheets := &feedServer{ |
| + t: t, |
| + pages: map[string]suite.Feed{ |
| + "": {Cursor: "sheets-9", Rows: []suite.FeedRow{row("42", 1, false)}}, |
| + "sheets-9": {Cursor: "sheets-9", Rows: []suite.FeedRow{}}, |
| + }, |
| + bodies: map[string]string{"42": "Sheets' document forty-two."}, |
| + } |
| + dSib, client := docs.start(t) |
| + sSib, _ := sheets.start(t) |
| + sSib.Name = "Sheets" |
| + |
| + if _, err := s.Poll(ctx, client, dSib); err != nil { |
| + t.Fatalf("poll Docs: %v", err) |
| + } |
| + if _, err := s.Poll(ctx, client, sSib); err != nil { |
| + t.Fatalf("poll Sheets: %v", err) |
| + } |
| + |
| + // Separate positions, or one app resumes from the other's place. |
| + if got := cursorOf(t, s, "Docs"); got != "docs-1" { |
| + t.Errorf("Docs cursor = %q, want docs-1", got) |
| + } |
| + if got := cursorOf(t, s, "Sheets"); got != "sheets-9" { |
| + t.Errorf("Sheets cursor = %q, want sheets-9", got) |
| + } |
| + assertHeld(t, s, "Docs", "42", 1) |
| + assertHeld(t, s, "Sheets", "42", 1) |
| + |
| + // Docs tombstones its "42". Sheets' must survive. |
| + docs.pages["docs-1"] = suite.Feed{Cursor: "docs-2", Rows: []suite.FeedRow{row("42", 2, true)}} |
| + docs.pages["docs-2"] = suite.Feed{Cursor: "docs-2", Rows: []suite.FeedRow{}} |
| + res, err := s.Poll(ctx, client, dSib) |
| + if err != nil { |
| + t.Fatalf("tombstone poll: %v", err) |
| + } |
| + if res.Deleted != 1 { |
| + t.Errorf("result = %+v, want 1 deleted", res) |
| + } |
| + if heldCount(t, s, "Docs") != 0 { |
| + t.Error("Docs' document survived its own tombstone") |
| + } |
| + assertHeld(t, s, "Sheets", "42", 1) |
| +} |
| + |
| +// A tombstone memoria has already applied deletes nothing and says so. |
| +// |
| +// A feed can legitimately re-announce a deletion, and a poll that failed |
| +// mid-page re-applies the rows before the failure by design — so the count |
| +// has to be of documents that actually went, not of rows that mentioned |
| +// one. Counting rows makes the sources page report deletions that did not |
| +// happen, on exactly the days something went wrong. |
| +// |
| +// MUTATION-TESTED 2026-09-07: `res.Deleted++` per tombstone row rather |
| +// than per document removed. Red. Reverted. |
| +func TestAReAnnouncedTombstoneDeletesNothing(t *testing.T) { |
| + ctx := context.Background() |
| + f := &feedServer{ |
| + t: t, |
| + pages: map[string]suite.Feed{ |
| + "": {Cursor: "c1", Rows: []suite.FeedRow{row("a", 2, true)}}, |
| + "c1": {Cursor: "c1", Rows: []suite.FeedRow{}}, |
| + }, |
| + bodies: map[string]string{}, |
| + } |
| + sib, client := f.start(t) |
| + s := storeForTest(t) |
| + |
| + // memoria never held "a" at all. |
| + res, err := s.Poll(ctx, client, sib) |
| + if err != nil { |
| + t.Fatalf("poll: %v", err) |
| + } |
| + if res.Deleted != 0 { |
| + t.Errorf("result = %+v: a tombstone for a document memoria does not "+ |
| + "hold removed nothing, and the count must say so", res) |
| + } |
| +} |
| + |
| +// A sibling answering 403 on S3 is a contract breach, not a missing item, |
| +// and it must not be quietly folded into the count that means "gone or |
| +// not visible". |
| +// |
| +// 403 says "this exists and you may not have it" — the one fact custody |
| +// hides — so a sibling emitting one is leaking, and the module keeps it |
| +// distinguishable on purpose. Counting it as Missing would hide a leaking |
| +// sibling behind a number that means something else. |
| +// |
| +// MUTATION-TESTED 2026-09-07: fold ErrContractBreach into the Missing |
| +// branch. Red. Reverted. |
| +func TestA403OnS3IsNotAMissingItem(t *testing.T) { |
| + f := &feedServer{ |
| + t: t, |
| + pages: map[string]suite.Feed{ |
| + "": {Cursor: "c1", Rows: []suite.FeedRow{row("a", 1, false)}}, |
| + "c1": {Cursor: "c1", Rows: []suite.FeedRow{}}, |
| + }, |
| + bodies: map[string]string{}, |
| + forbidS3: map[string]bool{"a": true}, |
| + } |
| + sib, client := f.start(t) |
| + s := storeForTest(t) |
| + |
| + res, err := s.Poll(context.Background(), client, sib) |
| + if err == nil { |
| + t.Fatalf("a 403 from S3 came back as a completed poll: %+v", res) |
| + } |
| + if res.Missing != 0 { |
| + t.Errorf("a 403 was counted as a missing item: %+v — that number "+ |
| + "means 'gone or not team-visible', and a 403 means the sibling "+ |
| + "is telling callers which ids exist", res) |
| + } |
| +} |
| + |
| +func documentOf(t *testing.T, s *Store, app, itemID string) Document { |
| + t.Helper() |
| + external, err := s.externalID(itemID) |
| + if err != nil { |
| + t.Fatalf("externalID: %v", err) |
| + } |
| + var d Document |
| + if err := s.db.G.Where("source = ? AND external_id = ?", app, external).First(&d).Error; err != nil { |
| + t.Fatalf("%s/%s not held: %v", app, itemID, err) |
| + } |
| + return d |
| +} |
| + |
| +func polledAt(t *testing.T, s *Store, app string) time.Time { |
| + t.Helper() |
| + var row SiblingFeed |
| + if err := s.db.G.Where("app = ?", app).First(&row).Error; err != nil { |
| + return time.Time{} |
| + } |
| + return row.PolledAt |
| +} |
diff --git a/internal/memoria/store.go b/internal/memoria/store.go| index c7d7d1f..2624c6f 100644 |
| --- a/internal/memoria/store.go |
| +++ b/internal/memoria/store.go |
| @@ -250,10 +250,38 @@ func (s *Store) Ingest(ctx context.Context, spec IngestSpec) (int64, error) { |
| return err |
| } |
| if !moved { |
| - // Genuinely unchanged. Do NOT bump the revision: a |
| - // revision that moves without the content moving |
| - // would make every cached authorization stale for no |
| - // reason. |
| + // Genuinely unchanged. Do NOT bump memoria's own |
| + // revision: a revision that moves without the content |
| + // moving would make every cached authorization stale |
| + // for no reason. |
| + // |
| + // **But the SIBLING's revision must still be |
| + // recorded, and this returned without it.** The two |
| + // numbers answer different questions. memoria's |
| + // counts its own edits; the sibling's is the token S2 |
| + // is asked about, and the sibling issues it for |
| + // reasons memoria cannot see — a permissions change, |
| + // a metadata edit, a re-save that touched no bytes. |
| + // |
| + // Left stale, the failure is permanent and invisible: |
| + // the sibling announces revision 8 of a body |
| + // identical to 7, memoria keeps 7, and every |
| + // subsequent query asks S2 about `(item, 7)`. The |
| + // sibling answers STALE — correctly, it has moved on |
| + // — so the passage is withheld loudly on every page, |
| + // forever, and re-polling cannot fix it because the |
| + // body never changes. "Docs has moved on from 1 |
| + // result; refetching" on every single query, for a |
| + // document the reader is entitled to. |
| + // |
| + // Found by a review, 2026-09-07. |
| + if spec.ItemID != "" && existing.SiblingRevision != spec.SiblingRevision { |
| + unchanged = true |
| + return tx.Model(&Document{}).Where("id = ?", docID).Updates(map[string]any{ |
| + "sibling_revision": spec.SiblingRevision, |
| + "sibling_item_id": sealedItemID, |
| + }).Error |
| + } |
| unchanged = true |
| return nil |
| } |
| @@ -262,9 +290,15 @@ func (s *Store) Ingest(ctx context.Context, spec IngestSpec) (int64, error) { |
| // chunks are left alone, because the path lives on the |
| // document. The index is refreshed below, since it is |
| // the index that carries the path. |
| + // Title or path moved. The sibling's revision travels |
| + // with it for the same reason as above: whatever made the |
| + // sibling issue a new one, S2 will be asked about the new |
| + // one or about nothing. |
| return tx.Model(&Document{}).Where("id = ?", docID).Updates(map[string]any{ |
| - "sealed_title": sealedTitle, |
| - "sealed_path": sealedPath, |
| + "sealed_title": sealedTitle, |
| + "sealed_path": sealedPath, |
| + "sibling_revision": spec.SiblingRevision, |
| + "sibling_item_id": sealedItemID, |
| }).Error |
| } |
| rechunk = true |
diff --git a/internal/memoria/warmup_test.go b/internal/memoria/warmup_test.go| index b97c058..7f06666 100644 |
| --- a/internal/memoria/warmup_test.go |
| +++ b/internal/memoria/warmup_test.go |
| @@ -285,10 +285,108 @@ func TestWarmSiblingsStartsThePoll(t *testing.T) { |
| // An instance with no siblings warms nothing and does not panic. |
| // |
| -// Slice 0's wiring: custody is nil, and `WarmSiblings` is still called by |
| -// main because the gate requires the call to be there unconditionally. |
| +// Slice 0's wiring: custody is nil. `WireSiblings` returns before warming |
| +// in that case, and this covers the method being called anyway — which is |
| +// what a test doing its own wiring will do. |
| +// |
| +// This comment used to say main called WarmSiblings "unconditionally", |
| +// which was not true of the code it described: main guarded it on a |
| +// non-empty roster. It is now behind WireSiblings, which is tested. |
| func TestWarmSiblingsWithNoRosterIsQuiet(t *testing.T) { |
| s := storeForTest(t) |
| a := &app{store: s, logger: slog.New(slog.NewTextHandler(io.Discard, nil))} |
| a.WarmSiblings(context.Background()) |
| } |
| + |
| +// WireSiblings is where the roster actually becomes an app's custody, and |
| +// this is the test main.go could not have. |
| +// |
| +// A review defeated the gate over `cmd/` twice — `if len(apps) > 1` |
| +// around the wiring, and `app.UseAuth` inside a closure nobody invoked — |
| +// both green, because an AST walk over one file sees that a call is |
| +// spelled and not that it is reached. The answer is not a cleverer walk; |
| +// it is to leave nothing in main worth getting wrong. So the logic is |
| +// here, and here it can be run. |
| +// |
| +// MUTATION-TESTED 2026-09-07, each verified red then reverted: |
| +// |
| +// guard UseSiblings on len(cfg.Apps) > 1 |
| +// -> the one-sibling case red, and it is the ordinary case |
| +// return nil before UseSiblings |
| +// -> the one-sibling case red |
| +// swallow the LoadSiblings error |
| +// -> the bad-roster case red |
| +// log cfg.Apps rather than cfg.Names() |
| +// -> the token case red |
| +func TestWireSiblings(t *testing.T) { |
| + good := map[string]string{ |
| + "MEMORIA_TEAM": "acme", |
| + "MEMORIA_SIBLINGS": "Docs=https://acme.docs.oficina.works", |
| + "MEMORIA_SIBLING_TOKEN_DOCS": "docs-token", |
| + } |
| + |
| + t.Run("one sibling is enough to get custody", func(t *testing.T) { |
| + // **One, not two.** A review's mutation guarded the wiring on |
| + // `len(apps) > 1`, which every multi-sibling fixture satisfies and |
| + // which leaves the ordinary case — a team that has adopted its |
| + // first source — running with nil custody, withholding everything |
| + // from the one app it has. |
| + s := storeForTest(t) |
| + a := &app{store: s, logger: slog.New(slog.NewTextHandler(io.Discard, nil))} |
| + if err := WireSiblings(a, env(good), nil); err != nil { |
| + t.Fatalf("WireSiblings: %v", err) |
| + } |
| + if s.custody == nil { |
| + t.Fatal("custody is nil after wiring one sibling: every passage " + |
| + "that app owns would be withheld, and the notice would blame it") |
| + } |
| + if _, ok := s.custody.Sibling("Docs"); !ok { |
| + t.Error("the configured app does not resolve from a document's source") |
| + } |
| + }) |
| + |
| + t.Run("an empty roster is not an error and leaves custody nil", func(t *testing.T) { |
| + s := storeForTest(t) |
| + a := &app{store: s, logger: slog.New(slog.NewTextHandler(io.Discard, nil))} |
| + if err := WireSiblings(a, env(nil), nil); err != nil { |
| + t.Fatalf("an instance with no siblings refused to wire: %v", err) |
| + } |
| + if s.custody != nil { |
| + t.Error("custody was set from an empty roster") |
| + } |
| + }) |
| + |
| + t.Run("a bad roster is an error, not a quiet nil custody", func(t *testing.T) { |
| + s := storeForTest(t) |
| + a := &app{store: s, logger: slog.New(slog.NewTextHandler(io.Discard, nil))} |
| + err := WireSiblings(a, env(map[string]string{ |
| + "MEMORIA_TEAM": "acme", |
| + "MEMORIA_SIBLINGS": "Docs=https://acme.docs.oficina.works", |
| + }), nil) |
| + if err == nil { |
| + t.Fatal("a sibling with no token was wired; every passage it owns " + |
| + "would be withheld with a notice blaming a healthy app") |
| + } |
| + if s.custody != nil { |
| + t.Error("custody was set from a roster that failed to load") |
| + } |
| + }) |
| + |
| + t.Run("the log line carries no token", func(t *testing.T) { |
| + var logged strings.Builder |
| + s := storeForTest(t) |
| + a := &app{store: s, logger: slog.New(slog.NewTextHandler(&logged, |
| + &slog.HandlerOptions{Level: slog.LevelDebug}))} |
| + if err := WireSiblings(a, env(good), nil); err != nil { |
| + t.Fatalf("WireSiblings: %v", err) |
| + } |
| + // Presence first: something was logged and it names the app, so |
| + // the absence below is an assertion. |
| + if !strings.Contains(logged.String(), "Docs") { |
| + t.Fatal("nothing identifying was logged, so the check below proves nothing") |
| + } |
| + if strings.Contains(logged.String(), "docs-token") { |
| + t.Errorf("a consumer token reached the log:\\n%s", logged.String()) |
| + } |
| + }) |
| +} |
diff --git a/internal/memoriatest/gates_test.go b/internal/memoriatest/gates_test.go| index 687970d..281458b 100644 |
| --- a/internal/memoriatest/gates_test.go |
| +++ b/internal/memoriatest/gates_test.go |
| @@ -479,7 +479,21 @@ func TestProductionDoesNotUseTheTestConstructor(t *testing.T) { |
| // scan above can stay a grep, because there the string appearing |
| // at all is the problem; here the problem is the call NOT |
| // happening, and only the parse can tell those apart. |
| - for _, call := range []string{"UseAuth", "LoadSiblings", "UseSiblings", "WarmSiblings"} { |
| + // **This proves the call is SPELLED, not that it is reached**, and |
| + // saying so is the point. A review wrapped `app.UseAuth(a)` in a |
| + // closure nobody invoked and this stayed green; so did putting |
| + // the sibling wiring behind `if len(apps) > 1`. An AST walk over |
| + // one file cannot see control flow, and a smarter walk is a race |
| + // against however the next person writes it. |
| + // |
| + // What makes that survivable is that there is now almost nothing |
| + // in main to get wrong: the sibling wiring is one call to |
| + // `memoria.WireSiblings`, which has its own tests, and this gate |
| + // only has to notice the call disappearing. The names below are |
| + // the ones whose absence is silent — UseAuth serves every route |
| + // unauthenticated, WireSiblings leaves every sibling passage |
| + // withheld while blaming apps that are fine. |
| + for _, call := range []string{"UseAuth", "WireSiblings"} { |
| if !callsFunction(t, p, raw, call) { |
| t.Errorf("%s builds the app but never calls %s", p, call) |
| } |