Go: pointerize the five optional timestamps that could not represent absence

[jeremy]

Closes #562.

⚠️ This is a silent breaking change. Read the migration note.

Five optional timestamps on the hand-written Go wrapper surface move from time.Time to *time.Time. Nothing at your call sites will stop compiling. hc.UpdatedAt.IsZero() compiles exactly as it did before — Go inserts the dereference for a value-receiver method call — and then panics on nil at runtime. No compiler error, just a crash in production the first time the API omits the field.

Type	Field	Before	After
HillChart	UpdatedAt	time.Time `json:"updated_at,omitempty"`	*time.Time `json:"updated_at,omitempty"`
Notification	ReadAt	time.Time `json:"read_at,omitempty"`	*time.Time `json:"read_at,omitempty"`
Notification	UnreadAt	time.Time `json:"unread_at,omitempty"`	*time.Time `json:"unread_at,omitempty"`
SearchResult	CreatedAt	time.Time `json:"created_at"`	*time.Time `json:"created_at,omitempty"`
SearchResult	UpdatedAt	time.Time `json:"updated_at"`	*time.Time `json:"updated_at,omitempty"`

Migration

Nil-check before touching any of the five. Every pattern in the first block below compiles both before and after this change; only the second block is safe after it.

// BEFORE — still compiles after this change, and panics on nil.
if hc.UpdatedAt.IsZero() { … }
fmt.Println(hc.UpdatedAt.Format(time.RFC3339))
age := time.Since(n.ReadAt)
sort.Slice(rs, func(i, j int) bool { return rs[i].CreatedAt.Before(rs[j].CreatedAt) })

// AFTER — nil means "the API did not send this field".
if hc.UpdatedAt == nil { … }                            // absent
if hc.UpdatedAt != nil && hc.UpdatedAt.IsZero() { … }   // present and zero

if hc.UpdatedAt != nil {
    fmt.Println(hc.UpdatedAt.Format(time.RFC3339))
}

if n.ReadAt != nil {
    age := time.Since(*n.ReadAt)
}

sort.Slice(rs, func(i, j int) bool {
    if rs[i].CreatedAt == nil || rs[j].CreatedAt == nil {
        return rs[j].CreatedAt != nil                    // absent sorts last
    }
    return rs[i].CreatedAt.Before(*rs[j].CreatedAt)
})

Two behavioural notes beyond the type:

• Absent no longer reads as the zero time. Code that treated IsZero() as "absent" must switch to == nil. That reading was never sound anyway: it could not distinguish an omitted key from a server that genuinely sent 0001-01-01T00:00:00Z.
• The wire form changes on re-marshal. An absent timestamp previously came back out as "updated_at":"0001-01-01T00:00:00Z"; it is now omitted. If you round-trip these structs through your own storage or golden files, that fabricated instant will stop appearing — which is the point, but it is a diff.

SearchResult additionally gains ,omitempty on both tags. Those two were previously tagged as required while the schema marks them optional and the generated client types them *time.Time; the tags now match reality.

The defect, precisely

All five schemas mark the field optional and the generated client types all five as *time.Time. The wrapper flattened them — through deref() in hillChartFromGenerated / searchResultFromGenerated, and via direct json.Unmarshal for Notification.

That did two things at once:

1. Absence collapsed to the zero time, indistinguishable from a real (if implausible) value.
2. ,omitempty was inert. encoding/json's empty set is false / 0 / nil pointer / nil interface / empty array, slice, map, string. A struct is never empty, and time.Time is a struct — so the key was re-emitted on every marshal, carrying the fabricated 0001-01-01T00:00:00Z.

Point 2 is the boundary that separates timestamps from the rest of this surface. Title string `json:"title,omitempty"` is lossy in a benign way: absent and "" collapse, and both are then omitted, so the value round-trips. A value-typed optional time.Time fabricates data on the wire.

How the five were found — and five more that are not fixed here

The issue body names HillChart.UpdatedAt; its title also names the SearchResult timestamps. Neither grep nor any existing gate can enumerate the class, because the wrapper source does not say the field is optional — that evidence lives in the schema and in the generated client. So the enumeration is a type cross-reference between the two sides:

