Skip to content

(feat): Generate types from upstream openapi spec (5/5) - #14

Open
bdchatham wants to merge 11 commits into
feat/alignment-04-turn-loopfrom
feat/alignment-05-generated-types
Open

(feat): Generate types from upstream openapi spec (5/5)#14
bdchatham wants to merge 11 commits into
feat/alignment-04-turn-loopfrom
feat/alignment-05-generated-types

Conversation

@bdchatham

@bdchatham bdchatham commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator

Stacked on #11. Accepts ADR 0001, merged in #13.

What changed

oapi-codegen produces internal/api, and every public type is a one-line declaration over it. A type with no methods gets an alias, so its doc comment survives and the public name does not move. Each event variant gets a defined type instead, because Go refuses a method on another package's type and the variants carry EventType() and the union's seal.

File Before After
types.go 270 43
session_types.go 939 94
event.go 1283 680
enums.go 151 151
internal/api/api.gen.go 5150 generated

client: false. The generated client was 24,188 lines nothing called, and turning it off dropped none of the three dependencies — the models need runtime.JSONMerge and openapi_types.File on their own. Its default doer is a bare &http.Client{} carrying none of this module's redirect or credential policy.

The public surface moves — read this before approving

An earlier version of this description claimed ten aliased types matched field for field and that a patch release would do. Both were wrong. There are 42 aliases; I had compared ten and generalised. Comparing all 42: 31 identical, 8 differing only as map[string]any against map[string]interface{} (one type, two spellings), and 3 real:

PaginatedList.Data                  []map[string]any -> []interface{}
ReasoningData.Content / .Summary    []map[string]any -> []map[string]string
SessionResponse.TerminalLaunchArgs  gains ,omitempty

In all three the generator is right where the hand-written type was wrong. PaginatedList.data's own description says "Items are heterogeneous … no single concrete type satisfies all callers", which []map[string]any contradicted. So they stay, and each breaks a caller at compile time. gorelease suggests v0.2.0.

Why generation reads a prepared copy

bin/generate.sh runs spec/preprocess.py first, then oapi-codegen over its output. Generating from spec/openapi.json directly compiles and is wrong in every currency field and every optional collection.

Transform Corrects
x-go-type: float64 a formatless number becomes float32
x-go-type-skip-optional-pointer *[]T and *map[K]V on optional collections
closed string to x-go-type: string one wire enum becoming a Go type per containing schema
x-go-name LlmModel, TotalCostUsd, McpStartup
drop additionalProperties: true a catch-all field, and a marshaller that ignores omitempty
schema rename McpServerStartup, SessionMcpStartupEvent
x-go-type: json.RawMessage deferred decode on ConversationItem.data

The script prints what each transform matched and refuses to run when any matches nothing, because a transform that stops matching does not break the build — it returns a type to being wrong the way it was wrong before the transform existed.

skip-prune: true is mandatory, not a preference: oapi-codegen does not read the OAS 3.2 itemSchema keyword, which is the only path from a route to ServerStreamEvent. Pruning therefore drops the whole union without a word.

The catch-all transform is worth a second look

additionalProperties: true on three schemas produced an AdditionalProperties field and a MarshalJSON that tests a field against nil and never reads a struct tag. omitempty stopped working on three public types:

before: {"id":"m1"}
after:  {"id":"m1","supportedReasoningEfforts":[]}

A wire change on a released module, found by the cross-review's dissenter. The transform drops the catch-all, restoring the shape the hand-written types had. Retaining unknown properties is a real improvement and it needs its own change, where the marshalling can be fixed rather than ridden along.

schemaFor finishes its journey here

#10 already cut it from 94 hand-written rows to 7 exceptions by letting a type find its schema by name. This replaces those 7 with a derivation — a type declared over api.Y names schema Y, so the declaration is the mapping — leaving 2 rows that undo the schema renames preprocess.py applies.

before  94 rows, 87 of them saying a name equals itself
#10      7 rows, all of them information
#14      2 rows plus a derivation

The derivation reproduced the original 94-row table exactly, all seven divergences included, which is the evidence it is equivalent rather than merely shorter. Two guards come with it, because an empty derivation would turn the conformance suite green while checking nothing: TestSchemaForIsDerivedFromDeclarations asserts a floor and that every derived schema exists, and TestEverySchemaRenameIsReal fails when a rename stops describing the document.

