From 972a466330d629129ee853b94bbae0f3575fc578 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Tue, 4 Aug 2026 11:00:45 -0700 Subject: [PATCH 1/2] Go: five wrapper timestamps could not represent absence (#620) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BREAKING. Five hand-written wrapper fields were typed value time.Time while their generated counterparts were *time.Time: TimelineEvent.CreatedAt timeline.go WebhookDelivery.CreatedAt webhooks.go QuestionReminder.RemindAt checkins.go ClientApprovalResponse.CreatedAt client_approvals.go ClientApprovalResponse.UpdatedAt client_approvals.go An omitted timestamp decoded to the zero time and re-marshaled as a fabricated 0001-01-01T00:00:00Z, indistinguishable from a real value. None of the five carried `,omitempty` — and it would not have helped: encoding/json's empty set has no entry for a struct, so `,omitempty` on a value-typed time.Time is inert. Same class as #562/#615, which fixed five and left these five. The converters drop their deref() and assign the pointer through. Also closes the class, which is the half #620's title names — "and no gate can see them". The evidence lives in the generated client, not the wrapper, so a guard has to read both sides: - TestNoValueTypedOptionalTimestamps keys on `,omitempty` and skips without it, so all five were invisible to it. Its doc comment named this gap and deferred it. - check-wrapper-drift has the pairing but compares tag names only, and teaching it types would still miss WebhookDelivery: its header excludes the webhook-flavored shapes BY DESIGN. TestNoWrapperTimestampNarrowerThanGenerated pairs by struct name + json key instead, which is blind to the converter tiers and covers all five. Scoped to timestamps: a nil-capability rule applied broadly reports ~345 deliberately flattened fields. An absent string marshals away harmlessly; an absent time.Time marshals as a wrong value. Red-proven against the un-fixed source: all five fail the absent-omit test reporting the fabricated instant, and all five fail the new guard — including WebhookDelivery, the one no gate could reach. --- go/pkg/basecamp/checkins.go | 13 +- go/pkg/basecamp/client_approvals.go | 37 ++-- go/pkg/basecamp/optional_timestamps_test.go | 191 +++++++++++++++++++- go/pkg/basecamp/timeline.go | 9 +- go/pkg/basecamp/webhooks.go | 9 +- 5 files changed, 228 insertions(+), 31 deletions(-) diff --git a/go/pkg/basecamp/checkins.go b/go/pkg/basecamp/checkins.go index 7de3044955..0dfd4a1970 100644 --- a/go/pkg/basecamp/checkins.go +++ b/go/pkg/basecamp/checkins.go @@ -144,10 +144,13 @@ type QuestionAnswer struct { // QuestionReminder represents a pending check-in reminder for the current user. type QuestionReminder struct { - GroupOn string `json:"group_on,omitempty"` - Question Question `json:"question"` - RemindAt time.Time `json:"remind_at"` - ReminderID *int64 `json:"reminder_id,omitempty"` + GroupOn string `json:"group_on,omitempty"` + Question Question `json:"question"` + // RemindAt is optional — nil means the API did not send it. A value type + // here would fabricate 0001-01-01T00:00:00Z for an absent timestamp, which + // no consumer can tell from a real one. + RemindAt *time.Time `json:"remind_at,omitempty"` + ReminderID *int64 `json:"reminder_id,omitempty"` } // QuestionNotificationSettings represents the current user's notification @@ -1239,7 +1242,7 @@ func questionAnswerFromGenerated(ga generated.QuestionAnswer) QuestionAnswer { // questionReminderFromGenerated converts a generated QuestionReminder to our clean type. func questionReminderFromGenerated(gr generated.QuestionReminder) QuestionReminder { r := QuestionReminder{ - RemindAt: deref(gr.RemindAt), + RemindAt: gr.RemindAt, ReminderID: gr.ReminderId, } diff --git a/go/pkg/basecamp/client_approvals.go b/go/pkg/basecamp/client_approvals.go index 802ffc16ac..550b622892 100644 --- a/go/pkg/basecamp/client_approvals.go +++ b/go/pkg/basecamp/client_approvals.go @@ -62,21 +62,24 @@ type ClientApproval struct { // ClientApprovalResponse represents a response to a client approval. type ClientApprovalResponse struct { - ID int64 `json:"id"` - Status string `json:"status"` - VisibleToClients bool `json:"visible_to_clients"` - CreatedAt time.Time `json:"created_at"` - UpdatedAt time.Time `json:"updated_at"` - Title string `json:"title"` - InheritsStatus bool `json:"inherits_status"` - Type string `json:"type"` - AppURL string `json:"app_url"` - BookmarkURL string `json:"bookmark_url"` - Parent *Parent `json:"parent,omitempty"` - Bucket *Bucket `json:"bucket,omitempty"` - Creator *Person `json:"creator,omitempty"` - Content string `json:"content"` - Approved bool `json:"approved"` + ID int64 `json:"id"` + Status string `json:"status"` + VisibleToClients bool `json:"visible_to_clients"` + // CreatedAt and UpdatedAt are optional — nil means the API did not send + // them. Value types here would fabricate 0001-01-01T00:00:00Z for an absent + // timestamp, which no consumer can tell from a real one. + CreatedAt *time.Time `json:"created_at,omitempty"` + UpdatedAt *time.Time `json:"updated_at,omitempty"` + Title string `json:"title"` + InheritsStatus bool `json:"inherits_status"` + Type string `json:"type"` + AppURL string `json:"app_url"` + BookmarkURL string `json:"bookmark_url"` + Parent *Parent `json:"parent,omitempty"` + Bucket *Bucket `json:"bucket,omitempty"` + Creator *Person `json:"creator,omitempty"` + Content string `json:"content"` + Approved bool `json:"approved"` } // ClientApprovalListResult contains the results from listing client approvals. @@ -290,8 +293,8 @@ func clientApprovalFromGenerated(ga generated.ClientApproval) ClientApproval { resp := ClientApprovalResponse{ Status: deref(gr.Status), VisibleToClients: deref(gr.VisibleToClients), - CreatedAt: deref(gr.CreatedAt), - UpdatedAt: deref(gr.UpdatedAt), + CreatedAt: gr.CreatedAt, + UpdatedAt: gr.UpdatedAt, Title: deref(gr.Title), InheritsStatus: deref(gr.InheritsStatus), Type: deref(gr.Type), diff --git a/go/pkg/basecamp/optional_timestamps_test.go b/go/pkg/basecamp/optional_timestamps_test.go index f158b41904..f626d644af 100644 --- a/go/pkg/basecamp/optional_timestamps_test.go +++ b/go/pkg/basecamp/optional_timestamps_test.go @@ -78,6 +78,34 @@ func timestampCarriers() []timestampCarrier { present: []byte(`{"id":1,"created_at":"2026-07-31T12:34:56Z","updated_at":"2026-07-30T01:02:03Z",` + `"content":null,"description":null}`), }, + // #620: the five holdouts of the same class. Each was typed value + // time.Time with a bare `json:"..."` tag — no omitempty to even hint the + // field was optional — while its generated counterpart was *time.Time. + { + name: "TimelineEvent", + decode: unmarshalInto[TimelineEvent], + keys: []string{"created_at"}, + present: []byte(`{"id":1,"created_at":"2026-07-31T12:34:56Z","kind":"todo_created"}`), + }, + { + name: "WebhookDelivery", + decode: unmarshalInto[WebhookDelivery], + keys: []string{"created_at"}, + present: []byte(`{"id":1,"created_at":"2026-07-31T12:34:56Z"}`), + }, + { + name: "QuestionReminder", + decode: unmarshalInto[QuestionReminder], + keys: []string{"remind_at"}, + present: []byte(`{"remind_at":"2026-07-31T12:34:56Z"}`), + }, + { + name: "ClientApprovalResponse", + decode: unmarshalInto[ClientApprovalResponse], + keys: []string{"created_at", "updated_at"}, + present: []byte(`{"id":1,"created_at":"2026-07-31T12:34:56Z",` + + `"updated_at":"2026-07-30T01:02:03Z","approved":true}`), + }, } } @@ -209,9 +237,12 @@ func TestNilOptionalTimestampPanicsOnValueReceiverCall(t *testing.T) { // time.Time fields carrying `,omitempty`. A value-typed time.Time with NO // omitempty whose generated counterpart is a pointer — the shape // SearchResult.CreatedAt had — is NOT detectable from this package's source -// alone; catching that class needs the (wrapper, generated) pairing that -// scripts/check-wrapper-drift already computes for tag names but not for -// types. That gap is reported on the PR, not closed here. +// alone; catching that class needs a (wrapper, generated) pairing. +// +// That gap is now closed by TestNoWrapperTimestampNarrowerThanGenerated below, +// which reads both sides and pairs them by struct name + json key (#620). This +// guard stays: it catches an inert `,omitempty` on a wrapper struct that has no +// generated counterpart at all, which the cross-reference cannot see. func TestNoValueTypedOptionalTimestamps(t *testing.T) { entries, err := os.ReadDir(".") if err != nil { @@ -282,6 +313,160 @@ func TestNoValueTypedOptionalTimestamps(t *testing.T) { } } +// timestampField is one (struct, json key) timestamp declaration, recorded from +// either the wrapper surface or the generated client. +type timestampField struct { + pointer bool + field string + file string +} + +// collectTimestampFields keys every time.Time / *time.Time struct field by +// (struct name, json key). The json key is the pairing axis rather than the Go +// field name: the wrapper renames fields (Id → ID) but the wire key is the +// thing both sides must agree about. +func collectTimestampFields(t *testing.T, files map[string]*ast.File) map[[2]string]timestampField { + t.Helper() + out := map[[2]string]timestampField{} + for path, file := range files { + ast.Inspect(file, func(n ast.Node) bool { + ts, ok := n.(*ast.TypeSpec) + if !ok { + return true + } + st, ok := ts.Type.(*ast.StructType) + if !ok { + return true + } + for _, f := range st.Fields.List { + if f.Tag == nil || len(f.Names) == 0 { + continue + } + value := isValueTime(f.Type) + star, isStar := f.Type.(*ast.StarExpr) + pointer := isStar && isValueTime(star.X) + if !value && !pointer { + continue + } + raw, err := strconv.Unquote(f.Tag.Value) + if err != nil { + continue + } + jsonTag, ok := reflect.StructTag(raw).Lookup("json") + if !ok { + continue + } + key := strings.Split(jsonTag, ",")[0] + if key == "" || key == "-" { + continue + } + out[[2]string{ts.Name.Name, key}] = timestampField{ + pointer: pointer, + field: f.Names[0].Name, + file: filepath.Base(path), + } + } + return true + }) + } + return out +} + +func parseGoFiles(t *testing.T, paths []string) map[string]*ast.File { + t.Helper() + fset := token.NewFileSet() + out := map[string]*ast.File{} + for _, p := range paths { + f, err := parser.ParseFile(fset, p, nil, 0) + if err != nil { + t.Fatalf("parse %s: %v", p, err) + } + out[p] = f + } + return out +} + +// TestNoWrapperTimestampNarrowerThanGenerated closes the class #620 named. +// +// A wrapper field typed value time.Time whose generated counterpart is +// *time.Time cannot represent absence, and — this is what makes it invisible — +// NOTHING IN THE WRAPPER SOURCE SAYS SO. The evidence lives in the generated +// client, so the guard has to read both sides. +// +// Why the two neighbouring gates cannot do this: +// +// - TestNoValueTypedOptionalTimestamps (above) keys on `,omitempty` and +// `continue`s without it. All five #620 fields were tagged bare +// `json:"created_at"`, so they were invisible to it. Its own doc comment +// named this gap and deferred it; this is the close. +// - scripts/check-wrapper-drift has the (wrapper, generated) pairing but +// compares tag NAMES only. Teaching it types would still miss +// WebhookDelivery, which its header excludes BY DESIGN as a parallel +// webhook-flavored shape — and WebhookDelivery.CreatedAt is one of the five. +// +// So the pairing here is by struct name + json key, which is blind to the +// converter tiers and therefore covers all of them. +// +// Scoped to timestamps deliberately. The wrapper flattens optional +// *string/*bool/*int to value types on purpose; a nil-capability rule applied +// broadly reports ~345 intentional fields. Timestamps are the principled +// carve-out: an absent string marshals away harmlessly, an absent time.Time +// marshals as a wrong value. +func TestNoWrapperTimestampNarrowerThanGenerated(t *testing.T) { + entries, err := os.ReadDir(".") + if err != nil { + t.Fatalf("read package dir: %v", err) + } + var wrapperPaths []string + for _, e := range entries { + name := e.Name() + if e.IsDir() || !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { + continue + } + wrapperPaths = append(wrapperPaths, name) + } + + wrapper := collectTimestampFields(t, parseGoFiles(t, wrapperPaths)) + gen := collectTimestampFields(t, parseGoFiles(t, []string{ + filepath.Join("..", "generated", "client.gen.go"), + })) + + if len(wrapper) == 0 || len(gen) == 0 { + t.Fatalf("collected %d wrapper and %d generated timestamp fields — the walk is broken, not the surface", + len(wrapper), len(gen)) + } + + var ( + violations []string + compared int + ) + for key, w := range wrapper { + g, ok := gen[key] + if !ok { + continue + } + compared++ + if w.pointer || !g.pointer { + continue + } + violations = append(violations, fmt.Sprintf( + "%s.%s (json:%q, %s) is time.Time but generated.%s.%s is *time.Time", + key[0], w.field, key[1], w.file, key[0], g.field)) + } + + // A name-keyed join that matches nothing passes vacuously. + if compared == 0 { + t.Fatal("zero (wrapper, generated) timestamp fields paired — the join key is wrong, not the surface") + } + + sort.Strings(violations) + for _, v := range violations { + t.Errorf("wrapper timestamp is narrower than the schema it decodes: %s. "+ + "An omitted value decodes to the zero time and re-marshals as a fabricated "+ + "0001-01-01T00:00:00Z, indistinguishable from a real timestamp", v) + } +} + // isValueTime reports whether expr is the type `time.Time` exactly — not // *time.Time, not []time.Time, not a named alias. func isValueTime(expr ast.Expr) bool { diff --git a/go/pkg/basecamp/timeline.go b/go/pkg/basecamp/timeline.go index 263a9d1273..f7da389562 100644 --- a/go/pkg/basecamp/timeline.go +++ b/go/pkg/basecamp/timeline.go @@ -16,8 +16,11 @@ const DefaultTimelineLimit = 100 // TimelineEvent represents an activity event in the timeline. type TimelineEvent struct { - ID int64 `json:"id"` - CreatedAt time.Time `json:"created_at"` + ID int64 `json:"id"` + // CreatedAt is optional — nil means the API did not send it. A value type + // here would fabricate 0001-01-01T00:00:00Z for an absent timestamp, which + // no consumer can tell from a real one. + CreatedAt *time.Time `json:"created_at,omitempty"` // Kind is an open, non-exhaustive vocabulary (BC3 adds new kinds over time); // treat unrecognized values as valid. Common values include message_created, // todo_created, todo_completed, upload_created, schedule_entry_created, @@ -497,7 +500,7 @@ func timelineEventFromGenerated(ge generated.TimelineEvent) TimelineEvent { e.ParentRecordingID = *ge.ParentRecordingId } - e.CreatedAt = deref(ge.CreatedAt) + e.CreatedAt = ge.CreatedAt if ge.Creator != nil { creator := personFromGenerated(*ge.Creator) diff --git a/go/pkg/basecamp/webhooks.go b/go/pkg/basecamp/webhooks.go index 2b5c247042..aa0d6c4bff 100644 --- a/go/pkg/basecamp/webhooks.go +++ b/go/pkg/basecamp/webhooks.go @@ -36,8 +36,11 @@ type Webhook struct { // WebhookDelivery represents a recent delivery attempt for a webhook. type WebhookDelivery struct { - ID int64 `json:"id"` - CreatedAt time.Time `json:"created_at"` + ID int64 `json:"id"` + // CreatedAt is optional — nil means the API did not send it. A value type + // here would fabricate 0001-01-01T00:00:00Z for an absent timestamp, which + // no consumer can tell from a real one. + CreatedAt *time.Time `json:"created_at,omitempty"` Request WebhookDeliveryRequest `json:"request"` Response WebhookDeliveryResponse `json:"response"` } @@ -362,7 +365,7 @@ func webhookFromGenerated(gw generated.Webhook) Webhook { w.RecentDeliveries = make([]WebhookDelivery, len(gw.RecentDeliveries)) for i, gd := range gw.RecentDeliveries { d := WebhookDelivery{ - CreatedAt: deref(gd.CreatedAt), + CreatedAt: gd.CreatedAt, } if gd.Request != nil { d.Request = WebhookDeliveryRequest{ From 71805573f64befbee0f0ed09066baf002e1a0599 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Tue, 4 Aug 2026 12:00:13 -0700 Subject: [PATCH 2/2] Guard the three pointer derefs this branch left in its own tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Making the five wrapper timestamps *time.Time left three assertions in this package's tests dereferencing them unguarded: checkins_test.go:1364 r1.RemindAt.IsZero() webhooks_test.go:177 delivery.CreatedAt.IsZero() timeline_test.go:81 !event.CreatedAt.Equal(expectedTime) They compile — Go auto-dereferences for a value receiver — and pass only because the fixtures happen to carry the key. Drop it from a fixture and they panic, taking the whole test binary down instead of failing with a message naming the field. That is exactly the hazard the branch documents for consumers. Require non-nil first, then assert what was being asserted. --- go/pkg/basecamp/checkins_test.go | 4 +++- go/pkg/basecamp/timeline_test.go | 6 ++++-- go/pkg/basecamp/webhooks_test.go | 4 +++- 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/go/pkg/basecamp/checkins_test.go b/go/pkg/basecamp/checkins_test.go index 6d3905dee1..869df0fc1a 100644 --- a/go/pkg/basecamp/checkins_test.go +++ b/go/pkg/basecamp/checkins_test.go @@ -1361,7 +1361,9 @@ func TestCheckinsService_ListQuestionReminders(t *testing.T) { if r1.GroupOn != "2022-10-28" { t.Errorf("expected GroupOn '2022-10-28', got %q", r1.GroupOn) } - if r1.RemindAt.IsZero() { + if r1.RemindAt == nil { + t.Error("expected RemindAt to be non-nil") + } else if r1.RemindAt.IsZero() { t.Error("expected RemindAt to be non-zero") } if r1.ReminderID == nil || *r1.ReminderID != 123 { diff --git a/go/pkg/basecamp/timeline_test.go b/go/pkg/basecamp/timeline_test.go index 3124e14a56..ba8672bde9 100644 --- a/go/pkg/basecamp/timeline_test.go +++ b/go/pkg/basecamp/timeline_test.go @@ -78,8 +78,10 @@ func TestTimelineEvent_Unmarshal(t *testing.T) { // Check timestamp expectedTime := time.Date(2024, 3, 15, 10, 30, 0, 0, time.UTC) - if !event.CreatedAt.Equal(expectedTime) { - t.Errorf("expected CreatedAt %v, got %v", expectedTime, event.CreatedAt) + if event.CreatedAt == nil { + t.Error("expected CreatedAt to be non-nil") + } else if !event.CreatedAt.Equal(expectedTime) { + t.Errorf("expected CreatedAt %v, got %v", expectedTime, *event.CreatedAt) } } diff --git a/go/pkg/basecamp/webhooks_test.go b/go/pkg/basecamp/webhooks_test.go index 25714268af..a2967babd3 100644 --- a/go/pkg/basecamp/webhooks_test.go +++ b/go/pkg/basecamp/webhooks_test.go @@ -174,7 +174,9 @@ func TestWebhook_UnmarshalGetWithRecentDeliveries(t *testing.T) { if delivery.ID != 1230 { t.Errorf("expected delivery ID 1230, got %d", delivery.ID) } - if delivery.CreatedAt.IsZero() { + if delivery.CreatedAt == nil { + t.Error("expected non-nil delivery CreatedAt") + } else if delivery.CreatedAt.IsZero() { t.Error("expected non-zero delivery CreatedAt") }