Review 1 on the provider: errors, proxies, ports, and a stricter parse
Reviewed by Codex GPT-6-Astra (dispatched: codex/gpt-6-astra; self-
reported "Codex / GPT-6, exact variant not exposed"). REQUEST CHANGES.
Spec draft 2: seven of Astra's eight earlier findings FIXED, one
PARTIAL (the provider must be named, not only the model — next commit).
The code findings were serious, and each was reproduced before fixing.
**Errors carried provider-controlled bytes.** The transport and read
paths wrapped Go's errors, and url.Error quotes the URL — which after a
redirect is whatever Location the provider sent. A 307 to
"/<canary>" came back in the error; so did a malformed header and a
chunked trailer. My comment said the URL was a constant; after a
redirect it is not. Every such failure is now a fixed sentinel with the
original DISCARDED, not wrapped; context errors stay recognisable.
**The pin let environment proxies through, and ignored the port.**
http.DefaultTransport honours HTTPS_PROXY: a listener set there received
"CONNECT api.tensorx.ai:443" while the host pin passed, because the pin
checks the URL and the URL was still TensorX's. That is decision 13's
objection to a configurable base URL arriving through the environment.
The client now owns a transport with proxies off and TLS verification
default. The pin is host AND port, with the scheme's default made
explicit; another listener on 127.0.0.1 took the body before, and
":8443" in BaseURL left every test green. Userinfo is refused.
**The strict parse was not strict.** Two claims verified before acting,
since both turn on encoding/json internals: the decoder folds "ſ" to
"s" (EqualFold) while my duplicate check used ToLower, so
{"passage":0,"paſſage":1} decoded as passage 1; and dec.More() is not
an end-of-input check, so {"addresses":[]}} was accepted. Duplicates
are now detected with EqualFold, end of input is io.EOF, the model's
content accepts only the exact schema keys, and the ENVELOPE is checked
for duplicates too — "finish_reason":"length","finish_reason":"stop"
decoded as stop and defeated the truncation refusal.
**Fourteen of the review's surviving mutations are now red**, and two of
my own fixes turned out unreachable: through ExtractAddresses the exact-
key check pre-empts the fold-aware duplicate check, and decodeAddresses
has its own EOF check. Both matter for the envelope, so rejectAmbiguous
is now tested directly (AGENTS.md: make it reachable or delete it). The
error test is now an exact whitelist built from the sentinels — the
canary search passed with eight bytes of content leaked. The deadline
is asserted against a literal 800 ms; cancellation is observed at the
listener; the happy path asserts POST, the path and the instructions.
**fact**: "e-mail address" primed [email, address] — an email query
sending passages to a model — because the two-token "e mail" matched
first and left "address" behind. The model-kind allowlist is pinned to
exactly [address], since adding one is Paul's to rule. RecognisedSpans,
LabelAt and WordBounded each gained the fixture a review's mutation got
past.
**And the timing test on main was flaking.** It timed every small run,
then every large one, so load arriving mid-test landed on one side of
the ratio. Now interleaved: pristine 1.90-2.01 idle and at most 2.56
with four CPUs pinned busy; the two quadratic mutants 3.54 and 3.89 at
their lowest. Three separates them with half a point either side.
Config asserts the loaded credential's value, and AI-on without one
fails at boot through provider.New rather than as a 401 on first search.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
5 files changed,
+435
−79
internal/memoria/fact/fact.go+5 −0internal/memoria/fact/fact_test.go+82 −29internal/memoria/model_config_test.go+12 −0internal/memoria/provider/provider.go+131 −33internal/memoria/provider/provider_test.go+205 −17
diff --git a/internal/memoria/fact/fact.go b/internal/memoria/fact/fact.go| index 4b2ae80..bddf462 100644 |
| --- a/internal/memoria/fact/fact.go |
| +++ b/internal/memoria/fact/fact.go |
| @@ -117,6 +117,11 @@ type phrase struct { |
| // app" primes Phone, finds no phone number, and renders nothing. A |
| // generous lexicon costs a box that does not appear. |
| var lexicon = []phrase{ |
| + // Three tokens, and it must come first: "e-mail address" splits to |
| + // e / mail / address, and without this the two-token "e mail" took |
| + // the first two and left "address" to prime the POSTAL kind — an |
| + // email query sending passages to a model. Found by review. |
| + {[]string{"e", "mail", "address"}, Email}, |
| {[]string{"email", "address"}, Email}, |
| {[]string{"postal", "address"}, Address}, |
| {[]string{"phone", "number"}, Phone}, |
diff --git a/internal/memoria/fact/fact_test.go b/internal/memoria/fact/fact_test.go| index c36351e..896ea82 100644 |
| --- a/internal/memoria/fact/fact_test.go |
| +++ b/internal/memoria/fact/fact_test.go |
| @@ -39,8 +39,13 @@ func TestWantedNamesTheKind(t *testing.T) { |
| {"address", []Kind{Address}}, |
| {"office address", []Kind{Address}}, |
| {"postal address", []Kind{Address}}, |
| - // The collision the phrase-first lexicon exists to prevent. |
| + // The collision the phrase-first lexicon exists to prevent — in |
| + // both spellings. "e-mail address" splits to e / mail / address, |
| + // and before the three-token phrase existed it primed [email, |
| + // address]: an email query sending passages to a model. |
| {"email address", []Kind{Email}}, |
| + {"e-mail address", []Kind{Email}}, |
| + {"what is the e-mail address", []Kind{Email}}, |
| {"phone number and postal address", []Kind{Phone, Address}}, |
| } { |
| if got := Wanted(c.q); !reflect.DeepEqual(got, c.want) { |
| @@ -436,44 +441,53 @@ func TestALabelDoesNotDependOnWhichKindsWereAskedFor(t *testing.T) { |
| // 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. |
| -// |
| -// 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. |
| +// when the input doubles; linear work doubles. The minimum of several |
| +// runs is the robust estimator for CPU-bound work: a loaded machine makes |
| +// runs slower, not faster, so the minimum discards the noise. |
| func TestLabellingIsNotQuadraticInAChunksFacts(t *testing.T) { |
| - scan := func(n int) (time.Duration, int) { |
| + // **Interleaved, and it flaked before it was.** The second version |
| + // timed every small run and then every large one, so load arriving |
| + // mid-test landed on one side of the ratio: a reviewer running |
| + // mutations in parallel saw it fail intermittently on correct code. |
| + // Alternating small and large makes contention hit both, and the |
| + // minimum of each discards it. Measured on this fixture, 2026-09-27, |
| + // three trials each: pristine 1.90-2.01 idle and 1.50-2.56 with four |
| + // CPUs pinned busy; the linear scan 3.54-3.63; the unbounded lookback |
| + // 3.89-3.93. Three separates them with about half a point each side. |
| + const small, large, rounds = 15000, 30000, 5 |
| + build := func(n int) string { |
| 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 b.String() |
| + } |
| + a, b := build(small), build(large) |
| + bestA, bestB := time.Hour, time.Hour |
| + var foundA, foundB int |
| + for r := 0; r < rounds; r++ { |
| + st := time.Now() |
| + foundA = len(Scan(a, "H", "T", []Kind{Eircode})) |
| + if d := time.Since(st); d < bestA { |
| + bestA = d |
| + } |
| + st = time.Now() |
| + foundB = len(Scan(b, "H", "T", []Kind{Eircode})) |
| + if d := time.Since(st); d < bestB { |
| + bestB = 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 foundA != small || foundB != large { |
| + t.Fatalf("found %d and %d candidates, want %d and %d distinct", foundA, foundB, small, large) |
| } |
| - if ratio := float64(large) / float64(small); ratio > 3 { |
| + if ratio := float64(bestB) / float64(bestA); 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) |
| + ratio, bestA, bestB) |
| } |
| } |
| @@ -560,6 +574,14 @@ func TestNoModelKindHasARecogniser(t *testing.T) { |
| t.Errorf("%s is both a model kind and has a recogniser; §8.9 forbids sending a validated kind to a model", k) |
| } |
| } |
| + // **The allowlist is pinned exactly.** Which metadata is worth a |
| + // model call is Paul's to rule (§13), so adding a kind — even one with |
| + // no recogniser, which the check above would let through — has to be |
| + // a deliberate edit here, not a one-line change in fact.go that no |
| + // test notices. A review added `url` and stayed green. |
| + if len(modelKinds) != 1 || !modelKinds[Address] { |
| + t.Errorf("modelKinds = %v; §8.9 rules in exactly [address]", modelKinds) |
| + } |
| // And the presence half: the allowlist is not empty, and every |
| // validator-backed kind answers false. |
| if !ModelKind(Address) { |
| @@ -576,20 +598,34 @@ func TestNoModelKindHasARecogniser(t *testing.T) { |
| // If it did, §8.8's box would show a postal address no checksum vouched |
| // for, without the 🤖 marking §8.9 requires of anything a model chose. |
| func TestScanNeverProducesAnAddress(t *testing.T) { |
| - text := "Office address: 12 Main Street, Dublin 2, D02 X285" |
| + text := "Office address: 12 Main Street, Dublin 2, D02 X285. Office phone: 01 547 7854" |
| if got := Scan(text, "Contact", "Tito", []Kind{Address}); len(got) != 0 { |
| t.Errorf("Scan produced %d address facts; only a model may", len(got)) |
| } |
| + // And in a MIXED request, where other kinds do produce facts: a |
| + // review fabricated an Address only when several kinds were asked for |
| + // and the single-kind case above stayed green. |
| + mixed := Scan(text, "Contact", "Tito", []Kind{Eircode, Phone, Address}) |
| + if len(mixed) == 0 { |
| + t.Fatal("the mixed scan found nothing; it proves nothing") |
| + } |
| + for _, f := range mixed { |
| + if f.Kind == Address { |
| + t.Errorf("a mixed scan produced an address: %+v", f) |
| + } |
| + } |
| } |
| func TestRecognisedSpansCoverEveryKind(t *testing.T) { |
| - text := "Mail info@tito.io or ring +353 1 547 7854, at D02 X285." |
| + // All five kinds, because a review dropped IBAN and VAT from the |
| + // function and the three-kind version of this fixture stayed green. |
| + text := "Mail info@tito.io or ring +353 1 547 7854, at D02 X285. VAT IE3384527RH, IBAN IE29AIBK93115212345678." |
| got := RecognisedSpans(text) |
| var spans []string |
| for _, sp := range got { |
| spans = append(spans, text[sp[0]:sp[1]]) |
| } |
| - want := []string{"info@tito.io", "+353 1 547 7854", "D02 X285"} |
| + want := []string{"info@tito.io", "+353 1 547 7854", "D02 X285", "IE3384527RH", "IE29AIBK93115212345678"} |
| if !reflect.DeepEqual(spans, want) { |
| t.Errorf("spans = %q, want %q (sorted by end, every kind)", spans, want) |
| } |
| @@ -598,14 +634,22 @@ func TestRecognisedSpansCoverEveryKind(t *testing.T) { |
| // LabelAt must agree with Scan for a value Scan found — that is the |
| // whole reason it is exported rather than reimplemented. |
| func TestLabelAtAgreesWithScan(t *testing.T) { |
| - text := officeChunk |
| + // Including an over-long label, so NoBox is compared too: with only |
| + // the office chunk, a LabelAt that always returned NoBox=false agreed |
| + // with Scan on every case and stayed green. |
| + text := officeChunk + " " + strings.Repeat("x", MaxLabel+1) + ": 01 555 0101" |
| + var sawNoBox bool |
| for _, f := range Scan(text, "Office Address", "Office", []Kind{Phone}) { |
| + sawNoBox = sawNoBox || f.NoBox |
| start := strings.Index(text, f.Span) |
| got, noBox := LabelAt(text, start, "Office Address", "Office") |
| if got != f.Label || noBox != f.NoBox { |
| t.Errorf("LabelAt(%q) = %q/%v, Scan said %q/%v", f.Span, got, noBox, f.Label, f.NoBox) |
| } |
| } |
| + if !sawNoBox { |
| + t.Fatal("no over-long label in the fixture; the NoBox half proves nothing") |
| + } |
| } |
| func TestWordBounded(t *testing.T) { |
| @@ -627,4 +671,13 @@ func TestWordBounded(t *testing.T) { |
| if WordBounded(text, 3, 3) { |
| t.Error("an empty range is not a bounded word") |
| } |
| + // Digits are word characters: "2 Main" inside "12 Main" is not a |
| + // bounded span. A review stopped treating digits as word runes and |
| + // every letter-only case above stayed green. |
| + if WordBounded("12 Main Street", 1, len("12 Main")) { |
| + t.Error("a span starting after a digit was treated as bounded") |
| + } |
| + if WordBounded("Unit 42", 0, len("Unit 4")) { |
| + t.Error("a span ending before a digit was treated as bounded") |
| + } |
| } |
diff --git a/internal/memoria/model_config_test.go b/internal/memoria/model_config_test.go| index 7153086..c169509 100644 |
| --- a/internal/memoria/model_config_test.go |
| +++ b/internal/memoria/model_config_test.go |
| @@ -60,6 +60,12 @@ func TestTheModelSwitch(t *testing.T) { |
| if cfg.AI != c.wantAI { |
| t.Errorf("AI = %v, want %v", cfg.AI, c.wantAI) |
| } |
| + // The credential that reaches the client is the one the owner |
| + // configured. A review replaced it with "wrong-key" in |
| + // LoadConfig and every case above stayed green. |
| + if c.wantAI && cfg.TensorXKey != c.key { |
| + t.Errorf("loaded a different credential from the one configured") |
| + } |
| }) |
| } |
| } |
| @@ -79,4 +85,10 @@ func TestWireModelOnlyWhenOn(t *testing.T) { |
| if _, ok := a.store.extractor.(*provider.Client); !ok { |
| t.Fatalf("AI on wired %T, want *provider.Client", a.store.extractor) |
| } |
| + // And AI on with no credential fails at boot, not on the first search: |
| + // provider.New refuses an empty key, and WireModel passes that up. |
| + b := &app{store: &Store{}, logger: slog.New(slog.NewTextHandler(io.Discard, nil))} |
| + if err := WireModel(b, Config{AI: true}); err == nil || b.store.extractor != nil { |
| + t.Errorf("wired a model with no credential: err=%v", err) |
| + } |
| } |
diff --git a/internal/memoria/provider/provider.go b/internal/memoria/provider/provider.go| index 53bbe2b..965c139 100644 |
| --- a/internal/memoria/provider/provider.go |
| +++ b/internal/memoria/provider/provider.go |
| @@ -65,6 +65,15 @@ var ( |
| // ErrForbiddenHost is a request to anywhere but the host the client |
| // was built for, or any redirect at all. |
| ErrForbiddenHost = errors.New("provider: refused a host or redirect other than the pinned one") |
| + // ErrTransport is every failure to send or to read, flattened. The |
| + // underlying error is DROPPED, not wrapped: a review found a redirect |
| + // error quoting the Location header, and malformed headers and |
| + // chunked trailers quoting themselves — all bytes the provider chose. |
| + ErrTransport = errors.New("provider: transport failure") |
| + // ErrMissingKey is a client built with no credential. Refused at |
| + // construction, so a wiring bug that loses the key fails at boot |
| + // rather than as a 401 on the first search. |
| + ErrMissingKey = errors.New("provider: no credential") |
| ) |
| // Pointer is one span a model says is an address: which sent passage, |
| @@ -86,6 +95,9 @@ type Client struct { |
| // New returns a client for TensorX. It takes a key and nothing else: |
| // there is no exported way to change where it dials. |
| func New(key string) (*Client, error) { |
| + if key == "" { |
| + return nil, ErrMissingKey |
| + } |
| return newClient(key, BaseURL) |
| } |
| @@ -103,14 +115,25 @@ func New(key string) (*Client, error) { |
| // asserts it against this constructor. |
| func newClient(key, base string) (*Client, error) { |
| probe, err := http.NewRequest(http.MethodPost, base, nil) |
| - if err != nil || probe.URL.Hostname() == "" { |
| + if err != nil || probe.URL.Hostname() == "" || probe.URL.User != nil { |
| return nil, ErrForbiddenHost |
| } |
| + // **Our own transport, with proxies switched off.** http.DefaultTransport |
| + // honours HTTPS_PROXY, and a review pointed it at a local listener and |
| + // watched that listener receive `CONNECT api.tensorx.ai:443` while the |
| + // host pin passed — because the pin checks the URL, and the URL was |
| + // still TensorX's. An ordinary CONNECT proxy sees only ciphertext; one |
| + // holding a trusted interception certificate sees every passage. Either |
| + // way it is a second party on the path, which is decision 13's whole |
| + // objection to a configurable base URL, arriving through the |
| + // environment instead. TLS verification stays the default. |
| + tr := http.DefaultTransport.(*http.Transport).Clone() |
| + tr.Proxy = nil |
| return &Client{ |
| base: base, |
| key: key, |
| hc: &http.Client{ |
| - Transport: &pinnedTransport{host: probe.URL.Hostname(), scheme: probe.URL.Scheme, next: http.DefaultTransport}, |
| + Transport: &pinnedTransport{scheme: probe.URL.Scheme, authority: authority(probe.URL.Scheme, probe.URL.Hostname(), probe.URL.Port()), next: tr}, |
| // Refuse every redirect. There is no legitimate one on this |
| // API, and following one hands the bearer key — and every |
| // passage in the body — to wherever it points. Go drops the |
| @@ -121,20 +144,43 @@ func newClient(key, base string) (*Client, error) { |
| }, nil |
| } |
| -// pinnedTransport refuses every host and scheme but the one it was built |
| -// for, on every request, before anything is dialled. |
| +// pinnedTransport refuses every scheme and authority — host AND port — |
| +// but the one it was built for, on every request, before anything is |
| +// dialled. |
| +// |
| +// **The port is part of it**, and the first version checked the host |
| +// alone: a review moved a test client to another listener on the same |
| +// hostname and it delivered the body there, and changing BaseURL to |
| +// `:8443` left every test green. A URL carrying userinfo is refused too; |
| +// nothing on this API needs it. |
| type pinnedTransport struct { |
| - host, scheme string |
| - next http.RoundTripper |
| + scheme, authority string |
| + next http.RoundTripper |
| } |
| func (p *pinnedTransport) RoundTrip(r *http.Request) (*http.Response, error) { |
| - if r.URL.Hostname() != p.host || r.URL.Scheme != p.scheme { |
| + if r.URL.Scheme != p.scheme || r.URL.User != nil || |
| + authority(r.URL.Scheme, r.URL.Hostname(), r.URL.Port()) != p.authority { |
| return nil, ErrForbiddenHost |
| } |
| return p.next.RoundTrip(r) |
| } |
| +// authority is host:port with the scheme's default port made explicit, |
| +// so "api.tensorx.ai" and "api.tensorx.ai:443" compare equal and |
| +// "api.tensorx.ai:8443" does not. |
| +func authority(scheme, host, port string) string { |
| + if port == "" { |
| + switch scheme { |
| + case "https": |
| + port = "443" |
| + case "http": |
| + port = "80" |
| + } |
| + } |
| + return strings.ToLower(host) + ":" + port |
| +} |
| + |
| // instructions are the whole of what memoria asks of the model. |
| // |
| // Extractive, exhaustive, exact. Every clause maps to a check the caller |
| @@ -181,7 +227,7 @@ func (c *Client) ExtractAddresses(ctx context.Context, passages []string) ([]Poi |
| } |
| user, err := json.Marshal(sent) |
| if err != nil { |
| - return nil, fmt.Errorf("provider: encode passages: %w", err) |
| + return nil, ErrMalformed |
| } |
| body, err := json.Marshal(chatRequest{ |
| Model: Model, |
| @@ -193,23 +239,19 @@ func (c *Client) ExtractAddresses(ctx context.Context, passages []string) ([]Poi |
| Temperature: 0, |
| }) |
| if err != nil { |
| - return nil, fmt.Errorf("provider: encode request: %w", err) |
| + return nil, ErrMalformed |
| } |
| req, err := http.NewRequestWithContext(ctx, http.MethodPost, c.base+"/chat/completions", bytes.NewReader(body)) |
| if err != nil { |
| - return nil, fmt.Errorf("provider: build request: %w", err) |
| + return nil, ErrForbiddenHost |
| } |
| req.Header.Set("Authorization", "Bearer "+c.key) |
| req.Header.Set("Content-Type", "application/json") |
| resp, err := c.hc.Do(req) |
| if err != nil { |
| - // A transport error can carry the URL; it never carries the body |
| - // or the key, and the URL is a constant. Context errors come |
| - // through here too, which is how a deadline or an abandoned |
| - // keystroke reaches the caller. |
| - return nil, fmt.Errorf("provider: request: %w", err) |
| + return nil, transportFailure(ctx, err) |
| } |
| defer resp.Body.Close() |
| @@ -225,7 +267,7 @@ func (c *Client) ExtractAddresses(ctx context.Context, passages []string) ([]Poi |
| // lesson). |
| raw, err := io.ReadAll(io.LimitReader(resp.Body, maxResponse+1)) |
| if err != nil { |
| - return nil, fmt.Errorf("provider: read response: %w", err) |
| + return nil, transportFailure(ctx, err) |
| } |
| if len(raw) > maxResponse { |
| return nil, ErrTooLarge |
| @@ -239,6 +281,13 @@ func (c *Client) ExtractAddresses(ctx context.Context, passages []string) ([]Poi |
| FinishReason string `json:"finish_reason"` |
| } `json:"choices"` |
| } |
| + // **The envelope is checked for duplicates too**, not only the model's |
| + // content. `"finish_reason":"length","finish_reason":"stop"` decodes as |
| + // "stop" — the last one wins — which would defeat the whole-response |
| + // truncation refusal below. Found by review. |
| + if err := rejectAmbiguous(raw, nil); err != nil { |
| + return nil, ErrMalformed |
| + } |
| if err := json.Unmarshal(raw, &envelope); err != nil || len(envelope.Choices) != 1 { |
| return nil, ErrMalformed |
| } |
| @@ -264,7 +313,7 @@ func (c *Client) ExtractAddresses(ctx context.Context, passages []string) ([]Poi |
| // pretending to be one. Every failure is ErrMalformed and carries none |
| // of the content — a json.SyntaxError would quote the byte it failed on. |
| func decodeAddresses(content string) ([]Pointer, error) { |
| - if err := rejectAmbiguous([]byte(content)); err != nil { |
| + if err := rejectAmbiguous([]byte(content), contentKeys); err != nil { |
| return nil, ErrMalformed |
| } |
| dec := json.NewDecoder(strings.NewReader(content)) |
| @@ -275,7 +324,7 @@ func decodeAddresses(content string) ([]Pointer, error) { |
| Text *string `json:"text"` |
| } `json:"addresses"` |
| } |
| - if err := dec.Decode(&out); err != nil || dec.More() { |
| + if err := dec.Decode(&out); err != nil || !atEOF(dec) { |
| return nil, ErrMalformed |
| } |
| if out.Addresses == nil { |
| @@ -293,23 +342,48 @@ func decodeAddresses(content string) ([]Pointer, error) { |
| return ptrs, nil |
| } |
| +// contentKeys are the only keys the model's content may use, spelled |
| +// exactly. encoding/json matches keys case-insensitively AND by Unicode |
| +// folding, so "PASSAGE" and "paſſage" (long s) both decode as passage; |
| +// accepting only the exact spelling closes that without having to |
| +// reproduce the decoder's folding rules. |
| +var contentKeys = map[string]bool{"addresses": true, "passage": true, "text": true} |
| + |
| // rejectAmbiguous refuses JSON whose meaning depends on which duplicate |
| -// key wins. encoding/json keeps the last duplicate and matches keys |
| -// case-insensitively, so `{"addresses":[…],"ADDRESSES":[…]}` decodes to |
| -// whichever came second. Mirrors oficina/suite's rejectAmbiguousJSON, |
| -// which is unexported; kept deliberately small. |
| -func rejectAmbiguous(data []byte) error { |
| +// key wins, or that is not exactly one complete document. |
| +// |
| +// encoding/json keeps the last of two duplicate keys, and matches keys by |
| +// Unicode case folding — so `{"passage":0,"paſſage":1}` decodes to |
| +// passage 1. The first version of this folded keys with strings.ToLower, |
| +// which does NOT equate "ſ" with "s", and a review walked that pair |
| +// straight through. Duplicates are now detected with strings.EqualFold, |
| +// which uses the same simple folding the decoder does. |
| +// |
| +// exact, when non-nil, is the complete set of permitted keys, compared |
| +// byte for byte — used for the model's content, whose schema is ours. |
| +// The provider's envelope carries fields we do not enumerate, so it gets |
| +// the duplicate check alone. |
| +func rejectAmbiguous(data []byte, exact map[string]bool) error { |
| dec := json.NewDecoder(bytes.NewReader(data)) |
| - if err := scanUnique(dec); err != nil { |
| + if err := scanUnique(dec, exact); err != nil { |
| return err |
| } |
| - if dec.More() { |
| + // **dec.More() is not an end-of-input check**, and the first version |
| + // used it: `{"addresses":[]}}` has More() == false and was accepted. |
| + // The only proof there is nothing after the document is io.EOF. |
| + if !atEOF(dec) { |
| return errors.New("trailing content") |
| } |
| return nil |
| } |
| -func scanUnique(dec *json.Decoder) error { |
| +// atEOF reports whether the decoder has nothing left but whitespace. |
| +func atEOF(dec *json.Decoder) bool { |
| + _, err := dec.Token() |
| + return err == io.EOF |
| +} |
| + |
| +func scanUnique(dec *json.Decoder, exact map[string]bool) error { |
| tok, err := dec.Token() |
| if err != nil { |
| return err |
| @@ -320,7 +394,7 @@ func scanUnique(dec *json.Decoder) error { |
| } |
| switch delim { |
| case '{': |
| - seen := map[string]bool{} |
| + var seen []string |
| for dec.More() { |
| k, err := dec.Token() |
| if err != nil { |
| @@ -330,18 +404,22 @@ func scanUnique(dec *json.Decoder) error { |
| if !ok { |
| return errors.New("non-string key") |
| } |
| - folded := strings.ToLower(key) |
| - if seen[folded] { |
| - return errors.New("duplicate key") |
| + if exact != nil && !exact[key] { |
| + return errors.New("unexpected key") |
| + } |
| + for _, s := range seen { |
| + if strings.EqualFold(s, key) { |
| + return errors.New("duplicate key") |
| + } |
| } |
| - seen[folded] = true |
| - if err := scanUnique(dec); err != nil { |
| + seen = append(seen, key) |
| + if err := scanUnique(dec, exact); err != nil { |
| return err |
| } |
| } |
| case '[': |
| for dec.More() { |
| - if err := scanUnique(dec); err != nil { |
| + if err := scanUnique(dec, exact); err != nil { |
| return err |
| } |
| } |
| @@ -349,3 +427,23 @@ func scanUnique(dec *json.Decoder) error { |
| _, err = dec.Token() // the closing delimiter |
| return err |
| } |
| + |
| +// transportFailure turns any failure to send or read into a fixed error. |
| +// |
| +// The caller's context decides first, so a deadline or an abandoned |
| +// keystroke is still recognisable — context.Canceled and |
| +// DeadlineExceeded are fixed strings of Go's, not the provider's. A pin |
| +// refusal stays a pin refusal. **Everything else is ErrTransport, and the |
| +// original error is discarded rather than wrapped**: url.Error quotes the |
| +// URL, which after a redirect is whatever Location the provider sent, and |
| +// net/http's header and trailer parse errors quote the bytes they failed |
| +// on. None of that may reach a caller who might log it. |
| +func transportFailure(ctx context.Context, err error) error { |
| + if ctxErr := ctx.Err(); ctxErr != nil { |
| + return ctxErr |
| + } |
| + if errors.Is(err, ErrForbiddenHost) { |
| + return ErrForbiddenHost |
| + } |
| + return ErrTransport |
| +} |
diff --git a/internal/memoria/provider/provider_test.go b/internal/memoria/provider/provider_test.go| index 72be326..adcc0b1 100644 |
| --- a/internal/memoria/provider/provider_test.go |
| +++ b/internal/memoria/provider/provider_test.go |
| @@ -1,12 +1,15 @@ |
| package provider |
| import ( |
| + "bufio" |
| "context" |
| "encoding/json" |
| "errors" |
| "io" |
| + "net" |
| "net/http" |
| "net/http/httptest" |
| + "regexp" |
| "strings" |
| "sync/atomic" |
| "testing" |
| @@ -20,20 +23,28 @@ const canary = "CANARY-7f3a-passage-content" |
| // stub serves one scripted reply and records what it was sent. |
| type stub struct { |
| - srv *httptest.Server |
| - status int |
| - body string |
| - delay time.Duration |
| - calls atomic.Int32 |
| - gotReq atomic.Value // chatRequest |
| - gotKey atomic.Value // string |
| + srv *httptest.Server |
| + status int |
| + body string |
| + delay time.Duration |
| + calls atomic.Int32 |
| + gotReq atomic.Value // chatRequest |
| + gotKey atomic.Value // string |
| + gotMethod atomic.Value // string |
| + gotPath atomic.Value // string |
| + // done is closed when the server sees the request's context end — |
| + // which is how a test observes cancellation AT THE LISTENER, rather |
| + // than trusting the error the client returned. |
| + done chan struct{} |
| } |
| func newStub(t *testing.T, status int, body string) *stub { |
| t.Helper() |
| - s := &stub{status: status, body: body} |
| + s := &stub{status: status, body: body, done: make(chan struct{})} |
| s.srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
| s.calls.Add(1) |
| + s.gotMethod.Store(r.Method) |
| + s.gotPath.Store(r.URL.Path) |
| s.gotKey.Store(r.Header.Get("Authorization")) |
| var req chatRequest |
| raw, _ := io.ReadAll(r.Body) |
| @@ -43,6 +54,7 @@ func newStub(t *testing.T, status int, body string) *stub { |
| select { |
| case <-time.After(s.delay): |
| case <-r.Context().Done(): |
| + close(s.done) |
| return |
| } |
| } |
| @@ -72,10 +84,28 @@ func reply(content, finish string) string { |
| return string(b) |
| } |
| +// allowedErrors is the complete set of error strings this package may |
| +// return, built from its own sentinels so it cannot drift from them. A |
| +// status error is the one parameterised case: the sentinel followed by |
| +// exactly a three-digit code. |
| +// |
| +// **An exact set, not a canary search.** The first version checked only |
| +// that the full canary was absent, and a review returned the first eight |
| +// bytes of the response in the status error — "CANARY-7" — and stayed |
| +// green. A pattern loose enough to admit "non-2xx" would admit lowercase |
| +// content too; an exact set admits nothing it does not name. |
| +var allowedErrors = map[string]bool{ |
| + ErrTooLarge.Error(): true, ErrMalformed.Error(): true, ErrTruncated.Error(): true, |
| + ErrForbiddenHost.Error(): true, ErrTransport.Error(): true, ErrMissingKey.Error(): true, |
| + context.Canceled.Error(): true, context.DeadlineExceeded.Error(): true, |
| +} |
| + |
| +var statusError = regexp.MustCompile(`^` + regexp.QuoteMeta(ErrStatus.Error()) + ` [0-9]{3}$`) |
| + |
| func assertNoCanary(t *testing.T, err error) { |
| t.Helper() |
| - if err != nil && strings.Contains(err.Error(), canary) { |
| - t.Errorf("error carries response content: %v", err) |
| + if err != nil && !allowedErrors[err.Error()] && !statusError.MatchString(err.Error()) { |
| + t.Errorf("error is not one of the fixed strings, so it may carry content: %q", err.Error()) |
| } |
| } |
| @@ -91,7 +121,16 @@ func TestExtractAddressesHappyPath(t *testing.T) { |
| t.Fatalf("pointers = %+v", got) |
| } |
| + if m := s.gotMethod.Load(); m != http.MethodPost { |
| + t.Errorf("method = %v, want POST", m) |
| + } |
| + if p := s.gotPath.Load(); p != "/chat/completions" { |
| + t.Errorf("path = %v, want /chat/completions", p) |
| + } |
| req := s.gotReq.Load().(chatRequest) |
| + if len(req.Messages) != 2 || req.Messages[0].Role != "system" || req.Messages[0].Content != instructions || instructions == "" { |
| + t.Errorf("the system message is not the extraction instructions") |
| + } |
| if req.Model != Model { |
| t.Errorf("model = %q, want the pinned %q", req.Model, Model) |
| } |
| @@ -144,6 +183,17 @@ func TestEveryFailureIsAnErrorAndCarriesNoContent(t *testing.T) { |
| {"trailing content", 200, reply(`{"addresses":[]}{"addresses":[{"passage":0,"text":"`+canary+`"}]}`, "stop"), ErrMalformed}, |
| {"null is not empty", 200, reply(`{"addresses":null}`, "stop"), ErrMalformed}, |
| {"a missing field", 200, reply(`{"addresses":[{"text":"`+canary+`"}]}`, "stop"), ErrMalformed}, |
| + // dec.More() is not an end-of-input check; the first version |
| + // accepted both of these. |
| + {"a stray closing brace", 200, reply(`{"addresses":[{"passage":0,"text":"`+canary+`"}]}}`, "stop"), ErrMalformed}, |
| + {"a stray closing bracket", 200, reply(`{"addresses":[]}]`, "stop"), ErrMalformed}, |
| + // encoding/json folds "ſ" to "s"; strings.ToLower does not. This |
| + // decoded as passage 1 through the first version's duplicate check. |
| + {"a long-s duplicate", 200, reply(`{"addresses":[{"passage":0,"pa\u017f\u017fage":1,"text":"`+canary+`"}]}`, "stop"), ErrMalformed}, |
| + {"a differently-cased key", 200, reply(`{"addresses":[{"PASSAGE":0,"text":"`+canary+`"}]}`, "stop"), ErrMalformed}, |
| + // The envelope, not the content: the last duplicate wins, which |
| + // would turn a truncation into a clean stop. |
| + {"a duplicate finish_reason", 200, `{"choices":[{"message":{"content":"{\"addresses\":[]}"},"finish_reason":"length","finish_reason":"stop"}]}`, ErrMalformed}, |
| {"two choices", 200, `{"choices":[{"message":{"content":"{\"addresses\":[]}"},"finish_reason":"stop"},{"message":{"content":"{\"addresses\":[]}"},"finish_reason":"stop"}]}`, ErrMalformed}, |
| } { |
| t.Run(c.name, func(t *testing.T) { |
| @@ -162,6 +212,12 @@ func TestEveryFailureIsAnErrorAndCarriesNoContent(t *testing.T) { |
| // §6.5's deadline, reused. A slow provider must not hold the page. |
| func TestTheDeadlineHolds(t *testing.T) { |
| + // Against a literal, not against Deadline itself: comparing the |
| + // elapsed time to Deadline+slack passed when a review raised the |
| + // constant to two seconds. |
| + if Deadline != 800*time.Millisecond { |
| + t.Fatalf("Deadline = %v; §8.9 and §6.5 say 800ms", Deadline) |
| + } |
| s := newStub(t, 200, reply(`{"addresses":[]}`, "stop")) |
| s.delay = 5 * time.Second |
| start := time.Now() |
| @@ -169,8 +225,8 @@ func TestTheDeadlineHolds(t *testing.T) { |
| if err == nil { |
| t.Fatal("a provider past the deadline returned no error") |
| } |
| - if took := time.Since(start); took > Deadline+400*time.Millisecond { |
| - t.Errorf("took %v; the deadline is %v", took, Deadline) |
| + if took := time.Since(start); took > 1200*time.Millisecond { |
| + t.Errorf("took %v; the deadline is 800ms", took) |
| } |
| } |
| @@ -187,6 +243,15 @@ func TestCancellationReachesTheProvider(t *testing.T) { |
| if took := time.Since(start); took > 400*time.Millisecond { |
| t.Errorf("took %v after cancel", took) |
| } |
| + // **At the listener.** The returned error proved only that the CLIENT |
| + // stopped waiting; a review detached the request so it kept running in |
| + // the background and this test stayed green. The server seeing its |
| + // request context end is what shows the provider stopped too. |
| + select { |
| + case <-s.done: |
| + case <-time.After(time.Second): |
| + t.Error("the provider never saw the request cancelled; it is still paying for an abandoned keystroke") |
| + } |
| } |
| // **What pays for this package's exemption from the outbound-network |
| @@ -204,14 +269,52 @@ func TestCancellationReachesTheProvider(t *testing.T) { |
| // that also failed the scheme check, and deleting the host pin left it |
| // green — a neighbouring error path supplying the failure. |
| func TestClientDialsOnlyTensorX(t *testing.T) { |
| - t.Run("New is pinned to TensorX over https", func(t *testing.T) { |
| + t.Run("New is pinned to TensorX, https, port 443, no proxy", func(t *testing.T) { |
| c, err := New("k") |
| if err != nil { |
| t.Fatal(err) |
| } |
| pin := c.hc.Transport.(*pinnedTransport) |
| - if c.base != BaseURL || pin.host != "api.tensorx.ai" || pin.scheme != "https" { |
| - t.Errorf("New dials %q pinned to %s://%s", c.base, pin.scheme, pin.host) |
| + if c.base != BaseURL || pin.scheme != "https" || pin.authority != "api.tensorx.ai:443" { |
| + t.Errorf("New dials %q pinned to %s://%s", c.base, pin.scheme, pin.authority) |
| + } |
| + // Structural, because behaviour cannot show it here: Go never |
| + // proxies a request to 127.0.0.1, so an httptest server cannot |
| + // tell a proxy-honouring transport from this one. A review showed |
| + // the production constructor sending CONNECT to an HTTPS_PROXY |
| + // listener while the host pin passed. |
| + if tr, ok := pin.next.(*http.Transport); !ok || tr.Proxy != nil { |
| + t.Errorf("the transport honours environment proxies: %T", pin.next) |
| + } |
| + }) |
| + |
| + t.Run("New refuses an empty credential", func(t *testing.T) { |
| + if _, err := New(""); !errors.Is(err, ErrMissingKey) { |
| + t.Errorf("err = %v, want ErrMissingKey", err) |
| + } |
| + }) |
| + |
| + // The first version pinned the host alone; another listener on the |
| + // same host took the body, and ":8443" in BaseURL left every test green. |
| + t.Run("another port on the same host is refused before dialling", func(t *testing.T) { |
| + s := newStub(t, 200, reply(`{"addresses":[]}`, "stop")) |
| + other := newStub(t, 200, reply(`{"addresses":[]}`, "stop")) |
| + c := s.client() |
| + c.base = other.srv.URL // same 127.0.0.1, different port |
| + if _, err := c.ExtractAddresses(context.Background(), []string{"x"}); !errors.Is(err, ErrForbiddenHost) { |
| + t.Fatalf("err = %v, want ErrForbiddenHost", err) |
| + } |
| + if n := other.calls.Load() + s.calls.Load(); n != 0 { |
| + t.Errorf("%d requests arrived; the passages left", n) |
| + } |
| + }) |
| + |
| + t.Run("userinfo in the URL is refused", func(t *testing.T) { |
| + s := newStub(t, 200, reply(`{"addresses":[]}`, "stop")) |
| + c := s.client() |
| + c.base = strings.Replace(s.srv.URL, "http://", "http://user:pw@", 1) |
| + if _, err := c.ExtractAddresses(context.Background(), []string{"x"}); !errors.Is(err, ErrForbiddenHost) { |
| + t.Fatalf("err = %v, want ErrForbiddenHost", err) |
| } |
| }) |
| @@ -248,7 +351,9 @@ func TestClientDialsOnlyTensorX(t *testing.T) { |
| var hits atomic.Int32 |
| srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
| if hits.Add(1) == 1 { |
| - http.Redirect(w, r, "/elsewhere", http.StatusTemporaryRedirect) |
| + // The Location is provider-chosen bytes. A review returned |
| + // this exact shape and the first version's error quoted it. |
| + http.Redirect(w, r, "/"+canary, http.StatusTemporaryRedirect) |
| return |
| } |
| _, _ = io.WriteString(w, reply(`{"addresses":[]}`, "stop")) |
| @@ -258,11 +363,94 @@ func TestClientDialsOnlyTensorX(t *testing.T) { |
| if err != nil { |
| t.Fatal(err) |
| } |
| - if _, err := c.ExtractAddresses(context.Background(), []string{"x"}); !errors.Is(err, ErrForbiddenHost) { |
| + _, err = c.ExtractAddresses(context.Background(), []string{"x"}) |
| + if !errors.Is(err, ErrForbiddenHost) { |
| t.Fatalf("err = %v, want ErrForbiddenHost", err) |
| } |
| + assertNoCanary(t, err) |
| if n := hits.Load(); n != 1 { |
| t.Errorf("server saw %d requests; the redirect was followed", n) |
| } |
| }) |
| } |
| + |
| +// A response the HTTP layer cannot parse is a transport failure, and the |
| +// bytes it choked on are not in the error. Go's own header parser quotes |
| +// what it failed on; a review planted a canary in a malformed header and |
| +// the first version returned it. Served from a raw socket, because an |
| +// http.Server will not write a malformed header. |
| +func TestAMalformedResponseCarriesNoContent(t *testing.T) { |
| + ln, err := net.Listen("tcp", "127.0.0.1:0") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + t.Cleanup(func() { ln.Close() }) |
| + go func() { |
| + conn, err := ln.Accept() |
| + if err != nil { |
| + return |
| + } |
| + defer conn.Close() |
| + // Consume the request line and headers, then answer badly. |
| + br := bufio.NewReader(conn) |
| + for { |
| + line, err := br.ReadString('\n') |
| + if err != nil || line == "\r\n" { |
| + break |
| + } |
| + } |
| + _, _ = conn.Write([]byte("HTTP/1.1 200 OK\r\n" + canary + " no colon here\r\n\r\n")) |
| + }() |
| + c, err := newClient("k", "http://"+ln.Addr().String()) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + _, err = c.ExtractAddresses(context.Background(), []string{"x"}) |
| + if !errors.Is(err, ErrTransport) { |
| + t.Fatalf("err = %v, want ErrTransport", err) |
| + } |
| + assertNoCanary(t, err) |
| +} |
| + |
| +// rejectAmbiguous is called DIRECTLY here, because through |
| +// ExtractAddresses two of its checks are unreachable: the content's |
| +// exact-key check rejects "paſſage" before the duplicate check runs, and |
| +// decodeAddresses has its own end-of-input check. Mutating either inside |
| +// rejectAmbiguous left every other test green (found 2026-09-27). They |
| +// still matter — for the ENVELOPE, whose keys are not enumerated — so |
| +// they are pinned where they can be reached (AGENTS.md: make a defence |
| +// reachable or delete it). |
| +func TestRejectAmbiguousDirectly(t *testing.T) { |
| + for _, c := range []struct { |
| + name, in string |
| + exact map[string]bool |
| + ok bool |
| + }{ |
| + {"a clean document", `{"a":1,"b":[{"c":2}]}`, nil, true}, |
| + {"a stray closing brace", `{"a":1}}`, nil, false}, |
| + {"a stray closing bracket", `{"a":1}]`, nil, false}, |
| + {"two documents", `{"a":1} {"a":2}`, nil, false}, |
| + {"a plain duplicate", `{"a":1,"a":2}`, nil, false}, |
| + {"a case duplicate", `{"a":1,"A":2}`, nil, false}, |
| + // Go's decoder folds ſ to s; strings.ToLower does not. With no |
| + // exact-key list — the envelope's case — only EqualFold sees it. |
| + {"a long-s duplicate", `{"finish_reason":"length","finiſh_reaſon":"stop"}`, nil, false}, |
| + {"a nested long-s duplicate", `{"x":{"passage":0,"paſſage":1}}`, nil, false}, |
| + {"an exact key", `{"text":"a"}`, contentKeys, true}, |
| + {"a differently-cased key", `{"TEXT":"a"}`, contentKeys, false}, |
| + } { |
| + if err := rejectAmbiguous([]byte(c.in), c.exact); (err == nil) != c.ok { |
| + t.Errorf("%s: err = %v, want ok=%v", c.name, err, c.ok) |
| + } |
| + } |
| +} |
| + |
| +// And the long-s duplicate end to end in the envelope, where it would |
| +// otherwise turn a truncation into a clean stop. |
| +func TestALongSEnvelopeDuplicateIsRefused(t *testing.T) { |
| + body := `{"choices":[{"message":{"content":"{\"addresses\":[]}"},"finish_reason":"length","finiſh_reaſon":"stop"}]}` |
| + s := newStub(t, 200, body) |
| + if _, err := s.client().ExtractAddresses(context.Background(), []string{"x"}); !errors.Is(err, ErrMalformed) { |
| + t.Errorf("err = %v, want ErrMalformed: the duplicate would have decoded as a clean stop", err) |
| + } |
| +} |