Gates this fixes

  • TestNoEventTargetCarriesAMethod is new. A defined type does not inherit its underlying type's method set, so an event schema gaining additionalProperties upstream would silently drop what the generated UnmarshalJSON collects. Verified failing by adding a method to a generated event type.
  • The attribution test globbed the root package alone while 483 upstream descriptions moved into internal/api, and its matcher was case-sensitive — so types.go and session_types.go read as carrying nothing and left an Apache-2.0 attribution list. Matcher folds case; both are named again; NOTICE no longer claims the measurement is a proof.
  • The doc-link check went blind at the package boundary, since a scan of this package sees an aliased type's name and none of its members. It follows the alias now.
  • doc.go published nothing above line 141. A blank line ended the comment group, so go doc . opened at LocalServer — an orphan paragraph about a symbol this package deliberately lacks — with 140 lines invisible.
  • git diff --exit-code -- internal/api was blind to an untracked generated file. Now git status --porcelain.
  • bin/generate.sh checked oapi-codegen's presence, not its version, and never checked python3. Both versions now, with a stated 3.10 floor.

One fix that is not about generation

doJSON and doUpload decoded a 2xx body with no io.LimitReader. The 64 KiB cap covers only the drain and the error path, and three types this package decodes carry an open-ended map — measured at 3.8x heap retention on a 400 KB response. One bounded helper for both sites, and a new ErrResponseTooLarge.

Pre-existing rather than introduced. Reviewable on its own if you would rather it were separate — it is one commit, b09e5de, with zero references to anything generated.

Verification

bin/check.sh      gofmt, build, vet, test -race, mod tidy -diff   all pass
golangci-lint     0 issues
staticcheck       clean
vale              0 errors on AGENTS.md, NOTICE, README.md, spec/README.md, the ADR
bin/generate.sh   byte-identical across runs, matches the committed file
gorelease         suggests v0.2.0

Cross-review: 5 blinded reviewers, 12 findings, all closed. Ledger at bdchatham-designs/designs/omnigent-go-sdk-stack/xreview/pr-14.md.

Review focus

The generated file is 5150 of the 6485 added lines. Read spec/preprocess.py, oapi-codegen.yaml, and the test changes — the output follows from those. The ADR's last section records the claims that building this corrected, including that schemaFor's tests did not retire as the ADR predicted, and that one of the two gates it claimed to add already existed.

bdchatham and others added 5 commits August 20, 2026 08:48
feat: core foundation implementation (2/4)
The module tracks spec/openapi.json by hand across four files and 2643 lines.
The snapshot is one event variant behind upstream today, and the suite is green,
which is the defect: the conformance test proves the module declares no field the
document lacks, and by design cannot prove the reverse.

AGENTS.md rules out a code generator, on reasoning about a previous one that
decided the shape of the public surface. This record asks whether that reasoning
still holds when the generated code lands in internal/, where a generated name
cannot reach a consumer, and carries the measurements either way.

Status is Proposed, so the AGENTS.md rule stays as it is, with a pointer so the
rule and the proposal do not silently disagree. A throwaway spike produced the
numbers and nothing here carries its code.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…types

docs: record the wire-type generation question as ADR 0001
Four files tracked spec/openapi.json by hand across 2643 lines, and nothing
proved they still matched it. The conformance suite reads a Go type and asks the
document about that type, so a schema no Go type declares is a schema nothing
checks: a new event variant arrived as UnknownEvent with a green suite.

oapi-codegen now produces internal/api, and the public types are one-line
declarations over it. A type with no methods gets an alias, so its doc comment
survives and the public name does not move. Each event variant gets a defined
type instead, because Go refuses a method on another package's type and the
variants carry EventType() and the union's seal.

Generation reads spec/preprocess.py's output rather than the document. Six
transforms stamp what an OpenAPI document cannot say about Go: a formatless
number is float64 and not float32, an absent collection is a nil slice and not a
pointer to one, MCP is not spelled Mcp, and a closed string stays a string so one
wire enum does not become 81 Go types.

Three gates changed, and each was blind in a way this exposes:

- TestEveryUnionVariantHasAGoType is new. It fails when the document declares an
  event the decoder would return as UnknownEvent, which is the direction the
  field checks cannot cover.
- The generated file reproduces 483 upstream descriptions, and the attribution
  test globbed the root package alone. It reported the attribution as complete
  while the file carrying the prose went unnamed.