• parse every non-test go/pkg/basecamp/*.go for struct fields typed exactly time.Time, keyed (struct name, json tag)
• parse go/pkg/generated/client.gen.go the same way
• report every wrapper field whose name-matched, tag-matched generated counterpart is *time.Time

Against this PR's base (a7354ae35) that scan reads 106 value-typed time.Time wrapper fields, of which 10 have an optional generated counterpart, 0 unpaired. It discriminates: generated.Webhook.CreatedAt, generated.Question.CreatedAt and generated.ClientApproval.CreatedAt are value time.Time (genuinely required) and do not appear; EverythingFile.CreatedAt/UpdatedAt are already *time.Time and do not appear.

This PR fixes 5 of those 10. The other 5 are real and are reported, not fixed, in #620:

Struct	Field	Wrapper	Generated
TimelineEvent	CreatedAt	timeline.go:20 — time.Time `json:"created_at"`	*time.Time
WebhookDelivery	CreatedAt	webhooks.go:40 — time.Time `json:"created_at"`	*time.Time
QuestionReminder	RemindAt	checkins.go:143 — time.Time `json:"remind_at"`	*time.Time
ClientApprovalResponse	CreatedAt	client_approvals.go:68 — time.Time `json:"created_at"`	*time.Time
ClientApprovalResponse	UpdatedAt	client_approvals.go:69 — time.Time `json:"updated_at"`	*time.Time

None is @required in spec/basecamp.smithy. Each is a silent break with the same failure mode as the five above, so each needs the same deliberate migration note — which is why they are a separate change rather than a late amendment to this one, not because they are less real.

The five that do ship together ship together because the failure mode is invisible to the compiler: splitting them would ask consumers to hunt for nil-safety three times over, each time with no build error to guide them.

Red proof

go/pkg/basecamp/optional_timestamps_test.go is written so every assertion compiles against both the fixed and un-fixed trees — it asserts on marshaled JSON keys, never on the field types, so nothing had to be edited between the two runs. Against the un-fixed tree, verbatim from go test ./pkg/basecamp/ -run 'TestOptionalTimestamps|TestNilOptionalTimestampPanics|TestNoValueTypedOptionalTimestamps' -v:

=== RUN   TestOptionalTimestampsOmitWhenAbsent
=== RUN   TestOptionalTimestampsOmitWhenAbsent/HillChart
    optional_timestamps_test.go:112: HillChart.updated_at was absent on the wire but re-marshaled as "0001-01-01T00:00:00Z"; a value-typed time.Time cannot represent absence and `,omitempty` never fires for a struct
=== RUN   TestOptionalTimestampsOmitWhenAbsent/Notification
    optional_timestamps_test.go:112: Notification.read_at was absent on the wire but re-marshaled as "0001-01-01T00:00:00Z"; a value-typed time.Time cannot represent absence and `,omitempty` never fires for a struct
    optional_timestamps_test.go:112: Notification.unread_at was absent on the wire but re-marshaled as "0001-01-01T00:00:00Z"; a value-typed time.Time cannot represent absence and `,omitempty` never fires for a struct
=== RUN   TestOptionalTimestampsOmitWhenAbsent/SearchResult
    optional_timestamps_test.go:112: SearchResult.created_at was absent on the wire but re-marshaled as "0001-01-01T00:00:00Z"; a value-typed time.Time cannot represent absence and `,omitempty` never fires for a struct
    optional_timestamps_test.go:112: SearchResult.updated_at was absent on the wire but re-marshaled as "0001-01-01T00:00:00Z"; a value-typed time.Time cannot represent absence and `,omitempty` never fires for a struct
--- FAIL: TestOptionalTimestampsOmitWhenAbsent (0.00s)
    --- FAIL: TestOptionalTimestampsOmitWhenAbsent/HillChart (0.00s)
    --- FAIL: TestOptionalTimestampsOmitWhenAbsent/Notification (0.00s)
    --- FAIL: TestOptionalTimestampsOmitWhenAbsent/SearchResult (0.00s)
=== RUN   TestOptionalTimestampsRoundTripWhenPresent
=== RUN   TestOptionalTimestampsRoundTripWhenPresent/HillChart
=== RUN   TestOptionalTimestampsRoundTripWhenPresent/Notification
=== RUN   TestOptionalTimestampsRoundTripWhenPresent/SearchResult
--- PASS: TestOptionalTimestampsRoundTripWhenPresent (0.00s)
    --- PASS: TestOptionalTimestampsRoundTripWhenPresent/HillChart (0.00s)
    --- PASS: TestOptionalTimestampsRoundTripWhenPresent/Notification (0.00s)
    --- PASS: TestOptionalTimestampsRoundTripWhenPresent/SearchResult (0.00s)
=== RUN   TestOptionalTimestampsSurviveGeneratedConversion
=== RUN   TestOptionalTimestampsSurviveGeneratedConversion/HillChart
    optional_timestamps_test.go:168: HillChart.updated_at: generated value carried a nil timestamp but the wrapper emitted "0001-01-01T00:00:00Z"
=== RUN   TestOptionalTimestampsSurviveGeneratedConversion/SearchResult
    optional_timestamps_test.go:168: SearchResult.created_at: generated value carried a nil timestamp but the wrapper emitted "0001-01-01T00:00:00Z"
    optional_timestamps_test.go:168: SearchResult.updated_at: generated value carried a nil timestamp but the wrapper emitted "0001-01-01T00:00:00Z"
--- FAIL: TestOptionalTimestampsSurviveGeneratedConversion (0.00s)
    --- FAIL: TestOptionalTimestampsSurviveGeneratedConversion/HillChart (0.00s)
    --- FAIL: TestOptionalTimestampsSurviveGeneratedConversion/SearchResult (0.00s)
=== RUN   TestNilOptionalTimestampPanicsOnValueReceiverCall
    optional_timestamps_test.go:191: expected a nil-pointer panic from IsZero() on an absent UpdatedAt: if this line is reached the field is still value-typed and absence reads as 0001-01-01T00:00:00Z
--- FAIL: TestNilOptionalTimestampPanicsOnValueReceiverCall (0.00s)
=== RUN   TestNoValueTypedOptionalTimestamps
    optional_timestamps_test.go:280: optional timestamp is value-typed and cannot represent absence (`,omitempty` is inert for a struct, so the key is emitted as 0001-01-01T00:00:00Z): hill_charts.go:15:2: UpdatedAt time.Time `json:"updated_at,omitempty"`
    optional_timestamps_test.go:280: optional timestamp is value-typed and cannot represent absence (`,omitempty` is inert for a struct, so the key is emitted as 0001-01-01T00:00:00Z): my_notifications.go:46:2: ReadAt time.Time `json:"read_at,omitempty"`
    optional_timestamps_test.go:280: optional timestamp is value-typed and cannot represent absence (`,omitempty` is inert for a struct, so the key is emitted as 0001-01-01T00:00:00Z): my_notifications.go:47:2: UnreadAt time.Time `json:"unread_at,omitempty"`
--- FAIL: TestNoValueTypedOptionalTimestamps (0.03s)
FAIL
FAIL	github.com/basecamp/basecamp-sdk/go/pkg/basecamp	0.310s
FAIL
REAL_EXIT=1

Note what TestNilOptionalTimestampPanicsOnValueReceiverCall actually proves. Pre-fix, hc.UpdatedAt.IsZero() on an absent timestamp does not panic and does not complain — it silently answers true. That silence is the disease. The panic is the post-fix migration hazard, and the test asserts it so the hazard lives in code rather than only in this description.

TestOptionalTimestampsRoundTripWhenPresent is the positive control and passes on both trees, so the fix cannot have been "omit everything".

Note also that the AST guard's red run lists only three of the five — that is not a defect in the run, it is the guard's coverage, and the next section is about exactly that.

Post-fix, all six subtests pass — see verification below.

Why the existing gate did not catch this, and what is still unguarded

scripts/check-go-optional-pointers enforces this invariant on the generated client, so the first question was whether it should have fired. Stating the answer plainly, because an earlier version of this description and a comment on #562 both got it wrong in the direction that hides a bug:

Pointed at the wrapper surface, it would have caught three of the five. HillChart.UpdatedAt, Notification.ReadAt and Notification.UnreadAt are value-typed and carry ,omitempty, which is exactly its violation shape. Run against the pre-fix files it exits 1 on each:

$ scripts/check-go-optional-pointers <pre-fix hill_charts.go>
check-go-optional-pointers: 5 optional field(s) cannot represent absence (non-pointer value type with omitempty):
  hill_charts.go:15: UpdatedAt time.Time
  …
REAL_EXIT=1

$ scripts/check-go-optional-pointers <pre-fix my_notifications.go>
  my_notifications.go:46: ReadAt time.Time
  my_notifications.go:47: UnreadAt time.Time
REAL_EXIT=1

It caught none of them only because make go-check-optional-pointers invokes it with no argument:

path = ARGV[0] || File.expand_path("../go/pkg/generated/client.gen.go", __dir__)

so its entire domain is go/pkg/generated/client.gen.go. That scoping should nonetheless stay: pointed at the whole pre-fix wrapper surface it reports 345 violations, because this package flattens optional *string / *bool / *int to value types on purpose — that is what makes it the "clean" surface.

It could not have caught the other two at any scope. It keys entirely on ,omitempty (next unless m[3].match?(OMITEMPTY)), and SearchResult.CreatedAt/UpdatedAt were tagged json:"created_at" with no options — tagged as if required. Run against the pre-fix search.go it reports four violations and neither timestamp is among them.

So this PR adds TestNoValueTypedOptionalTimestamps: an AST guard over go/pkg/basecamp/*.go rejecting any value-typed time.Time carrying ,omitempty in any tag key. It runs under make go-test — no new Makefile target, no new CI wiring, nothing for a sibling lane to collide with. It asserts it scanned a non-zero number of time.Time fields, so a broken walk fails loudly instead of reporting success forever.

Be clear about what that guard does not do. It inherits the same ,omitempty blind spot (if !hasOmitempty(tag) { continue }), so it would not have caught the two SearchResult fields either, and it does not catch the five holdouts in #620 — all five are tagged without options. It prevents regression of the three-field shape, not of the class.

Two gaps stay open, reported rather than quietly closed:

1. Type-level wrapper/generated drift is unguarded — this is the one that matters. scripts/check-wrapper-drift already computes the (wrapper, generated) struct pairing from the *FromGenerated converters, but compares JSON tag names only. A wrapper field typed time.Time whose generated counterpart is *time.Time passes it cleanly. That is the only place in the tree holding both sides of the comparison, and it is where the 10-field cross-reference above belongs as a permanent gate. Two limits for whoever writes it: Notification has no *FromGenerated converter (direct json.Unmarshal), so converter-derived pairing misses it; and the comparison must be scoped to timestamps or it re-derives the 345-violation wall. Tracked in Five more Go wrapper timestamps cannot represent absence, and no gate can see them #620.

2. Six non-timestamp fields are in the same inert-omitempty class:

   go/pkg/basecamp/account_service.go:28: Limits       AccountLimits       `json:"limits,omitempty"`
   go/pkg/basecamp/account_service.go:29: Settings     AccountSettings     `json:"settings,omitempty"`
   go/pkg/basecamp/account_service.go:30: Subscription AccountSubscription `json:"subscription,omitempty"`
   go/pkg/basecamp/my_assignments.go:24:  Bucket       MyAssignmentBucket  `json:"bucket,omitempty"`
   go/pkg/basecamp/my_assignments.go:25:  Parent       MyAssignmentParent  `json:"parent,omitempty"`
   go/pkg/basecamp/people.go:571:         Person       OutOfOfficePerson   `json:"person,omitempty"`
   

   Named structs, so omitempty is equally inert, and each emits a fully zero-valued nested object instead of being omitted. Less harmful than a fabricated instant — a zero {} reads as obviously empty in a way 0001-01-01T00:00:00Z does not — and outside this lane, so they are named in Five more Go wrapper timestamps cannot represent absence, and no gate can see them #620 rather than swept in.

Review findings, all real, all fixed

1. The SDK's own godoc demonstrated the hazard (chatgpt-codex-connector). ExampleSearchService_Search_sorted called r.CreatedAt.Format(…) with no nil check — the exact unsafe pattern this PR warns about, shipped in the documentation. Fixed in 1fa9c13b4. Reproduced literally first: that expression against a SearchResult{Title: "Kickoff notes"} panics with runtime error: invalid memory address or nil pointer dereference.

make check was green over it because godoc examples without an // Output: comment are compiled but never executed, and because TestNoValueTypedOptionalTimestamps is a declaration guard — it can prove no field is declared with the unsafe shape, but it cannot see an unsafe use.

So rather than fix the one named site and hope, I enumerated the class with the compiler. In a scratch copy all five fields became a defined pointer type with an empty method set:

type probeTime *time.Time   // defined, not an alias: no method set, no selector auto-deref

Every auto-dereferencing use site then becomes a compile error, so the type checker lists them exhaustively. Three passes, neutralizing each batch to reach the next package:

pkg/basecamp/hill_charts_test.go:84:18: hc.UpdatedAt.IsZero undefined (type probeTime has no field or method IsZero)
pkg/basecamp/hill_charts_test.go:87:18: hc.UpdatedAt.Year undefined (type probeTime has no field or method Year)
pkg/basecamp/hill_charts_test.go:88:55: hc.UpdatedAt.Year undefined (type probeTime has no field or method Year)
pkg/basecamp/optional_timestamps_test.go:196:19: hc.UpdatedAt.IsZero undefined (type probeTime has no field or method IsZero)
pkg/basecamp/search_test.go:193:25: r1.CreatedAt.IsZero undefined (type probeTime has no field or method IsZero)
pkg/basecamp/search_test.go:198:25: r1.UpdatedAt.IsZero undefined (type probeTime has no field or method IsZero)

vet: pkg/basecamp/example_test.go:255:26: r.CreatedAt.Format undefined (type basecamp.probeTime has no field or method Format)

$ go vet ./...
REAL_EXIT=0

Seven sites in the whole module; the final pass is clean. Six are deliberately guarded (hill_charts_test.go behind if hc.UpdatedAt == nil, search_test.go behind if … == nil { … } else if, and optional_timestamps_test.go:196 is the intentional nil-deref in the panic test). The seventh was the example. The probe is a one-off technique, not a committed guard — a permanent version needs a type-aware nilness pass, which belongs with the check-wrapper-drift gap above.

2. A doc cross-reference that named a field that does not exist (copilot-pull-request-reviewer). The HillChart.UpdatedAt comment cited TimelineEvent.UpdatedAt as precedent. TimelineEvent (timeline.go:18) has no UpdatedAt; the optional *time.Time pair at timeline.go:85-86 belongs to TimelineAttachment (timeline.go:62). Now cites EverythingFile.UpdatedAt (everything.go:64), re-resolved to its declaring struct rather than assumed. Fixed in 1ec2c7583.

That error came from #562's own table. My correction on the issue then overshot — it called TimelineEvent.CreatedAt "required and value-typed", when structure TimelineEvent carries no @required on created_at and the generated client types it *time.Time. It is value-typed, and it is a holdout, not a counter-example. Re-corrected on the issue and tracked in #620.

3. A planning doc left stale by this change (copilot-pull-request-reviewer, raised in the review body rather than as an inline thread). go/BRIEF-bc5-forward-compat-wrappers.md:147 told the next author to type Notification.BubbleUpAt as *time.Time "rather than the bare time.Time pattern the wrapper uses for ReadAt / UnreadAt" — a counter-example this PR deletes. The advice was right and its only concrete anchor inverted. Now cites Card.CompletedAt (cards.go:101), Todo.CompletedAt (todos.go:52) and the newly-pointerized ReadAt/UnreadAt (my_notifications.go:52-53), each re-resolved to source.

Scope

Go only. No spec change: spec/basecamp.smithy already marks all five optional (no @required on HillChart.updated_at, Notification.read_at / unread_at, SearchResult.created_at / updated_at), and the generated client already types them *time.Time — the wrapper was the only thing out of step. spec/basecamp.smithy and openapi.json are untouched, as is every generated artifact.

No conformance fixture and no runner dispatch: nothing under conformance/runner/** reads any of the five fields, so no runner gains a new case in any of the six.

[jeremy]

npm Audit (TypeScript SDK) is red, and it is not this branch

It is a freshly published advisory hitting main's own lockfile, not anything here.

• This PR's diff is six files, all under go/pkg/basecamp/. git diff --name-only origin/main...HEAD lists no JavaScript, and typescript/package.json / typescript/package-lock.json are byte-identical to origin/main.
• The advisory is GHSA-rgw5-rvv9-x895 — brace-expansion DoS, high, a transitive dep. It was published after main's last CI run (that job went green at 14:46 UTC; this one failed at 17:50 UTC on the same lockfile).
• Running CI's exact command in this worktree, against the lockfile that is identical to main:

$ npm audit --audit-level=high
# npm audit report

brace-expansion  4.0.0 - 5.0.8
Severity: high
brace-expansion: DoS via unbounded intermediate arrays, bypassing the CVE-2026-14257 mitigation - https://github.com/advisories/GHSA-rgw5-rvv9-x895
fix available via `npm audit fix`
node_modules/brace-expansion

1 high severity vulnerability

To address all issues, run:
  npm audit fix
REAL_EXIT=1

Deliberately not fixed here. Bumping the lockfile in a Go-only breaking-change PR would put a dependency change in the wrong review and would conflict with every other branch currently open against main. It wants its own PR (or the dependabot run that will pick it up), and it will be red on main and on every open branch until then.

Every other check on this PR is green.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4abc5d71f6

ℹ️ 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]

Pull request overview

This PR fixes an inconsistency on the hand-written Go wrapper surface (go/pkg/basecamp) where five optional timestamps were typed as value time.Time. Because encoding/json never treats a struct as "empty", ,omitempty was inert for these fields: an absent timestamp was indistinguishable from a real value and re-marshaled as a fabricated 0001-01-01T00:00:00Z. Converting them to *time.Time lets absence round-trip as an omitted key and aligns the wrapper with the spec (all five are optional) and the generated client (all five already *time.Time). This is a deliberate, silent breaking change for consumers (t.IsZero() still compiles against a pointer and panics on nil), scoped to Go only with no spec/generated-artifact changes.

Tip

If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.

Changes:

• Pointerize HillChart.UpdatedAt, Notification.ReadAt/UnreadAt, SearchResult.CreatedAt/UpdatedAt; drop deref(...) in the *FromGenerated converters and add ,omitempty to the two SearchResult tags.
• Add optional_timestamps_test.go: round-trip proofs, a nil-pointer migration-hazard test, and an AST regression guard (TestNoValueTypedOptionalTimestamps) rejecting value-typed time.Time fields carrying ,omitempty.
• Update existing tests to nil-check before dereferencing the now-pointer fields.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
go/pkg/basecamp/hill_charts.go	UpdatedAt → *time.Time; converter drops deref; adds explanatory comment (contains an inaccurate TimelineEvent.UpdatedAt cross-reference).
go/pkg/basecamp/my_notifications.go	ReadAt/UnreadAt → *time.Time with rationale comment; decoded directly via json.Unmarshal, no converter.
go/pkg/basecamp/search.go	CreatedAt/UpdatedAt → *time.Time with ,omitempty; converter assigns generated pointers directly.
go/pkg/basecamp/hill_charts_test.go	Adds nil-check before IsZero() on UpdatedAt.
go/pkg/basecamp/search_test.go	Adds nil-checks before IsZero() on CreatedAt/UpdatedAt.
go/pkg/basecamp/optional_timestamps_test.go	New round-trip, migration-hazard, and AST-guard tests for optional timestamps.

Note: the planning doc go/BRIEF-bc5-forward-compat-wrappers.md:148 still describes ReadAt/UnreadAt as bare time.Time, which is now stale — it is outside this PR's changed regions, so it is flagged here rather than as an inline comment.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

[jeremy]

@codex review

Re-review request at f3b46ae98. Your earlier pass was pinned to 4abc5d71f6; two commits have landed since, both from review findings:

• 34efdfc30 — your example nil-check finding, plus a compile-level sweep (all five fields swapped to a method-less defined pointer type) proving the example was the only unguarded use site in the module.
• f3b46ae98 — a doc cross-reference that named a nonexistent field, found by Copilot.

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. 🎉

Reviewed commit: f3b46ae98a

ℹ️ 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.

[jeremy]

@copilot review

[jeremy]

@codex review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Bravo.

Reviewed commit: c5b1b42858

ℹ️ 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".

[jeremy]

Convergence note — head c5b1b4285

The npm Audit (TypeScript SDK) failure was branch staleness, not this PR. This branch carried "brace-expansion": ">=5.0.7" in typescript/package.json overrides; GHSA-rgw5-rvv9-x895 covers 4.0.0–5.0.8. main had already raised the floor in #616. This PR touches zero TypeScript files, so the fix was to take main's version, not to hand-edit the override or run npm audit fix.

Rebased onto origin/main (a7354ae35, 2 commits ahead). The rebase was a clean replay — the branch never touched Makefile or .github/workflows/test.yml, so no gate-wiring conflict arose; check: still lists 35 targets, and git diff base..HEAD is byte-identical before and after the rebase.

Resolved state, verified in the committed tree:

override:  >=5.0.9
lockfile:  node_modules/brace-expansion -> 5.0.9
installed: 5.0.9
$ npm ci && npm audit --audit-level=high     # CI's exact command
found 0 vulnerabilities
NPM_AUDIT_REAL_EXIT=0

npm Audit (TypeScript SDK) is green at this head.

Full local make check: MAKE_CHECK_REAL_EXIT=0, with PRE_SHA == POST_SHA == c5b1b42858b120b7206597bbf62aca692749923c so the run is attributable to one tree. Swift executed rather than skipped (374 passing test cases in the log). Local Kotlin jvmTest reported UP-TO-DATE, which is not evidence of execution — CI's Kotlin Tests job runs it from a clean checkout and is green here. make check dirtied typescript/package-lock.json with the known macOS libc-stripping churn (#612); the diff was inspected (24 deletions, all libc entries for Linux-only optional deps), restored from HEAD, and not committed.

Beyond the rebase

A third review finding, raised in a review body rather than an inline thread, and so easy to miss at "0 unresolved": copilot-pull-request-reviewer noted that go/BRIEF-bc5-forward-compat-wrappers.md:147 still described ReadAt/UnreadAt as the bare-time.Time counter-example. Pointerizing them here deleted the only concrete anchor of that advice. Fixed in c5b1b4285; see finding 3 in the description.

Five more holdouts of this exact defect class exist and are not fixed here. The description previously implied the sweep was complete. It was not: a type cross-reference between the wrapper and the generated client finds 10 value-typed time.Time wrapper fields whose generated counterpart is *time.Time. This PR fixes 5. The other 5 — TimelineEvent.CreatedAt, WebhookDelivery.CreatedAt, QuestionReminder.RemindAt, ClientApprovalResponse.CreatedAt/UpdatedAt — are reported in #620, not fixed, because each is a separate silent break deserving its own migration note.

Two corrections to things this PR previously asserted, both of which pointed away from a real bug:

1. "scripts/check-go-optional-pointers did not miss these" was wrong. At wrapper scope it would have caught 3 of the 5 (it exits 1 on the pre-fix hill_charts.go and my_notifications.go); it caught none only because make go-check-optional-pointers passes no argument, so its domain is client.gen.go alone. It could not have caught the 2 SearchResult fields at any scope, because it keys on ,omitempty and those carried none. The new TestNoValueTypedOptionalTimestamps inherits that same blind spot — stated plainly in the description now, because a guard that appears to close a class but closes half of it is worse than no guard.
2. My earlier comment on Go wrapper surface types optional timestamps inconsistently (HillChart.UpdatedAt, SearchResult timestamps) #562 called TimelineEvent.CreatedAt "required and value-typed". It is value-typed, but not required — no @required in structure TimelineEvent, and generated.TimelineEvent.CreatedAt is *time.Time. It is a holdout, not a counter-example. Re-corrected on the issue.

Reviews at this head

reviewThreads totalCount: 2, both returned (no pagination truncation), 0 unresolved. Codex re-reviewed the exact head c5b1b42858 — "Didn't find any major issues."

copilot-pull-request-reviewer is red with Copilot encountered an error and was unable to review this pull request — the known infrastructure flake, not a finding. Re-requested once; it errored again. Its last successful pass (18:08:14Z) reviewed 7 of 7 files with no new comments; the only file added since is the BRIEF doc fix, which implements Copilot's own review-body finding. Every other check is green.