Make the release guide and guards tell the truth before v0.15.0 - #808
Make the release guide and guards tell the truth before v0.15.0#808jeremy wants to merge 9 commits into
Conversation
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.
Sensitive Change Detection (shadow mode)This PR modifies control-plane files:
|
github-actions
Bot
added
the
github-actions
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Updates release documentation and pre-tag safeguards for v0.15.0.
Changes:
- Corrects and expands migration guidance.
- Adds missing version checks to
make release. - Documents the generated release-notes limitation.
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 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
MIGRATING.md |
Corrects release history and adds missing migration entries. |
Makefile |
Expands pre-release version guards. |
.github/workflows/release-github.yml |
Identifies MIGRATING.md as authoritative. |
💡 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: c79f9ef96f
ℹ️ 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".
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.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d527abd55
ℹ️ 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".
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.
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
Makefile:253
- This fixed-string search does not actually verify the root package version: a stale top-level
.versionstill passes if any nested object contains"version": "$(VERSION)". Since this guard decides whether tagging may proceed, read the root field withjq, as the lockfile checks already do.
@grep -qF '"version": "$(VERSION)"' package.json || \
{ echo "ERROR: Root package.json version does not match $(VERSION). Run 'make bump VERSION=$(VERSION)' first."; exit 1; }
Makefile:276
ruby/Gemfile.lockcurrently records the SDK version twice (the PATH spec and the CHECKSUMS entry).grep -qFsucceeds when either one matches, so an internally stale lockfile can still pass this pre-tag guard and fail only in the post-tag release. Require both tracked records to match.
@grep -qF 'basecamp-sdk ($(VERSION))' ruby/Gemfile.lock || \
{ echo "ERROR: ruby/Gemfile.lock records a stale SDK version. Run 'make bump VERSION=$(VERSION)' first."; exit 1; }
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd09a39fb2
ℹ️ 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".
- 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.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9df7b6cdf6
ℹ️ 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".
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.
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
Makefile:258
- This guard searches for the version text anywhere in the file rather than validating the exported constant. A stale
export const VERSIONplus a comment containing the requested value would pass and allow the wrong SDK version to be tagged. Match the complete declaration line, as the new Python guard does.
@grep -qF 'export const VERSION = "$(VERSION)"' typescript/src/client.ts || \
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac56b92a00
ℹ️ 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".
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.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d2f3bb0bd
ℹ️ 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 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).
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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: 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.
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
MIGRATING.md:3925
- After
make bumppromotes the heading, this sentence points to a section that no longer exists:promote-migrating.shremoves the only exact# Unreleasedheading. Use release-neutral wording so the tagged v0.15.0 guide remains accurate.
Every change the `# Unreleased` section describes was merged by `fa15fc126`,
scripts/promote-migrating.sh:10
- This idempotence check searches the entire migration history, not the current top section. Once
# Unreleasedhas been promoted,make bump VERSION=<old release>will find that historical heading and report success after rolling all version files back; the Makefile guard uses the same global test. Restrict both checks to the first release heading so only the just-promoted section can satisfy them.
This issue also appears on line 19 of the same file.
if grep -qxF "# v$VERSION" "$FILE"; then
scripts/promote-migrating.sh:21
- After this promotion, nothing recreates
# Unreleased(the only repository references are this script and the guards). A later patch release with no migration-worthy changes therefore legitimately has neither heading—consistent with MIGRATING.md's “one section per release that breaks something” convention—butmake bumpwill abort here, andmake releasealso requires an otherwise-empty target heading. Handle the no-notes release case or automatically seed the next Unreleased section.
if ! grep -qxF "# Unreleased" "$FILE"; then
echo "ERROR: $FILE has neither '# Unreleased' nor '# v$VERSION' — nothing to promote." >&2
exit 1
The release surface carried nine catalogued defects under the standing rule "don't tag before fixing". This PR repairs the seven that live in the tree; the other two happen at tag time (
# Unreleased→# v0.15.0promotion withmake bump; the version-specific release-body line). Doc + tooling only — no SDK behaviour. While it was in review,mainmoved by four merges (#804, #807, #809, #810); those are absorbed too.What this fixes
Two post-tag entries re-filed out of
# v0.14.0into# Unreleased: the AuthorizationInfo.ExpiresAt fabricates 0001-01-01T00:00:00Z, and named time wrappers are invisible to both timestamp guards #662 absent-expiry changes and theTimelineEventDataretype (both landed in ExpiresAt absence: zero-time sentinel, Expiry(), and wrapper-aware timestamp guards #703, after the tag —git show go/v0.14.0:MIGRATING.mdcontains neither heading).Five missing entries written (four from the original catalogue, one found by review):
Get/GetEntryand the composites built on them — return a transport failure verbatim instead of stamping it permanently malformed.ListResultthey always returned.updateMyPreferenceswas uncallable outright,updateGaugeNeedlecallable only as the nil-payload{}bc3 400s).1919e77f7(bare commit): maxPages enforced at runtime in TS/Ruby.ApiErrorclass (bare commits2f21c9de7,328153020,478514642, surfaced by Codex):mailto:/hostless server-supplied URLs in Link headers, redirects, and the download hop used to crash with rawURI/ArgumentErrorexceptions; all three now refuse withApiError. One combined entry records the class and the rescue that stops matching.Tool.name and Tool.enabled are @required but never emitted — affects GetTool, CreateTool and UpdateTool #650 recount re-derived from both schemas (two of the seven new keys conditional, plus
position, which was already modeled).Isolate the response decoder from the auth/transport phase in Kotlin and Swift BaseService #604's Kotlin row now names
decodeFailureas the discriminator, matching the KDoc.Trailer rewritten: verification commit and the true in-flight set (now
fa15fc126; the event-feed stack and two drafts).make releaseguard gap closed, then hardened by review: the target originally grepped seven of the ten filesscripts/bump-version.shwrites. Now: root and TSpackage.jsonread viajqfield access; TS client constant exact;python/pyproject.tomlparsed from its[project]table (awk section-scoped); bothGemfile.lockrecords (PATH spec + CHECKSUMS) exact whole-line matched in both lockfiles; TSpackage-lock.jsonboth fields viajq. Every guard mutation-proven, including the literal bypasses (nested JSON"version"field; aversion =assignment in a foreign TOML table) demonstrated against the old checks and refused by the new ones. Restore by copy,diff -qclean, real exit codes captured.Release bodies state the notes' blind spot: generated notes are built from merged PRs and cannot see bare commits; MIGRATING.md records consumer-visible changes regardless of how they merged.
Main-merge absorption (Judge the advertised OAuth issuer's address, not just its spelling #804/Quiet known-noise CodeQL alerts without losing coverage #807/Refuse redirects on the signed download hop in every SDK #809/Police the endpoints a discovered issuer names, not only the issuer #810): Refuse redirects on the signed download hop in every SDK #809/Police the endpoints a discovered issuer names, not only the issuer #810 wrote their own entries and carry
breaking; Quiet known-noise CodeQL alerts without losing coverage #807 is CI-internal; Judge the advertised OAuth issuer's address, not just its spelling #804 was breaking-labeled with no entry — its entry is added (address-policed discovery hop 2, the variadicNewDiscoverercompile note, remedies in policy order).Labels
breakingapplied to: #703, #716, #723, #725, #726, #754, #779, #782, #796 (joining #772; #804/#809/#810 arrived already labeled). Declined in writing: #749 (not a break), #781, #727, #751 (rationales in the thread history).Verification
LC_ALL=C make doc-constants-checkgreen (REAL_EXIT captured) after every MIGRATING edit.# Unreleasedentry cross-checked againstgit log go/v0.14.0..origin/main; Sweep stale Python modules, and ask TS and Ruby whether their clients reach what was generated #770/Check that an operation lands on the same service in every SDK #771 ruled out as unentried breaks by inspection.Not doing (declined in writing): a CHANGELOG — MIGRATING.md plus label-generated notes is the convention; a third surface is a third thing to drift.