amadan
Review 7 polish: the deadline's position, and LabelAt made impossible
Review 7 (Codex GPT-6-Astra, dispatched codex/gpt-6-astra) APPROVED 075e3d6 with no must-fix, and left two cheap polish items. Taken before landing because they can only make guards stricter; neither changes behaviour. **Which check catches the deadline was untested.** The clock test starts already expired, so the entry check caught it and nothing could tell whether the post-processing checks read the clock too: swapping either set back to ctx.Err() passed the suite. Two tests now move the deadline mid-run on a context whose Err() never fires — one past it during a ~70 ms recogniser pass, one past it at the tenth examination — and each of the two mutations now fails its own subtest, with the mutated tree confirmed to BUILD first. The moving deadline starts at 700 ms so that modelFacts' own 800 ms WithTimeout defers to it. Stable three times with ten CPUs busy. **LabelAt is gone.** Swapping LabelAtSpans for LabelAt passed the suite while recomputing every recogniser per label, because LabelAt did its own recomputation where the probe could not count it. Nothing in production called it. With it deleted the mistake does not compile, which is a stronger guard than any count.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
3 files changed,
+86
−15
internal/memoria/fact/fact.go+11 −12internal/memoria/fact/fact_test.go+3 −3internal/memoria/modelfacts_test.go+72 −0
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 3.310s ok amadan.net/oficina/memoria/internal/memoria/chunk (cached) ok amadan.net/oficina/memoria/internal/memoria/fact 0.521s ? 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.703s ? 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/internal/memoria/fact/fact.go b/internal/memoria/fact/fact.go| index 40c276a..82ffd1a 100644 |
| --- a/internal/memoria/fact/fact.go |
| +++ b/internal/memoria/fact/fact.go |
| @@ -895,18 +895,17 @@ func RecognisedSpans(text string) [][2]int { |
| return spans |
| } |
| -// LabelAt applies §8.8's five label rules to a value at byte offset start. |
| -// It returns the label, and whether the kind must render no box because |
| -// the document labelled the value too long to show. |
| -func LabelAt(text string, start int, heading, title string) (string, bool) { |
| - return LabelAtSpans(text, start, RecognisedSpans(text), heading, title) |
| -} |
| - |
| -// LabelAtSpans is LabelAt with the passage's recognised spans computed |
| -// once by the caller. RecognisedSpans runs every recogniser over the |
| -// whole passage, and a caller labelling many values in one passage must |
| -// not pay that per value — a review measured 6.8 seconds for a legal- |
| -// sized model response doing exactly that. |
| +// LabelAtSpans applies §8.8's five label rules to a value at byte offset |
| +// start. It returns the label, and whether the kind must render no box |
| +// because the document labelled the value too long to show. |
| +// |
| +// spans are the passage's RecognisedSpans, computed once by the caller. |
| +// **There is deliberately no variant that computes them itself.** One |
| +// existed — LabelAt — and a review showed that swapping it in for this |
| +// function passed the whole suite while recomputing every recogniser per |
| +// label: 6.8 seconds for a legal-sized model response. Removing it makes |
| +// that mistake a compile error instead of a timing regression nothing |
| +// could see. |
| func LabelAtSpans(text string, start int, spans [][2]int, heading, title string) (string, bool) { |
| return labelFor(text, start, spans, heading, title) |
| } |
diff --git a/internal/memoria/fact/fact_test.go b/internal/memoria/fact/fact_test.go| index 896ea82..efa61b3 100644 |
| --- a/internal/memoria/fact/fact_test.go |
| +++ b/internal/memoria/fact/fact_test.go |
| @@ -633,7 +633,7 @@ func TestRecognisedSpansCoverEveryKind(t *testing.T) { |
| // LabelAt must agree with Scan for a value Scan found — that is the |
| // whole reason it is exported rather than reimplemented. |
| -func TestLabelAtAgreesWithScan(t *testing.T) { |
| +func TestLabelAtSpansAgreesWithScan(t *testing.T) { |
| // Including an over-long label, so NoBox is compared too: with only |
| // the office chunk, a LabelAt that always returned NoBox=false agreed |
| // with Scan on every case and stayed green. |
| @@ -642,9 +642,9 @@ func TestLabelAtAgreesWithScan(t *testing.T) { |
| for _, f := range Scan(text, "Office Address", "Office", []Kind{Phone}) { |
| sawNoBox = sawNoBox || f.NoBox |
| start := strings.Index(text, f.Span) |
| - got, noBox := LabelAt(text, start, "Office Address", "Office") |
| + got, noBox := LabelAtSpans(text, start, RecognisedSpans(text), "Office Address", "Office") |
| if got != f.Label || noBox != f.NoBox { |
| - t.Errorf("LabelAt(%q) = %q/%v, Scan said %q/%v", f.Span, got, noBox, f.Label, f.NoBox) |
| + t.Errorf("LabelAtSpans(%q) = %q/%v, Scan said %q/%v", f.Span, got, noBox, f.Label, f.NoBox) |
| } |
| } |
| if !sawNoBox { |
diff --git a/internal/memoria/modelfacts_test.go b/internal/memoria/modelfacts_test.go| index 749d0ac..8424de0 100644 |
| --- a/internal/memoria/modelfacts_test.go |
| +++ b/internal/memoria/modelfacts_test.go |
| @@ -1018,6 +1018,68 @@ func TestAModelResponseIsCheapToProcess(t *testing.T) { |
| } |
| }) |
| + // **The deadline passing DURING processing**, with the timer still not |
| + // fired. The test above starts already expired, so it could not tell |
| + // which check caught it: a review swapped the post-processing checks |
| + // back to ctx.Err() — and separately the per-proposal entry check — |
| + // and both passed. These two fix the deadline's position. |
| + // |
| + // movingDeadline's Err() is always nil and its deadline is set by the |
| + // test mid-run. It starts at 700 ms so that modelFacts' own 800 ms |
| + // WithTimeout defers to it (a parent deadline that is sooner is kept), |
| + // and a deadline the test moves is then the one modelFacts reads. |
| + t.Run("a deadline passing mid-processing drops the row", func(t *testing.T) { |
| + for _, late := range []bool{false, true} { |
| + s := build(t, "12 Main Street, Dublin 2,", 20000) // ~70 ms recogniser pass |
| + dl := time.Now().Add(700 * time.Millisecond) |
| + ctx := movingDeadline{Context: context.Background(), dl: &dl} |
| + rec := &recorder{} |
| + s.extractor = ctxAwareRecorder(rec, func(context.Context) { |
| + if late { |
| + dl = time.Now().Add(20 * time.Millisecond) // passes during the pass |
| + } else { |
| + dl = time.Now().Add(time.Hour) |
| + } |
| + }, point(0, "12 Main Street, Dublin 2")) |
| + p, err := s.Search(ctx, suite.Subject{Kind: suite.SubjectAddress, Value: "paul@tito.io"}, "office address", 25) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + onePassage(t, rec.calls) |
| + rows := modelRows(p) |
| + 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 though the deadline passed during processing: %+v", rows) |
| + } |
| + } |
| + }) |
| + |
| + t.Run("a deadline passing between proposals stops the scan", func(t *testing.T) { |
| + s := build(t, "", 200) |
| + dl := time.Now().Add(700 * time.Millisecond) |
| + ctx := movingDeadline{Context: context.Background(), dl: &dl} |
| + s.probe = &modelProbe{afterExamine: func() {}} |
| + s.probe.afterExamine = func() { |
| + if s.probe.examinations == 10 { |
| + dl = time.Now().Add(-time.Millisecond) // passed; Err() stays nil |
| + } |
| + } |
| + 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 |
| + if _, err := s.Search(ctx, suite.Subject{Kind: suite.SubjectAddress, Value: "paul@tito.io"}, "office address", 25); err != nil { |
| + t.Fatal(err) |
| + } |
| + if s.probe.examinations != 10 { |
| + t.Errorf("examined %d proposals; the deadline passed by the clock at 10", s.probe.examinations) |
| + } |
| + }) |
| + |
| // The whole model path runs under one deadline: the extractor is |
| // handed a context that expires within the provider's budget even |
| // when the request's own context never does. |
| @@ -1089,3 +1151,13 @@ func TestExpiredReadsTheClock(t *testing.T) { |
| t.Error("expired() reported a context with no deadline") |
| } |
| } |
| + |
| +// movingDeadline is a context whose deadline the test moves mid-run and |
| +// whose Err() never fires — the lagging-timer state, arriving at a point |
| +// the test chooses. |
| +type movingDeadline struct { |
| + context.Context |
| + dl *time.Time |
| +} |
| + |
| +func (c movingDeadline) Deadline() (time.Time, bool) { return *c.dl, true } |