Go: optional fields are pointers — absence-capability by type, no waivers

[jeremy]

Summary

Closes #436 with the decided Go end-state: every optional field in the generated client can represent absence, classified by type — no zero-value sentinels, no waiver list. Fixes #537 as a verified side effect.

Breaking for external pkg/generated consumers (deliberate, pre-1.0): optional value types are now pointers. The hand-written pkg/basecamp surface keeps its ergonomic value types.

What changed

Generator policy (commit 1):

• go/oapi-codegen.yaml drops the global prefer-skip-optional-pointer: true — optional strings/booleans/numerics/time.Time/types.Date/nested structs generate as pointers (a value type cannot represent absence; omitempty made explicit zeros unsendable on request shapes).
• scripts/enhance-openapi-go-types.sh: ten hand-maintained per-field pointerization passes and waivers collapse into one generic pass — optional non-nullable arrays on response-shaped schemas keep native []T (nil already represents absence). Request-shaped arrays stay pointers so an explicit empty array is sendable (omitempty drops a len-0 slice — the Create* subscriptions nil-vs-[] distinction), and nullable arrays keep the pointer for present-null. The time.Time/types.Date waivers are removed on principle: IsZero() is a zero-value sentinel, not absence.
• The ten required-and-nullable jsonAdd fields (MyNote id/timestamps, Draft parent/scheduled_posting_at, SearchResult content/description, SearchType key, Wormhole color/destination_url) drop their hand-spelled pointer x-go-types — those compensated for the old flag suppressing oapi-codegen's nullable star and stacked to **T once it was removed.

Generated end-state (re-inventoried from the fresh diff): 770 omitempty fields — 711 pointers, 58 native slices, 1 interface{} (ValidationError.Details, where oapi-codegen intentionally never emits *interface{}), zero value-typed leftovers.

Wrapper/test absorption (commit 2): ~670 sites across 41 wrapper files + 6 test files. Reads use deref/derefInt64; writes preserve existing guard structure (&opts.X inside guards, omitzero() for zero-means-unset, ptr() for always-meaningful values); nested generated structs are nil-guarded with pointer presence replacing the old Id != 0 || Name != "" heuristics.

Two live crash bugs found and fixed (Go auto-derefs pointer field access, so gm.Category.Id compiles and then nil-panics at runtime): messages.go Category and events.go Details both crashed on payloads omitting those objects. Caught by test fixtures + a 97-site static sweep of pointer-field accesses; every other site verified guarded. events.go gains a red-proofed regression test (panics without the guard, passes with it).

Guard (commit 3): make go-check-optional-pointers (in make check + CI) classifies every omitempty field in client.gen.go: pointer/slice/map/interface pass, value types fail. No waiver list — the classifier is the policy. Red-proofed by injecting a value-typed violation; ValidationError.Details is the nil-capable negative control. SPEC §10 replaces the waiver-mechanics knob table with the resolved policy; rubric-audit.json 1B.4 → pass with history preserved.

Downstream regen (commit 4), incl. the #537 fix: the Ruby generator's datetime coercion matches the exact x-go-type spelling time.Time, so the previously starred fields were passing through as raw strings. De-starring fixes Draft.scheduled_posting_at + MyNote.created_at/updated_at coercion (Ruby suite 949 runs, 0 failures). The openapi.json change is otherwise Go-only: TS/Ruby/Python/Swift/Kotlin regenerate with zero real drift.

Wire-behavior notes (reviewed intentional improvements)

A handful of previously unconditionally-sent zero-valued query params are now omitted — each was a blank-default or an active hazard:

• search.exclude_chat=false (Rails presence-checks make an explicit "false" string truthy territory), todos.completed=false, everything.*.page=0 (server defaults to 1), timesheet.person_id=0 (a bogus person filter), empty sort=/direction= pairs under compound guards.

Read-side: position: 0 on the wire now maps to &0 instead of nil (positions are 1-based, so unobserved in practice); present-but-empty nested objects (e.g. project.clientside) now yield a non-nil empty wrapper instead of nil — pointer presence is the truth.

Verification

