Go wrapper surface types optional timestamps inconsistently (HillChart.UpdatedAt, SearchResult timestamps)

[jeremy]

Split out of #560's review, where changing one field drive-by proved to be the wrong venue.

The hand-written Go wrapper surface is inconsistent about how it types optional timestamps:

Type	Field	Go type
Card	CompletedAt	*time.Time
TimelineEvent	CreatedAt, UpdatedAt	*time.Time
EverythingFile	CreatedAt, UpdatedAt	*time.Time
HillChart	UpdatedAt	time.Time

All four schemas mark the field optional, so HillChart.UpdatedAt is the outlier: an absent updated_at surfaces as 0001-01-01T00:00:00Z, indistinguishable from a real (if implausible) value. Its omitempty is also inert — encoding/json never omits a zero time.Time, so the key is always emitted on re-marshal.

Why this was NOT fixed in #560, and why it needs its own PR: flipping the field to *time.Time is a silent breaking change for callers. hc.UpdatedAt.IsZero() still compiles — Go auto-dereferences the pointer receiver — and then panics at runtime on nil. Verified with a standalone repro. A change with that failure mode needs to be deliberate, announced in release notes, and ideally landed with the other wrapper-surface breaks rather than smuggled into a generated-types PR.

Options to decide:

1. Make HillChart.UpdatedAt a *time.Time (consistent with the other three; breaking, needs a release note).
2. Leave it value-typed and accept that absent reads as the zero time, documenting it — but then the omitempty tag should go, since it never fires.

Worth resolving alongside any future audit of value-typed optional fields on the hand-written surface generally; this issue is the timestamp instance, not necessarily the whole class.

🤖 Drafted with agent assistance; filed by @jeremy.

[jeremy]

Widening this from HillChart.UpdatedAt to the class, after a second instance surfaced in review.

SearchResult.created_at/updated_at are optional in the schema (required is app_url, content, description, id, title, type, url), but the wrapper types them as value time.Time, so an absent timestamp decodes to 0001-01-01T00:00:00Z and re-marshals as a value the API never sent.

Current state of the class:

Type	Field	Go type	Optional in schema
Card	CompletedAt	*time.Time	yes
TimelineEvent	CreatedAt, UpdatedAt	*time.Time	yes
EverythingFile	CreatedAt, UpdatedAt	*time.Time	yes
HillChart	UpdatedAt	time.Time	yes
SearchResult	CreatedAt, UpdatedAt	time.Time	yes

So the pointer form is already the majority convention and the two value-typed holdouts are the outliers — but the migration is a silent break in both cases: x.UpdatedAt.IsZero() keeps compiling (Go auto-dereferences the pointer receiver) and panics at runtime on nil. Verified with a standalone repro.

That is why #560 does not do it: a change whose failure mode is "compiles, then crashes in the caller's code" needs to be deliberate and announced, not folded into a generated-types PR. The options are unchanged — pointerize both for consistency (breaking, release note), or keep them value-typed and drop the inert omitempty (it never fires for a time.Time) while documenting the collapse.

Worth doing as one sweep across the whole class rather than field by field.

[jeremy]

Correction to the table in the description, surfaced while implementing this in #615.

The row

| TimelineEvent | CreatedAt, UpdatedAt | *time.Time |

is wrong on both counts. TimelineEvent (go/pkg/basecamp/timeline.go:18) declares CreatedAt time.Time — required and value-typed — and has no UpdatedAt member at all. The optional *time.Time pair at timeline.go:85-86 belongs to TimelineAttachment (timeline.go:62). The other three rows check out: Card.CompletedAt (cards.go:101), EverythingFile.CreatedAt/UpdatedAt (everything.go:63-64), and HillChart.UpdatedAt are as described.

I copied the row into a doc comment before re-deriving it from source; copilot-pull-request-reviewer caught it on the PR. Noting it here so the next reader of this table is not misled.

Two other findings from the sweep, both in the PR description:

• The holdout set is five fields, not one. Beyond HillChart.UpdatedAt, Notification.ReadAt and Notification.UnreadAt carry the same inert-omitempty shape, and SearchResult.CreatedAt/UpdatedAt are value-typed while the schema marks them optional and their tags claim required. All five ship together in Go: pointerize the five optional timestamps that could not represent absence #615 so consumers migrate once.
• scripts/check-go-optional-pointers did not miss these. Its domain is go/pkg/generated/client.gen.go only; the hand-written wrapper surface has never been in scope, correctly so. The real gap is that scripts/check-wrapper-drift pairs wrapper and generated structs but compares JSON tag names, never types — so a time.Time wrapper field against a *time.Time generated field passes cleanly.

[jeremy]

Correcting my own correction above, which was half wrong in the direction that hides a bug.

I wrote that the TimelineEvent row "is wrong on both counts" because CreatedAt is "required and value-typed". Value-typed, yes. Required, no — and that half mattered.

• spec/basecamp.smithy, structure TimelineEvent: created_at: ISO8601Timestamp carries no @required. It is optional.
• generated.TimelineEvent.CreatedAt is *time.Time `json:"created_at,omitempty"`.
• TimelineEvent.CreatedAt (go/pkg/basecamp/timeline.go:20) is time.Time `json:"created_at"`.

So TimelineEvent.CreatedAt is not an example of the good convention, and it is not a required field either — it is another instance of exactly the defect this issue is about, in the same shape as SearchResult.CreatedAt: optional in the schema, pointer in the generated client, value-typed in the wrapper under a tag claiming it is always present.

What stands from the earlier comment: TimelineEvent has no UpdatedAt member (that half of the row is genuinely wrong), the optional pair at timeline.go:85-86 belongs to TimelineAttachment, and Card.CompletedAt / EverythingFile.CreatedAt/UpdatedAt do check out as *time.Time.

Also withdrawing this line: "scripts/check-go-optional-pointers did not miss these." Pointed at the wrapper surface it would have flagged three of the five in #615 — HillChart.UpdatedAt, Notification.ReadAt, Notification.UnreadAt all carry ,omitempty and are value-typed. It flagged none only because make go-check-optional-pointers runs it with no argument, so its domain is go/pkg/generated/client.gen.go alone. It could never have flagged SearchResult.CreatedAt/UpdatedAt at any scope, because it keys on ,omitempty and those two carry none.

A type cross-reference between the wrapper and the generated client — the check no gate performs today — finds 10 such fields at #615's base. #615 fixes 5. The other 5 are now tracked in #620, with the gate gap.

🤖 Drafted with agent assistance; posted by @jeremy.