Resolve the release heading from the top of the guide, not anywhere in it - #812
Resolve the release heading from the top of the guide, not anywhere in it#812jeremy wants to merge 5 commits into
Conversation
…n it The tagged promote-migrating.sh matched "# v$VERSION" anywhere, so after v0.15.0's promotion a bump rolled backward to an old version found that version's HISTORICAL heading, reported "already promoted", and rewrote every version file backward -- and make release, using the same global grep, would have pushed the rollback to main before failing on the existing tag. A release with no migration-worthy changes also aborted mid-bump, after the version files were already rewritten. Copilot's final-head review of #808 flagged all of this 26 minutes before the merge and the suppressed block went unread -- the review discipline failed, not the reviewer. The script now resolves the CURRENT release heading as the first release heading in the file (the guide is newest-first; code blocks quoting old headings all sit below the newest section, pinned by a test case): - first heading "# Unreleased" -> promote; refuse if "# v$VERSION" already exists below (rollback). - first heading "# v$VERSION" -> idempotent no-op. - first heading an OLDER version -> the legitimate no-notes release: succeed without touching the file, say so. - first heading a NEWER version -> refuse: backward release. --check mode gives make release the same closed set of verdicts in place of the two global greps, and bump-version.sh calls the promotion as step 0, before any version file is touched, so every refusal aborts with a clean tree. scripts/test-promote-migrating.sh pins all of it (15 assertions; wired into check-targets and its own CI step, since check-targets membership alone is not CI coverage here). Mutation proof: the self-test run against the tagged script fails 5 assertions, the backward-bump regression case first among them. Also from that suppressed review: the trailer sentence naming "the # Unreleased section" now says "the newest release section above" -- promotion renames the heading, so the tagged guide pointed at a section that no longer existed.
|
@codex review |
Sensitive Change Detection (shadow mode)This PR modifies control-plane files:
|
github-actions
Bot
added
the
github-actions
There was a problem hiding this comment.
Pull request overview
Hardens release-guide promotion and rollback checks.
Changes:
- Resolves only the newest migration heading.
- Adds no-notes handling and release validation.
- Adds regression tests and CI coverage.
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 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
scripts/promote-migrating.sh |
Implements heading-state validation. |
scripts/test-promote-migrating.sh |
Tests promotion and rollback cases. |
scripts/bump-version.sh |
Runs migration validation before mutation. |
Makefile |
Integrates checks into bump, release, and test targets. |
.github/workflows/test.yml |
Runs the self-test in CI. |
MIGRATING.md |
Makes trailer wording release-neutral. |
Suppressed comments (1)
scripts/promote-migrating.sh:54
- Comparing only with the newest documented heading loses release history in the newly accepted no-notes case. After
v0.16.0ships without notes while this file still starts atv0.15.0, bothmake bump VERSION=0.15.1and the later--checkaccept0.15.1as forward, so the release target can again push a version rollback tomain. Compare the bump against the currently recorded SDK version and the release check against existing release tags (or persist a no-notes release marker), and add a sequential no-notes-then-rollback regression case.
if newer "$VERSION" "$TOP"; then
# Legitimate: a release with nothing migration-worthy has no section,
# per the guide's one-section-per-release-that-breaks-something rule.
echo "$FILE: no '# Unreleased' section — v$VERSION ships without migration notes (newest documented: v$TOP)."
💡 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: 9f742e562f
ℹ️ 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".
…lity Round-one findings on #812, all taken: - The promote branch never compared the target against the released order: bump 0.9.0 with 0.15.0 released would mint "# v0.9.0" at the top and file today's notes behind history. The target must now be newer than the newest released section. - A misplaced "# Unreleased" below the first version heading rode through the idempotent and no-notes branches silently — its notes would miss the release. Both branches (both modes) refuse it. - The version comparator is pure component-wise arithmetic. Both bots claimed macOS's sort lacks -V; empirically Apple's sort 2.3 has it, but the portable spelling costs six lines and retires the question across BSD userlands -- and unlike a lexicographic fallback it orders 0.10.0 after 0.9.0, which a new test case pins. Self-test grows to 20 assertions; the three new failure cases run red against the pre-fix script (the backward-promote P1 first among them) and green after.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9af0966b0f
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 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:306
- Unlike the repository's other test/check goals (for example,
Makefile:1289andMakefile:1543), this new target is not declared phony. If a root path namedtest-promote-migratingexists, both the CI invocation andcheck-targetswill treat it as up to date and silently skip this regression suite.
test-promote-migrating:
Round two, all three taken: - Codex's P1: after a no-notes release the newest heading legitimately lags the SDK version, so heading comparisons cannot see a rollback landing between them -- packages at 0.16.0, newest heading v0.15.0, bump 0.15.5 sailed through. The script now reads the current version from go/pkg/basecamp/version.go (the constant make release re-verifies anyway) and refuses any target older than it, before the heading rules run. At bump time that constant still holds the pre-bump version; at release time the version guards have pinned it to the target, so the comparison degrades to a harmless equality. The self-test pins the authority via an env override so scratch cases control it. - A second "# Unreleased" heading is refused before any mutation, rather than promoting the first and leaving the release to fail late with the version files already rewritten. - test-promote-migrating is declared .PHONY like its sibling test goals, so a root path by that name cannot silently satisfy it. Self-test grows to 25 assertions; the four new ones fail against the pre-fix script (the rollback P1 first) and pass after.
|
Also taken from the suppressed block of the round-two review: test-promote-migrating is now declared .PHONY like its sibling test goals, so a root path by that name cannot silently satisfy the target. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 11c886d246
ℹ️ 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".
…story Round three, all three taken: - An unreadable or format-drifted version.go left CURRENT empty and current_blocks vacuously true -- the guard failed open into exactly the rollback it exists to refuse, and the self-test's override meant CI could never see it. An unresolvable current version is now fatal before anything is examined or mutated, pinned by a case that runs a copy of the script from outside the repo. - Promoting NEW notes to the version that already shipped (SDK released 0.16.0 without notes; fresh Unreleased; bump 0.16.0) filed them under a tag that already exists, and release would push main before failing on that tag. The promote branch now requires a strictly newer target; equality stays legal for the no-mutation paths. - A bump corrected to a higher version before committing stranded the first promotion: the second run called it a no-notes release and left the notes attributed to a version no tag would ever name. The release tag is the offline authority on what shipped: a newest heading with no tag is a PENDING promotion, which bump re-promotes to the corrected target and --check refuses instead of calling no-notes. Self-test grows to 30 assertions; the four new ones fail against the pre-fix script and pass after.
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
scripts/promote-migrating.sh:40
- Checking only for an empty value does not fail closed on a malformed matched constant. For example,
const Version = "dev"sets a nonemptyCURRENT; each arithmetic test innewerreturns an error status, which is treated as false whilecurrent_blocksruns in anif, so the rollback authority is bypassed. ValidateCURRENTas the supported numeric three-component format before any comparison.
if [ -z "$CURRENT" ]; then
# Fail closed: with no rollback authority, current_blocks would accept
# every target — including the rollback it exists to refuse.
echo "ERROR: cannot read the current SDK version from go/pkg/basecamp/version.go — refusing to run without the rollback authority." >&2
exit 1
fi
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a980b04a4
ℹ️ 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".
Round four, all three taken -- and CI caught the P1 itself: the new
spec-gates step runs in a default shallow checkout with no tag history,
so `released` classified v0.15.0 as pending and the self-test failed
three assertions there. Worse than the red CI: `make bump` from any
--no-tags clone would have renamed real historical notes into the new
release.
- A local tag's absence now counts only when the checkout demonstrably
knows release tags at all; a checkout that knows none fails closed
with the fetch instruction, at the one point where the answer matters.
The self-test pins both directions and is hermetic: it supplies the
released-versions authority explicitly and never consults the repo's
real tag state, so it passes identically in a tagless CI checkout.
- The suppressed bypass rides along: a malformed current version ("dev")
errored every arithmetic test in `newer`, which an `if` reads as
false. CURRENT and the target are validated as X.Y.Z before any
comparison; the self-test asserts the explicit diagnostic, since the
pre-fix script exited 1 on one path only by the error landing in the
blocking direction.
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ 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". |
| local any | ||
| any=$(git -C "$(dirname "$0")/.." tag -l 'v[0-9]*' 2>/dev/null | head -1) | ||
| if [ -z "$any" ]; then | ||
| echo "ERROR: no release tags visible in this checkout, so whether '# v$1' ever shipped cannot be established (shallow or --no-tags clone?). Run 'git fetch --tags' and retry." >&2 | ||
| exit 1 | ||
| fi | ||
| echo "$FILE already promoted to v$VERSION" | ||
| exit 0 | ||
| fi | ||
| git -C "$(dirname "$0")/.." tag -l "v$1" 2>/dev/null | grep -qx "v$1" |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90d6489509
ℹ️ 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".
| any=$(git -C "$(dirname "$0")/.." tag -l 'v[0-9]*' 2>/dev/null | head -1) | ||
| if [ -z "$any" ]; then | ||
| echo "ERROR: no release tags visible in this checkout, so whether '# v$1' ever shipped cannot be established (shallow or --no-tags clone?). Run 'git fetch --tags' and retry." >&2 | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
In a shallow or filtered clone with a nonempty but incomplete tag set, finding one unrelated v* tag is not proof that the missing v$TOP never shipped. For example, if only v0.14.0 is present while the historical top section is v0.15.0, bumping from 0.15.0 to 0.16.0 makes released 0.15.0 return false and rewrites the shipped 0.15.0 notes as 0.16.0 notes. Fresh evidence beyond the earlier tagless-checkout finding is that the final code fails closed only when zero tags are visible; require verified-complete tags or query the authoritative remote before treating absence as unreleased.
AGENTS.md reference: AGENTS.md:L149-L154
Useful? React with 👍 / 👎.
Repairs the release-guard regression #808 shipped, flagged by Copilot's final-head review (suppressed block) 26 minutes before the merge and not absorbed — the review discipline failed, not the reviewer. All three suppressed findings taken:
The historical-heading bypass (the dangerous one). The tagged
promote-migrating.shmatched# v$VERSIONanywhere in MIGRATING.md, so after v0.15.0's promotion,make bump VERSION=0.14.0found the historical v0.14.0 heading, reported "already promoted", and rewrote every version file backward;make releaseused the same global grep and would have pushed the rollback tomainbefore failing on the existing tag. The script now resolves the current release heading as the first release heading in the file, with a closed verdict set: promote / idempotent no-op / no-notes forward release (succeed, untouched) / backward release (refuse) / rollback-duplicate (refuse).make releasecalls--checkmode instead of the two global greps.The no-notes release aborted mid-bump. A release with nothing migration-worthy legitimately has neither heading (the guide's one-section-per-release-that-breaks-something convention); the old script errored on it — after the version files were already rewritten. The no-notes forward case now succeeds explicitly, and the promotion runs as bump step 0, before any file is touched, so every refusal aborts with a clean tree.
The tagged trailer pointed at a removed heading. "Every change the
# Unreleasedsection describes…" — promotion renames that heading, so the v0.15.0 guide referenced a section that no longer existed. Now release-neutral ("the newest release section above").Proofs.
scripts/test-promote-migrating.sh: 15 assertions covering every verdict, including a fence-quoted-heading case; wired intocheck-targetsand its own CI step (check-targets membership alone is not CI coverage in this repo). Mutation proof: the self-test run against the tagged script fails 5 assertions — the backward-bump regression first among them — then passes 15/15 restored.make release VERSION=0.15.0on this branch prints the idempotent verdict and proceeds to the expected dirty-tree stop, proving the--checkwiring executes.Summary by cubic
Resolve the current release heading from the top of MIGRATING.md and use the SDK’s version constant (and tag awareness) to prevent rollbacks. Old behavior matched any historical “# v” and let backward bumps and mid-bump aborts pass; new behavior treats the first release heading as current, validates versions, accounts for tagless clones, and returns explicit promote/check verdicts.
Key behavior changes
scripts/promote-migrating.sh: resolves the current release as the first heading; supports no-notes releases; adds--checkformake release.go/pkg/basecamp/version.goas rollback authority; refuses any target older than it; fails closed if unreadable; validates both current and target as X.Y.Z.--checkrefuses pending.sort -Vdependency).Tooling and CI
scripts/bump-version.shruns the promotion as step 0 to fail early before version files change.Makefileuses./scripts/promote-migrating.sh --check; adds.PHONYtest-promote-migratingand includes it incheck-targets..github/workflows/test.ymlruns a hermetic self-test that pins tag visibility and current-version authority and covers all verdicts;MIGRATING.mdwording now references “the newest release section above.”Written for commit 90d6489. Summary will update on new commits.