🤖 fix: the review's thirteen findings, two of them live bugs
Fable 5.1 reviewed slice 1 with the mandate §1.7 requires — pick the
mutations the author did not. Twenty-two compiled mutations, **eleven
survived**, and two of those were not weak gates but shipped bugs.
**S3 was fired at the wrong route.** `Content` built
`/suite/s3?item_id=X&revision=N`; the module declares the route as
`/suite/s3/{item_id}?revision=` on the line that names the constant, and
its conformance harness — which every sibling is held to — fires exactly
that. In production every fetch would have 404'd, memoria reads a 404 as
"not team-visible, or gone", and §8.1's S3 breaker would then have
reported a *permissions* anomaly for a routing bug: the plausible empty
list on the one call that fills the corpus.
**And the test pinned the wrong shape too**, which is why it was green.
That is the finding under the finding: a fixture written from the same
model as the code cannot catch a wrong assumption, it records one. What
found it was somebody who opened the module.
**S3 truncated an oversized body in silence.** S1 and S2 get this free —
a truncated JSON does not parse — but a body is plaintext, so a 2 MiB
prefix of a larger document indexed happily as the whole thing with its
tail unfindable. It now reads one byte past the cap and errors.
The rest, each with its gate:
- **A sixth `Disposition` was born silent.** `Notices()` enumerated the
loud values, so a new constant neither rendered nor noticed — and the
AHEAD tripwire instructs the next person to add exactly that constant.
Written as its two exceptions, a new disposition is loud until somebody
deliberately quiets it. `numDispositions` lets the test iterate the set
instead of a list of the five its author remembered.
- **Gate 23 was looser than the spec** and stayed green with `Deadline`
widened to 1200ms and 1400ms. Now the spec's own fixtures: 3×300ms
under 810ms, and 5s/10ms/10ms under 1.2s.
- **Two revisions of one item were never fixtured**, so keying the dedupe
on the id alone survived. That mutation renders a withdrawn revision on
a later one's permission — retraction-by-edit, undone.
- **"Several concurrent batches" had no timing at all.** Sequential
batches within one sibling passed everything.
- **`Rule` no longer returns notices.** Its only caller recomputed them
in its own order and discarded them, so the Outcome sat unused on a
concurrent path — and moving the recording into the goroutines is an
ordinary refactor that left every test green and is a concurrent map
write. Deleting the return removes where that bug can live.
- **`Withheld` is gone.** It rendered the same notices in different
words, so the page's copy depended on which of two renderers a caller
reached.
- **`NewSiblings` refuses a duplicate or empty name**, since the name is
the key a document's source resolves through.
- Notice counts pinned to passages, the cursor fixture given reserved
characters, S3's 5xx path tested, the header check made a closed set,
and four comments corrected that cited sections not saying what they
claimed.
Twelve mutations re-run against the fixes: ten red immediately. The two
survivors were real: the escaping fixture could not tell escaped from raw
(a space alone is not enough — it wanted "?" and "#"), and the sixth-
disposition case needs two edits at once, which no single-edit mutation
sees. Both closed, plus a control proving a new constant is loud.
`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
12 files changed,
+628
−175
AGENTS.md+37 −4internal/memoria/custody.go+25 −4internal/memoria/custody_test.go+146 −4internal/memoria/sibling/feed.go+39 −8internal/memoria/sibling/feed_test.go+110 −17internal/memoria/sibling/page.go+14 −10internal/memoria/sibling/page_test.go+151 −70internal/memoria/sibling/policy.go+46 −12internal/memoria/sibling/policy_test.go+21 −5internal/memoria/sibling/sibling.go+10 −28internal/memoria/sibling/sibling_test.go+28 −12internal/memoria/store.go+1 −1
diff --git a/AGENTS.md b/AGENTS.md| index fe46f20..6eba2e1 100644 |
| --- a/AGENTS.md |
| +++ b/AGENTS.md |
| @@ -114,6 +114,22 @@ so; it never renders as a confident zero. |
| findable" was said about a document whose previous revision was still |
| answering searches. |
| +**The suite contract** |
| +- **Read the route out of the module, never out of memory.** `RouteS3` is |
| + `/suite/s3` and the item id is a **path segment** — |
| + `/suite/s3/{item_id}?revision=` — which the constant's own line says and |
| + the conformance harness fires. Written as `?item_id=` it 404s against |
| + every conforming sibling, and memoria reads a 404 as "not team-visible, |
| + or gone". `url.PathEscape`, not `QueryEscape`: a space is `%20` in a |
| + path and `+` in a query. |
| +- **A cap on a response has to be detectable, not just applied.** S1 and |
| + S2 get that free — truncated JSON does not parse. An S3 body is |
| + plaintext, so read one byte past the cap and error on it, or a large |
| + document is silently indexed as its own first 2 MiB. |
| +- A sibling document's coordinate is `(sealed item id, sibling revision)`. |
| + **`Document.Revision` is memoria's own counter and is not it**, and |
| + `ExternalID` is a one-way fingerprint that cannot be asked about. |
| + |
| **Sealing** |
| - `internal/memoria/seal` is the only encryption surface. One HKDF purpose |
| per kind of thing sealed. |
| @@ -164,7 +180,7 @@ so; it never renders as a confident zero. |
| Every spec and every branch is reviewed by a model that did not write it, |
| before it is approved or merged, and the reviewing model is recorded. |
| -**Fourteen passes so far, on two strands that ran in parallel and are |
| +**Fifteen passes so far, on two strands that ran in parallel and are |
| counted together here because they are one project.** Spec drafts 1–3 by |
| GPT Sol and Fable; draft 4 by Fable 5.1; draft 5 by a reviewer **whose |
| attribution is disputed** — it named itself as the model that wrote the |
| @@ -172,7 +188,9 @@ draft, so that pass may not satisfy this rule at all. The slice-0 code by |
| Fable, the drag-and-drop branch by Fable 5.1, and five on the suite strand: |
| the §8.3 provider amendment, §8.4's shape, gate 17 with the live-key |
| verification, the doctrine adoption, and an adversarial mutation pass on |
| -the sign-in dial gate (gate 33 since the merge renumbered it). |
| +the sign-in dial gate (gate 33 since the merge renumbered it). Slice 1's |
| +S1/S3 clients, §4.1's policy layer and §6.5's fan-out by Fable 5.1 |
| +(2026-09-06): twenty-two mutations, eleven survived. |
| **Every one returned REQUEST CHANGES.** The most useful have been the ones |
| aimed at tests rather than prose. The provider review found a rule broken |
| @@ -185,11 +203,26 @@ forbade it, plus eight contradictions in sections the commit never opened |
| (§1.10); and the gate-27 pass found **two live bypasses in shipped code**, |
| not mutations. The drag-and-drop review found two more the same way — by |
| re-running the mutations their author claimed to have run, rather than |
| -taking the claim. |
| +taking the claim. The slice-1 review found two more, and the first is the |
| +worst kind: `Content` fired S3 at `?item_id=`, and the module declares |
| +the route as `/suite/s3/{item_id}?revision=` on the line that names the |
| +constant. Against a conforming sibling every fetch would have 404'd, read |
| +as "not team-visible, or gone" — the plausible empty list on the one call |
| +that fills the corpus — and §8.1's S3 breaker would then have reported a |
| +permissions anomaly for a routing bug. **The test pinned the wrong shape |
| +too**, which is why it was green: an author's fixture is written from the |
| +same wrong model as the code, so it cannot be the thing that catches it. |
| **The pattern is one failure wearing several coats:** the writer checks |
| their work against their own model of it rather than against the thing. |
| -Open the section you are citing. Run the mutation you did not think of. |
| +Open the section you are citing. Open the constant you are calling. Run |
| +the mutation you did not think of. |
| + |
| +**A test written from the same model as the code is part of the code.** |
| +It cannot catch a wrong assumption; it records one. The S3 route, gate |
| +23's bounds, the sixth disposition born silent — each had a test, each |
| +test agreed with the bug. What found them was somebody who had read the |
| +module and the spec instead. |
| When you retire something, grep for it by name — the sections you did not |
| open are where an adoption commit goes wrong, because a rewritten section |
| has been read twice and an untouched dependant never. |
diff --git a/internal/memoria/custody.go b/internal/memoria/custody.go| index 36f4537..d3e2dfa 100644 |
| --- a/internal/memoria/custody.go |
| +++ b/internal/memoria/custody.go |
| @@ -2,6 +2,8 @@ package memoria |
| import ( |
| "context" |
| + "errors" |
| + "fmt" |
| "amadan.net/oficina/suite" |
| @@ -32,7 +34,11 @@ type Custody interface { |
| // Rule answers for every candidate, in the order given. It does not |
| // fail: §4.1's four values already cover every outcome. |
| - Rule(ctx context.Context, asker suite.Subject, cands []sibling.Candidate) ([]sibling.Disposition, *sibling.Outcome) |
| + // |
| + // Dispositions only. The notices are the caller's to aggregate, |
| + // because they belong in the order the reader meets the results — |
| + // see Store.rule. |
| + Rule(ctx context.Context, asker suite.Subject, cands []sibling.Candidate) []sibling.Disposition |
| } |
| // Siblings is the production Custody: a roster of configured apps of |
| @@ -44,12 +50,27 @@ type Siblings struct { |
| // NewSiblings builds the roster. `team` binds the transport to one team's |
| // hosts; §6.4 is what that enforces and it is the module's to enforce. |
| -func NewSiblings(team string, dial suite.DialFunc, apps []sibling.Sibling) *Siblings { |
| +// |
| +// **A duplicate name is refused**, because the name is the key: it is what |
| +// a Document.Source resolves through and what a withholding notice prints. |
| +// Two apps sharing one would silently send one's item ids to the other, |
| +// and a conforming sibling answers NOT_VISIBLE for ids it has no row for |
| +// (§4.1's no-row rule) — which is the value that may be silent. So the |
| +// misconfiguration would present as passages quietly missing from every |
| +// page, with no notice and nothing in the logs. Refused at construction, |
| +// where it is a startup error somebody reads. |
| +func NewSiblings(team string, dial suite.DialFunc, apps []sibling.Sibling) (*Siblings, error) { |
| byName := make(map[string]sibling.Sibling, len(apps)) |
| for _, s := range apps { |
| + if s.Name == "" { |
| + return nil, errors.New("siblings: an app of record with no name cannot be resolved from a document's source") |
| + } |
| + if _, dup := byName[s.Name]; dup { |
| + return nil, fmt.Errorf("siblings: two apps configured as %q; the name is the key a document's source resolves through", s.Name) |
| + } |
| byName[s.Name] = s |
| } |
| - return &Siblings{client: sibling.New(team, dial), byName: byName} |
| + return &Siblings{client: sibling.New(team, dial), byName: byName}, nil |
| } |
| func (s *Siblings) Sibling(source string) (sibling.Sibling, bool) { |
| @@ -57,6 +78,6 @@ func (s *Siblings) Sibling(source string) (sibling.Sibling, bool) { |
| return got, ok |
| } |
| -func (s *Siblings) Rule(ctx context.Context, asker suite.Subject, cands []sibling.Candidate) ([]sibling.Disposition, *sibling.Outcome) { |
| +func (s *Siblings) Rule(ctx context.Context, asker suite.Subject, cands []sibling.Candidate) []sibling.Disposition { |
| return s.client.Rule(ctx, asker, cands) |
| } |
diff --git a/internal/memoria/custody_test.go b/internal/memoria/custody_test.go| index dcd80ef..6cdcd7a 100644 |
| --- a/internal/memoria/custody_test.go |
| +++ b/internal/memoria/custody_test.go |
| @@ -2,6 +2,7 @@ package memoria |
| import ( |
| "context" |
| + "fmt" |
| "io" |
| "log/slog" |
| "path/filepath" |
| @@ -36,16 +37,14 @@ func (f *fixedCustody) Sibling(source string) (sibling.Sibling, bool) { |
| return s, ok |
| } |
| -func (f *fixedCustody) Rule(ctx context.Context, asker suite.Subject, cands []sibling.Candidate) ([]sibling.Disposition, *sibling.Outcome) { |
| +func (f *fixedCustody) Rule(ctx context.Context, asker suite.Subject, cands []sibling.Candidate) []sibling.Disposition { |
| f.asker = asker |
| out := make([]sibling.Disposition, len(cands)) |
| - notices := &sibling.Outcome{} |
| for i, c := range cands { |
| f.pairs = append(f.pairs, c.Item) |
| out[i] = sibling.Decide(f.give) |
| - notices.Record(c.Sibling.Name, out[i]) |
| } |
| - return out, notices |
| + return out |
| } |
| func docsRoster() map[string]sibling.Sibling { |
| @@ -399,3 +398,146 @@ func TestNoticesAreInResultOrder(t *testing.T) { |
| } |
| } |
| } |
| + |
| +// **A count in a notice counts passages, not documents**, and this pins it |
| +// because nothing did. |
| +// |
| +// The two readings differ the moment a document contributes more than one |
| +// chunk, which is the ordinary case. "3 results withheld" for a |
| +// three-passage document is right, because the page counts results and a |
| +// result is a passage — the reader is being told how much shorter their |
| +// page is, not how many files exist. A review found the choice unmade: |
| +// recording per distinct pair instead of per candidate left every test |
| +// green and quietly changed the sentence. |
| +// |
| +// MUTATION-TESTED 2026-09-06, each verified red then reverted: |
| +// |
| +// Store.rule records per distinct (item, revision) instead of per result |
| +// -> "1 result withheld"; red |
| +// Record is moved after the keep check so only drops are recorded |
| +// -> green, and correctly so: Record ignores a Render. Listed here so |
| +// the next reader knows it was tried and is not a finding. |
| +func TestAWithholdingCountCountsPassages(t *testing.T) { |
| + ctx := context.Background() |
| + |
| + // One document that chunks into several passages, all of which carry |
| + // the same (item_id, revision). Paragraphs, because the chunker splits |
| + // on them — a first version repeated one long line and produced a |
| + // single chunk, which made the assertion below vacuous. |
| + var body strings.Builder |
| + for i := 0; i < 12; i++ { |
| + body.WriteString("The quarterly planning process runs in March. ") |
| + body.WriteString(strings.Repeat("Supporting detail follows in some length. ", 20)) |
| + body.WriteString("\n\n") |
| + } |
| + seed := func(t *testing.T, give suite.Authorization) (*Store, *fixedCustody) { |
| + t.Helper() |
| + cus := &fixedCustody{apps: docsRoster(), give: give} |
| + s := storeForTest(t) |
| + s.custody = cus |
| + if _, err := s.Ingest(ctx, IngestSpec{ |
| + Source: "Docs", Key: "docs/long", Title: "Long", |
| + Body: body.String(), ItemID: "docs-long", SiblingRevision: 2, |
| + }); err != nil { |
| + t.Fatalf("ingest: %v", err) |
| + } |
| + return s, cus |
| + } |
| + |
| + // **How many passages this document contributes is asked, not |
| + // assumed.** Chunking is the chunker's business and it changes; a test |
| + // that hard-codes the number tests the chunker by accident and goes |
| + // green on 1 when the assertion needs >1 to mean anything. |
| + visible, _ := seed(t, suite.VISIBLE) |
| + shown, err := visible.Search(ctx, testAsker, "quarterly planning", 10) |
| + if err != nil { |
| + t.Fatalf("search: %v", err) |
| + } |
| + passages := len(shown.Results) |
| + if passages < 2 { |
| + t.Fatalf("the fixture document contributes %d passage(s); with fewer "+ |
| + "than 2 this test cannot tell passages from documents", passages) |
| + } |
| + |
| + withheld, cus := seed(t, suite.UNKNOWN) |
| + page, err := withheld.Search(ctx, testAsker, "quarterly planning", 10) |
| + if err != nil { |
| + t.Fatalf("search: %v", err) |
| + } |
| + if len(page.Results) != 0 { |
| + t.Fatalf("%d passages rendered from an UNKNOWN sibling", len(page.Results)) |
| + } |
| + // The store hands the client **one candidate per passage**, and the |
| + // pairs are identical because they are the same document. Collapsing |
| + // them into one question is the client's job (`sibling.Client.Rule`, |
| + // and TestOneDocumentIsAskedAboutOnce is where that is gated) — it has |
| + // to be, because the client is what knows about batches and the wire. |
| + // |
| + // Keeping the store's side one-per-passage is what makes the mapping |
| + // back from answers to results a plain index, which is the thing that |
| + // must not go wrong. Asserted here so the division is written down |
| + // rather than inferred from whichever test happens to fail. |
| + if len(cus.pairs) != passages { |
| + t.Errorf("the store offered %d candidates for %d passages of one "+ |
| + "document, want one each", len(cus.pairs), passages) |
| + } |
| + for _, p := range cus.pairs { |
| + if want := (suite.ItemRevision{ItemID: "docs-long", Revision: 2}); p != want { |
| + t.Errorf("candidate %+v, want %+v — every passage of a document "+ |
| + "carries that document's coordinate", p, want) |
| + } |
| + } |
| + want := []string{fmt.Sprintf("Docs did not answer; %d results withheld.", passages)} |
| + if !reflect.DeepEqual(page.Notices, want) { |
| + t.Errorf("notices = %v, want %v — the count is passages, which is what "+ |
| + "the page calls results, and not documents", page.Notices, want) |
| + } |
| +} |
| + |
| +// Two siblings both withholding on one page, which is §6.5's ordinary |
| +// cold-wake case and had no fixture at all. |
| +// |
| +// It is here for two reasons. The obvious one is the copy: two apps, two |
| +// lines, in result order. The other is that this is the shape that makes a |
| +// concurrency bug in notice accumulation observable — a reviewer moved the |
| +// recording inside the fan-out goroutines and it stayed green under |
| +// `-race` purely because no test ever had two loud siblings at once. The |
| +// aggregation has since moved out of the concurrent path entirely, and |
| +// this runs with `-race` in CI so the fixture is there if it ever moves |
| +// back. |
| +func TestTwoSiblingsWithholdingOnOnePage(t *testing.T) { |
| + // Neither on the roster, so both are unreachable. |
| + cus := &fixedCustody{apps: map[string]sibling.Sibling{}, give: suite.VISIBLE} |
| + s := storeForTest(t) |
| + s.custody = cus |
| + ctx := context.Background() |
| + for _, d := range []struct{ source, key, body, item string }{ |
| + {"Sheets", "s/1", "Quarterly planning, quarterly planning, quarterly planning, quarterly planning.", "s-1"}, |
| + {"Sheets", "s/2", "Quarterly planning, quarterly planning, quarterly planning.", "s-2"}, |
| + {"Docs", "d/1", "A shorter note on quarterly planning buried in a great many other unrelated words indeed.", "d-1"}, |
| + } { |
| + if _, err := s.Ingest(ctx, IngestSpec{ |
| + Source: d.source, Key: d.key, Title: d.key, Body: d.body, |
| + ItemID: d.item, SiblingRevision: 1, |
| + }); err != nil { |
| + t.Fatalf("ingest %s: %v", d.key, err) |
| + } |
| + } |
| + |
| + page, err := s.Search(ctx, testAsker, "quarterly planning", 10) |
| + if err != nil { |
| + t.Fatalf("search: %v", err) |
| + } |
| + if len(page.Results) != 0 { |
| + t.Fatalf("%d passages rendered from two unreachable apps", len(page.Results)) |
| + } |
| + want := []string{ |
| + "Sheets did not answer; 2 results withheld.", |
| + "Docs did not answer; 1 result withheld.", |
| + } |
| + if !reflect.DeepEqual(page.Notices, want) { |
| + t.Errorf("notices = %v, want %v — grouped per app, counted per "+ |
| + "passage, ordered by the results they are about", |
| + page.Notices, want) |
| + } |
| +} |
diff --git a/internal/memoria/sibling/feed.go b/internal/memoria/sibling/feed.go| index 44dd3be..cb86044 100644 |
| --- a/internal/memoria/sibling/feed.go |
| +++ b/internal/memoria/sibling/feed.go |
| @@ -77,17 +77,38 @@ func (c *Client) Feed(ctx context.Context, s Sibling, cursor string) (suite.Feed |
| // ingest — and §1.3 records that the fix was deleting the false claim |
| // rather than adding a person to the call. |
| // |
| -// A missing item is **404, never 403**, so the two are indistinguishable |
| -// and nobody learns which ids exist. The module's DecodeS3 turns both into |
| -// ErrNotAvailable and this returns that unchanged: a caller that branched |
| -// on which one it was would rebuild the oracle §4.2 closes — the same |
| -// oracle the no-row S2 rule opened and had to be withdrawn for (§4.1). |
| +// A missing item is **404, never 403**, and that rule binds the SIBLING: |
| +// its 404 for an item it will not serve is the same 404 as for an item it |
| +// does not have, so nobody learns which ids exist. DecodeS3 turns that |
| +// 404 into ErrNotAvailable and this returns it unchanged. |
| +// |
| +// A 403 is a different thing and stays distinguishable on purpose — the |
| +// module answers ErrContractBreach — because a sibling answering 403 is |
| +// leaking, and folding that into ErrNotAvailable would hide the leak from |
| +// the only person who could fix it. |
| func (c *Client) Content(ctx context.Context, s Sibling, itemID string, revision int64) ([]byte, error) { |
| ctx, cancel := context.WithTimeout(ctx, Deadline) |
| defer cancel() |
| - u := fmt.Sprintf("%s%s?item_id=%s&revision=%d", |
| - s.BaseURL, suite.RouteS3, url.QueryEscape(itemID), revision) |
| + // **The item id is a PATH segment, not a query parameter.** The |
| + // module says so where it declares the route — `RouteS3 = "/suite/s3" |
| + // // GET, /suite/s3/{item_id}?revision=` — and its conformance |
| + // harness, which every sibling is held to, fires exactly that shape. |
| + // |
| + // This was written as `?item_id=` and a review caught it. Against a |
| + // conforming sibling every fetch would have 404'd, memoria would have |
| + // read each as "not team-visible, or gone", and §8.1's S3 breaker |
| + // (twenty fetches, all 404) would have reported a permissions anomaly |
| + // for a routing bug — the plausible empty list on the one call that |
| + // fills the corpus. The test pinned the wrong shape too, which is why |
| + // it went green: checked against my own model of the route rather |
| + // than against the route. |
| + // |
| + // PathEscape, not QueryEscape: a space is %20 in a path segment and |
| + // "+" in a query, and a sibling reading "+" back as a space finds no |
| + // such item. |
| + u := fmt.Sprintf("%s%s/%s?revision=%d", |
| + s.BaseURL, suite.RouteS3, url.PathEscape(itemID), revision) |
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, u, nil) |
| if err != nil { |
| return nil, fmt.Errorf("sibling %s: build S3 request: %w", s.Name, err) |
| @@ -100,9 +121,19 @@ func (c *Client) Content(ctx context.Context, s Sibling, itemID string, revision |
| } |
| defer res.Body.Close() |
| - raw, err := io.ReadAll(io.LimitReader(res.Body, maxContentResponse)) |
| + // Read one byte past the cap, so going over it is **detectable** |
| + // rather than silent. S1 and S2 get this for free — a truncated JSON |
| + // does not parse — but an S3 body is plaintext, and a 2 MiB prefix of |
| + // a larger document indexes perfectly happily as the whole thing, |
| + // with its tail unfindable and nothing said. That is a document |
| + // quietly shortened, which is the same shape as a page quietly |
| + // shortened and no better for being about bytes. |
| + raw, err := io.ReadAll(io.LimitReader(res.Body, maxContentResponse+1)) |
| if err != nil { |
| return nil, fmt.Errorf("sibling %s: reading S3: %w", s.Name, err) |
| } |
| + if len(raw) > maxContentResponse { |
| + return nil, fmt.Errorf("sibling %s: S3 body over %d bytes", s.Name, maxContentResponse) |
| + } |
| return suite.DecodeS3(res.StatusCode, raw) |
| } |
diff --git a/internal/memoria/sibling/feed_test.go b/internal/memoria/sibling/feed_test.go| index e11f83c..c5bbd8c 100644 |
| --- a/internal/memoria/sibling/feed_test.go |
| +++ b/internal/memoria/sibling/feed_test.go |
| @@ -103,12 +103,16 @@ func TestAFailedPollIsNeverAnEmptyFeed(t *testing.T) { |
| }, |
| }) |
| }) |
| - feed, err := newTestClient().Feed(ctx, s, "stored-cursor") |
| + // A cursor with reserved characters in it. Cursors are opaque and |
| + // base64 more often than not, so "+/=&" is the ordinary case and |
| + // not an exotic one — and with a plain alphanumeric fixture, |
| + // deleting the escaping entirely stayed green. Found by a review. |
| + feed, err := newTestClient().Feed(ctx, s, "a+b/c=d&e") |
| if err != nil { |
| t.Fatalf("a good page failed to decode: %v — if Feed always errors, "+ |
| "every assertion above passes for the wrong reason", err) |
| } |
| - if gotCursor != "stored-cursor" { |
| + if gotCursor != "a+b/c=d&e" { |
| t.Errorf("cursor sent = %q, want the stored one: a poll that forgets "+ |
| "its position re-reads the whole feed", gotCursor) |
| } |
| @@ -155,6 +159,21 @@ func TestAFailedPollIsNeverAnEmptyFeed(t *testing.T) { |
| // -> closed-set case red |
| // drop the revision from the URL |
| // -> closed-set case red |
| +// send the item id as ?item_id= instead of a path segment |
| +// -> path case red. **This was the shipped bug**, and the first |
| +// version of this test asserted the query shape, so it pinned the |
| +// bug rather than catching it. Found by a review that opened the |
| +// module's route declaration. |
| +// build the path without url.PathEscape |
| +// -> path case red — but ONLY since the fixture id gained "?" and |
| +// "#". Against `doc a/b` this mutation survived, because Go |
| +// accepts a raw space and hands back the same decoded path. |
| +// escape the path segment with QueryEscape |
| +// -> path case red (a space becomes "+", which is a different item) |
| +// return the body on a 5xx |
| +// -> the 502 case red |
| +// read exactly maxContentResponse instead of one byte more |
| +// -> the oversized case red |
| func TestContentFetch(t *testing.T) { |
| ctx := context.Background() |
| @@ -206,14 +225,58 @@ func TestContentFetch(t *testing.T) { |
| } |
| }) |
| + t.Run("a 5xx is an error and not a body", func(t *testing.T) { |
| + // An error page is a plausible document: it is text, it is UTF-8, |
| + // and it indexes. Returning it would store a sibling's 502 as the |
| + // content of a document memoria then cites. |
| + s := serve(t, func(w http.ResponseWriter, r *http.Request) { |
| + w.WriteHeader(http.StatusBadGateway) |
| + _, _ = w.Write([]byte("<html>502 Bad Gateway</html>")) |
| + }) |
| + body, err := newTestClient().Content(ctx, s, "doc-a", 1) |
| + if err == nil { |
| + t.Fatalf("a 502 decoded as success, body %q", body) |
| + } |
| + if body != nil { |
| + t.Errorf("a 502 returned a body: %q", body) |
| + } |
| + }) |
| + |
| + t.Run("an oversized body is an error, not a shorter document", func(t *testing.T) { |
| + // **The cap has to be detectable, not merely applied.** S1 and S2 |
| + // get this free — a truncated JSON does not parse — but a body is |
| + // plaintext, and a prefix of a larger document indexes perfectly |
| + // well as the whole thing, its tail unfindable and nothing said. |
| + // A review found this by serving one byte over the cap and |
| + // watching it come back as success. |
| + s := serve(t, func(w http.ResponseWriter, r *http.Request) { |
| + _, _ = w.Write(make([]byte, maxContentResponse+1)) |
| + }) |
| + body, err := newTestClient().Content(ctx, s, "doc-a", 1) |
| + if err == nil { |
| + t.Fatalf("a body over the cap came back as a %d-byte document", len(body)) |
| + } |
| + if body != nil { |
| + t.Errorf("an over-cap fetch returned a body: %d bytes", len(body)) |
| + } |
| + }) |
| + |
| t.Run("the call names an item and a revision and nobody", func(t *testing.T) { |
| + var path string |
| var query url.Values |
| var headers http.Header |
| s := serve(t, func(w http.ResponseWriter, r *http.Request) { |
| - query, headers = r.URL.Query(), r.Header.Clone() |
| + path, query, headers = r.URL.Path, r.URL.Query(), r.Header.Clone() |
| _, _ = w.Write([]byte("the body")) |
| }) |
| - body, err := newTestClient().Content(ctx, s, "doc-a", 7) |
| + // An id carrying the characters that CHANGE THE REQUEST when they |
| + // are not escaped. A space alone does not: Go accepts it, the path |
| + // arrives decoded, and a raw concatenation is indistinguishable |
| + // from an escaped one — a review's mutation dropping the escaping |
| + // entirely stayed green against `doc a/b`. With "?" and "#" in it |
| + // the unescaped path truncates at the "?", the fragment eats the |
| + // rest, and the revision never arrives at all. |
| + body, err := newTestClient().Content(ctx, s, "doc a?b#c", 7) |
| if err != nil { |
| t.Fatalf("a good fetch failed: %v — with this red, every assertion "+ |
| "above passes for a Content that only ever errors", err) |
| @@ -222,26 +285,56 @@ func TestContentFetch(t *testing.T) { |
| t.Errorf("body = %q, want %q", body, "the body") |
| } |
| + // **The item id is a path segment.** The module declares the route |
| + // as `/suite/s3/{item_id}?revision=` and its conformance harness |
| + // fires exactly that, so a consumer sending `?item_id=` 404s |
| + // against every conforming sibling. This test asserted the query |
| + // shape and so pinned the bug in place; a review caught both. |
| + // |
| + // PathEscape rather than QueryEscape: a space is %20 in a path and |
| + // "+" in a query, and a sibling reading "+" back as a space finds |
| + // no such item. |
| + if want := "/suite/s3/doc a?b#c"; path != want { |
| + t.Errorf("S3 path = %q, want %q", path, want) |
| + } |
| // A closed set, not a list of forbidden names. §6.1 says the call |
| // carries no person, and the way to assert that is to enumerate what |
| // it may carry and fail on anything else — a check spelled |
| // `query.Get("asker") == ""` passes for `user`, `on_behalf_of`, and |
| // every other spelling of the same mistake. |
| - want := url.Values{"item_id": {"doc-a"}, "revision": {"7"}} |
| + want := url.Values{"revision": {"7"}} |
| if !reflect.DeepEqual(query, want) { |
| t.Errorf("S3 query = %v, want exactly %v", query, want) |
| } |
| - if got := headers.Get("Authorization"); got != "Bearer test-consumer-token" { |
| - t.Errorf("Authorization = %q, want the consumer token", got) |
| - } |
| - // The consumer token identifies the *app* (§4.3). A person would |
| - // have to arrive as some second header, so the headers get the same |
| - // treatment as the query: named ones checked, and the rest is what |
| - // the Go client adds on its own. |
| - for _, h := range []string{"From", "X-Asker", "X-User", "Cookie"} { |
| - if v := headers.Get(h); v != "" { |
| - t.Errorf("S3 carried %s: %q — the ingest call has nobody on it", h, v) |
| - } |
| - } |
| + // **The headers get the closed-set treatment too**, and the first |
| + // version of this did not: it named four headers a person might |
| + // arrive in, which is the shape the paragraph above rejects. A |
| + // review sent `X-On-Behalf-Of` straight through it. |
| + assertNoPersonInHeaders(t, "S3", headers) |
| }) |
| } |
| + |
| +// assertNoPersonInHeaders pins the request's headers to a closed set. |
| +// |
| +// Only three may be present: the consumer token, which identifies the |
| +// APP and not a person (§4.3), and the two the Go client adds on its own. |
| +// Anything else is a finding whatever it is called — the point of a |
| +// closed set is that it does not need to guess the name. |
| +func assertNoPersonInHeaders(t *testing.T, call string, h http.Header) { |
| + t.Helper() |
| + if got := h.Get("Authorization"); got != "Bearer test-consumer-token" { |
| + t.Errorf("%s Authorization = %q, want the consumer token", call, got) |
| + } |
| + allowed := map[string]bool{ |
| + "Authorization": true, |
| + "User-Agent": true, |
| + "Accept-Encoding": true, |
| + } |
| + for name := range h { |
| + if !allowed[name] { |
| + t.Errorf("%s carried the header %q: the call is custody-scoped and "+ |
| + "person-less (§6.1), and a closed set is the only way to say "+ |
| + "that without guessing what a person would be called", call, name) |
| + } |
| + } |
| +} |
diff --git a/internal/memoria/sibling/page.go b/internal/memoria/sibling/page.go| index 1b86560..986353c 100644 |
| --- a/internal/memoria/sibling/page.go |
| +++ b/internal/memoria/sibling/page.go |
| @@ -46,11 +46,21 @@ type Candidate struct { |
| // Every candidate gets exactly one Disposition. There is no error return |
| // and no short slice: a caller with a fifth outcome to handle has somewhere |
| // to forget to handle it, and the forgotten branch renders. |
| -func (c *Client) Rule(ctx context.Context, asker suite.Subject, candidates []Candidate) ([]Disposition, *Outcome) { |
| +// |
| +// **It returns dispositions and not notices**, and it used to return both. |
| +// The caller that renders a page has to aggregate in ITS order anyway — |
| +// the reader meets the notices next to the results they are about — so the |
| +// Outcome built here was recomputed and discarded, which is a value nobody |
| +// uses sitting on a concurrent code path. A review then showed what that |
| +// costs: moving the recording inside the goroutines is an ordinary |
| +// refactor, it left every test green, and it is a concurrent map write — |
| +// `fatal error` on §6.5's most ordinary case, a cold wake where two |
| +// siblings both time out. Deleting the return removes the place that bug |
| +// can live rather than adding a test that watches for it. |
| +func (c *Client) Rule(ctx context.Context, asker suite.Subject, candidates []Candidate) []Disposition { |
| out := make([]Disposition, len(candidates)) |
| - notices := &Outcome{} |
| if len(candidates) == 0 { |
| - return out, notices |
| + return out |
| } |
| // Group by sibling, deduping pairs as we go. positions maps a pair back |
| @@ -92,13 +102,7 @@ func (c *Client) Rule(ctx context.Context, asker suite.Subject, candidates []Can |
| } |
| } |
| wg.Wait() |
| - |
| - // Notices are accumulated after the fan-in, in candidate order, so a |
| - // page's lines do not depend on which sibling answered first. |
| - for i, cand := range candidates { |
| - notices.Record(cand.Sibling.Name, out[i]) |
| - } |
| - return out, notices |
| + return out |
| } |
| // scatter writes one batch's answers out to every candidate that asked |
diff --git a/internal/memoria/sibling/page_test.go b/internal/memoria/sibling/page_test.go| index b8f0a39..213fef9 100644 |
| --- a/internal/memoria/sibling/page_test.go |
| +++ b/internal/memoria/sibling/page_test.go |
| @@ -68,29 +68,46 @@ func answering(t *testing.T, name string, pause time.Duration, result suite.Auth |
| // 2.4 s, which no per-call test can see. A concurrency assertion alone is |
| // satisfied by a client that gives up instantly. |
| // |
| -// **The wall-clock bounds are literals, not multiples of Deadline**, and |
| -// that is the point of writing them out. A test bounded against the |
| -// constant it is testing widens when the constant does: an earlier review |
| -// of this package mutated `Deadline` to 30 s and the timing test stayed |
| -// green, because it was measuring the code against itself. These numbers |
| -// go red if the deadline moves, which is the correct behaviour — §6.5 owes |
| -// a measurement (§13), and the day it arrives this test is where the new |
| -// number gets argued for. |
| +// **Two fixtures, and the spec is emphatic that one cannot hold both.** |
| +// §10 gate 23 works through why: against the deadline fixture, an overlap |
| +// assertion is either always red or never red depending on whose clock you |
| +// read, so the concurrency half needs its own fixture of comparable |
| +// sub-deadline latencies. |
| +// |
| +// The numbers are the spec's, not this file's: |
| +// |
| +// deadline half 3 siblings — one at 5s, two at 10ms — page under 1.2s |
| +// concurrency half 3 siblings at 300ms each — page under 810ms |
| +// |
| +// 810 ms is 2.7x the concurrent time and 0.9x the sequential one, so it |
| +// separates the two by a wide margin at both ends rather than sitting on |
| +// top of either. An earlier version of this test used 3x400ms/900ms and |
| +// 3s/1.5s, and a review showed what the slack bought: the deadline half |
| +// stayed **green with Deadline widened to 1200ms and to 1400ms**. The |
| +// bound has to be the spec's or the spec is not what is being tested. |
| +// |
| +// **The bounds are literals, not multiples of Deadline.** A test bounded |
| +// against the constant it is testing widens when the constant does, which |
| +// is the failure above in its purest form. §6.5 owes a measurement (§13), |
| +// and the day it arrives this test is where the new number is argued for. |
| // |
| // TWO-SIDED throughout: every timing bound is paired with an assertion |
| // about what came back, so a client that returns instantly with everything |
| // UNKNOWN fails. |
| // |
| -// MUTATION-TESTED 2026-09-06, each verified red then reverted — see the |
| -// per-subtest notes. |
| +// MUTATION-TESTED 2026-09-06, each verified red against BOTH fixtures then |
| +// reverted — per the spec's instruction, since the point of the split is |
| +// that neither fixture sees both faults: |
| +// |
| +// batches run sequentially -> concurrency half red at ~900ms |
| +// the per-call deadline goes -> deadline half red at ~5s |
| +// Deadline widened to 1200ms -> deadline half red (it was green before) |
| func TestOnePageIsOneRoundOfCalls(t *testing.T) { |
| - t.Run("three siblings cost one sibling's time, not three", func(t *testing.T) { |
| - // MUTATION: run the batches sequentially (wg.Wait() inside the loop) |
| - // -> elapsed ~1.2s, red. |
| + t.Run("concurrency: three siblings cost one sibling's time", func(t *testing.T) { |
| var a, b, c int64 |
| - sheets := answering(t, "Sheets", 400*time.Millisecond, suite.VISIBLE, &a) |
| - docs := answering(t, "Docs", 400*time.Millisecond, suite.VISIBLE, &b) |
| - cal := answering(t, "Calendar", 400*time.Millisecond, suite.VISIBLE, &c) |
| + sheets := answering(t, "Sheets", 300*time.Millisecond, suite.VISIBLE, &a) |
| + docs := answering(t, "Docs", 300*time.Millisecond, suite.VISIBLE, &b) |
| + cal := answering(t, "Calendar", 300*time.Millisecond, suite.VISIBLE, &c) |
| cands := []Candidate{ |
| {sheets, suite.ItemRevision{ItemID: "s1", Revision: 1}}, |
| @@ -99,48 +116,42 @@ func TestOnePageIsOneRoundOfCalls(t *testing.T) { |
| } |
| start := time.Now() |
| - got, notices := newTestClient().Rule(context.Background(), askerForTest(), cands) |
| + got := newTestClient().Rule(context.Background(), askerForTest(), cands) |
| elapsed := time.Since(start) |
| - // 1.2s is the sum; 900ms leaves room for scheduling on a loaded |
| - // machine while staying far below it. |
| - if elapsed > 900*time.Millisecond { |
| - t.Errorf("three 400ms siblings took %v: the page is paying their "+ |
| - "sum, not their slowest", elapsed) |
| + if elapsed > 810*time.Millisecond { |
| + t.Errorf("three 300ms siblings took %v, over the spec's 810ms: the "+ |
| + "page is paying their sum, not their slowest", elapsed) |
| } |
| - // The other half. Without it, `return allUnknown` in 0ms passes. |
| + // The other half. Without it, `return allUnknown` in 0ms passes, |
| + // and so does every upper bound in this file. |
| for i, d := range got { |
| if !d.Renders() { |
| t.Errorf("candidate %d = %v, want Render — a page that answers "+ |
| "fast by answering nothing is not a fast page", i, d) |
| } |
| } |
| - if lines := notices.Lines(); len(lines) != 0 { |
| - t.Errorf("a fully-answered page carried notices: %v", lines) |
| - } |
| }) |
| - t.Run("a slow sibling costs its deadline and no more, and the rest render", func(t *testing.T) { |
| - // MUTATION: drop the ctx deadline in Authorize (use context.Background) |
| - // -> elapsed ~3s, red. |
| - // MUTATION: return the caller's ctx unchanged into Do |
| - // -> same, red. |
| - var slow, fast int64 |
| - sheets := answering(t, "Sheets", 3*time.Second, suite.VISIBLE, &slow) |
| - docs := answering(t, "Docs", 50*time.Millisecond, suite.VISIBLE, &fast) |
| + t.Run("deadline: one sibling at 5s costs the page its deadline", func(t *testing.T) { |
| + var slow, f1, f2 int64 |
| + sheets := answering(t, "Sheets", 5*time.Second, suite.VISIBLE, &slow) |
| + docs := answering(t, "Docs", 10*time.Millisecond, suite.VISIBLE, &f1) |
| + cal := answering(t, "Calendar", 10*time.Millisecond, suite.VISIBLE, &f2) |
| cands := []Candidate{ |
| {sheets, suite.ItemRevision{ItemID: "s1", Revision: 1}}, |
| {docs, suite.ItemRevision{ItemID: "d1", Revision: 1}}, |
| + {cal, suite.ItemRevision{ItemID: "c1", Revision: 1}}, |
| } |
| start := time.Now() |
| - got, notices := newTestClient().Rule(context.Background(), askerForTest(), cands) |
| + got := newTestClient().Rule(context.Background(), askerForTest(), cands) |
| elapsed := time.Since(start) |
| - if elapsed > 1500*time.Millisecond { |
| - t.Errorf("a sibling sleeping 3s cost the page %v: the deadline is "+ |
| - "not being applied, or not per call", elapsed) |
| + if elapsed > 1200*time.Millisecond { |
| + t.Errorf("a sibling sleeping 5s cost the page %v, over the spec's "+ |
| + "1.2s: the deadline is not applied, or not per call", elapsed) |
| } |
| if got[0].Renders() { |
| t.Error("the passage from the sibling that never answered rendered") |
| @@ -149,24 +160,22 @@ func TestOnePageIsOneRoundOfCalls(t *testing.T) { |
| t.Error("a sibling past its deadline was silent: §6.5 makes that " + |
| "UNKNOWN, which withholds loudly") |
| } |
| - if !got[1].Renders() { |
| - t.Error("a fast sibling's passage was withheld because a different " + |
| - "sibling was slow — partial pages are the normal case, not an " + |
| - "error path") |
| - } |
| - want := "Sheets did not answer; 1 result withheld." |
| - if lines := notices.Lines(); len(lines) != 1 || lines[0] != want { |
| - t.Errorf("notices = %v, want [%q]", lines, want) |
| + // Partial pages are the normal case, not an error path. |
| + for _, i := range []int{1, 2} { |
| + if !got[i].Renders() { |
| + t.Errorf("candidate %d was withheld because a DIFFERENT sibling "+ |
| + "was slow", i) |
| + } |
| } |
| }) |
| - t.Run("the page waits for a sibling inside the deadline", func(t *testing.T) { |
| - // Without this, a Rule that never waits at all passes both bounds |
| - // above: it would answer in 0ms and every timing assertion is an |
| - // upper bound. This one is a lower bound on the answer, not on time. |
| + t.Run("a sibling inside the deadline is waited for", func(t *testing.T) { |
| + // Without this, a Rule that never waits passes both bounds above: |
| + // it answers in 0ms and every timing assertion is an upper bound. |
| + // This one is a lower bound on the answer rather than on time. |
| var n int64 |
| sheets := answering(t, "Sheets", 300*time.Millisecond, suite.VISIBLE, &n) |
| - got, _ := newTestClient().Rule(context.Background(), askerForTest(), |
| + got := newTestClient().Rule(context.Background(), askerForTest(), |
| []Candidate{{sheets, suite.ItemRevision{ItemID: "s1", Revision: 1}}}) |
| if !got[0].Renders() { |
| t.Error("a sibling answering in 300ms, well inside an 800ms " + |
| @@ -178,6 +187,72 @@ func TestOnePageIsOneRoundOfCalls(t *testing.T) { |
| }) |
| } |
| +// **Two revisions of one item are two questions.** |
| +// |
| +// The dedupe key is the whole `(item_id, revision)` pair and not the id, |
| +// and nothing proved it: a review keyed the dedupe on ItemID alone and it |
| +// stayed green, because no fixture held two revisions of one document. |
| +// |
| +// What that mutation does in production is the leak §6.1 says STALE exists |
| +// to prevent. memoria holds revision 4 and revision 5 of the same |
| +// document; the sibling answers STALE for 4 and VISIBLE for 5; keyed on |
| +// the id, whichever pair is later in the batch wins for both, and the |
| +// withdrawn revision-4 passage renders on revision 5's permission — |
| +// retraction-by-edit, undone. |
| +// |
| +// MUTATION-TESTED 2026-09-06, each verified red then reverted: |
| +// |
| +// dedupe on c.Item.ItemID |
| +// -> one pair on the wire; red |
| +// scatter applies an answer to every position sharing an ItemID |
| +// -> rev 4 renders; red |
| +func TestTwoRevisionsOfOneItemAreTwoQuestions(t *testing.T) { |
| + var asked []suite.ItemRevision |
| + s := serve(t, func(w http.ResponseWriter, r *http.Request) { |
| + var req suite.S2Request |
| + if err := json.NewDecoder(r.Body).Decode(&req); err != nil { |
| + t.Errorf("undecodable S2 request: %v", err) |
| + return |
| + } |
| + rows := make([]map[string]any, 0, len(req.Pairs)) |
| + for _, p := range req.Pairs { |
| + asked = append(asked, p) |
| + // The sibling has moved past revision 4 and stands behind 5. |
| + result := string(suite.VISIBLE) |
| + if p.Revision == 4 { |
| + result = string(suite.STALE) |
| + } |
| + rows = append(rows, map[string]any{ |
| + "item_id": p.ItemID, "revision": p.Revision, "result": result, |
| + }) |
| + } |
| + raw, err := json.Marshal(map[string]any{"results": rows}) |
| + if err != nil { |
| + t.Fatalf("marshal: %v", err) |
| + } |
| + _, _ = w.Write(raw) |
| + }) |
| + s.Name = "Sheets" |
| + |
| + got := newTestClient().Rule(context.Background(), askerForTest(), []Candidate{ |
| + {s, suite.ItemRevision{ItemID: "doc-a", Revision: 4}}, |
| + {s, suite.ItemRevision{ItemID: "doc-a", Revision: 5}}, |
| + }) |
| + |
| + if len(asked) != 2 { |
| + t.Fatalf("%d pairs went on the wire for two revisions of one item, "+ |
| + "want 2 — one question cannot carry two answers", len(asked)) |
| + } |
| + if got[0] != WithholdAndRefetch { |
| + t.Errorf("revision 4 = %v, want WithholdAndRefetch: a passage the "+ |
| + "sibling has moved past must not render on the permission it "+ |
| + "gave a later revision", got[0]) |
| + } |
| + if got[1] != Render { |
| + t.Errorf("revision 5 = %v, want Render", got[1]) |
| + } |
| +} |
| + |
| // **A document's ten chunks ask about it once.** |
| // |
| // Every chunk of one document carries the same `(item_id, revision)`, so |
| @@ -217,7 +292,7 @@ func TestOneDocumentIsAskedAboutOnce(t *testing.T) { |
| // One other document, so "send exactly one pair always" does not pass. |
| cands = append(cands, Candidate{s, suite.ItemRevision{ItemID: "doc-b", Revision: 1}}) |
| - got, _ := newTestClient().Rule(context.Background(), askerForTest(), cands) |
| + got := newTestClient().Rule(context.Background(), askerForTest(), cands) |
| if pairsSeen != 2 { |
| t.Errorf("%d pairs went on the wire for 11 chunks of 2 documents, want 2 — "+ |
| @@ -266,6 +341,14 @@ func TestASiblingOverTheBatchCapIsSplitNotTruncated(t *testing.T) { |
| t.Errorf("a batch of %d exceeds the cap of %d", len(req.Pairs), suite.MaxS2Pairs) |
| } |
| atomic.AddInt64(&seen, int64(len(req.Pairs))) |
| + // Each batch is slow enough that three of them in a row would be |
| + // visible against the bound below, and fast enough that three at |
| + // once are not. |
| + select { |
| + case <-time.After(300 * time.Millisecond): |
| + case <-r.Context().Done(): |
| + return |
| + } |
| _, _ = w.Write(answerBody(req.Pairs, suite.VISIBLE)) |
| }) |
| s.Name = "Sheets" |
| @@ -278,11 +361,24 @@ func TestASiblingOverTheBatchCapIsSplitNotTruncated(t *testing.T) { |
| }} |
| } |
| - got, notices := newTestClient().Rule(context.Background(), askerForTest(), cands) |
| + start := time.Now() |
| + got := newTestClient().Rule(context.Background(), askerForTest(), cands) |
| + elapsed := time.Since(start) |
| if calls != 3 { |
| t.Errorf("%d calls for %d pairs at a cap of %d, want 3", calls, n, suite.MaxS2Pairs) |
| } |
| + // **The batches are concurrent, and this is what says so.** The claim |
| + // was in page.go's comment with nothing holding it: a review made one |
| + // goroutine per sibling with its batches sequential inside, and every |
| + // assertion here stayed green because there was no timing at all. The |
| + // bound is the spec's concurrency number for the same reason gate 23 |
| + // uses it — three 300ms calls concurrently is ~300ms, sequentially |
| + // ~900ms, and 810ms separates them at both ends. |
| + if elapsed > 810*time.Millisecond { |
| + t.Errorf("three 300ms batches took %v: a page whose cost grows with "+ |
| + "the number of batches grows with the corpus", elapsed) |
| + } |
| if seen != n { |
| t.Errorf("%d pairs reached the sibling, want %d: a batch was truncated, "+ |
| "and the pairs that fell off the end withhold while looking like "+ |
| @@ -293,26 +389,15 @@ func TestASiblingOverTheBatchCapIsSplitNotTruncated(t *testing.T) { |
| t.Fatalf("candidate %d of %d = %v, want Render", i, n, d) |
| } |
| } |
| - if lines := notices.Lines(); len(lines) != 0 { |
| - t.Errorf("notices on a fully-answered page: %v", lines) |
| - } |
| } |
| // Totality at the page boundary: no candidate ever goes unanswered, and |
| // nothing about an empty page is special-cased into a nil the caller then |
| // ranges over expecting content. |
| func TestRuleAnswersEveryCandidate(t *testing.T) { |
| - got, notices := newTestClient().Rule(context.Background(), askerForTest(), nil) |
| - if len(got) != 0 { |
| + if got := newTestClient().Rule(context.Background(), askerForTest(), nil); len(got) != 0 { |
| t.Errorf("an empty page produced %d dispositions", len(got)) |
| } |
| - if notices == nil { |
| - t.Fatal("an empty page produced a nil Outcome, which the page will " + |
| - "call Lines() on") |
| - } |
| - if lines := notices.Lines(); len(lines) != 0 { |
| - t.Errorf("an empty page carried notices: %v", lines) |
| - } |
| t.Run("an unreachable sibling still answers for every candidate", func(t *testing.T) { |
| dead := Sibling{Name: "Sheets", BaseURL: "http://127.0.0.1:1", Token: "t"} |
| @@ -320,7 +405,7 @@ func TestRuleAnswersEveryCandidate(t *testing.T) { |
| {dead, suite.ItemRevision{ItemID: "a", Revision: 1}}, |
| {dead, suite.ItemRevision{ItemID: "b", Revision: 1}}, |
| } |
| - got, notices := newTestClient().Rule(context.Background(), askerForTest(), cands) |
| + got := newTestClient().Rule(context.Background(), askerForTest(), cands) |
| if len(got) != 2 { |
| t.Fatalf("%d dispositions for 2 candidates", len(got)) |
| } |
| @@ -330,10 +415,6 @@ func TestRuleAnswersEveryCandidate(t *testing.T) { |
| "withheld loudly", i, d) |
| } |
| } |
| - want := "Sheets did not answer; 2 results withheld." |
| - if lines := notices.Lines(); len(lines) != 1 || lines[0] != want { |
| - t.Errorf("notices = %v, want [%q]", lines, want) |
| - } |
| }) |
| } |
diff --git a/internal/memoria/sibling/policy.go b/internal/memoria/sibling/policy.go| index 0f0066b..2a1a924 100644 |
| --- a/internal/memoria/sibling/policy.go |
| +++ b/internal/memoria/sibling/policy.go |
| @@ -51,6 +51,15 @@ const ( |
| // is recorded rather than hidden — see aheadNotYetOnTheWire below, which |
| // is the tripwire that fails the day it becomes reachable. |
| WithholdAndReannounce |
| + |
| + // numDispositions is one past the last, so a test can iterate the |
| + // whole set instead of a list of the ones it remembered. |
| + // |
| + // It exists because a test claimed to assert something "about the |
| + // whole set" while ranging a literal of five values: adding a sixth |
| + // constant left it green and the new value silent. A set a test |
| + // enumerates by hand is a set that stops being enumerated. |
| + numDispositions |
| ) |
| // Renders reports whether the passage's text may be spoken. |
| @@ -60,12 +69,21 @@ func (d Disposition) Renders() bool { return d == Render } |
| // |
| // Not the negation of Renders: `OmitSilently` neither renders nor notices, |
| // and it is the only disposition that does neither. |
| +// |
| +// **Written as the two exceptions rather than as a list of the loud |
| +// values, and that is the whole point.** The list version enumerated |
| +// Withhold, WithholdAndRefetch and WithholdAndReannounce, so a sixth |
| +// constant added to the block below was born silent — it rendered |
| +// nothing and said nothing, a second value permitted to hide a passage |
| +// without admitting to it. A review found it by appending one constant |
| +// and touching nothing else, which is precisely the edit |
| +// TestAheadIsStillAShippingLag instructs the next person to make when the |
| +// module ships AHEAD. |
| +// |
| +// Written this way, a new disposition is loud until somebody deliberately |
| +// makes it quiet. That is the direction the default should point. |
| func (d Disposition) Notices() bool { |
| - switch d { |
| - case Withhold, WithholdAndRefetch, WithholdAndReannounce: |
| - return true |
| - } |
| - return false |
| + return d != Render && d != OmitSilently |
| } |
| // Decide maps one S2 answer to what memoria does about it. §4.1's table. |
| @@ -121,11 +139,20 @@ const aheadNotYetOnTheWire = suite.Authorization("AHEAD") |
| // or a metric label — which is the same argument gate 4 makes about |
| // content in logs, made structurally instead of by grep. |
| // |
| -// Counts are carried for the two dispositions §8.1's copy counts, and |
| -// **not** for reconciling: §4.1's post-restore rule 1 requires one line for |
| -// the source rather than one per withheld passage, because a source being |
| -// reconciled leaves the ranking wholesale and a per-passage count would |
| -// describe a page that was never built. |
| +// Counts are carried for the withholding that names a number and **not** |
| +// for reconciling. |
| +// |
| +// §8.1 gives counted copy for one value — *"Sheets did not answer; 7 |
| +// results withheld."* — and this said "the two dispositions §8.1's copy |
| +// counts", which §8.1 does not support: there is no STALE line there. The |
| +// refetch line's count is this file's own choice, made for the same reason |
| +// and said plainly here rather than borrowed from a section that does not |
| +// say it. |
| +// |
| +// Reconciling carries no count because §4.1's post-restore rule 1 requires |
| +// one line for the source rather than one per withheld passage: a source |
| +// being reconciled leaves the ranking wholesale, so the number of passages |
| +// it would have contributed describes a page that was never built. |
| type Outcome struct { |
| counts map[noticeKey]int |
| order []noticeKey |
| @@ -152,8 +179,15 @@ func (o *Outcome) Record(app string, d Disposition) { |
| o.counts[k]++ |
| } |
| -// Lines renders the page's withholding notices, in the order the sources |
| -// were first heard from, so a page is stable across reloads. |
| +// Lines renders the page's withholding notices in the order they were |
| +// recorded, so a page is stable across reloads. |
| +// |
| +// That is **candidate order, not arrival order** — the caller records |
| +// after the fan-in, and the ordering the reader gets is the ordering of |
| +// the results the lines are about. This comment said "the order the |
| +// sources were first heard from", which describes ordering by which |
| +// sibling answered first: the one thing a concurrent fan-out must not let |
| +// the page depend on. |
| func (o *Outcome) Lines() []string { |
| out := make([]string, 0, len(o.order)) |
| for _, k := range o.order { |
diff --git a/internal/memoria/sibling/policy_test.go b/internal/memoria/sibling/policy_test.go| index e4ebbd5..bd95a8d 100644 |
| --- a/internal/memoria/sibling/policy_test.go |
| +++ b/internal/memoria/sibling/policy_test.go |
| @@ -30,6 +30,19 @@ import ( |
| // the default arm -> OmitSilently -> red on PERHAPS |
| // Notices() returns true for OmitSilently -> red |
| // Renders() returns true for Withhold -> red |
| +// |
| +// And the one a review found, which needed two edits at once and so was |
| +// invisible to every single-edit mutation: |
| +// |
| +// Notices() written as `switch { case <the loud three>: true }` |
| +// PLUS a sixth constant added to the block |
| +// -> red on the quiet-count assertion. Either edit alone is |
| +// harmless: the switch form agrees with the current form for the |
| +// five values that exist, and a sixth constant under the current |
| +// form is loud by default. Together they produce a disposition |
| +// that neither renders nor notices — a second value permitted to |
| +// hide a passage without admitting to it — and the AHEAD tripwire |
| +// instructs the next person to make exactly one of the two. |
| func TestEveryS2ResultIsHandled(t *testing.T) { |
| for _, c := range []struct { |
| result suite.Authorization |
| @@ -89,12 +102,15 @@ func TestTheZeroDispositionWithholds(t *testing.T) { |
| } |
| // And exactly one disposition is quiet without rendering, because "the |
| - // only value that may be silent" is a claim about the whole set, not |
| - // about NOT_VISIBLE alone. |
| + // only value that may be silent" is a claim about the whole set. |
| + // |
| + // **Iterated to numDispositions rather than over a literal**, because |
| + // the literal version made that claim about the five values its author |
| + // remembered: a reviewer appended a sixth constant, touched nothing |
| + // else, and got a disposition that neither rendered nor noticed while |
| + // this test stayed green. The set has to enumerate itself. |
| quiet := 0 |
| - for _, d := range []Disposition{ |
| - Withhold, Render, OmitSilently, WithholdAndRefetch, WithholdAndReannounce, |
| - } { |
| + for d := Disposition(0); d < numDispositions; d++ { |
| if !d.Renders() && !d.Notices() { |
| quiet++ |
| if d != OmitSilently { |
diff --git a/internal/memoria/sibling/sibling.go b/internal/memoria/sibling/sibling.go| index 652cf5a..cbf6451 100644 |
| --- a/internal/memoria/sibling/sibling.go |
| +++ b/internal/memoria/sibling/sibling.go |
| @@ -19,7 +19,6 @@ import ( |
| "bytes" |
| "context" |
| "encoding/json" |
| - "fmt" |
| "io" |
| "net/http" |
| "time" |
| @@ -158,35 +157,18 @@ func allUnknown(n int) []suite.Authorization { |
| // §6.5 and §8.1: withholding is loud and names the shape of the loss, and |
| // only NOT_VISIBLE is silent. A count of hidden results with no cause is |
| // the notice a reader learns to ignore. |
| -type Withheld struct { |
| - Sibling string |
| - Count int |
| - Reason suite.Authorization |
| -} |
| - |
| -func (w Withheld) String() string { |
| - switch w.Reason { |
| - case suite.UNKNOWN: |
| - return fmt.Sprintf("%s did not answer; %d results withheld.", w.Sibling, w.Count) |
| - case suite.STALE: |
| - return fmt.Sprintf("%s has newer versions of %d results; refetching.", w.Sibling, w.Count) |
| - case suite.NOT_VISIBLE: |
| - // The ONE value that may be silent (§4.1, §8.1). A count here would |
| - // tell a reader how many things they are not allowed to see, which |
| - // is the number the whole four-value design exists to not say. The |
| - // default branch used to render it as "Sheets: 7 results withheld." |
| - return "" |
| - default: |
| - return fmt.Sprintf("%s: %d results withheld.", w.Sibling, w.Count) |
| - } |
| -} |
| - |
| +// |
| +// **The type that used to live here is gone.** `Withheld` rendered these |
| +// notices too, in different words — "has newer versions of 7 results" |
| +// against Outcome's "has moved on from 7 results" — so the page's copy |
| +// depended on which of two renderers a caller happened to reach. A review |
| +// found the pair. Outcome (policy.go) is the one renderer: it is the one |
| +// the page uses, it groups per (app, kind) rather than being constructed |
| +// per notice, and it is where the silence of NOT_VISIBLE is enforced by |
| +// never recording it rather than by rendering it as "". |
| +// |
| // Not built, and recorded here rather than discovered later: |
| // |
| // - §8.1's durable per-(sibling, asker, day) VISIBLE/NOT_VISIBLE counters. |
| // The spec says they are written "on the path that already made the |
| // call" — this is that path, and Authorize is where they belong. |
| -// - §6.5's fan-out. A page is ONE round of concurrent calls with the |
| -// floor at the slowest sibling; this client asks one sibling at a time |
| -// and nothing here yet makes a page's cost the max rather than the sum. |
| -// Gate 23 holds both halves together and is not written. |
diff --git a/internal/memoria/sibling/sibling_test.go b/internal/memoria/sibling/sibling_test.go| index a2a63aa..427fca7 100644 |
| --- a/internal/memoria/sibling/sibling_test.go |
| +++ b/internal/memoria/sibling/sibling_test.go |
| @@ -265,26 +265,42 @@ func TestEveryPairGetsExactlyOneAnswer(t *testing.T) { |
| // notice that says only "some results hidden" is one a reader stops |
| // reading — and a reader who stops reading it is in the same position as |
| // one who was told nothing (§6.5). |
| -func TestWithheldNamesTheSiblingAndTheReason(t *testing.T) { |
| +// |
| +// Asserted on `Outcome`, which is now the only renderer of these lines. |
| +// It used to be asserted on a second type, `Withheld`, which rendered the |
| +// same notices in different words — so what the page said depended on |
| +// which renderer a caller reached. A review found the pair; the type is |
| +// gone and its claim is here. |
| +func TestWithholdingNamesTheSiblingAndTheReason(t *testing.T) { |
| for _, c := range []struct { |
| reason suite.Authorization |
| want string |
| }{ |
| {suite.UNKNOWN, "Sheets did not answer; 7 results withheld."}, |
| - {suite.STALE, "Sheets has newer versions of 7 results; refetching."}, |
| - // NOT_VISIBLE is the one value that may be silent, so it renders |
| - // nothing at all. It used to fall through to the default and say |
| - // "Sheets: 7 results withheld." — a count of what a reader is not |
| - // allowed to see, which is the number this vocabulary exists to |
| - // avoid saying. |
| + {suite.STALE, "Sheets has moved on from 7 results; refetching."}, |
| + // NOT_VISIBLE is the one value that may be silent, so it produces |
| + // no line at all — and now by never being recorded rather than by |
| + // rendering as "". A count here would tell a reader how many |
| + // things they are not allowed to see, which is the number this |
| + // whole vocabulary exists to avoid saying. |
| {suite.NOT_VISIBLE, ""}, |
| } { |
| - got := Withheld{Sibling: "Sheets", Count: 7, Reason: c.reason}.String() |
| - if got != c.want { |
| - t.Errorf("Withheld(%s) = %q, want %q", c.reason, got, c.want) |
| + var o Outcome |
| + for i := 0; i < 7; i++ { |
| + o.Record("Sheets", Decide(c.reason)) |
| } |
| - if c.want != "" && !strings.Contains(got, "Sheets") { |
| - t.Errorf("withholding notice does not name the app: %q", got) |
| + lines := o.Lines() |
| + switch { |
| + case c.want == "": |
| + if len(lines) != 0 { |
| + t.Errorf("%s produced %v, want silence", c.reason, lines) |
| + } |
| + case len(lines) != 1 || lines[0] != c.want: |
| + t.Errorf("%s produced %v, want [%q]", c.reason, lines, c.want) |
| + default: |
| + if !strings.Contains(lines[0], "Sheets") { |
| + t.Errorf("withholding notice does not name the app: %q", lines[0]) |
| + } |
| } |
| } |
| } |
diff --git a/internal/memoria/store.go b/internal/memoria/store.go| index b8aa526..c7d7d1f 100644 |
| --- a/internal/memoria/store.go |
| +++ b/internal/memoria/store.go |
| @@ -672,7 +672,7 @@ func (s *Store) rule(ctx context.Context, asker suite.Subject, all []Result) Pag |
| } |
| if len(cands) > 0 { |
| - got, _ := s.custody.Rule(ctx, asker, cands) |
| + got := s.custody.Rule(ctx, asker, cands) |
| for j, d := range got { |
| disp[at[j]] = d |
| } |