# 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 |