- The doc-link check now follows an alias into internal/api. A scan of this
  package alone sees an aliased type's name and none of its members.

schemaFor's 94 hand-written rows are derived now: a type declared over api.Y
names schema Y, so the declaration is the mapping. The derivation reproduces the
old table exactly, including all seven divergences.

CI regenerates and diffs, folded into the lint job rather than added as its own,
because a new job name is advisory until the required-checks list is edited. It
is the only check that catches a new field on an existing schema.

Accepts docs/adr/0001-generate-wire-types-behind-a-facade.md, whose last section
records the four claims that building this corrected.

BREAKING CHANGE: the module now depends on github.com/oapi-codegen/runtime and
the two modules it pulls in, where it previously had none. The public API is
unchanged: all ten aliased types match field for field, type for type and tag for
tag.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@bdchatham bdchatham changed the title feat!: generate the wire types from the spec, behind the facade (5/5) (feat): Generate types from upstream openapi spec (5/5) Aug 20, 2026
bdchatham and others added 2 commits August 20, 2026 14:42
Five blinded reviewers found twelve. The pipeline held — determinism, the union
seal, and the schemaFor derivation all survived attack — and the public-surface
claim did not.

Turn the generated client off. It was 24188 lines nothing called, and it dropped
none of the three dependencies, because the models need runtime.JSONMerge and
openapi_types.File on their own. Its default doer carried none of this module's
redirect or credential policy, so the surface it added was the surface a caller
would most easily pick up by mistake.

Drop the three catch-all fields. additionalProperties: true produced an
AdditionalProperties field and a MarshalJSON that tests a field against nil and
never reads a struct tag, so omitempty stopped working on three public types: an
empty non-nil slice marshalled as [] where it had been absent. That is a wire
change, and the hand-written types declared no catch-all. Retaining unknown
properties is worth having and needs its own change.

Compare all 42 aliases rather than the spike's ten: 31 identical, 8 differing
only as map[string]any against map[string]interface{}, and 3 real. In all three
the generator is right where the hand-written type was wrong, so they stay, and
the record now says the public surface moves rather than claiming it does not.

Refuse a rename that would overwrite a schema, which silently destroyed one and
exited zero. Honour the docstring's setdefault promise in
drop_collection_pointers, which alone assigned directly. Assert the transform
counts, because a transform that stops matching returns a type to being wrong the
way it was wrong before the transform existed.

Fold case in the attribution matcher. The house comment form lowercases the
description's first letter, so a case-sensitive match read types.go and
session_types.go as carrying nothing and they left an Apache-2.0 attribution
list. They carry 9 and 19 descriptions. NOTICE names them again and no longer
claims the measurement is a proof.

Delete TestEveryUnionVariantHasAGoType. TestEveryUnionMemberIsRegistered already
read the discriminator mapping and asserted set equality in both directions; the
surviving test keeps the better doc comment.

Close the blank line at doc.go:141, which ended the comment group and left 140
lines of package documentation unpublished — go doc opened at LocalServer. Correct
the section that still said no code generator runs, AGENTS.md's instruction to
edit a table that derives itself, its test count, spec/README.md's refresh recipe
that never regenerated, and README.md's account of the gate.

Check both tool versions in generate.sh rather than one tool's presence, and use
mktemp -d, which BSD and GNU agree about. Gate on git status --porcelain, since
git diff is blind to an untracked generated file.

Ledger: bdchatham-designs/designs/omnigent-go-sdk-stack/xreview/pr-14.md

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An event variant is a defined type over its generated counterpart, because Go
refuses a method on another package's type and each variant carries Event's two
methods. A defined type does not inherit the underlying type's method set, which
costs nothing while no generated event type has a method — true today, and not a
property of the document.

Give an event schema additionalProperties upstream and the generator emits an
UnmarshalJSON the defined type will not call. DecodeEvent would drop what it
collects and no field check would notice, because the fields still match. So
assert the premise: no api type an event variant is defined over may carry a
method.

Raised by the cross-review's idiom lens as an undocumented, untested invariant.
Verified failing by adding a method to a generated event type.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@bdchatham

Copy link
Copy Markdown
Collaborator Author

Cross-review: 12 findings, 11 closed