• Full make check green (verified REAL_EXIT=0 from the log, not a mid-run banner) — all six SDKs, conformance suites, all drift gates, the new guard.
• Go: full test suite + vet green; go-check-drift/go-check-wrapper-drift/go-check-generated-drift clean (wrapper drift: 88 pairs, 1155 generated fields verified); conformance runner module builds clean.
• Red proofs: guard (injected violation), events nil-Details regression test (stripped guard → panic), Go replay skip-marker tests unaffected.

Review round (all findings were real)

Guard had two silent holes of its own:

• It matched only tags starting with json:, so every generated query parameter — which tags form: first — was exempt from the policy this PR establishes. Now matches omitempty in any tag: 859 fields checked, up from 770.
• File.foreach inherited a US-ASCII external encoding under a non-UTF-8 locale and raised on client.gen.go's multibyte doc comments, so the guard died on the file it guards. Reads UTF-8 explicitly; verified under LC_ALL=C.

Request-reachability was a name match, now a $ref closure. QuestionSchedule is reached through Create/UpdateQuestion without carrying the RequestContent suffix, so its days array was wrongly kept native — an explicit empty array was unsendable. One schema was affected; the closure makes the class impossible.

Four wrapper sites carried len(x) > 0 guards forward from the value-typed world, where nil and empty were indistinguishable. With pointers they are not, and the guard silently drops an explicit empty array: hill_charts tracked/untracked, schedules CreateEntry participant_ids (UpdateEntry already used != nil — the file disagreed with itself), and questionScheduleToMap days. All now key on nil.

QuestionSchedule.Hour/Minute are *int so absence survives; the deref-then-address pattern manufactured a non-nil zero that reads back as "explicitly midnight". intPtrFrom carries nil through. Regression tests for both presence contracts, red-proofed against the pre-fix code.

#537 fixed at the mechanism, not the symptom. The de-star alone would have left ruby/scripts/generate-types.rb's exact-string x-go-type match in place, so a future *time.Time would regress silently. timestamp_go_type? now normalizes a leading star; types.FlexibleTime is deliberately excluded and documented (it also accepts date-only values — including it would be a behavior change, not a spelling fix). Regression tests cover all three affected fields plus the null case, red-proofed by reconstructing the pre-fix world (exact-match generator + starred spec → raw String → test fails), and the forward claim verified directly (new generator + starred spec still emits parse_datetime).

