amadan
Review 5: a late row got through the deadline; three guards saw nothing
Reviewed by Codex GPT-6-Astra (dispatched: codex/gpt-6-astra; self- reported "GPT-6 (Codex)"), of a8287b5 alone. REQUEST CHANGES: one runtime defect, two ineffective guards, and the Home gap 5b1ac4e had already half-closed. **A row could still come back after its deadline.** The deadline was checked on entry to each proposal and nowhere after, so the last proposal's recogniser pass could run past it and its row be returned: a proposal arriving with 10 ms left against a 1.26 MB passage produced a model row at 1.01 s. The deadline is now also checked after the acceptance loop, in the labelling loop, and before returning. A single recogniser pass is not interruptible, so latency can still overrun by one passage's scan — the spec comment says so — but the rows are never late. Guarded by the reviewer's shape with its presence half: the same proposal with time to spare returns the row. **Caching, dedupe and the per-proposal check were all invisible to the suite** — the thousand-identical-proposals test passed with any of them removed, because dedupe collapsed the thousand to one before the others mattered. Rather than delete them, measured whether they CAN be seen: one recogniser pass over 1.23 MB is ~200 ms, over 410 KB ~70 ms, and a near-miss strings.Index over 1.23 MB ~1 ms. At those sizes each one's absence lands past the 800 ms deadline, so each now has a guard sized from the measurement: a thousand distinct present proposals (caching, 166 ms against 881 ms), a thousand identical near-misses (dedupe, 219 ms against 1.02 s), 2,500 distinct near-misses (the per-proposal check, 1.02 s against 2.58 s). Each fixture asserts it is ONE chunk, because the first was split small by the chunker. One threshold was set at 300 ms against a 219 ms baseline and widened to 600 ms before commit. **The Home footer guard now names every route and ignores status.** It accepted "at least one page made a claim" and checked only 200s, so the confirm page — which renders its failure at 400 — was never looked at. The memoriatest walk likewise checks a footer at any status now.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
5 files changed,
+173
−59
docs/superpowers/specs/2026-09-03-memoria-design.md+6 −1internal/memoria/home_router_test.go+15 −11internal/memoria/modelfacts_test.go+131 −45internal/memoria/store.go+17 −1internal/memoriatest/footer_model_test.go+4 −1
CI passed — run details
CI log
=== vet === go vet ./... === fmt === === test === go test ./... ? amadan.net/oficina/memoria/cmd/memoria [no test files] ok amadan.net/oficina/memoria/internal/memoria 4.372s ok amadan.net/oficina/memoria/internal/memoria/chunk (cached) ok amadan.net/oficina/memoria/internal/memoria/fact (cached) ? amadan.net/oficina/memoria/internal/memoria/icons [no test files] ok amadan.net/oficina/memoria/internal/memoria/index (cached) ok amadan.net/oficina/memoria/internal/memoria/notion (cached) ok amadan.net/oficina/memoria/internal/memoria/provider (cached) ok amadan.net/oficina/memoria/internal/memoria/query (cached) ok amadan.net/oficina/memoria/internal/memoria/seal (cached) ok amadan.net/oficina/memoria/internal/memoria/sibling (cached) ok amadan.net/oficina/memoria/internal/memoriatest 1.575s ? amadan.net/oficina/memoria/tools/bench [no test files] ? amadan.net/oficina/memoria/tools/drive [no test files] ? amadan.net/oficina/memoria/tools/seed [no test files] ? amadan.net/oficina/memoria/tools/vendorassets [no test files] === migration-check === go run github.com/carlosframework/rastrillo/cmd/rastrillo migration check rastrillo migration check: models and migrations agree
diff --git a/docs/superpowers/specs/2026-09-03-memoria-design.md b/docs/superpowers/specs/2026-09-03-memoria-design.md| index b57e8b5..03c68ec 100644 |
| --- a/docs/superpowers/specs/2026-09-03-memoria-design.md |
| +++ b/docs/superpowers/specs/2026-09-03-memoria-design.md |
| @@ -3659,7 +3659,12 @@ claim could be false **outside** the path the section above describes: |
| instance" while a search had just sent passages. It now states the |
| switch: with AI on it names TensorX and the trigger; with AI off it |
| makes the strong claim, which is then true. A gate walks every declared |
| - route, so a page cannot skip it. |
| + GET route and checks each footer at whatever status it renders; a route |
| + that renders no footer must be on a list with its reason, or the walk |
| + fails. The pages that exist only when Home is configured are checked by |
| + name, through the Home fixture. (The first version of that gate skipped |
| + non-200 routes silently and so missed all four Home pages — found by |
| + review, then fixed.) |
| - **Processing time.** The deadline covered the call and not the work on |
| its response; a legal-sized response of repeated proposals took 6.8 s to |
| validate. One deadline now covers both, identical proposals are |
diff --git a/internal/memoria/home_router_test.go b/internal/memoria/home_router_test.go| index 1a326ad..59fc973 100644 |
| --- a/internal/memoria/home_router_test.go |
| +++ b/internal/memoria/home_router_test.go |
| @@ -304,37 +304,41 @@ func TestHomeSigninClearsOnlyDefinitivelyStaleSession(t *testing.T) { |
| // The footer's egress claim on the pages that exist only when Home is |
| // configured. TestTheFooterStatesTheModelSwitch (memoriatest) walks every |
| // declared route, but its harness does not configure Home, so these 404 |
| -// there and cannot be checked — this is where they are. They render |
| -// through renderHome, the Render callback the Home library is given. |
| +// there — this is where they are checked. They render through renderHome, |
| +// the Render callback both the Home routes and its Bridge are given. |
| +// |
| +// **Every route explicitly, at whatever status it renders.** The first |
| +// version accepted "at least one page made a claim" and checked only |
| +// 200s, so /account/home/confirm — which renders its failure page at 400 — |
| +// was never looked at, and removing ModelOn from renderHome left the |
| +// suite green until the next commit added this. A review found both. |
| func TestHomePagesStateTheModelSwitch(t *testing.T) { |
| + routes := []string{"/signed-out", "/account/home", "/account/home/confirm", "/signin/local"} |
| for _, on := range []bool{true, false} { |
| f := newHomeRouterFixture(t) |
| if on { |
| f.app.store.extractor = &recorder{} |
| } |
| f.local(t) |
| - checked := 0 |
| - for _, path := range []string{"/signed-out", "/account/home", "/signin/local"} { |
| + for _, path := range routes { |
| w := f.do("GET", path, "") |
| b := w.Body.String() |
| i := strings.Index(b, "<footer") |
| - if w.Code != 200 || i < 0 { |
| + if i < 0 { |
| + t.Errorf("AI on=%v, %s (%d): no footer, so this route is unchecked", on, path, w.Code) |
| continue |
| } |
| foot := strings.Join(strings.Fields(b[i:]), " ") |
| if !strings.Contains(foot, "leaves this instance") { |
| + t.Errorf("AI on=%v, %s (%d): the footer makes no egress claim", on, path, w.Code) |
| continue |
| } |
| - checked++ |
| if on && (strings.Contains(foot, "no document ever") || !strings.Contains(foot, "TensorX")) { |
| - t.Errorf("AI on, %s: footer does not disclose TensorX: %q", path, foot) |
| + t.Errorf("AI on, %s (%d): footer does not disclose TensorX", path, w.Code) |
| } |
| if !on && (!strings.Contains(foot, "no document ever") || strings.Contains(foot, "TensorX")) { |
| - t.Errorf("AI off, %s: footer does not make the true claim: %q", path, foot) |
| + t.Errorf("AI off, %s (%d): footer does not make the true claim", path, w.Code) |
| } |
| } |
| - if checked == 0 { |
| - t.Fatalf("AI on=%v: no Home page rendered a footer with a claim; this proves nothing", on) |
| - } |
| } |
| } |
diff --git a/internal/memoria/modelfacts_test.go b/internal/memoria/modelfacts_test.go| index 6b51bd9..f3bb9f3 100644 |
| --- a/internal/memoria/modelfacts_test.go |
| +++ b/internal/memoria/modelfacts_test.go |
| @@ -840,60 +840,128 @@ func TestAModelRowNamesTheProviderAndTheModel(t *testing.T) { |
| } |
| // **A model response is bounded in bytes, and its processing must be |
| -// bounded in time too.** A review sent 1,000 identical proposals — a |
| -// valid 54 KB response, under the cap — against a 64 KB passage, and |
| -// validation took 6.8 seconds, after the provider's deadline had already |
| -// expired and with nothing checking for cancellation. The recognisers ran |
| -// once per proposal; they now run once per passage, identical proposals |
| -// are validated once, and the cap stops the work as soon as it is certain. |
| +// bounded in time too.** Each subtest guards one mechanism, and each was |
| +// found unguarded by review: a thousand-identical-proposals test passed |
| +// with caching removed, with dedupe removed, and with the per-proposal |
| +// deadline check removed. The fixtures are sized from measurement so the |
| +// version WITHOUT the mechanism lands past the 800 ms deadline and the |
| +// version with it lands far inside: one recogniser pass over this 1.23 MB |
| +// passage is ~200 ms, one over the 410 KB one ~70 ms, and a near-miss |
| +// strings.Index over 1.23 MB ~1 ms (measured 2026-09-27). |
| +// |
| +// Every passage is ONE chunk — chunk.Split bounds by words, and the dense |
| +// part has no spaces — asserted, because the first fixture here was split |
| +// small by the chunker and passed against the unfixed code. |
| func TestAModelResponseIsCheapToProcess(t *testing.T) { |
| - // ONE large chunk, dense with recognisable facts. chunk.Split bounds a |
| - // chunk by WORDS, so text without spaces stays whole: the first version |
| - // of this fixture used ordinary sentences, the chunker cut it into |
| - // small passages, and the old per-proposal behaviour ran fast enough to |
| - // pass. This is the reviewer's shape — a single 60 KB passage. |
| - big := "## Contact\n\nOffice address: 12 Main Street, Dublin 2.\n" + |
| - strings.Repeat("D02X285,info@tito.io,", 3000) |
| - |
| - t.Run("a thousand identical proposals", func(t *testing.T) { |
| + const unit = "D02X285,info@tito.io," // two recognised facts, no spaces |
| + build := func(t *testing.T, prefix string, reps int) *Store { |
| + t.Helper() |
| s := storeForTest(t) |
| - upload(t, s, "Tito office", big) |
| - s.extractor = &recorder{reply: func([]string) ([]provider.Pointer, error) { |
| - out := make([]provider.Pointer, 1000) |
| - for i := range out { |
| - out[i] = provider.Pointer{Passage: 0, Text: "12 Main Street, Dublin 2"} |
| + upload(t, s, "Tito office", "Office address: "+prefix+strings.Repeat(unit, reps)) |
| + return s |
| + } |
| + onePassage := func(t *testing.T, calls [][]string) { |
| + t.Helper() |
| + if len(calls) != 1 || len(calls[0]) != 1 { |
| + t.Fatalf("sent %v passages; the fixture must be ONE chunk or it measures nothing", lens(calls)) |
| + } |
| + } |
| + timed := func(t *testing.T, s *Store, q string) (time.Duration, []Fact) { |
| + t.Helper() |
| + st := time.Now() |
| + p := searchFacts(t, s, q) |
| + took := time.Since(st) |
| + t.Logf("search took %v", took) |
| + return took, modelRows(p) |
| + } |
| + |
| + // Mutation: the only deadline check is on entry to each proposal. The |
| + // proposal arrives with 10 ms left, and its recogniser pass takes ~200 |
| + // ms: the row must not come back. The presence half, with no wait, is |
| + // what shows the row exists to be dropped. |
| + t.Run("a row whose processing crosses the deadline is dropped", func(t *testing.T) { |
| + for _, late := range []bool{false, true} { |
| + s := build(t, "12 Main Street, Dublin 2,", 60000) |
| + rec := &recorder{} |
| + s.extractor = ctxAwareRecorder(rec, func(ctx context.Context) { |
| + if late { |
| + if dl, ok := ctx.Deadline(); ok { |
| + time.Sleep(time.Until(dl) - 10*time.Millisecond) |
| + } |
| + } |
| + }, point(0, "12 Main Street, Dublin 2")) |
| + _, rows := timed(t, s, "office address") |
| + onePassage(t, rec.calls) |
| + if !late && len(rows) != 1 { |
| + t.Fatalf("no row with time to spare (%d); the late case proves nothing", len(rows)) |
| + } |
| + if late && len(rows) != 0 { |
| + t.Errorf("a row came back after its deadline had passed: %+v", rows) |
| } |
| - return out, nil |
| - }} |
| - start := time.Now() |
| - rows := modelRows(searchFacts(t, s, "office address")) |
| - // Against the 6.8 s measured, with an order of magnitude to spare |
| - // for a slow machine; the fixed code takes a few milliseconds. |
| - if took := time.Since(start); took > 700*time.Millisecond { |
| - t.Errorf("took %v to process one repeated proposal", took) |
| } |
| - if len(rows) != 1 { |
| - t.Errorf("rows = %d, want 1: a thousand copies are one proposal", len(rows)) |
| + }) |
| + |
| + // Mutation: recompute the recognisers per proposal. A thousand DISTINCT |
| + // proposals that are all present, against a dense passage: cached, one |
| + // ~70 ms pass; uncached, a thousand, cut off at the deadline. |
| + t.Run("the recognisers run once per passage", func(t *testing.T) { |
| + var prefix strings.Builder |
| + var props []provider.Pointer |
| + for i := 0; i < 1000; i++ { |
| + a := fmt.Sprintf("Unit%d-Harbour-Cork", i) |
| + prefix.WriteString(a + ",") |
| + props = append(props, provider.Pointer{Passage: 0, Text: a}) |
| + } |
| + s := build(t, prefix.String(), 20000) |
| + rec := &recorder{reply: func([]string) ([]provider.Pointer, error) { return props, nil }} |
| + s.extractor = rec |
| + took, _ := timed(t, s, "office address") |
| + onePassage(t, rec.calls) |
| + if took > 400*time.Millisecond { |
| + t.Errorf("took %v: the recognisers are running per proposal", took) |
| } |
| }) |
| - t.Run("hundreds of distinct proposals are refused by the cap, quickly", func(t *testing.T) { |
| - s := storeForTest(t) |
| - body := "## Our addresses\n\n" |
| - var want []provider.Pointer |
| - for i := 0; i < 400; i++ { |
| - a := fmt.Sprintf("%d Harbour Street, Cork", i+1) |
| - body += "Address: " + a + ".\n" |
| - want = append(want, provider.Pointer{Passage: 0, Text: a}) |
| + // Mutation: drop the dedupe. A thousand IDENTICAL near-miss proposals: |
| + // each would be a full ~1 ms scan of 1.23 MB; deduped, one. |
| + t.Run("identical proposals are examined once", func(t *testing.T) { |
| + s := build(t, "", 60000) |
| + props := make([]provider.Pointer, 1000) |
| + for i := range props { |
| + props[i] = provider.Pointer{Passage: 0, Text: "D02X285,info@tito.iq"} |
| } |
| - upload(t, s, "Tito office", body) |
| - s.extractor = &recorder{reply: func([]string) ([]provider.Pointer, error) { return want, nil }} |
| - start := time.Now() |
| - if rows := modelRows(searchFacts(t, s, "address")); len(rows) != 0 { |
| - t.Errorf("rows = %d; four hundred addresses are past the cap", len(rows)) |
| + rec := &recorder{reply: func([]string) ([]provider.Pointer, error) { return props, nil }} |
| + s.extractor = rec |
| + took, _ := timed(t, s, "office address") |
| + onePassage(t, rec.calls) |
| + // 600 ms: the search alone — opening and snippeting a 1.23 MB chunk — |
| + // is ~220 ms before the model path starts; without dedupe it is |
| + // ~1.02 s, deadline-bound (measured 2026-09-27). 300 ms, the first |
| + // threshold, left 80 ms of margin. |
| + if took > 600*time.Millisecond { |
| + t.Errorf("took %v: identical proposals are each being examined", took) |
| + } |
| + }) |
| + |
| + // Mutation: drop the per-proposal deadline check. 2,500 DISTINCT |
| + // near-miss proposals — dedupe cannot help — at ~1 ms each is ~2.5 s of |
| + // scanning; the check stops it at the 800 ms deadline. The threshold |
| + // sits between, and only the deadline can get under it. |
| + t.Run("the deadline stops the scan, not the end of the list", func(t *testing.T) { |
| + s := build(t, "", 60000) |
| + props := make([]provider.Pointer, 2500) |
| + for i := range props { |
| + props[i] = provider.Pointer{Passage: 0, Text: fmt.Sprintf("D02X285,info@tito.%dq", i)} |
| + } |
| + rec := &recorder{reply: func([]string) ([]provider.Pointer, error) { return props, nil }} |
| + s.extractor = rec |
| + took, rows := timed(t, s, "office address") |
| + onePassage(t, rec.calls) |
| + if took > 1400*time.Millisecond { |
| + t.Errorf("took %v: the scan ran past the deadline to the end of the list", took) |
| } |
| - if took := time.Since(start); took > 700*time.Millisecond { |
| - t.Errorf("took %v", took) |
| + if len(rows) != 0 { |
| + t.Errorf("rows = %+v", rows) |
| } |
| }) |
| @@ -916,3 +984,21 @@ func TestAModelResponseIsCheapToProcess(t *testing.T) { |
| } |
| }) |
| } |
| + |
| +// ctxAwareRecorder records like recorder, runs wait with the context it |
| +// was given (to spend the budget), then replies. |
| +func ctxAwareRecorder(rec *recorder, wait func(context.Context), reply func([]string) ([]provider.Pointer, error)) Extractor { |
| + return ctxThen{rec: rec, wait: wait, reply: reply} |
| +} |
| + |
| +type ctxThen struct { |
| + rec *recorder |
| + wait func(context.Context) |
| + reply func([]string) ([]provider.Pointer, error) |
| +} |
| + |
| +func (c ctxThen) ExtractAddresses(ctx context.Context, passages []string) ([]provider.Pointer, error) { |
| + c.rec.calls = append(c.rec.calls, append([]string(nil), passages...)) |
| + c.wait(ctx) |
| + return c.reply(passages) |
| +} |
diff --git a/internal/memoria/store.go b/internal/memoria/store.go| index 01df9e4..7a85f3d 100644 |
| --- a/internal/memoria/store.go |
| +++ b/internal/memoria/store.go |
| @@ -1283,6 +1283,19 @@ func (s *Store) modelFacts(ctx context.Context, results []Result, texts map[int6 |
| } |
| accepted = append(accepted, coord{ptr.Passage, start, end}) |
| } |
| + // **Checked AFTER the work, not only before each proposal.** The loop |
| + // above checks on entry, so the last proposal's recogniser pass could |
| + // run past the deadline and its row still be returned: a review had |
| + // one proposal arrive with 10 ms left against a 1.26 MB passage and got |
| + // a model row back at 1.01 s. A row the deadline has passed is dropped |
| + // here, again in the labelling loop, and once more before returning. |
| + // |
| + // What this does NOT bound: a single recogniser pass is not |
| + // interruptible, so the path can overrun by one passage's scan. The |
| + // rows are never late; the latency can be, by that much. |
| + if ctx.Err() != nil { |
| + return nil |
| + } |
| // **Sorted by (rank, offset) before anything is chosen**, so the |
| // provider's return order never decides which occurrence is cited. |
| @@ -1298,6 +1311,9 @@ func (s *Store) modelFacts(ctx context.Context, results []Result, texts map[int6 |
| var out []Fact |
| seen := map[string]bool{} |
| for _, c := range accepted { |
| + if ctx.Err() != nil { |
| + return nil |
| + } |
| text := passages[c.passage] |
| // 6. The span is sliced from the document. The model's own string |
| // is not used again. |
| @@ -1330,7 +1346,7 @@ func (s *Store) modelFacts(ctx context.Context, results []Result, texts map[int6 |
| // is all this layer can apply them to. §8.9 says what that costs: an |
| // address the model did not return cannot trip the cap, and nothing |
| // here can know it was there. |
| - if len(out) > MaxFacts || collides(out) { |
| + if len(out) > MaxFacts || collides(out) || ctx.Err() != nil { |
| return nil |
| } |
| return out |
diff --git a/internal/memoriatest/footer_model_test.go b/internal/memoriatest/footer_model_test.go| index f4f1242..a096263 100644 |
| --- a/internal/memoriatest/footer_model_test.go |
| +++ b/internal/memoriatest/footer_model_test.go |
| @@ -98,7 +98,10 @@ func footers(t *testing.T, h http.Handler, docID int64) map[string]string { |
| rec := get(t, h, path) |
| body := rec.Body.String() |
| i := strings.Index(body, "<footer") |
| - if rec.Code != http.StatusOK || i < 0 { |
| + // A footer is checked at WHATEVER status it renders with: an error |
| + // page carries the same footer and the same claim. The first |
| + // version checked 200s only. |
| + if i < 0 { |
| // **A skip has to be explained**, or the walk proves less than |
| // it says. Every route below renders no layout under this |
| // harness, for the reason given; anything else that fails to |