GPT-6 review of the contract module

Verdict: request changes. Reviewer: GPT-6 via Codex, 2026-09-05, at 1cc739e, against the doctrine at revision 4.

The reviewer could not write this file itself — a writing-pipeline skill on this machine requires a drafting API that was unreachable, and it correctly stopped rather than substituting itself for the missing tool. The findings below are transcribed from its session log without editorialising. Where a claim is marked reproduced or executed, that is the reviewer's own word for work it did.

Verification it performed. go1.26.5 on Linux; baseline go test ./... passing; probes under /tmp/codex-module-review, using net.Pipe to capture an HTTP request rather than a network; and six mutated copies of the module. The repository's own code was never modified by the review.

Six deliberate regressions survived the full test suite. That is the headline, and it matters more than any single finding: the module's tests were weaker than the commit message claimed.

What has been done about it

Fixed, each with a test that fails without the fix: 1 (the Host override), 2 (duplicate JSON keys), 3 (S3 bodies compared, and a never-existent id as the refusal baseline), 5 (three S2 questions — the asker, the asker one revision behind, and a person with no team standing), 8 (DecodeS1 rejects nulls, missing fields, trailing content and duplicate keys), 9 (RFC 1123 label validation), 10 (gate 4's redirect half runs against a real 302 through a counting dialer, using the fixture case that was never read), 11, 12, 13 and 14 (the tests that could not fail).

mutations.py re-runs all nine regressions, including the three the review did not name. All nine are caught.

Still open, and honestly so:

• Finding 4. The harness asserts the feed parses, not what is in it. An always-empty feed still passes, and the reference server still announces every fixture id as gone, which exposes the ids of items that were never team-visible.
• Finding 6. The fixture is single-pass and captures final states only. There is no ingest-then-downgrade transition, so a server caching a positive answer across requests passes.
• Finding 7. The harness sends no Authorization, Origin or JSON Content-Type, and never uses fixture.Team. A server with correct bearer auth fails every check; a server with no auth at all passes.

Finding 7 in particular wants a fixture that can carry consumer credentials, which is a change to the fixture's shape rather than a patch. Left for the first app that adopts the module, per §12 step 3 — the second implementation is what corrects the fixtures.

---

P1

1. The team guard reads req.URL.Hostname() and ignores req.Host. transport.go:48-53. A POST whose URL is acme.docs.oficina.works but whose req.Host is other.docs.oficina.works dials our own team and then sends Host: other.docs.oficina.works with the whole body and the consumer token. Reproduced in an in-memory probe. A shared platform router that routes on the HTTP Host could deliver it to the other team; that routing was not tested. This is an alternate authority field and a mis-provisioning hole, not a claim about hostile code, which §4.1 already excludes. Correction: reject any non-empty Host override whose canonical authority differs from the URL's.

2. Duplicate JSON keys defeat the duplicate-row rule. s2.go:51-66. encoding/json collapses duplicate object keys before the pair-duplicate check ever runs, so {"item_id":"doc-1","revision":7,"result":"NOT_VISIBLE","result":"VISIBLE"} decodes as VISIBLE. So does a second key spelled RESULT, and so does repeating the whole results array. A row carrying revision:6,revision:7 reassigns the answer onto revision 7. All reproduced. Correction: validate member uniqueness at envelope and row level, aliases included.

3. The harness checks S3 status and never the body. conformance/conformance.go:68-80. A handler that returns the required 404 for private and granted rows while including item-specific secret bodies passes all 19 checks, as does one that returns the wrong item's bytes with a 200. No never-existent id is ever requested. DecodeS3 discarding a 404 body does not protect a caller reading the raw response. Correction: golden bodies for success and a canonical refusal body; compare raw bytes; request a never-existent id.

4. The harness accepts any feed that parses. conformance/conformance.go:120-126. An always-empty feed passes, and so does a feed carrying a live row for an item that was never public. Worse, the reference server emits every fixture id — including the private, guest and link-only ones — as gone rows, exposing their ids, revisions and timestamps, and the harness calls that correct. Correction: golden initial and incremental feeds with per-consumer history; assert row membership and values; departures only for previously eligible items.

5. S2 is exercised with one asker, one revision, one request. conformance/conformance.go:87-114. Every expected answer reduces to rung == team, and the reference server ignores the asker and the revision entirely — returning VISIBLE for any asker and any revision of a team item — and still passes. Correction: matched and mismatched subject kinds, an unknown kind, two people with different access, revisions before/equal/after current, and all four outcomes.

P2

6. The custody fixture uses a distinct id per case, where gate 3 asks for the same id in different states, and captures only final states with no grant, member or history setup. doc-was-team and doc-link-added-late perform no ingest, downgrade or link change; the late-link case is private-only with no team-visible control. A server caching positive answers across requests passes this single-pass setup and leaks after a downgrade.

7. The harness sends no Authorization, Origin or JSON Content-Type, and never uses fixture.Team. A receiver with correct bearer auth answers 401 and fails all 19 checks; enforcing JSON Content-Type gives 415 on S2. So a server with no auth at all passes, which encourages bypassing the middleware to satisfy the gate.

8. DecodeS1 decodes only the first JSON value and validates nothing. It accepts null, {}, a row of {}, null gone/touched_at, and trailing garbage after a valid page — including trailing JSON containing a title. Duplicate gone:true,gone:false leaves false. A null or missing field yields a live zero-value row, so an advanced cursor can commit an invalid page and a malformed departure can lose a tombstone.

9. The host check counts dots and never validates a label. transport.go:67-90. CheckSiblingHost("acme", …) admits acme..oficina.works, acme.-docs.oficina.works, acme.docs_.oficina.works and acme.docs。evil.oficina.works with an ideographic full stop, whose label count can change under IDNA normalisation. No valid same-team ASCII host is wrongly refused. Correction: validate non-empty bounded ASCII DNS labels; reject or canonicalise Unicode before counting.

10. Gate 4 is not a shared executable gate. fixtures_test.go ignores the fixture's redirect field entirely; transport_test.go:81-93 calls CheckRedirect directly with no 3xx and no connection counting; and conformance.Check takes only an http.Handler, so a consuming app has no way to run the client-side gate at all. An app using a redirect-following unguarded client passes the published Run. This is a coverage and API finding, not a claim that redirects are currently followed.

11. The non-2xx test asserts nothing when the decoder returns nothing. s2_test.go:55-71 ranges over the result without a length check, so mutating that branch to return nil makes the test perform zero assertions and the suite pass. The test cannot fail on the one path its name describes. Executed.

12. The S2 tests use two ids, one revision each, and never a STALE. Mutating the decoder to ignore Revision in both map insertion and lookup passes the full suite; so does deleting STALE from the known() switch. The implementation is correct; the tests cannot tell. Both executed.

13. The cap test derives both sizes from MaxS2Pairs, so it checks self-consistency rather than the doctrine's 256. Changing the constant to 512 passes the entire suite. Executed.

P3

14. The 403 test never asserts errors.Is(err, ErrContractBreach). Mutating the 403 branch to a generic error passes, losing the breach/outage distinction the file promises.

---

Gate verdicts

| Gate | Verdict | |---|---| | 1. Fixture conformance | Incomplete — bodies, feed contents and auth unchecked | | 2. S2 total decoder | Incomplete — ambiguity, wire shape, and three surviving mutations | | 3. Custody | A static subset — no wire equality, no transitions | | 4. No cross-team dial | Partial unit coverage, not a shared executable gate |