Synced with current main (#541 field-keyed 422, #557 Ruby pagination) and regenerated; rubric 1B.5 truthed up.

Closes #436. Fixes #537.
🤖 Authored with agent assistance by @jeremy.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 65ea0b91b4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[cubic-dev-ai]

7 issues found across 60 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="go/pkg/basecamp/schedules.go">

<violation number="1" location="go/pkg/basecamp/schedules.go:366">
P2: Creating an entry with `ParticipantIDs: []int64{}` now omits `participant_ids`, so callers cannot send the explicit empty array supported by the generated request type. Preserve nil-vs-empty semantics by checking whether the slice is nil rather than its length.</violation>
</file>

<file name="go/pkg/basecamp/hill_charts.go">

<violation number="1" location="go/pkg/basecamp/hill_charts.go:91">
P1: Callers can no longer send `"tracked": []` to explicitly clear/set an empty tracked list: an empty non-nil slice is omitted by this length guard. Preserve the nil-versus-empty distinction by assigning the pointer whenever the slice is non-nil.</violation>

<violation number="2" location="go/pkg/basecamp/hill_charts.go:94">
P1: Callers can no longer send `"untracked": []`: this guard omits an empty but present slice. Check for nil instead so explicit empty-array requests remain representable.</violation>
</file>

<file name="go/pkg/basecamp/checkins.go">

<violation number="1" location="go/pkg/basecamp/checkins.go:1090">
P2: Responses omitting schedule `hour` or `minute` now expose non-nil pointers to `0`, losing the absence distinction promised by `QuestionSchedule`. Preserve nil unless the corresponding generated pointer is present; explicit zero must remain non-nil.</violation>
</file>

<file name="scripts/enhance-openapi-go-types.sh">

<violation number="1" location="scripts/enhance-openapi-go-types.sh:94">
P2: Generated check-in requests cannot send an explicit empty `schedule.days`: `QuestionSchedule.Days` is a native `[]int32` with `omitempty`, so empty and absent both omit the field. `QuestionSchedule` is request-reachable through CreateQuestion/UpdateQuestion; retain its pointer or derive the response-only exemption from request reachability instead of the schema-name suffix.</violation>
</file>

<file name="scripts/check-go-optional-pointers">

<violation number="1" location="scripts/check-go-optional-pointers:33">
P1: The new optional-pointer guard crashes on the very file it guards. `File.foreach` under a non-UTF-8 locale reads client.gen.go as US-ASCII, and the generated file carries multibyte bytes (e.g. the → arrow in schema descriptions that flow into godoc), so `line.match(...)` at line 33 raises `ArgumentError: invalid byte sequence in US-ASCII` and the script exits non-zero. This breaks both `make go-check-optional-pointers` and `make check`, i.e. the governance gate central to this PR fails at runtime instead of printing the summary. Fix by reading the file as UTF-8, e.g. `File.foreach(path, mode: ‘r:UTF-8’)` (or `line.force_encoding(‘UTF-8’)` before matching).</violation>

<violation number="2" location="scripts/check-go-optional-pointers:33">
P2: This guard currently only matches struct tags that begin with `json:`, so optional query params with `form:` first are skipped. That leaves query fields outside the optional-pointer policy check and can let value-typed regressions pass CI. Matching the full tag and checking for `,omitempty` regardless of tag order would close that gap.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[cubic-dev-ai]

13 issues found across 65 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="go/pkg/basecamp/client_approvals.go">

<violation number="1" location="go/pkg/basecamp/client_approvals.go:243">
P2: A present zero `due_on` is still treated as absent. Presence is now represented by the pointer, so gate only on `ga.DueOn != nil` and preserve any supplied date.</violation>
</file>

<file name="go/pkg/basecamp/tools.go">

<violation number="1" location="go/pkg/basecamp/tools.go:289">
P1: Tool reads panic when the API omits `bucket`, because `gt.Bucket.Id` dereferences the newly optional `*RecordingBucket`. Guard `gt.Bucket != nil` before reading its fields.</violation>
</file>

<file name="go/pkg/basecamp/webhooks.go">

<violation number="1" location="go/pkg/basecamp/webhooks.go:293">
P2: Updating with `Types: []string{}` still omits `types` instead of sending an explicit empty array. Preserve the nil-versus-empty slice distinction when setting this pointer so callers can clear the list under the new optional-array semantics.</violation>

<violation number="2" location="go/pkg/basecamp/webhooks.go:494">
P2: Bio and Location are the only fields in webhookPersonFromGenerated that pass the generated pointer through directly (`p.Bio = gp.Bio`) instead of copying the value, so the returned WebhookEventPerson aliases the source generated.Person's storage. The rest of the mapping goes through deref(), and the sibling change in wormholes.go deliberately copies nullable-string pointers with the comment "Copy the value so the clean type doesn't alias gw" — so this is an inconsistency with the intended contract. If a caller dereferences and mutates p.Bio/p.Location, it will silently corrupt the underlying generated data. Consider copying the pointed-to value (e.g. `v := *gp.Bio; p.Bio = &v`) to match the rest of the mapping.</violation>
</file>

<file name="rubric-audit.json">

<violation number="1" location="rubric-audit.json:17">
P3: The rewritten 1B.4 note marks the criterion Met (pass=true, #436 closed) but preserves verbatim a historical block that still asserts 'REMAINING GAP (why pass stays false) ... tracked in #436' and instructs remediation via the now-removed 'global prefer-skip-optional-pointer: true default'. A reader running go-check-optional-pointers or auditing this entry will hit directly contradictory statements about the same pass flag. Trim the preserved note to the still-relevant Kotlin correction only (or explicitly mark the REMAINING GAP/#436 paragraphs as superseded) so the entry reads consistently with pass=true.</violation>
</file>

<file name="go/pkg/basecamp/schedules.go">

<violation number="1" location="go/pkg/basecamp/schedules.go:368">
P3: CreateEntry's new explicit-empty participant behavior has no regression coverage. Add a wire-level test for `ParticipantIDs: []int64{}` so a future simplification cannot silently omit the field again.</violation>
</file>

<file name="go/pkg/basecamp/checkins.go">

<violation number="1" location="go/pkg/basecamp/checkins.go:1085">
P2: A present schedule without `frequency` is returned as `nil`, losing fields such as explicit `days`, hour, or dates. Preserve the wrapper whenever `gq.Schedule` is non-nil; its zero-valued `Frequency` already represents the absent field ergonomically.</violation>

<violation number="2" location="go/pkg/basecamp/checkins.go:1104">
P2: Explicit zero interval values are lost on reads because these guards use zero as an absence sentinel. Check pointer presence and convert the pointed value, matching the new `Hour`/`Minute` handling.</violation>
</file>

<file name="go/pkg/basecamp/hill_charts.go">

<violation number="1" location="go/pkg/basecamp/hill_charts.go:121">
P2: An absent `updated_at` is exposed as a zero timestamp, so callers cannot distinguish it from a real value and re-marshaling may emit a sentinel timestamp. Preserve this optional timestamp as `*time.Time` in the wrapper and assign `ghc.UpdatedAt` directly.

(Based on your team's feedback about optional timestamps.) [FEEDBACK_USED]</violation>
</file>

<file name="scripts/enhance-openapi-go-types.sh">

<violation number="1" location="scripts/enhance-openapi-go-types.sh:95">
P3: Nullable-array nulls still collapse with absence: `*[]T` decodes both an omitted key and JSON `null` as nil, and cannot marshal an explicit null with `omitempty`. The stated present-null guarantee needs a tri-state nullable wrapper/custom marshal path; otherwise document the intentional collapse.</violation>

<violation number="2" location="scripts/enhance-openapi-go-types.sh:98">
P2: The generator pass that decides which optional arrays stay native []T keys off the request-body naming convention (`RequestContent$`) rather than the actual set of request bodies. Today `QuestionAnswerPayload` and `QuestionAnswerUpdatePayload` are request bodies that don't match that convention and aren't reachable from any `*RequestContent` schema, so they're excluded from `$request_reachable` — and would be treated as response-shaped if they ever carried an optional non-nullable array. The `go-check-optional-pointers` guard won't catch that (it passes any nil-capable `[]T`), so a future request payload could silently regress to an empty-array-unsendable state. Consider deriving the seed set from the actual requestBody `$ref`s in `.paths` rather than from schema names, which makes the closure robust to naming conventions and matches the stated 'request-shaped arrays stay pointers' intent.</violation>
</file>

<file name="scripts/check-go-optional-pointers">

<violation number="1" location="scripts/check-go-optional-pointers:32">
P2: The FIELD regex can silently skip optional fields, defeating the guard. The \A\t anchor requires exactly one leading tab and the type token \S+ cannot match a Go type containing whitespace (e.g. an inline anonymous struct or generic instantiation); such a field parses as no-match, the guard still counts other fields, and total.zero? never fires — so a value-type optional would evade the no-waiver invariant without any signal. Consider loosening the anchor/type capture so unparseable optional fields fail loudly rather than disappearing.</violation>
</file>

<file name="go/pkg/basecamp/cards.go">

<violation number="1" location="go/pkg/basecamp/cards.go:812">
P2: This change makes Position 0 indistinguishable from 'not provided'. For a column move, the position is an explicit destination; a caller that passes 0 (move to the front/highlighted slot) now has it omitted from the payload instead of sent as 0, which can yield different server placement or a missing-field rejection depending on the API. If 0 is a valid destination for a move, keep sending it explicitly (pointer to the value) rather than routing through omitzero, which drops zero by design.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0363b9e50a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[cubic-dev-ai]

4 issues found across 69 files

_{You’re at about 95% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.}

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="scripts/check-go-optional-pointers">

<violation number="1" location="scripts/check-go-optional-pointers:79">
P3: A future optional field using a named map/slice/interface will fail `make go-check-optional-pointers` even though it can represent absence as nil. Resolve named nil-capable declarations (including aliases), or classify by underlying type, before treating all other names as value types.</violation>
</file>

<file name="go/pkg/basecamp/optional_presence_test.go">

<violation number="1" location="go/pkg/basecamp/optional_presence_test.go:80">
P3: The httptest server + client wiring (DefaultConfig/BaseURL/ForAccount/StaticTokenProvider) is copied verbatim into five of the six tests. Extracting one helper that sets up the test server, decodes the request body, and returns a configured service would remove the duplicated boilerplate and make the pointer-presence assertions the only per-test content.</violation>
</file>

<file name="go/pkg/basecamp/timesheet_test.go">

<violation number="1" location="go/pkg/basecamp/timesheet_test.go:490">
P3: The new assertStrParam helper cleanly factors the string-param presence asserts, but PersonId repeats the same nil/expectation branching inline. Consider a parallel assertInt64PtrParam (or make the helper generic over *T) so all three optional params assert presence the same way and the PersonId branch stays as readable as the strings.</violation>
</file>

<file name="scripts/enhance-openapi-go-types.sh">

<violation number="1" location="scripts/enhance-openapi-go-types.sh:56">
P3: This binds the full schema map without using it, adding misleading dead jq state to the array-policy pass. Remove the binding.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8b07b5c9a2

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[cubic-dev-ai]

4 issues found across 70 files

_{You’re at about 99% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.}

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="scripts/check-go-optional-pointers">

<violation number="1" location="scripts/check-go-optional-pointers:82">
P2: Optional fields using named slice/map (or other nil-capable) types are rejected even though nil represents absence, so a valid future generated field will fail this required guard. Track named nil-capable declarations alongside named interfaces before treating a named type as a value.</violation>
</file>

<file name="go/pkg/basecamp/wrapper_propagation_test.go">

<violation number="1" location="go/pkg/basecamp/wrapper_propagation_test.go:63">
P3: Now that Person.CreatedAt/UpdatedAt are pointers, assertCreatorFullyPropagated dereferences them via gp.CreatedAt.Format(...) without a nil guard, so a fixture that omits either field would panic instead of failing the assertion. Every other pointer field in this helper is nil-safe (deref()/Company switch). Consider guarding or using deref for consistency.</violation>
</file>

<file name="go/pkg/basecamp/optional_presence_test.go">

<violation number="1" location="go/pkg/basecamp/optional_presence_test.go:259">
P3: These presence tests only assert empty vs non-empty for the propagated timestamps, so a source swap (e.g., CreatedAt populated from UpdatedAt) would still pass. Consider comparing to the exact expected RFC3339 string ("0001-01-01T00:00:00Z" for a present zero) alongside the presence check to catch wiring mistakes, per the team's timestamp-test guidance.</violation>
</file>

<file name="scripts/enhance-openapi-go-types.sh">

<violation number="1" location="scripts/enhance-openapi-go-types.sh:105">
P2: Generation now rejects an inline request-body schema even though no component schema belongs in this closure. `REQUEST_REACHABLE` is legitimately `[]` in that case, so remove or narrow this abort to avoid blocking enhancement when a future operation uses an inline body.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[jeremy]

Merging: 40 checks passing, zero unresolved threads, and this round produced no new findings.

Sixteen review rounds, which is a lot — worth recording what they actually bought, since almost none of it was the original change:

• Three live nil-deref crashes the compiler could not flag, because Go auto-dereferences pointer field access: messages.go Category, events.go Details, tools.go Bucket. Found by a type-resolved sweep after my first name-based sweep missed the third.
• A wire-behaviour bug fixed on upstream evidence: MoveCardColumn.position is documented required and zero-indexed, so omitzero broke the documented first-slot call. I initially waved this through and was wrong.
• ~25 zero-as-absence sentinels removed across the wrapper layer — len(x) > 0 guards that silently dropped an explicit empty array, and !IsZero() guards that discarded a present zero the pointer was carrying.
• Two guards that certified without checking: the optional-pointer guard skipped every query parameter (they tag form: first) and crashed outright under LC_ALL=C; the enhancer's reachability closure was one-hop and misclassified everything behind an alias chain.

Two things I got wrong and reversed:

• I made HillChart.UpdatedAt a *time.Time on request, then reverted it when a later round showed x.UpdatedAt.IsZero() compiles and panics on nil — a silent break in caller code. Declined three subsequent re-asks for consistency; tracked with the full table and both options in Go wrapper surface types optional timestamps inconsistently (HillChart.UpdatedAt, SearchResult timestamps) #562.
• I broke openapi.json and pushed it: an apostrophe in a jq comment closed the shell string, generation failed, and I missed it by piping to a grep instead of checking the exit status. Restored, verified semantically, and the script now carries an editing note.

Deferred deliberately, not forgotten: #562 (wrapper timestamp types — needs its own release-noted PR), #553 (replay decoder drift).

🤖 Authored with agent assistance by @jeremy.