ExpiresAt absence: zero-time sentinel, Expiry(), and wrapper-aware timestamp guards - #703
Merged
Merged
Conversation
bc3 renders `expires_at.to_i`, so a wire 0 would be its spelling of an unstated expiry — and time.Unix(0, 0) is a *valid* 1970 instant with IsZero() == false, so "no expiry" read as "expired 56 years ago". RFC 7591 compounds the collision: bc3's own client_secret_expires_at uses 0 to mean "never expires", the exact inverse. 0 now decodes to the zero time, joining null and absent in one "no expiry known" class, matching TypeScript's parseExpiresAt (Invalid Date sentinel). No production issuer has ever sent 0 — BC3 tokens carry null: false plus presence validation and are always set at mint; legacy Signal tokens lazily self-default — so this is hardening, not a live-bug fix. The zero FlexTime also marshals as null now, ending the fabricated 0001-01-01T00:00:00Z an absent expires_at re-marshaled as. AuthorizationInfo.Expiry() (time.Time, bool) is the documented front door: ok is false when the document stated no expiry.
The generated field is *types.FlexibleTime — the bounds are required-and-nullable: schedule_entry_* events always carry them, but the value may be null — and the hand-written struct flattened them through deref(), decoding a null bound to the zero time and fabricating 0001-01-01T00:00:00Z on re-marshal. Same class as #658's four fields. nil now means the API sent null, and re-marshals as null (bare tag mirrored from the generated struct — no omitempty, the key must stay). Promoted method calls compile unchanged and nil-panic on a null bound; see MIGRATING.md.
Both AST guards keyed on the literal time.Time selector, so FlexTime, types.FlexibleTime and types.Date were invisible to them — which is how TimelineEventData's bounds sat value-typed against pointer-typed generated counterparts with neither guard able to see it. The predicate is now isTimeLike: wrappers discovered by parsing this package and ../types for structs embedding a value time.Time (plus types.Date, which stores year/month/day but occupies the same optionality class), matched by final type name in both Ident and Selector spellings, on both sides of the (wrapper, generated) pairing. AuthorizationInfo.ExpiresAt stays structurally out of reach forever — no generated counterpart, bare tag — so the root protection for that class is behavioral: TestNamedTimeWrappersMarshalZeroAsNull asserts every named wrapper marshals its zero as null, with the table cross-checked against the same discovery so a new wrapper cannot ship without a row.
Two Class-A silent behavior changes (0 decodes to the zero time; the zero marshals null) and one Class-B compile-clean runtime break (the pointerized timeline bounds) join the existing Unreleased section. The brief's Go hand-off paragraph now records what shipped and corrects the premise #681 wrote down: a nil expiry rendering as 0 is unreachable today (BC3 tokens validate presence and are always set at mint; Signal tokens self-default), so the 0 handling is defense-in-depth against the RFC 7591 collision, not a live production bug.
There was a problem hiding this comment.
Pull request overview
Fixes Go expiry absence handling and extends timestamp guards to named wrappers.
Changes:
- Adds zero-time expiry semantics and
Expiry(). - Preserves nullable timeline bounds with pointers.
- Expands behavioral and AST regression tests.
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.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
spec/api-gaps/bc5-authorization-document-shape.md |
Records the resolved expiry behavior. |
MIGRATING.md |
Documents behavioral and pointer migrations. |
go/README.md |
Demonstrates Expiry(). |
go/pkg/basecamp/authorization.go |
Implements expiry sentinel behavior and accessor. |
go/pkg/basecamp/authorization_test.go |
Tests absent, null, zero, and real expiries. |
go/pkg/basecamp/timeline.go |
Makes nullable event bounds pointers. |
go/pkg/basecamp/timeline_test.go |
Tests pointer and null round-tripping. |
go/pkg/basecamp/optional_timestamps_test.go |
Extends timestamp-wrapper guards. |
Suppressed comments (1)
go/pkg/basecamp/authorization_test.go:441
- The PR establishes that bc3 never emitted
0, so describing this as a legacy rendering conflicts with the behavior being tested: defensive acceptance of an otherwise unreachable spelling.
// TestAuthorizationInfo_NoExpiryKnown pins the sentinel contract for #662: an
// absent expires_at, an explicit null, and bc3's legacy `0` rendering all
// decode to the zero ExpiresAt, report Expiry() ok=false, and re-marshal as
// null — never as 0001-01-01T00:00:00Z, and never as a valid 1970 date.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 71e45d82e7
ℹ️ 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".
Review caught the PR contradicting its own source analysis: the README and a fixture comment claimed Launchpad omits expires_at for a non-expiring token (it never omits the field), and three spots called the wire 0 a "legacy rendering" (bc3 has never emitted 0; the handling is defensive). All five sites now state the defensive framing: no production issuer emits absent/null/0 today, and the branch is robustness for whatever GetInfoOptions.Endpoint points at.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (1)
MIGRATING.md:176
types.FlexibleTimealready marshals its zero value asnull(go/pkg/types/flexible_time.go:47-53), so the previous value-typed fields did not fabricate0001-01-01T00:00:00Zon re-marshal. The migration note should describe the actual behavior change: null now surfaces asnilinstead of the zero sentinel.
the hand-written struct flattened them to value types, fabricating
`0001-01-01T00:00:00Z` for a null bound on re-marshal.
This was referenced Aug 19, 2026
jeremy
added a commit
that referenced
this pull request
Aug 22, 2026
* Make the release guide and guards tell the truth before v0.15.0 MIGRATING.md and the release tooling carried nine catalogued defects; this repairs the seven that live in the tree. The retro-labels on merged PRs and the Unreleased -> v0.15.0 promotion happen at tag time. - Re-file the two post-tag entries out of "# v0.14.0" into "# Unreleased": the #662 absent-expiry changes and the TimelineEventData pointer retype both landed in #703, after the tag. Proof: `git show go/v0.14.0:MIGRATING.md` contains neither heading. - Write the four entries the section was missing: #773 (the merge-safe Go reads return a transport failure verbatim -- errors.As and Retryable results move), #737 (four TS paginated methods now declare the ListResult they always returned), #735 (every generated Swift model has a public init -- recorded as NOT a break: no existing initializer changed shape, the 35 affected models were previously unconstructible so no consumer code exists against them), and the maxPages runtime cap (`1919e77f7`, a bare commit label-generated notes cannot list). - Fix the #650 miscount: `position` is conditional on the wire but was modeled before #723, so it is not one of "the seven" -- two of the seven new keys are conditional, plus `position`. Derived from the v0.14.0 and current Tool schemas and the bc3 partial's own `if`s. - Refresh the #604 table's Kotlin row to agree with the #750 entry and the KDoc it cites: the SerializationException lands in `decodeFailure`, the discriminator; `cause` mirrors it and is explicitly not one. - Rewrite the "# Not in this release" trailer: "Nothing is in flight ... merged at 9a819e4" was 53 commits stale. It now names the verification commit and the actual in-flight set, and dates the historical record below it. - Close the `make release` guard gap: it grepped seven of the ten files scripts/bump-version.sh writes, so a truncated bump could tag with the root package.json, typescript/src/client.ts or python/pyproject.toml constant stale. All three join the guard. Proven by mutating each file and watching `make release` refuse with the new message; restored by copy, verified with diff -q. - Release bodies now say that a change merged without a pull request appears only in MIGRATING.md, since generate_release_notes builds from merged PRs and structurally cannot list bare commits. * Absorb the bot round: exact guards, honest scope, the missing Ruby entry Seven fixes from the Copilot and Codex reviews, all taken: - The pyproject guard is an exact whole-line match (grep -qxF). The old regex left dots unescaped and the end unanchored, so a valid-PEP-440 "0.15+0" passed it and failed only in the Python release workflow, after other SDKs had published -- the exact post-tag failure class this PR exists to close. - typescript/package-lock.json (both SDK-version fields, via jq) and ruby/Gemfile.lock join the lockfile guards; bump-version.sh rewrites both, and neither was checked. All three new/changed guards proven by mutation: each refused with its message and exit 2, restored by copy, diff -q clean. - The trailer no longer claims every count in the guide was measured at 8fcb39a -- v0.13.0's totals state their own 9a819e4 baseline. The claim is scoped to the Unreleased section and the in-flight survey. - The #735 entry tells the two Swift shapes apart: updateGaugeNeedle was callable only as the nil-payload {} that bc3 400s; updateMyPreferences was not callable at all (outer requires the unconstructible payload). - The maxPages entry described sloppy-mode assignment wrong: [[Set]] on an inherited getter-only accessor creates no own property -- the assignment is silently ignored, not shadowed. - Three Ruby bare commits (2f21c9d, 3281530, 4785146) were consumer-visible -- crashes on mailto:/hostless server-supplied URLs became ApiError refusals -- and had entries nowhere. One combined entry records the class and the rescue that stops matching. - The release-body sentence no longer promises MIGRATING.md is a complete record of PR-less commits; it states the mechanism (the generated notes cannot see them) and points at the guide for consumer-visible changes. * Scope the Ruby entry's no-request claim to the rejected target Codex round 2: the request that returned the malformed Link or Location header was necessarily already sent — only the follow-up to the rejected target is prevented. Saying "before anything is sent" misled anyone reasoning about hooks or request counts. * Absorb Codex round 3: getter scope, PATH-spec anchor, test-import claim - The #773 entry scoped the classification change to the four composites; Documents.Get installs markBodyReadFailures itself and Schedules.GetEntry delegates to getEntryWithBody, so direct getter callers see it too. The entry now names the getters and the composites built on them. - Both ruby Gemfile.lock guards anchor to the 4-space PATH-spec line with grep -qxF; the loose match could be satisfied by the version-bearing 2-space entry while the PATH spec stayed stale. Proven: mutating only the PATH-spec line now refuses with exit 2, and a clean tree passes the guard block. - The #735 entry claimed every Swift test file uses @testable import; the generator-only test files import BasecampGenerator plain. Narrowed to every test file that imports the SDK module. * Read the JSON versions as fields, and check both Gemfile.lock records Codex round 4 and Copilot round 2 converged on the same hole: grep -qF over package.json matches a "version" string anywhere in the document, so a stale top-level version passed while any nested metadata field carried the requested one. Proven literally: a crafted package.json with top-level 0.13.9 and a nested 0.14.0 satisfied the old grep and is refused by the new jq field read. typescript/package.json gets the same treatment -- same shape, same class. Copilot also wanted both version-bearing Gemfile.lock records checked, not just the PATH spec: a lockfile whose CHECKSUMS entry lags the PATH spec would pass the anchored guard and fail only post-tag. Both files now check both exact lines; a CHECKSUMS-only staleness is refused with its own message, proven by mutation with restore-by-copy. * Absorb the four main merges and Codex round 5 Main moved by four while this PR was in review: #804, #807, #809, #810. Merged in; what each needed here, verified against the tree: - #809 and #810 wrote their own Unreleased entries when they merged (#805, #806) and carry `breaking` -- nothing to add. - #807 is CI-internal -- nothing to add. - #804 carries `breaking` but had no entry: resource-first discovery's second hop now rides the address-policed shared client, refusing special-use-space issuers non-retryably and dropping the caller's transport for that hop. Entry added beside #806's, with the variadic NewDiscoverer compile note and the remedies in policy order. - The trailer's verification commit moves to fa15fc1 and its in-flight list shrinks to what is actually in flight. Codex round 5's guard finding rides along: the pyproject check now parses the [project] table (awk section-scoped exact line) instead of matching a version assignment anywhere in the file. Proven with the literal bypass -- a stale [project].version plus an exact assignment in another table passed the old whole-file grep and is refused now; restore by copy, diff -q clean. * Guard the heading promotion, and fix two entry misstatements Copilot round 3 caught the release procedure's last unguarded step: the "# Unreleased" -> "# v(VERSION)" promotion was a hand edit nothing enforced, so a tag could ship with its notes still filed as unreleased. scripts/promote-migrating.sh now does the rewrite (exact-line, idempotent, refusing the both-headings and neither-heading states), bump-version.sh calls it as step 11, and make release guards both directions: the promoted heading must exist and "# Unreleased" must not. Proven: release refuses on today's tree; the script promotes a scratch copy correctly, is idempotent, and errors on both degenerate states. Codex round 6's two rides along: - The TS client guard is an exact whole-line match including the semicolon, so a comment carrying the assignment text cannot satisfy it while the real constant lags. - The #804 entry's remedy list dropped the address-class split in transcription: AllowLoopback re-admits loopback and nothing else, and Allow does not pierce the IANA tables -- for RFC 1918 the policy must be built without them, which is the implementation's own documented spelling. The entry now says so, and the #735 entry stops claiming UpdateGaugeNeedleRequest has a required member (its member is optional; the outer init exists because request models always got one). * Watch promote-migrating.sh with the gate that watches its caller Copilot: delegating the promotion to a new script moved release-bump behavior out of the sensitive-change gate's sight -- bump-version.sh is listed in extra-patterns and the new script was not. It is now.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #662.
What this fixes
AuthorizationInfo.ExpiresAtfabricated0001-01-01T00:00:00Zfor an absent or nullexpires_at— the #562/#615/#620 failure mode, on the field whose whole purpose is answering "is this credential still good". And a second, distinct bug exploration surfaced: a wireexpires_at: 0decoded totime.Unix(0, 0)— a valid 1970 instant withIsZero() == false— so "no expiry" read as "expired 56 years ago". TypeScript already collapses0/null/absent into one "no expiry known" class (parseExpiresAt's Invalid-Date sentinel); Go did not.The model: zero-time sentinel + comma-ok accessor
FlexTimestays a value type. Absent,null, and0all decode to the zero time; the zero marshals back asnull; andAuthorizationInfo.Expiry() (time.Time, bool)is the documented front door (ok == falsewhen the document stated no expiry). This matches TS's sentinel,oauth.Token.ExpiresAt, andCredentials.ExpiresAt <= 0— no pointer, no panic class, no compile break.Pointerization was considered and fails its own stress test:
encoding/jsonallocates on any non-null token, so a wire0can never become a nil*FlexTimewithout struct-level unmarshal machinery — you keep either the 1970 bug or two absence spellings.Premise correction (from bc3 source)
#681's brief claimed a nil expiry arrives as
0. It cannot, today: BC3 tokens carrynull: falseplus presence validation and are always set at mint (10-year PATs are the "non-expiring" case and still carry real timestamps), and legacy Signal tokens lazily self-default (expires_at ||= expires_in.from_now). Launchpad never omits the field either. So the0handling is defense-in-depth against the RFC 7591 collision — bc3's ownclient_secret_expires_at: 0means "never expires", the exact inverse of "expired at epoch" — not a live-bug fix. The brief now records this.The guard blind spot, and what actually closes it
Both AST timestamp guards keyed on the literal
time.Timeselector, so the named wrappers (FlexTime,types.FlexibleTime,types.Date) were invisible to them. The predicate is nowisTimeLike: wrappers discovered by parsing the package and../typesfor structs embedding a valuetime.Time(plustypes.Date, same optionality class), matched by final type name on both sides of the (wrapper, generated) pairing.The widened guard caught exactly one real narrowing —
TimelineEventData.StartsAt/EndsAt, hand-written valuetypes.FlexibleTimeflattening the generated*types.FlexibleTime(required-and-nullable) throughderef(). Pointerized here, mirroring the generated field's bare tag: nil means the API sent null and re-marshals as null. Same class as #658; same Class-B migration hazard (promoted method calls compile and nil-panic), documented in MIGRATING.md.But
ExpiresAtitself stays structurally out of reach of both AST guards forever — no generated counterpart to pair with, bare tag. What protects that class at the root is behavioral:TestNamedTimeWrappersMarshalZeroAsNullasserts every named time wrapper marshals its zero asnull, with the table cross-checked against the same wrapper discovery so a new wrapper type cannot ship without a row.Verification
Red proofs first, against the un-fixed tree,
REAL_EXITcaptured from log files:TestFlexTime_UnmarshalJSON0-case rewritten to expectIsZero()— failed on main:FlexTime.IsZero() = false for input 0 … got 1969-12-31 16:00:00 -0800 PST.TestNamedTimeWrappersMarshalZeroAsNull— failed on main:zero FlexTime marshaled as "0001-01-01T00:00:00Z", want null(FlexibleTime and Date already passed; the fix is FlexTime'sMarshalJSON).TestNoWrapperTimestampNarrowerThanGenerated— failed on main naming exactlyTimelineEventData.StartsAt/EndsAt(value-typed vs generated pointer), before the pointerization commit; green after. No other violations surfaced —ScheduleEntry's bounds pair value-to-value with their generated counterparts.The new Launchpad-fixture absence assertions and
Expiry()tests are assertions on new surface, not red proofs; the red proofs are 1–3.Then: full
makegreen (REAL_EXIT=0; the first pass failed only on aprealloclint in the new guard code, fixed and re-run). Drift checks unchanged: 91 wrapper pairs walked, 1206 generated fields verified, no critical drift, no UNWRAPPED/MISSING movement. No generated artifacts in the diff — this is entirely the hand-written Go lane plus test guards: no Smithy, noopenapi.json, no count pins.Migration surface
Under the existing
# UnreleasedMIGRATING.md section:0→ zero time: Class A (silent behavior change; previously a valid 1970 date).null(was0001-01-01T00:00:00Z): Class A.TimelineEventData.StartsAt/EndsAt→ pointers: Class B (compiles via promoted methods, nil-panics on a null bound).Expiry(): additive, not counted.Companion
A bc3 PR converges
/authorization.json'sexpires_aton ISO-8601 (it is the only integer-epoch timestamp in bc3's entire public JSON API, contradicts the endpoint's own "mirrors Launchpad" intent, and collides with RFC 7591's0) and documents the endpoint indoc/api— the missing piece holdingspec/api-gaps/bc5-authorization-document-shape.mdatpartial-coverage. Non-blocking: both Go and TS already accept both spellings. Link: basecamp/bc3#12646.Side note: #667 (v0.13.0 freeze) closed as stale; v0.13.0 shipped 2026-08-07.
Summary by cubic
Handle absent expiry correctly by treating
0/null/absentexpires_atas “no expiry” and marshaling zero asnull. AddsAuthorizationInfo.Expiry()and pointerizesTimelineEventDatabounds; timestamp guards now recognize named wrappers. Docs clarified this is defensive hardening (production issuers don’t send0/null/absent).Bug Fixes
FlexTime: decode0/null/absent to zero time; zero marshals asnull(no more0001-01-01T00:00:00Zor “expired at 1970”).AuthorizationInfo.Expiry() (time.Time, bool)for safe expiry reads.FlexTime,types.FlexibleTime,types.Date) and assert zero named wrappers marshal asnull.Migration
TimelineEventData.StartsAt/EndsAtare now*types.FlexibleTime; nil-check before using them. Nil re-marshals asnull.expires_at: 0now reads as “no expiry”. Preferinfo.Expiry()over accessingExpiresAtdirectly.Written for commit 0427381. Summary will update on new commits.