Five blinded reviewers on isolated trees — systems (dissenter), platform, security, idiom, prose. The dissenter DISSENTed. Ledger: bdchatham-designs/designs/omnigent-go-sdk-stack/xreview/pr-14.md.

The PR description's headline claim was wrong, and this is the correction. I claimed "all ten aliased types match field for field … the public surface does not move, so this can land as a patch." There are 42 aliases; I compared ten and generalised. Comparing all 42: 31 identical, 8 differing only as map[string]any against map[string]interface{} (one type, two spellings), and 3 real:

PaginatedList.Data                     []map[string]any -> []interface{}
ReasoningData.Content / .Summary       []map[string]any -> []map[string]string
SessionResponse.TerminalLaunchArgs     gains ,omitempty

In all three the generator is right where the hand-written type was wrong — PaginatedList.data's own description says "Items are heterogeneous … no single concrete type satisfies all callers," which []map[string]any contradicted. So they stay. But the public surface does move, and a patch release is the wrong vehicle.

The wire change, found by the dissenter

additionalProperties: true on three schemas produced an AdditionalProperties field and a MarshalJSON that tests a field against nil and never reads a struct tag. omitempty stopped working on three public types:

OLD: {"id":"m1"}
NEW: {"id":"m1","supportedReasoningEfforts":[]}

A seventh transform drops the catch-all, restoring the shape the hand-written types had. Retaining unknown properties is a real improvement and needs its own change, where someone can fix the marshalling rather than ride it along.

client: false

Measured: client: true emitted 24,188 lines nothing called, and turning it off dropped none of the three dependencies — the models need runtime.JSONMerge and openapi_types.File on their own. Its default doer is a bare &http.Client{} carrying none of this module's redirect or credential policy.

Generated file: 29,735 → 5,150 lines. The ADR's "gains the client half at no cost" is corrected.

Two demonstrated bugs in the stage

  • rename_schemas overwrote a colliding schema and exited zero, leaving a $ref resolving to the wrong shape. Now refuses with a named cause.
  • drop_collection_pointers assigned directly while the module docstring promised setdefault for all transforms. Now honours it. The two documented empty-vs-absent sites (terminal_launch_args, config) are recorded as a known gap — fixing them changes the public surface.
  • The seven transform counts are now asserted. A transform that stops matching does not break the build; it returns a type to being wrong the way it was wrong before the transform existed.

An attribution error I made

I removed types.go and session_types.go from NOTICE because the test reported zero upstream descriptions. The matcher was case-sensitive, and the house comment form lowercases the description's first letter — the strings differ from the document by exactly one character. They carry 9 and 19 descriptions. Matcher folds case, both files named again, and NOTICE no longer claims the measurement is a proof.

This was the one reviewer disagreement: prose said the removal was wrong, idiom said it was correct by the test's own definition. Resolved by measuring — the definition was the defect.

doc.go was publishing nothing

Line 141 was blank, which ends the comment group. go doc . opened at LocalServer, an orphan paragraph about a symbol this package deliberately lacks; 140 lines of package documentation were unpublished. Also fixed: the section still saying "No code generator runs, and none is committed," AGENTS.md's instruction to edit a table that derives itself, its test count, spec/README.md's refresh recipe that never regenerated, and README.md's account of the gate.

Gates that were weaker than claimed

  • TestEveryUnionVariantHasAGoType duplicated the pre-existing TestEveryUnionMemberIsRegistered. Deleted; the survivor keeps the better doc comment. The ADR no longer claims two new gates.
  • git diff --exit-code -- internal/api was blind to an untracked generated file. Now git status --porcelain.
  • bin/generate.sh checked oapi-codegen's presence, not its version, and never checked python3. Both versions now checked. mktemp -d, which BSD and GNU agree about.
  • New: TestNoEventTargetCarriesAMethod. A defined type does not inherit its underlying type's method set, so an event schema gaining additionalProperties upstream would silently drop what the generated UnmarshalJSON collects. Verified failing by adding a method to a generated event type.

What held up under attack

Worth recording, because these were the real risks:

  • Determinism. Reproduced independently across varying PYTHONHASHSEED, LC_ALL, LANG, TZ, TMPDIR — identical hash, matching the committed file, idempotent on a second pass.
  • The seal and decode path. 0 of 52 event targets carry a method; nested UnmarshalJSON fires through the alias field; 52 = 52 = 52 across document, registry, and oneOf.
  • The schemaFor derivation. Reproduces the retired 94-row table exactly, all seven divergences included, and resists a renaming alias and a non-schema target.
  • skip-prune: true is genuinely mandatory: with it off, SessionStatusEvent does not generate at all.

