noitu/plans/reports/code-reviewer-260904-2329-phase-04-protobuf-contract.md

title, date, reviewer, scope, status

title	date	reviewer	scope	status
Code Review — Phase 4: Protobuf Contract and Codegen	2026-09-04	code-reviewer	phase-04	DONE_WITH_CONCERNS

Code Review — Phase 4: Protobuf Contract and Codegen

Scope

• Hand-written: buf.yaml, buf.gen.yaml, proto/noitu/v1/game.proto (209), server/internal/wsapi/convert.go (113), convert_test.go (168), wire_test.go (289), web/tests/game-wire.test.js (94), web/package.json, .github/workflows/proto.yml (77), Makefile, README.md, server/go.mod/go.sum. ~980 LOC.
• Generated + committed: server/gen/noitu/v1/game.pb.go (1874), web/src/lib/proto/noitu/v1/game_pb.{js,d.ts} (969). Config reviewed, style not.
• Fixtures: 17 files in proto/testdata/.
• Checks actually run: go vet ./..., go test ./... -race, npm test (23 pass), buf lint (clean), buf generate + byte-for-byte hash comparison, plus two reproduction experiments in a scratch git repo (see Critical #1 and Edge Case #1).

Overall Assessment

The contract itself is good work: one schema, both targets generated from it, no hand-written message type anywhere, an explicit engine↔wire mapping instead of a cast, and a genuine cross-language fixture test rather than two self-consistent round trips. Comments explain invariants and carry no plan/phase IDs, matching game/state.go and dictionary/store.go. Phases 1-3 are untouched.

Two defects are blocking, both in the guard rails rather than the contract: buf breaking cannot resolve its baseline on any pull request, and the regen-diff check cannot see a missing generated file. Both mean a check reported as passing is not actually protecting anything. Beyond those, one exported helper has no wire field to feed and no caller, and ServerError carries two fields with identical documented meaning — cheap to fix now, breaking to fix after a client ships.

---

Acceptance Criteria

#	Criterion	Verdict	Evidence
a	make proto regenerates both targets with zero diff	MET	Hashed all 3 generated files, ran buf generate, re-hashed: identical.
b	buf lint clean; buf breaking wired into CI	PARTIAL	buf lint exits 0 with no output. buf breaking is wired but cannot run — see Critical #1.
c	Go round trip covers every oneof variant, both directions	MET	wire_test.go:126 TestRoundTripEveryVariant over 7 client + 10 server arms; TestVariantTablesCoverEveryOneofArm asserts arm coverage reflectively (with a gap — High #3).
d	JS decodes a Go-produced binary fixture, reads correct values	MET	web/tests/game-wire.test.js; 23 tests pass. Asserts UTF-8 diacritics, int64→bigint, enum numbering, empty-arm tag survival.
e	Mapping exhaustive; test fails on a new engine reason	PARTIAL	convert_test.go:41,77 walk engine constants and also assert every wire value is reachable — good, both directions. But the walk terminates on String() == "unknown", so a new constant added without a String() case is invisible. See High #4.
f	No hand-written message struct in server/ or web/	MET	grep "^type .* struct" server/internal/wsapi/ → none; web/src contains only the generated tree.
g	Protocol version field in the handshake	MET (underspecified)	game.proto:62 Hello.protocol_version, :112 Welcome.protocol_version. Semantics undefined — see Medium #6.
h	Generated code is committed	MET	server/gen/, web/src/lib/proto/ present in the change; .gitignore does not exclude them.

---

Critical / MUST-FIX

1. buf breaking fails on every pull request — the check protects nothing

.github/workflows/proto.yml:60

run: buf breaking --against '.git#branch=main'

actions/checkout on a pull_request event leaves a detached HEAD with no local branch main — only refs/remotes/origin/main. buf's git input clones the local .git as a remote and asks for the ref by name, so branch=main is not found.

Reproduced in a scratch clone (detached HEAD, local main deleted, origin/main present):

Failure: could not clone file://...\.git: exit status 128
fatal: couldn't find remote ref main

Meanwhile the guard immediately above (:51) probes origin/main:proto/..., which does resolve — so the two steps disagree on how to name the baseline. Once the schema lands on main the guard says exists=true and the breaking step then hard-fails every PR.

The push case is the mirror image: checkout creates a local main at the commit being built, so --against branch=main compares the schema against itself and passes unconditionally. Net effect: the check either errors or is a no-op; it never catches a break.

Fix — both forms verified working in the same scratch clone (exit 0):

run: buf breaking --against '.git#ref=origin/main'

and align the guard on the same ref so the two steps cannot diverge again.

2. The regen-diff check cannot see a missing generated file

.github/workflows/proto.yml:67 and Makefile:57

git diff --exit-code -- server/gen web/src/lib/proto

git diff never reports untracked files. Verified: touch server/internal/game/zzz_fake.go inside a fully tracked directory, then git diff --exit-code -- server/internal/game → exit 0.

So the failure mode the step's own comment names ("a committed generated tree that no longer matches the schema is worse than no generated tree at all") is only half covered. Modified and deleted files are caught; a new output file is not. That is not hypothetical here — a second .proto, a plugin option change that splits output, or a protoc-gen-es version that emits an extra file all produce new untracked files and a green CI with an incomplete committed tree, which then fails at build time in phase 6.

Fix (catches all three states):

buf generate
git add --intent-to-add -- server/gen web/src/lib/proto
git diff --exit-code -- server/gen web/src/lib/proto

or test -z "$(git status --porcelain -- server/gen web/src/lib/proto)".

---

High Priority

3. clientNames()/serverNames() are a hand-maintained duplicate of the variant tables

wire_test.go:261,268 feed assertCoversOneof (:246), which only asserts proto arms ⊆ covered. covered is a second hardcoded literal list, unconnected to clientVariants()/serverVariants(). Drift path: delete an arm from the variants table, run -update (which also deletes the orphan fixture), and both the fixture-count check (:161) and assertCoversOneof still pass — that arm is now untested in both languages while the test's own doc comment claims otherwise.

Fix: derive the covered set from the tables instead of restating it — m.ProtoReflect().WhichOneof(oneof).Name() per variant. Removes 12 lines and closes the hole.

4. The exhaustiveness guard depends on String(), not on the constant set

convert_test.go:15-37 enumerate engine reasons by incrementing until String() == "unknown". If a contributor adds ReasonFoo to the iota block but forgets the String() case — the same edit, one file, easy to half-do — the walk stops before ReasonFoo, both exhaustiveness tests pass, and convert.go silently returns REJECT_REASON_UNSPECIFIED at runtime. This is exactly the failure criterion (e) exists to prevent, guarded by a proxy rather than by the thing itself.

Fix: add a sentinel to the engine (a trailing reasonCount in the iota block, and endReasonCount likewise) and assert len(engineRejectReasons()) == int(game.ReasonCount). Then a missing String() case fails loudly instead of shrinking the test's domain.

5. WireDifficulty has no caller and no wire field to carry its result

convert.go:85-99. Its comment says it exists "so a server that picked or clamped a difficulty can report what it actually used" — but no server→client message has a Difficulty field. grep WireDifficulty outside internal/wsapi → no hits; its only consumer is convert_test.go:110. Exported API that exists to satisfy its own test.

Pick one: add Difficulty difficulty = 7; to GameStarted (additive, free now, and the client genuinely needs it to label a resumed bot game), or delete WireDifficulty and its test. Do not leave it as-is — the comment documents a capability the schema cannot express.

---

Medium Priority

6. protocol_version has no defined semantics and no constant

game.proto:62,112. A bare uint32 with no statement of the current value or whether it is a major, a major.minor packed integer, or a monotonic counter. The phase requirement is "server rejects mismatched majors", which a single field cannot express without a stated convention. Right now 1 appears only as a literal in wire_test.go:31,59.

Since a version field's meaning is part of the contract, pin it in the schema comment now ("incompatible-change counter; bump only when an old client can no longer be served; current value 1"). Phase 5 can then define const ProtocolVersion = 1 against a written rule instead of inventing one.

7. ServerError.code and ServerError.message are documented as the same thing

game.proto:184-189. The comment says message is "a UI key such as room_not_found, never prose", and code is a code — two fields, one semantic, and the fixture sets both to "room_not_found" (wire_test.go:110). The field named message will attract prose in phase 5 (that is what the name means everywhere else), and prose on this path is the classic internal-detail leak to an untrusted consumer.

Removing a field after a client ships is the breaking change this whole phase exists to avoid, so resolve it now: either drop message and keep code alone, or give the two distinct roles (code = machine-stable identifier; message = optional structured detail, explicitly never rendered) and say so in the comment.

8. No message can restore a resumed game

Hello.resume_token exists, and the plan's success criteria require "reconnect within grace window restores the game". Nothing in ServerMessage carries the word history, the scores, or the current position as a snapshot: GameStarted has only the opening word and TurnUpdate only the last move. A reconnecting client can decode everything it receives and still render an empty board. Also missing: an OpponentReconnected counterpart to OpponentLeft.

Both are purely additive, so this is not a schema defect today — but phase 5/7 will need them, and adding them while the fixture tables are already open is cheaper than a second pass.

9. log.Printf from an internal package is a new convention

convert.go:44,64,96. Before this change, log appears only in cmd/build-dictionary (a main package); internal/{game,bot,dictionary,vietnamese} return errors and never log. The global logger in a request-path library is unsilenceable, has no request context, and is awkward to assert on in a test. The intent (an unmapped value is a bug, so make noise) is right; the mechanism should be slog or an injected logger, decided in phase 5 when the server acquires a logging story. Worth deciding before four more packages copy this.

---

Low Priority

10. -update deletes before it writes. wire_test.go:204 removes every *.bin, then :210 writes them. A t.Fatalf on marshal or write leaves the fixture directory empty or half-populated and the test failing — recoverable via git, but noisy. Write to a temp dir and rename, or delete only names not in allVariants().
11. go test ./... -update does not work. The flag is defined only in the wsapi test binary; every other package's binary rejects it. README correctly documents go test ./internal/wsapi -update; worth a one-line note on the flag itself.
12. make web is broken. Makefile:66 runs npm run build and web/package.json has no build script, while README.md advertises the target. Pre-existing, and phase 6 fixes it, but the diff touched this target without noticing.
13. clean: true in buf.gen.yaml wipes whole output directories. Fine today; in phase 6 a hand-written web/src/lib/proto/index.js barrel would be silently deleted by make proto. Worth a comment next to the out: lines.
14. No gofmt check in CI, and proto.yml runs only go test ./internal/wsapi/... — the repo now has exactly one workflow, so phases 1-3 have no CI coverage at all. The dictionary tests do not need data/noitu.db (verified: data/ holds only the licence files, and the full suite passes locally without it), so broadening to go vet ./... && go test ./... -race is free.
15. No buf.lock. The phase file listed one under "Create". Correct to omit — buf.yaml declares no deps, so buf generates no lock. No action; noted so it is not read as missing.
16. No concurrency: group in the workflow; superseded PR runs keep burning minutes.

---

Edge Cases Scouted

1. CRLF mangling of the binary fixtures — checked, not a defect. core.autocrlf=true is set both globally and in this repo, there is no .gitattributes, and 15 of the 17 fixtures contain no NUL byte while 9 contain 0x0a. That is the shape of a file git converts. Empirically it does not: committed all 17 into a scratch repo with autocrlf=true and re-cloned — git ls-files --eol reports i/-text w/-text for all 17 (a control plain.txt in the same repo came back CRLF), and every SHA-256 matched. Git's heuristic counts the UTF-8 continuation bytes and protobuf tag bytes as non-printable and classifies the files binary. NICE-TO-HAVE: add proto/testdata/*.bin binary to a .gitattributes anyway — the classification is content-dependent, so a future all-ASCII fixture could flip to text and be corrupted silently on a Windows checkout.
2. Will the workflow pass on the PR that first introduces proto/? Yes. The baseline guard (:51) finds no schema on origin/main and skips the breaking step; buf lint is clean; the regen check has the generated trees tracked within that same PR so it compares correctly; both test suites pass. The failure begins on the second PR — see Critical #1.
3. Fixture drift in the other direction. The scheme is sound for value and field changes: a stale fixture fails proto.Equal in Go, and the JS assertions are independent hardcoded values, so regenerating fixtures without updating the JS test fails the JS suite. The only silent path is the one in High #3.
4. Difficulty() fails safe on hostile input. Returns (0, false); bot.Difficulty(0) is not a valid strategy and bot.New rejects it, so even a caller that ignores ok cannot be handed a default opponent. Correct, and correctly justified in the comment.
5. Enum renumbering resistance. Every mapping in convert.go switches on symbols, so reordering the engine's iota blocks cannot change a wire value. This is the property the phase set out to buy, and it holds.
6. int64 as bigint in JS. deadline_unix_ms / client_time_ms surface as bigint, which the test asserts deliberately. Phase 6 will need Number(...) at every countdown site; [jstype = JS_NUMBER] on those fields is an option if that becomes noisy. Informational.

Enum Mapping Correctness (explicit check)

All 8 game.RejectReason and all 4 game.EndReason constants are mapped; no wire value is produced by two engine values; no non-transport wire value is unreachable; ReasonNone and EndNone land on UNSPECIFIED by design and are excluded from the reachability assertion. GAME_END_REASON_OPPONENT_LEFT is correctly allow-listed as transport-only. Nothing is mis-mapped and nothing collapses to UNSPECIFIED where it would be wrong at runtime. The one soft spot is the guard, not the mapping — High #4.

Regression Check (phases 1-3)

• git status --porcelain: only Makefile, README.md, server/go.mod, server/go.sum modified. No file under server/internal/{game,bot,dictionary,vietnamese} or server/cmd/build-dictionary changed. No public contract touched.
• go.mod: + google.golang.org/protobuf v1.36.12 (direct) and tool google.golang.org/protobuf/cmd/protoc-gen-go. No existing require changed, nothing moved between direct and indirect. go 1.25.0 satisfies the 1.24+ tool requirement.
• go.sum: 4 added lines only (protobuf, and go-cmp as its test dependency). No existing hash altered. tool directives are main-module-only, so no dependent inherits protoc-gen-go.
• go vet ./... clean; go test ./... -race all packages ok.
• Accepted deviations (buf STANDARD enum prefixes; minimal npm web/) introduce no inconsistency elsewhere: the JS tsEnum strips the prefix, so RejectReason.WRONG_LINK reads the same on both sides, and the plan/phase files' old unprefixed names appear only in historical plan prose, not in code.

Recommended Actions

1. Fix the buf breaking baseline (.git#ref=origin/main) and align the guard step on the same ref. Blocking — currently the check is inert or red on every PR.
2. Make the regen-diff check see untracked files (git add --intent-to-add, or git status --porcelain), in both proto.yml:67 and Makefile:57. Blocking.
3. Derive clientNames()/serverNames() from the variant tables via WhichOneof.
4. Add reason-count sentinels to the engine and assert against them, so the exhaustiveness guard stops depending on String().
5. Decide WireDifficulty: add GameStarted.difficulty, or delete the helper.
6. Resolve ServerError.code vs .message before anything ships against this schema.
7. Document what protocol_version counts and what its current value is, in the schema.
8. Decide whether the resume snapshot / OpponentReconnected messages land now or in phase 5.
9. Low-priority sweep: .gitattributes for *.bin, broaden CI to go vet ./... && go test ./..., add a concurrency: group, note the make web gap for phase 6.

Metrics

• buf lint: clean, zero carve-outs, STANDARD category, no //buf:lint:ignore anywhere.
• go vet ./...: clean. go test ./... -race: all packages pass.
• internal/wsapi statement coverage: 80.0% (the uncovered statements are the three unreachable-by-design log.Printf default branches and their returns).
• npm test: 23/23 pass (1 file). No JS lint or typecheck configured yet (phase 6).
• buf generate reproducibility: byte-identical on all 3 generated files.
• Hand-written message structs: 0.

Plan Status

Phase 4 implementation steps 1-7 are all present. Success criteria: 5 met, 3 partial (b, e, and the CI half of step 7). I have not edited the plan file — recommend the lead mark phase 4 complete only after Critical #1 and #2, since both success criteria that reference CI are currently unenforced.

Unresolved Questions

1. ServerError.message — is it meant to be a second UI key, structured telemetry detail, or was it left over from the sketch? The answer decides whether the field survives, and it has to be answered before a client ships against this schema.
2. Does the resume flow (phase 5/7) get a state-snapshot message, or will GameStarted be overloaded with history and scores? Both are additive; picking now avoids a second fixture pass.
3. Should proto.yml become the repo's general CI (covering the phase 1-3 suites), or will a separate workflow land later? Right now nothing else in the repo is tested in CI.