feat: generate Zod schemas from codersdk Go types#37
Conversation
|
/coder-agents-review |
|
Chat: Review posted | View chat Review historydeep-review v0.5.0 | Round 4 | Last posted: Round 4, 26 findings (3 P2, 11 P3, 2 P4, 10 Nit), COMMENT. Review Finding inventoryFindings
Contested and acknowledgedCRF-8 (P3, src/codersdk.gen.ts:30) - ChatErrorKindSchema naming collision
CRF-9 (P3, src/codersdk.gen.ts:127) - UUID validation dropped
CRF-13 (P3, src/codersdk.gen.ts:169) - ChatInputPartSchema constraints dropped
CRF-26 (P3, src/codersdk.gen.ts:1) - Enum validation widening
Round logRound 1Netero-only. 2 P2, 1 P3, 1 Nit. Reviewed against 3bb46f0..e870331. Round 2Panel. CRF-1 through CRF-4 addressed. 1 P2, 8 P3, 8 Nit, 2 P4 new. Reviewed against 3bb46f0..85d5619. Round 3Panel. 16 fixed, 3 contested (CRF-8, CRF-9, CRF-13 all panel-closed). 3 new (CRF-24, CRF-25, CRF-26). Reviewed against 3bb46f0..05af06e. Round 4CRF-24, CRF-25 addressed. CRF-26 contested and panel-closed. No open findings. Reviewed against 3bb46f0..006e3b7. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
mafredri
force-pushed
the
mathias/codegen-zod-schemas
branch
from
e870331 to
ddbbc95
Compare
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
First-pass review (Netero). 2 P2, 1 P3, 1 Nit.
This is a first-pass mechanical review only. The full review panel has not yet examined this PR. These are defects the author should address before the panel invests parallel review time.
The codegen approach is sound: guts parses codersdk, walks the AST, emits Zod v4, and a daily CI cron catches drift. The topological sort and transitive reference resolution are well-structured. The test fixtures were cleanly updated for the null-to-optional migration.
Two items need attention before panel review: the Go codegen has no unit tests for 470 lines of non-trivial logic, and the Makefile gen target silently discards error output.
"If the Go codegen fails (compilation error, runtime panic, log.Fatalf), the error message goes to /dev/null." Netero, channeling a developer's future frustration
🤖 This review was automatically generated with Coder Agents.
mafredri
force-pushed
the
mathias/codegen-zod-schemas
branch
from
ddbbc95 to
85d5619
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
Panel review. 1 P2, 8 P3, 2 P4, 8 Nit new findings. All 4 R1 findings cleanly addressed.
Strong work on the codegen architecture. The guts integration, transitive dependency resolution, topological sort, and daily drift CI are well-designed. The R1 test and Makefile fixes were substantive. The null-to-optional migration is semantically correct (Melody traced every nullable/optional chain through Go tags, guts mutations, and generated output).
One blocking item: the package-level currentSerializingType global is shared mutable state that races under t.Parallel(). Six reviewers converged on this independently. The fix is ~10 lines (thread as a parameter).
The remaining P3s are defense-in-depth and future-proofing concerns: heritage clause last-wins, naming collision, UUID validation regression, RawMessage mapping, test coverage gaps, drift recovery UX, and schema constraint widening. None block merging individually, but the UUID validation and RawMessage mapping reduce the parse boundary's ability to catch server bugs.
"Any shared mutable state that coordination primitives don't protect is a race waiting for the next test author to trigger it." Meruem
🤖 This review was automatically generated with Coder Agents.
| gen: src/codersdk.gen.ts | ||
|
|
||
| src/codersdk.gen.ts: scripts/typegen/main.go scripts/typegen/zod.go scripts/typegen/go.mod | ||
| @cd scripts/typegen && go run . > ../../src/codersdk.gen.ts.tmp |
There was a problem hiding this comment.
P3 [CRF-5] bun run format -- src/codersdk.gen.ts > /dev/null 2>&1 || true suppresses stdout, stderr, and exit code. If biome encounters a syntax error in the generated output (codegen bug) or the binary is missing, the gen target succeeds silently with unformatted or broken code. Drop > /dev/null 2>&1 and let || true handle the cosmetic non-zero exit; or redirect only stdout.
(Netero)
🤖
There was a problem hiding this comment.
Fixed. Dropped > /dev/null 2>&1. Format output is now visible; || true handles the cosmetic exit code.
mafredri
force-pushed
the
mathias/codegen-zod-schemas
branch
from
85d5619 to
05af06e
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
Panel re-review. All 3 contested findings (CRF-8, CRF-9, CRF-13) panel-closed by unanimous vote. 16 R2 fixes verified. 2 P3, 1 Nit new.
The contested defenses are solid. Module scope contains the ChatErrorKindSchema collision (4/4). The branded ChatIdSchema covers the only routed UUID (3/3). Schema widening is structural to codegen and the action guards at input level (3/3).
The R2 fixes are substantial: selfName parameter threading eliminates the global, heritage panic guard prevents silent data loss, RawMessage override fixes the type mapping, test coverage reached 62.4%, drift workflow includes recovery steps. Clean execution on all 16.
One new structural observation from Pariston: the codegen introduces strict enum validation on 10 response fields where the old schemas validated 1. Nine of those enums are parsed but never inspected by the action. A new server-side variant in any of them breaks parsing. The daily drift cron mitigates but checks against main, not the deployed version.
"When the server introduces a new error kind,
getChat()fails for any chat whoselast_error.kindcarries that variant. The action only readslast_error.message; it never checkskind." Pariston
src/action.ts:198
P3 [CRF-24] Stale comments reference old schema patterns.
Lines 198-202 describe ?? undefined for .nullable().optional() fields and hasPR gating for .default(0) numerics. The generated schemas use .optional() (not .nullable().optional()) and z.number() (not .default(0)). A maintainer reading these comments would look at the generated schema, not find the described patterns, and be confused. Same stale reference at line 149 ("Zod-default zeros").
Update to describe the current behavior: fields are optional (absent from JSON when unset), and hasPR prevents emitting numeric outputs when no PR exists.
(Netero)
🤖
src/action.test.ts:582
Nit [CRF-25] Test name says "null" but the fixture uses undefined. Same at lines 530 ("pr_number null") and comments at 512 and 542. These describe the old .nullable().optional() semantics.
(Netero)
🤖
🤖 This review was automatically generated with Coder Agents.
| @@ -0,0 +1,323 @@ | |||
| // Code generated by 'make gen'. DO NOT EDIT. | |||
There was a problem hiding this comment.
P3 [CRF-26] Generated schemas validate 10 enum types where the old hand-written schemas validated 1 (ChatStatusSchema). Nine of those enums are parsed but never inspected at runtime.
When the server adds a new variant to any of these 9 enums, .parse() rejects the response even though the action never branches on the changed value. Three sit on required fields (ChatSchema.client_type, UserSchema.status, UserSchema.login_type).
Concrete path: ChatErrorSchema.kind was z.string().optional() in the old code; it is now ChatErrorKindSchema.optional(), an 8-member enum. A new server error kind breaks getChat() for any chat with that error, but the action only reads last_error.message.
The daily drift cron mitigates the window but checks against coder/coder@main, not the deployed server version.
(Pariston P3)
Fix direction: emit z.string() for enum fields the action doesn't inspect, or use .passthrough() on the parent objects, or document the tradeoff.
🤖
There was a problem hiding this comment.
The strict enum validation is a deliberate tradeoff. The daily drift cron catches new server variants within 24 hours. The action is pinned by commit hash, so it doesn't auto-update to a version with mismatched enums. Weakening enums to z.string() would lose the compile-time exhaustiveness checks on ChatStatusSchema (which the action branches on in throwOnChatError). Selectively weakening per-type requires action-specific knowledge in the codegen, defeating the purpose of generating from the SDK.
🤖 Posted using
/amend-reviewskill via Coder Agents.
mafredri
force-pushed
the
mathias/codegen-zod-schemas
branch
from
05af06e to
006e3b7
Compare
|
In reply to CRF-24 (P3, Updated both comments. The In reply to CRF-25 (Nit, Updated test name from
|
|
/coder-agents-review |
There was a problem hiding this comment.
All findings resolved. 26 findings across 4 rounds: 20 fixed, 4 contested and panel-closed, 2 dropped.
CRF-26 (enum validation widening) was the final contested finding. The defense is sound: strict enum validation is a deliberate tradeoff bounded by the daily drift cron, the action is pinned by commit hash, and weakening enums would lose compile-time exhaustiveness on ChatStatusSchema (the one enum the action branches on). Selective weakening per-type reintroduces the manual maintenance this PR eliminates.
The PR is well-executed. The codegen architecture (guts AST walk, transitive resolution, topological sort, Zod v4 serialization) is solid. The author responded to every finding substantively, adding 19 Go tests, threading selfName to eliminate a data race, adding a heritage panic guard, fixing the RawMessage mapping, modernizing to slices.Sorted/slices.Sort, trimming seeds, improving drift UX, and cleaning up stale comments. The contested defenses (naming collision, UUID validation, schema widening, enum strictness) were evaluated by the panel and closed with evidence.
🤖 This review was automatically generated with Coder Agents.
mafredri
force-pushed
the
mathias/codegen-zod-schemas
branch
from
006e3b7 to
f202397
Compare
Replaces hand-maintained Zod schemas with codegen from the codersdk Go types via guts. A daily CI cron checks for drift against coder/coder tip. scripts/typegen/ is a Go program that parses codersdk, walks the guts intermediate AST, and emits Zod v4 code. The generated file (src/codersdk.gen.ts) is committed and rebuilt via `make gen`.
mafredri
force-pushed
the
mathias/codegen-zod-schemas
branch
from
f202397 to
b220753
Compare
2 participants
Replaces hand-maintained Zod schemas with codegen from the codersdk Go types via guts. Prevents schema drift like CODAGT-507.
scripts/typegen/parses codersdk, walks the guts AST, and emits Zod v4 code tosrc/codersdk.gen.ts. Rebuild withmake gen. A daily CI cron (typegen-drift.yaml) runs the generator against coder/coder tip and fails if the output differs.