One finding still open — needs a decision

client.go:732 decodes a 2xx body with no io.LimitReader. The 64 KiB cap covers the drain and the error path only. Security measured 3.8x heap retention on a 400 KB response, bounded only by the 90s unary timeout. Pre-existing, and in a file this PR does not touch — the change makes it load-bearing rather than creating it. stream.go's maxFrameBytes = 8 << 20 is the precedent. Either a one-line fix here or a follow-up.

Verification

bin/check.sh      all five legs green
golangci-lint     0 issues
staticcheck       clean
vale              0 errors on AGENTS.md, NOTICE, README.md, spec/README.md, the ADR
bin/generate.sh   byte-identical across runs, matches the committed file

doJSON and doUpload decoded a 2xx body with no limit. maxErrorBodyBytes covers
the failure path and the pooling drain, and neither reaches the decode: it reads
until the JSON value ends, so the ceiling was whatever the unary timeout let a
server send. Three types this package decodes carry an open-ended map, which
turns a large body into a larger heap — measured at 3.8x retention on a 400 KB
response.

One helper for both sites, reading one byte past the cap so a body that fills it
is distinguishable from one that ends on it. Over the cap returns
ErrResponseTooLarge and names the limit, because a caller who hits it cannot see
the body and has nothing else to reason from. Under it, the decoder's own error
still surfaces as itself.

The cap is the largest legitimate response with room to spare, not a tuned
number. A session snapshot is the biggest one, since the server returns its
newest 100 items. Downloads take their bound from the caller instead.

Raised by the cross-review's security lens as the one finding left open. It is
pre-existing rather than introduced, and the generated types made it
load-bearing.

Verified by two mutations: raising the cap makes the endless-body test run until
the harness kills it, and removing the over-cap branch turns the failure back
into an opaque unexpected EOF.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@bdchatham

Copy link
Copy Markdown
Collaborator Author

The last open finding is closed

client.go:732 and the same decode in doUpload now go through one bounded helper. maxResponseBytes = 32 << 20, read one byte past so a body that fills the cap is distinguishable from one that ends on it. Over the cap returns a new ErrResponseTooLarge naming the limit; under it, the decoder's own error still surfaces as itself.

Verified by two mutations rather than by reading:

  • Raise the cap and the endless-body test runs until the harness kills it. That hang is the exposure the bound closes.
  • Remove the over-cap branch and the failure reverts to an opaque decode GET /v1/sessions/s response: unexpected EOF, which is the reason the branch exists rather than just the LimitReader.

Three tests: the refusal, a 1 MiB body still decoding, and a short malformed body reporting its own error instead of being called too large.

Release note, since the PR description got this wrong

gorelease suggests v0.2.0, not a patch. Two caveats on reading its output: it compares against v0.1.2, which predates the whole rebuild, so (*Client).CreateSession: removed and the enum changes it lists belong to PRs 1–4 rather than this one.

This PR's own public delta is the three field changes measured directly: PaginatedList.Data to []interface{}, ReasoningData.Content/.Summary to []map[string]string, and SessionResponse.TerminalLaunchArgs gaining omitempty. Each is the generator correcting a type the hand-written version got wrong, and each breaks a caller at compile time.

bin/check.sh      all five legs green
golangci-lint     0 issues
staticcheck       clean

All 12 cross-review findings are now closed.

bdchatham and others added 2 commits August 21, 2026 07:54
…o feat/alignment-05-generated-types

# Conflicts:
#	AGENTS.md
#	conformance_test.go
Three numbers described the document rather than the code, so they were true on
the day they were written and no longer once upstream moves. "Twelve currency
fields and seventy-one collections" appeared twice; both now say every currency
field and every optional collection, and point at the counts bin/generate.sh
prints, which cannot go stale because the script measures them.

"Drops all 53 variants" said variants where it meant schemas: the union has 52
members, and the 53rd is the union schema itself. It now names both without
counting either.

Also resolves the merge from PR 10: the conformance dimension list keeps
mapping-complete-in-both-directions, which holds again here because the
derivation restores what an identity default made unfailable.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…o feat/alignment-05-generated-types

# Conflicts:
#	doc.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant