oficina / suite Public

Download this file

1	# GPT-6 review of the contract module
2
3	**Verdict: request changes.** Reviewer: GPT-6 via Codex, 2026-09-05, at
4	`1cc739e`, against the doctrine at revision 4.
5
6	The reviewer could not write this file itself — a `writing-pipeline` skill
7	on this machine requires a drafting API that was unreachable, and it
8	correctly stopped rather than substituting itself for the missing tool.
9	The findings below are transcribed from its session log without
10	editorialising. Where a claim is marked reproduced or executed, that is the
11	reviewer's own word for work it did.
12
13	**Verification it performed.** go1.26.5 on Linux; baseline `go test ./...`
14	passing; probes under `/tmp/codex-module-review`, using `net.Pipe` to
15	capture an HTTP request rather than a network; and six mutated copies of
16	the module. The repository's own code was never modified by the review.
17
18	**Six deliberate regressions survived the full test suite.** That is the
19	headline, and it matters more than any single finding: the module's tests
20	were weaker than the commit message claimed.
21
22	## What has been done about it
23
24	Fixed, each with a test that fails without the fix: **1** (the `Host`
25	override), **2** (duplicate JSON keys), **3** (S3 bodies compared, and a
26	never-existent id as the refusal baseline), **5** (three S2 questions — the
27	asker, the asker one revision behind, and a person with no team standing),
28	**8** (`DecodeS1` rejects nulls, missing fields, trailing content and
29	duplicate keys), **9** (RFC 1123 label validation), **10** (gate 4's
30	redirect half runs against a real 302 through a counting dialer, using the
31	fixture case that was never read), **11**, **12**, **13** and **14** (the
32	tests that could not fail).
33
34	`mutations.py` re-runs all nine regressions, including the three the review
35	did not name. All nine are caught.
36
37	**Still open, and honestly so:**
38
39	- **Finding 4.** The harness asserts the feed parses, not what is in it. An
40	always-empty feed still passes, and the reference server still announces
41	every fixture id as `gone`, which exposes the ids of items that were
42	never team-visible.
43	- **Finding 6.** The fixture is single-pass and captures final states only.
44	There is no ingest-then-downgrade transition, so a server caching a
45	positive answer across requests passes.
46	- **Finding 7.** The harness sends no Authorization, Origin or JSON
47	Content-Type, and never uses `fixture.Team`. A server with correct bearer
48	auth fails every check; a server with no auth at all passes.
49
50	Finding 7 in particular wants a fixture that can carry consumer credentials,
51	which is a change to the fixture's shape rather than a patch. Left for the
52	first app that adopts the module, per §12 step 3 — the second implementation
53	is what corrects the fixtures.
54
55	---
56
57	## P1
58
59	**1. The team guard reads `req.URL.Hostname()` and ignores `req.Host`.**
60	`transport.go:48-53`. A POST whose URL is `acme.docs.oficina.works` but
61	whose `req.Host` is `other.docs.oficina.works` dials our own team and then
62	sends `Host: other.docs.oficina.works` with the whole body and the consumer
63	token. Reproduced in an in-memory probe. A shared platform router that
64	routes on the HTTP Host could deliver it to the other team; that routing
65	was not tested. This is an alternate authority field and a
66	mis-provisioning hole, not a claim about hostile code, which §4.1 already
67	excludes. *Correction:* reject any non-empty `Host` override whose
68	canonical authority differs from the URL's.
69
70	**2. Duplicate JSON keys defeat the duplicate-row rule.** `s2.go:51-66`.
71	`encoding/json` collapses duplicate object keys before the pair-duplicate
72	check ever runs, so
73	`{"item_id":"doc-1","revision":7,"result":"NOT_VISIBLE","result":"VISIBLE"}`
74	decodes as `VISIBLE`. So does a second key spelled `RESULT`, and so does
75	repeating the whole `results` array. A row carrying `revision:6,revision:7`
76	reassigns the answer onto revision 7. All reproduced. *Correction:*
77	validate member uniqueness at envelope and row level, aliases included.
78
79	**3. The harness checks S3 status and never the body.**
80	`conformance/conformance.go:68-80`. A handler that returns the required 404
81	for private and granted rows while including item-specific secret bodies
82	passes all 19 checks, as does one that returns the wrong item's bytes with
83	a 200. No never-existent id is ever requested. `DecodeS3` discarding a 404
84	body does not protect a caller reading the raw response. *Correction:*
85	golden bodies for success and a canonical refusal body; compare raw bytes;
86	request a never-existent id.
87
88	**4. The harness accepts any feed that parses.**
89	`conformance/conformance.go:120-126`. An always-empty feed passes, and so
90	does a feed carrying a live row for an item that was never public. Worse,
91	the reference server emits every fixture id — including the private, guest
92	and link-only ones — as `gone` rows, exposing their ids, revisions and
93	timestamps, and the harness calls that correct. *Correction:* golden
94	initial and incremental feeds with per-consumer history; assert row
95	membership and values; departures only for previously eligible items.
96
97	**5. S2 is exercised with one asker, one revision, one request.**
98	`conformance/conformance.go:87-114`. Every expected answer reduces to
99	`rung == team`, and the reference server ignores the asker and the revision
100	entirely — returning `VISIBLE` for any asker and any revision of a team
101	item — and still passes. *Correction:* matched and mismatched subject
102	kinds, an unknown kind, two people with different access, revisions
103	before/equal/after current, and all four outcomes.
104
105	## P2
106
107	**6. The custody fixture uses a distinct id per case**, where gate 3 asks
108	for the same id in different states, and captures only final states with no
109	grant, member or history setup. `doc-was-team` and `doc-link-added-late`
110	perform no ingest, downgrade or link change; the late-link case is
111	private-only with no team-visible control. A server caching positive
112	answers across requests passes this single-pass setup and leaks after a
113	downgrade.
114
115	**7. The harness sends no Authorization, Origin or JSON Content-Type**, and
116	never uses `fixture.Team`. A receiver with correct bearer auth answers 401
117	and fails all 19 checks; enforcing JSON Content-Type gives 415 on S2. So a
118	server with no auth at all passes, which encourages bypassing the
119	middleware to satisfy the gate.
120
121	**8. `DecodeS1` decodes only the first JSON value and validates nothing.**
122	It accepts `null`, `{}`, a row of `{}`, null `gone`/`touched_at`, and
123	trailing garbage after a valid page — including trailing JSON containing a
124	title. Duplicate `gone:true,gone:false` leaves `false`. A null or missing
125	field yields a live zero-value row, so an advanced cursor can commit an
126	invalid page and a malformed departure can lose a tombstone.
127
128	**9. The host check counts dots and never validates a label.**
129	`transport.go:67-90`. `CheckSiblingHost("acme", …)` admits
130	`acme..oficina.works`, `acme.-docs.oficina.works`, `acme.docs_.oficina.works`
131	and `acme.docs。evil.oficina.works` with an ideographic full stop, whose
132	label count can change under IDNA normalisation. No valid same-team ASCII
133	host is wrongly refused. *Correction:* validate non-empty bounded ASCII DNS
134	labels; reject or canonicalise Unicode before counting.
135
136	**10. Gate 4 is not a shared executable gate.** `fixtures_test.go` ignores
137	the fixture's `redirect` field entirely; `transport_test.go:81-93` calls
138	`CheckRedirect` directly with no 3xx and no connection counting; and
139	`conformance.Check` takes only an `http.Handler`, so a consuming app has no
140	way to run the client-side gate at all. An app using a redirect-following
141	unguarded client passes the published `Run`. This is a coverage and API
142	finding, not a claim that redirects are currently followed.
143
144	**11. The non-2xx test asserts nothing when the decoder returns nothing.**
145	`s2_test.go:55-71` ranges over the result without a length check, so
146	mutating that branch to `return nil` makes the test perform zero
147	assertions and the suite pass. The test cannot fail on the one path its
148	name describes. *Executed.*
149
150	**12. The S2 tests use two ids, one revision each, and never a `STALE`.**
151	Mutating the decoder to ignore `Revision` in both map insertion and lookup
152	passes the full suite; so does deleting `STALE` from the `known()` switch.
153	The implementation is correct; the tests cannot tell. *Both executed.*
154
155	**13. The cap test derives both sizes from `MaxS2Pairs`**, so it checks
156	self-consistency rather than the doctrine's 256. Changing the constant to
157	512 passes the entire suite. *Executed.*
158
159	## P3
160
161	**14. The 403 test never asserts `errors.Is(err, ErrContractBreach)`.**
162	Mutating the 403 branch to a generic error passes, losing the breach/outage
163	distinction the file promises.
164
165	---
166
167	## Gate verdicts
168
169	| Gate | Verdict |
170	|---|---|
171	| 1. Fixture conformance | Incomplete — bodies, feed contents and auth unchecked |
172	| 2. S2 total decoder | Incomplete — ambiguity, wire shape, and three surviving mutations |
173	| 3. Custody | A static subset — no wire equality, no transitions |
174	| 4. No cross-team dial | Partial unit coverage, not a shared executable gate |
175