amadan
The pre-merge review: stop truncating phone numbers
Commit 073c17c was the response to the implementation review and landed
after it, so nobody had read it. Reviewed by Fable 5.1 before merge
(dispatched: fable). REQUEST CHANGES, three must-fix. The first is the
worst defect this branch has produced.
**The recogniser was truncating real phone numbers and citing the
result.** "Mobile: +353 87 1234567" — the commonest international
spelling of an Irish mobile — was boxed as "+353 87 12345". "+49 30 12
34 56 78" was boxed as "+49 30 12 34 56". Both are well-formed numbers
that pass every other check and are not the number the document
contains. Two mechanisms: a five-digit cap on a group cut a solid
subscriber part, and the pattern ended in an OPTIONAL extension, so a
match could stop mid-number. Either way this is §8.7's "cites correctly
while being wrong", manufactured by the guard written to prevent it —
and it was live in the commit that fixed the date and eircode cases.
The group bound came out of the pattern and went into the validator,
where the package's own doctrine says it belongs: loose regex, strict
validator. The pattern must now end on a word boundary or an extension,
never on nothing. Numbers are found whole or refused; never shortened.
Germany and France write more groups than the cap allows and are now
refused rather than truncated, which is the stated cost. Four spellings
that previously found nothing — "+353 1 5477854", "01 5477854" among
them — now work.
**"Deduped before labels changes no outcome" was false**, and the
outcome depended on where chunk.Split happened to cut: the same value
written twice, once with a short label and once with an over-long one,
silenced its kind when the two landed in different passages and did not
when they landed in the same one. The identity check now runs before
rule 4, so only the occurrence that is actually cited decides.
**The performance test guarded one of the three things it named.** It
repeated a single value, so dedupe left one label to derive, at byte
zero, where the backward walk returns immediately; unbounding the
lookback and reverting the binary search both stayed green. A second
version with distinct values and a five-second limit ALSO left both
green — 1.5s and 0.5s are slow and still inside any limit loose enough
not to flake. It now asserts the doubling ratio, which is the shape of
the curve rather than a wall-clock number: 2.32 pristine, 3.40 with the
linear scan, 4.49 with the bound removed. Both mutations red.
Also from the review: spans are sorted by END, not start, because they
NEST — a phone number inside an email address, sorted by start, came
back labelled "Mail info.087.123.4567". My own first test for this
sorted its own input and could not see Scan sorting wrongly; the
fixture that catches it goes through Scan. The IBAN tail is digits only,
so BE/ES/AT/PL/SE/LU IBANs followed by a BIC are found. isWordBreak and
isAbbreviation decode runes instead of bytes, which was reading 0xBA as
a letter and cutting sentences wrongly in both directions at once.
Two spec sentences it made false are corrected, and the all-caps eircode
residual ("SEE FORM P45 THEN P60") is now stated rather than left to be
found. Gate 34 asserts the fifth field it claimed; gate 35's order
fixture now disagrees with both sort-by-value and sort-by-label, where
before it agreed with both.
Still failing, still pre-existing on main: TestCreditsMatchProductionDependencies
reads $GOROOT/LICENSE, absent in the Homebrew layout.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
6 files changed,
+294
−49
docs/superpowers/specs/2026-09-03-memoria-design.md+25 −3internal/memoria/fact/fact.go+93 −21internal/memoria/fact/fact_test.go+145 −13internal/memoria/factbox_test.go+7 −0internal/memoria/store.go+15 −6internal/memoriatest/factbox_test.go+9 −6
diff --git a/docs/superpowers/specs/2026-09-03-memoria-design.md b/docs/superpowers/specs/2026-09-03-memoria-design.md| index 788a48a..1a5f0a9 100644 |
| --- a/docs/superpowers/specs/2026-09-03-memoria-design.md |
| +++ b/docs/superpowers/specs/2026-09-03-memoria-design.md |
| @@ -3102,15 +3102,37 @@ review found all three live, not as mutations: |
| separators walk straight past the 9-or-10 bound above: *"The deadline |
| is 01-02-2024 10:30"* matched `01-02-2024 10`, ten digits, valid, and |
| boxed a date. Three styles, never mixed. |
| -- **Four digit groups, never five.** *"Numbers drawn: 01 05 12 23 34"* |
| - canonicalises to ten digits and passes every other test. No phone |
| - number anybody writes has five groups. |
| +- **Four digit groups, or five with a country code — refused by count, |
| + never truncated.** *"Numbers drawn: 01 05 12 23 34"* canonicalises to |
| + ten digits and passes every other test, so something has to refuse it. |
| + **The count is the validator's job and not the pattern's**, and putting |
| + it in the pattern was a worse bug than the one it fixed: a pattern |
| + capped at three groups and five digits a group does not refuse a long |
| + number, it *shortens* it. *"Mobile: +353 87 1234567"* — the commonest |
| + international spelling of an Irish mobile — was boxed as |
| + **`+353 87 12345`**, and *"+49 30 12 34 56 78"* as `+49 30 12 34 56`. |
| + Both are well-formed numbers that pass every other check and are not |
| + the number in the document: §8.7's *"cites correctly while being |
| + wrong"*, manufactured by the guard meant to prevent it. |
| + |
| + So the recogniser is generous, it must end on a word boundary or an |
| + extension, and the validator refuses by group count. **Germany and |
| + France genuinely write more groups than this and their numbers are |
| + refused.** That is the stated cost, and it is the right way round: a |
| + refusal shows nothing, a truncation shows a wrong number. |
| - **The international floor is ten digits, not E.164's eight.** Eight |
| admits `+10 000 000` out of *"Growth of +10 000 000 users"*. A country |
| code plus a national number reaches ten everywhere this corpus dials, |
| and refusing a genuinely short international number is the safe |
| direction. |
| +**One residual is stated rather than left to be found: an ALL-CAPS |
| +heading.** `SEE FORM P45 THEN P60` still yields `P45 THEN`, because |
| +case-sensitivity cannot separate an eircode from uppercase English, and |
| +uploads do carry all-caps headings. No regex fixes this; only a label or |
| +a dictionary would, and both are bigger than the problem. Recorded so the |
| +next person finds the limit rather than the surprise. |
| + |
| **And eircodes are matched case-sensitively**, which is not fussiness. |
| The identifier alphabet is most of the consonants, so case-insensitively |
| it matches lowercase English after a road number or a form code: *"Take |
diff --git a/internal/memoria/fact/fact.go b/internal/memoria/fact/fact.go| index e6d9beb..904a104 100644 |
| --- a/internal/memoria/fact/fact.go |
| +++ b/internal/memoria/fact/fact.go |
| @@ -180,10 +180,19 @@ const sep = ` \x{00A0}` |
| // phoneGroups builds the digit-group half of the phone pattern for one |
| // separator style. |
| // |
| -// **Three repetitions, not four.** Four admits a five-group run of |
| -// two-digit numbers — "Numbers drawn: 01 05 12 23 34" canonicalises to |
| -// ten digits and passes every other test. No phone number anybody writes |
| -// has five groups. |
| +// **Loose, and the group count is the validator's job — not this |
| +// pattern's.** Bounding it here truncated real numbers instead of |
| +// refusing them, which is far worse than either: capped at five digits a |
| +// group, "Mobile: +353 87 1234567" was boxed as "+353 87 12345" — the |
| +// commonest international spelling of an Irish mobile, rendered as a |
| +// different number, cited, above the results. Capped at three groups, |
| +// "+49 30 12 34 56 78" was boxed as "+49 30 12 34 56". Both pass every |
| +// other check because what is left is a well-formed number; they are |
| +// §8.7's "cites correctly while being wrong", produced by the pattern |
| +// that was supposed to prevent it. |
| +// |
| +// So the groups are generous and phoneValid refuses by count. A refusal |
| +// shows nothing; a truncation shows a wrong number. |
| // |
| // **And at least one, which is what makes the three styles alternate.** |
| // Written {0,3} the first branch matched zero groups and won, because |
| @@ -192,7 +201,7 @@ const sep = ` \x{00A0}` |
| // with the dash branch never tried. A branch that cannot match zero |
| // groups fails, and the next one gets its turn. |
| func phoneGroups(sepClass string) string { |
| - return `(?:` + sepClass + `[0-9]{2,5}){1,3}` |
| + return `(?:` + sepClass + `[0-9]{2,7}){1,6}` |
| } |
| type recogniser struct { |
| @@ -255,11 +264,17 @@ var recognisers = []recogniser{ |
| // phone number uses one separator for all of it. RE2 has no |
| // backreferences, so the three styles are three alternatives. |
| re: regexp.MustCompile(`(?:\+[0-9]{1,3}[` + sep + `]*(?:\(0\))?|\b0)[` + sep + `]*` + |
| - `[0-9]{1,4}(?:` + phoneGroups(`[`+sep+`]`) + `|` + phoneGroups(`-`) + `|` + phoneGroups(`\.`) + `)?` + |
| + `[0-9]{1,9}(?:` + phoneGroups(`[`+sep+`]`) + `|` + phoneGroups(`-`) + `|` + phoneGroups(`\.`) + `)?` + |
| // An extension is part of the span, because §8.8 says the span |
| // is what the document wrote, and a number whose extension has |
| // been dropped is a number that does not reach the person. |
| - `(?i:[` + sep + `]*(?:ext|extension|x)\.?[` + sep + `]*[0-9]{1,6})?`), |
| + // |
| + // **An extension OR a word boundary, never "optionally |
| + // neither".** Written `(?:…)?` the match could stop in the |
| + // middle of a run of digits, which is the other half of how |
| + // "+353 87 1234567" became "+353 87 12345". Requiring one or |
| + // the other means a partial number is not a match at all. |
| + `(?:(?i:[` + sep + `]*(?:ext|extension|x)\.?[` + sep + `]*[0-9]{1,6})|\b)`), |
| canon: phoneCanon, |
| valid: phoneValid, |
| }, |
| @@ -278,7 +293,13 @@ var recognisers = []recogniser{ |
| // at the space before "and", because "and" is not four |
| // characters — and case-sensitivity means it would not match |
| // even if it were. |
| - re: regexp.MustCompile(`\b[A-Z]{2}[0-9]{2}(?:[A-Z0-9]{11,30}|(?:[` + sep + `][A-Z0-9]{4}){2,7}(?:[` + sep + `][A-Z0-9]{1,3})?)\b`), |
| + // |
| + // **The tail is digits only**, because an uppercase 1-3 character |
| + // token is exactly what follows an IBAN in practice: "BE68 5390 |
| + // 0754 7034 BIC: GEBABEBB" found nothing while the tail admitted |
| + // letters, and every country whose length is a multiple of four |
| + // (BE, ES, AT, PL, SE, LU) has no real tail for it to sit in. |
| + re: regexp.MustCompile(`\b[A-Z]{2}[0-9]{2}(?:[A-Z0-9]{11,30}|(?:[` + sep + `][A-Z0-9]{4}){2,7}(?:[` + sep + `][0-9]{1,3})?)\b`), |
| canon: func(s string) string { return strings.ToUpper(stripSep(s)) }, |
| valid: func(_, c string) bool { return ibanValid(c) }, |
| }, |
| @@ -377,6 +398,22 @@ func phoneValid(span, _ string) bool { |
| if !strings.HasPrefix(base, "+") && !strings.ContainsAny(base, " ().-") { |
| return false |
| } |
| + // **Refused by group count, never truncated.** A phone number is at |
| + // most four separator-delimited runs, or five once a country code is |
| + // counted: "01 547 7854" is three, "+353 87 1234567" is three. |
| + // "Numbers drawn: 01 05 12 23 34" is five, and "+49 30 12 34 56 78" |
| + // is six — both refused here rather than cut down to something that |
| + // would validate. Germany and France genuinely do write more groups |
| + // than this; refusing their numbers is the cost, and it is the right |
| + // way round, because the alternative is showing most of one. |
| + groups := digitRuns(base) |
| + max := 4 |
| + if strings.HasPrefix(base, "+") { |
| + max = 5 |
| + } |
| + if groups > max { |
| + return false |
| + } |
| digits := strings.TrimPrefix(phoneCanon(base), "+") |
| if strings.HasPrefix(base, "+") { |
| // E.164 caps at fifteen. The floor here is TEN rather than the |
| @@ -399,6 +436,22 @@ func phoneValid(span, _ string) bool { |
| return n == 9 || n == 10 |
| } |
| +// digitRuns counts the separator-delimited runs of digits in a span. |
| +func digitRuns(s string) int { |
| + n, in := 0, false |
| + for i := 0; i < len(s); i++ { |
| + if s[i] >= '0' && s[i] <= '9' { |
| + if !in { |
| + n++ |
| + in = true |
| + } |
| + continue |
| + } |
| + in = false |
| + } |
| + return n |
| +} |
| + |
| // extRe finds a trailing extension, so canon and valid can treat the base |
| // number and the extension separately while the SPAN keeps both. |
| var extRe = regexp.MustCompile(`(?i)[` + sep + `]*(?:ext|extension|x)\.?[` + sep + `]*[0-9]{1,6}$`) |
| @@ -548,10 +601,15 @@ func isSeparator(r rune) bool { |
| // bound: 0.49s at 100 KB, 1.94s at 200 KB, 48.5s at 1 MB, which at the |
| // upload cap is minutes of search latency bought by one person's file. |
| // |
| -// It changes no outcome. The run has to END in a separator near the |
| -// value and be MaxLabel characters or fewer to become a label, so a walk |
| -// that gets this far has already found a run too long to use; capped or |
| -// not, it falls through the same way. |
| +// **It changes an outcome in one case, and draft 2's claim that it |
| +// changes none was wrong.** Four hundred bytes is always more than |
| +// MaxLabel runes UNLESS most of them are whitespace, which TrimSpace |
| +// then removes: five hundred characters, four hundred and fifty spaces, |
| +// then "Phone: 01 547 7854" yields the document's own label where the |
| +// uncapped walk yielded none. That is the better answer and it is the |
| +// direction this file prefers everywhere else, so the bound stays — but |
| +// stated, because "changes no outcome" is the kind of claim that stops |
| +// anybody checking. |
| const maxLookback = 400 |
| // sentenceStart walks back from at to the start of the enclosing sentence |
| @@ -571,8 +629,12 @@ func sentenceStart(text string, at int) int { |
| // A full stop followed by a space. Whether it ends a sentence |
| // depends on the word it is attached to. |
| w := i |
| - for w > floor && !isWordBreak(text[w-1]) { |
| - w-- |
| + for w > floor { |
| + r, size := utf8.DecodeLastRuneInString(text[floor:w]) |
| + if size == 0 || isWordBreak(r) { |
| + break |
| + } |
| + w -= size |
| } |
| if isAbbreviation(text[w:i]) { |
| continue |
| @@ -588,10 +650,8 @@ func sentenceStart(text string, at int) int { |
| // label came back as "Smith's direct line" — the composing failure the |
| // abbreviation list exists to prevent, arriving through a space nobody |
| // can see. |
| -func isWordBreak(b byte) bool { |
| - // The second byte of U+00A0 in UTF-8 is 0xA0; the first is 0xC2, and |
| - // stopping on either is enough to end the token. |
| - return b == ' ' || b == '\n' || b == '\t' || b == 0xa0 || b == 0xc2 |
| +func isWordBreak(r rune) bool { |
| + return r == ' ' || r == '\n' || r == '\t' || r == '\u00a0' |
| } |
| // isAbbreviation reports whether a token ending in "." is one that does |
| @@ -604,7 +664,14 @@ func isAbbreviation(tok string) bool { |
| if abbreviations[strings.ToLower(tok)] { |
| return true |
| } |
| - return len(tok) == 2 && tok[1] == '.' && unicode.IsLetter(rune(tok[0])) |
| + // Decoded as RUNES, not bytes. `len(tok) == 2 && rune(tok[0])` read a |
| + // raw byte as a code point, so "Planta 2\u00ba. Tel\u00e9fono:" saw |
| + // 0xBA as the letter "\u00ba" and swallowed the sentence boundary, |
| + // while "\u00c9. Dupont's line:" was still cut to "Dupont's line" — |
| + // wrong in both directions at once, and the second is the composing |
| + // direction §8.8 calls the unsafe one. |
| + r, size := utf8.DecodeRuneInString(tok) |
| + return size > 0 && len(tok) == size+1 && tok[size] == '.' && unicode.IsLetter(r) |
| } |
| // labelFor implements §8.8's five rules. It returns the label and whether |
| @@ -648,7 +715,7 @@ func labelFor(text string, start int, spans [][2]int, heading, title string) (st |
| } |
| // lastSpanEndBefore returns the end of the latest span finishing at or |
| -// before at. spans must be sorted by start. |
| +// before at. spans must be sorted by END — see the sort in Scan. |
| func lastSpanEndBefore(spans [][2]int, at int) int { |
| lo, hi, out := 0, len(spans)-1, 0 |
| for lo <= hi { |
| @@ -720,7 +787,12 @@ func Scan(text, heading, title string, want []Kind) []Found { |
| } |
| } |
| } |
| - sort.Slice(spans, func(i, j int) bool { return spans[i][0] < spans[j][0] }) |
| + // Sorted by END, not by start, because lastSpanEndBefore searches on |
| + // the end and spans NEST: a phone number inside an email address, or |
| + // inside an IBAN, is a real pair. Sorted by start the predicate is |
| + // non-monotonic and the binary search can miss the enclosing span, |
| + // which puts a recognised value back inside another fact's label. |
| + sort.Slice(spans, func(i, j int) bool { return spans[i][1] < spans[j][1] }) |
| sort.SliceStable(hits, func(i, j int) bool { return hits[i].at[0] < hits[j].at[0] }) |
| // **Deduped BEFORE labels are derived, not after.** A label costs a |
diff --git a/internal/memoria/fact/fact_test.go b/internal/memoria/fact/fact_test.go| index 9d6bfdb..ef8be3b 100644 |
| --- a/internal/memoria/fact/fact_test.go |
| +++ b/internal/memoria/fact/fact_test.go |
| @@ -2,6 +2,7 @@ package fact |
| import ( |
| "reflect" |
| + "sort" |
| "strings" |
| "testing" |
| "time" |
| @@ -254,6 +255,14 @@ func TestLabelComesFromTheDocument(t *testing.T) { |
| text: "Dr. Smith's direct line: 01 547 7854", head: "Contact", title: "Tito", |
| want: []string{"Dr. Smith's direct line"}, |
| }, |
| + { |
| + // "1." is not an abbreviation, so the boundary IS taken and |
| + // the list marker is not part of the label. The rune check in |
| + // isAbbreviation is the only thing holding this. |
| + name: "a numbered list marker is not part of the label", |
| + text: "1. Office phone: 01 547 7854", head: "Contact", title: "Tito", |
| + want: []string{"Office phone"}, |
| + }, |
| { |
| name: "no headings at all falls to the title, never empty", |
| text: "Ring us on +353 1 547 7854 any weekday.", head: "", title: "Pasted contacts", |
| @@ -315,6 +324,12 @@ func TestOrdinaryTextIsNotAFact(t *testing.T) { |
| {Phone, "Numbers drawn: 01 05 12 23 34", "five two-digit groups"}, |
| {Phone, "Growth of +10 000 000 users this year.", "a plus and grouped thousands"}, |
| {Phone, "Companies House 12345678 is the reference.", "a bare run of digits"}, |
| + {Phone, "Card 0123 4567 8901 2345 expires soon.", "sixteen digits in four groups"}, |
| + // Germany and France really do write more groups than this, and |
| + // refusing their numbers is the stated cost of never truncating |
| + // one. The alternative was boxing "+49 30 12 34 56". |
| + {Phone, "Berlin +49 30 12 34 56 78", "six groups"}, |
| + {Phone, "Paris +33 1 23 45 67 89", "six groups"}, |
| } { |
| if got := Scan(c.text, "Contact", "Notes", []Kind{c.kind}); len(got) != 0 { |
| t.Errorf("%s: %q boxed %q as a %s", c.why, c.text, got[0].Span, c.kind) |
| @@ -336,6 +351,14 @@ func TestTheShapesDocumentsActuallyWrite(t *testing.T) { |
| {Phone, "Office phone: +353 (0)1 547 7854", "+353 (0)1 547 7854"}, |
| {Phone, "Ring 087 123 4567 today.", "087 123 4567"}, |
| {Phone, "Call 01-547-7854 for support.", "01-547-7854"}, |
| + // The commonest international spelling of an Irish mobile, and |
| + // the one a five-digit group cap rendered as "+353 87 12345" — a |
| + // different, well-formed, confidently cited number. |
| + {Phone, "Mobile: +353 87 1234567", "+353 87 1234567"}, |
| + {Phone, "Mobile: +353 (0)87 1234567", "+353 (0)87 1234567"}, |
| + {Phone, "Office phone: +353 1 5477854", "+353 1 5477854"}, |
| + {Phone, "Call 01 5477854 today.", "01 5477854"}, |
| + {Phone, "Tel +44 20 7123 4567 in London.", "+44 20 7123 4567"}, |
| {Eircode, "Eircode: D02 X285", "D02 X285"}, |
| {Eircode, "We are at A65 F4E2.", "A65 F4E2"}, |
| // Grouped, and followed by a BIC — which is how bank details are |
| @@ -344,6 +367,12 @@ func TestTheShapesDocumentsActuallyWrite(t *testing.T) { |
| {IBAN, "Our IBAN is IE29 AIBK 9311 5212 3456 78 and our BIC is AIBKIE2D.", "IE29 AIBK 9311 5212 3456 78"}, |
| {IBAN, "IBAN: IE29 AIBK 9311 5212 3456 78 BIC AIBKIE2D", "IE29 AIBK 9311 5212 3456 78"}, |
| {IBAN, "IBAN IE29AIBK93115212345678.", "IE29AIBK93115212345678"}, |
| + // Countries whose length is a multiple of four leave no tail for |
| + // a following token to hide in, so the tail must be digits only. |
| + {IBAN, "IBAN: BE68 5390 0754 7034 BIC: GEBABEBB", "BE68 5390 0754 7034"}, |
| + {IBAN, "IBAN ES91 2100 0418 4502 0005 1332 BIC CAIXESBB", "ES91 2100 0418 4502 0005 1332"}, |
| + {IBAN, "Norway: NO93 8601 1117 947.", "NO93 8601 1117 947"}, |
| + {IBAN, "Poland PL61 1090 1014 0000 0712 1981 2874 please.", "PL61 1090 1014 0000 0712 1981 2874"}, |
| {VAT, "Our VAT number is IE3384527RH.", "IE3384527RH"}, |
| } { |
| got := Scan(c.text, "Contact", "Notes", []Kind{c.kind}) |
| @@ -388,21 +417,124 @@ func TestALabelDoesNotDependOnWhichKindsWereAskedFor(t *testing.T) { |
| // Label derivation was quadratic in the facts a chunk holds, and |
| // chunk.Split bounds a chunk by WORDS — so a 2 MiB upload with no |
| // whitespace is one chunk, one line, and minutes of search latency |
| -// bought by one person's file. Measured before the bound: 48.5s at 1 MB. |
| +// bought by one person's file. Measured before the fix: 48.5s at 1 MB. |
| +// |
| +// **A DOUBLING RATIO, not a wall-clock limit, and the first two versions |
| +// of this test were both unreachable.** Version one repeated a single |
| +// value, so dedupe left one label to derive, at byte zero, where the |
| +// backward walk returns immediately: a review mutated the lookback bound |
| +// to 1<<40 and reverted the binary search to a linear scan and both |
| +// stayed green. Version two used distinct values and a five-second |
| +// limit, and both mutations stayed green again — 1.5s and 0.5s are |
| +// slow, and still well inside any limit loose enough not to flake on a |
| +// loaded machine. |
| +// |
| +// So the assertion is the SHAPE of the curve. Quadratic work quadruples |
| +// when the input doubles; linear work doubles. Measured on this fixture, |
| +// best of five: **2.32 pristine, 3.40 with the linear scan, 4.49 with |
| +// the bound removed.** Three is the line, and it is the only threshold |
| +// here that separates the real implementation from both mutations. |
| // |
| -// A wall-clock assertion, which is a blunt instrument, so the bound is |
| -// loose enough not to flake and tight enough that the quadratic version |
| -// cannot pass it by two orders of magnitude. |
| +// Best-of-five because the minimum of several runs is the robust |
| +// estimator for CPU-bound work: a loaded machine makes runs slower, not |
| +// faster, so noise inflates the measurement and the minimum discards it. |
| func TestLabellingIsNotQuadraticInAChunksFacts(t *testing.T) { |
| - text := strings.Repeat("D02X285.", 125000) // 1 MB, one line, no whitespace |
| - done := make(chan int, 1) |
| - go func() { done <- len(Scan(text, "H", "T", []Kind{Eircode})) }() |
| - select { |
| - case n := <-done: |
| - if n != 1 { |
| - t.Errorf("found %d candidates, want 1: they are all the same value", n) |
| + scan := func(n int) (time.Duration, int) { |
| + var b strings.Builder |
| + for i := 0; i < n; i++ { |
| + b.WriteString(eircodeNo(i)) |
| + b.WriteByte(',') // a separator, NOT a sentence boundary |
| + } |
| + text, best, found := b.String(), time.Hour, 0 |
| + for r := 0; r < 5; r++ { |
| + st := time.Now() |
| + found = len(Scan(text, "H", "T", []Kind{Eircode})) |
| + if d := time.Since(st); d < best { |
| + best = d |
| + } |
| + } |
| + return best, found |
| + } |
| + |
| + small, n := scan(10000) |
| + large, m := scan(20000) |
| + |
| + // The presence half: without it this measures the cost of finding |
| + // nothing, which is cheap and flat whatever the implementation does. |
| + if n != 10000 || m != 20000 { |
| + t.Fatalf("found %d and %d candidates, want 10000 and 20000 distinct", n, m) |
| + } |
| + if ratio := float64(large) / float64(small); ratio > 3 { |
| + t.Errorf("doubling the input multiplied the time by %.2f (%v to %v); "+ |
| + "linear is about 2 and quadratic about 4, so label derivation is quadratic again", |
| + ratio, small, large) |
| + } |
| +} |
| + |
| +// eircodeNo builds the i-th distinct valid eircode: a fixed routing key |
| +// and a base-25 identifier over the published alphabet. |
| +func eircodeNo(i int) string { |
| + const alphabet = "0123456789ACDEFHKNPRTVWXY" |
| + id := make([]byte, 4) |
| + for j := 3; j >= 0; j-- { |
| + id[j] = alphabet[i%len(alphabet)] |
| + i /= len(alphabet) |
| + } |
| + return "D02 " + string(id) |
| +} |
| + |
| +// lastSpanEndBefore searches on a span's END, so Scan sorts by end — and |
| +// that matters because spans NEST. A phone number sits inside an email |
| +// address and inside an IBAN, both of which really occur. Sorted by |
| +// start, the predicate "ends at or before here" is not monotonic, the |
| +// binary search can walk past the enclosing span, and a recognised value |
| +// goes back into another fact's label — the thing the earlier-span rule |
| +// exists to prevent. |
| +// |
| +// A direct unit test rather than a corpus fixture, because the timing |
| +// test above cannot see correctness and the corpus shapes that trigger |
| +// nesting are contrived enough that nobody would think to write one. |
| +func TestLastSpanEndBeforeHandlesNestedSpans(t *testing.T) { |
| + // A long outer span with two short ones inside it, as an email |
| + // address containing phone-shaped runs produces. |
| + spans := [][2]int{{12, 20}, {22, 30}, {4, 40}} |
| + sort.Slice(spans, func(i, j int) bool { return spans[i][1] < spans[j][1] }) |
| + for _, c := range []struct{ at, want int }{ |
| + {3, 0}, // before everything |
| + {20, 20}, // exactly on an inner end |
| + {21, 20}, |
| + {30, 30}, |
| + {45, 40}, // the OUTER span, which a start-sorted search can miss |
| + } { |
| + if got := lastSpanEndBefore(spans, c.at); got != c.want { |
| + t.Errorf("lastSpanEndBefore(%d) = %d, want %d", c.at, got, c.want) |
| + } |
| + } |
| +} |
| + |
| +// The same rule, asserted through Scan rather than against the helper, |
| +// because the helper's contract and Scan's obligation to meet it are two |
| +// different things. |
| +// |
| +// TestLastSpanEndBeforeHandlesNestedSpans sorts its own input, so it |
| +// cannot see Scan sorting by the wrong key — mutating that sort left it |
| +// green. This is the fixture that catches it: an email address whose |
| +// local part contains two phone-shaped runs, so a phone span really does |
| +// nest inside an email span. Sorted by start, the third number comes back |
| +// labelled "Mail info.087.123.4567" — a recognised value inside another |
| +// fact's label, which is the exact failure the earlier-span rule exists |
| +// to prevent, reappearing through the search that implements it. |
| +func TestScanSortsSpansSoNestingCannotLeakIntoALabel(t *testing.T) { |
| + const text = "Tel 01 547 7854. Mail info.087.123.4567-087-123-4568@example.ie" |
| + got := Scan(text, "Contact", "Notes", []Kind{Phone, Email}) |
| + if len(got) < 3 { |
| + t.Fatalf("found %d facts, want at least three: this fixture needs the nesting to be wrong about", len(got)) |
| + } |
| + for _, f := range got { |
| + for _, other := range got { |
| + if other.Span != f.Span && strings.Contains(f.Label, other.Span) { |
| + t.Errorf("%s label %q contains another fact's value %q", f.Kind, f.Label, other.Span) |
| + } |
| } |
| - case <-time.After(5 * time.Second): |
| - t.Fatal("scanning 1 MB took over 5s; label derivation is quadratic again") |
| } |
| } |
diff --git a/internal/memoria/factbox_test.go b/internal/memoria/factbox_test.go| index 2eff9a6..d9be148 100644 |
| --- a/internal/memoria/factbox_test.go |
| +++ b/internal/memoria/factbox_test.go |
| @@ -234,6 +234,13 @@ func TestGate34TheFactBoxNeverSpeaksAWithheldPassage(t *testing.T) { |
| if got.DocumentID != shown { |
| t.Errorf("citation links to document %d, want %d: the href points at the withheld document", got.DocumentID, shown) |
| } |
| + // The fifth field, which the comment above claimed and the |
| + // assertions did not make. Both documents here are single-chunk |
| + // so it cannot currently differ — asserted anyway, because the |
| + // claim should be true and the next fixture may have two. |
| + if got.Ordinal != 0 { |
| + t.Errorf("ordinal = %d, want 0", got.Ordinal) |
| + } |
| }) |
| // 7. A WITHHELD PASSAGE CANNOT SILENCE A KIND. §8.8's rule 4 — a |
diff --git a/internal/memoria/store.go b/internal/memoria/store.go| index e1343a3..5f8fa23 100644 |
| --- a/internal/memoria/store.go |
| +++ b/internal/memoria/store.go |
| @@ -1044,21 +1044,30 @@ func (s *Store) addFacts(ctx context.Context, p *Page, query string) { |
| continue |
| } |
| for _, f := range fact.Scan(text, r.Heading, r.Title, want) { |
| + if seen[f.Kind] == nil { |
| + seen[f.Kind] = map[string]bool{} |
| + } |
| + // **Identity is checked BEFORE rule 4, so only the occurrence |
| + // that is actually cited decides whether its kind renders.** |
| + // The other order made one document give two different |
| + // answers depending on where chunk.Split happened to cut: the |
| + // same value written twice, once with a short label and once |
| + // with an over-long one, silenced the kind when the two |
| + // landed in different passages and did not when they landed |
| + // in the same one. A boundary the reader cannot see is not |
| + // allowed to decide what the page says. |
| + if seen[f.Kind][f.Canonical] { |
| + continue |
| + } |
| if f.NoBox { |
| silenced[f.Kind] = true |
| continue |
| } |
| - if seen[f.Kind] == nil { |
| - seen[f.Kind] = map[string]bool{} |
| - } |
| // **Identity is the canonical value**, so one number written |
| // two ways is one candidate and the same number in five |
| // forwarded signatures is one too. First occurrence wins the |
| // citation, and because p.Results is in rank order that is |
| // the best-ranked passage carrying it. |
| - if seen[f.Kind][f.Canonical] { |
| - continue |
| - } |
| seen[f.Kind][f.Canonical] = true |
| found[f.Kind] = append(found[f.Kind], Fact{ |
| Kind: string(f.Kind), |
diff --git a/internal/memoriatest/factbox_test.go b/internal/memoriatest/factbox_test.go| index d493342..73f16e1 100644 |
| --- a/internal/memoriatest/factbox_test.go |
| +++ b/internal/memoriatest/factbox_test.go |
| @@ -181,12 +181,15 @@ func TestGate35CandidatesAmbiguityAndTheCap(t *testing.T) { |
| // nothing here could see it. Found by review, 2026-09-14. |
| t.Run("rows are in first-seen order", func(t *testing.T) { |
| h := newApp(t) |
| - // Sorted by canonical the office number comes FIRST; seen in |
| - // document order it comes second. The two orders disagree, which |
| - // is what makes this assertion able to fail. |
| - store(t, h, "Office address", "## Office Address\n\nTurn the knob. "+ |
| - "The office phone number is: +353 (0)1 598 1234. "+ |
| - "We also have the following unused phone numbers: +353 (0)1 547 7854.") |
| + // Both orderings a wrong implementation would pick disagree with |
| + // document order here: sorted by VALUE the 547 number comes |
| + // first, and sorted by LABEL "Alternate" sorts before "Work". |
| + // Only first-seen gives 598, so this one fixture holds against |
| + // both — and the version it replaces held against neither, |
| + // because its two labels happened to sort as they were seen. |
| + store(t, h, "Tito office", "## Contact\n\n"+ |
| + "Work phone is: +353 (0)1 598 1234. "+ |
| + "Alternate phone is: +353 (0)1 547 7854.") |
| got := factValues(search(t, h, "phone number")) |
| if len(got) != 2 { |
| t.Fatalf("values = %q, want two", got) |