Resolve the release heading from the top of the guide, not anywhere in it - #812
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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4792935cf6
ℹ️ 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: three more, all taken. An abandoned unshipped section below a fresh "# Unreleased" would be orphaned forever the moment promotion passed it -- every unshipped version heading below Unreleased now refuses with the fold-or-re-title instruction. Both awk rewrite passes track fence state, so a fenced "# Unreleased" example BEFORE the real section survives promotion (the judgments already read prose; the rewrites now match). And the SIGPIPE-net case was restaged onto the shipped-authority semantics -- its old target died at the ordering gate before reaching the check it claimed to cover. 50 assertions; the abandoned-section and leading-fence cases fail against the pre-fix script.
|
@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 2 comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
scripts/test-promote-migrating.sh:11
- This comment still describes a “current-version authority,” but the test suite no longer defines or uses one; only the shipped-tag override remains. Remove the stale explanation so future changes do not look for a nonexistent test seam.
# Pin the current-version authority so the repo's real constant cannot leak
# into scratch-file cases; individual cases override it to probe the gate.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05b6b5b5f2
ℹ️ 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: three test/parsing nits taken. The fence detector now recognizes tilde fences and up-to-three-space indents in all three passes (the judgments and both rewrites), pinned by an exotic-fence fixture. The remote-tag fixture snapshots the guide so a released-classification regression cannot pass by re-promoting. And the SIGPIPE-net case's shipped maximum drops below its target so the ordering gate cannot shadow the duplicate check it covers.
|
@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)
Previously missed (1) — in code that hasn't changed since the last review.
scripts/test-promote-migrating.sh:159
- This snapshots the file only after the first failing invocation, so the “leaves the file untouched” assertion cannot detect a script that mutates and then exits 1; the second run may simply leave that already-mutated state unchanged. Snapshot before the invocation and compare immediately afterward.
cp "$DIR/s.md" "$DIR/s.before" 2>/dev/null
PROMOTE_MIGRATING_RELEASED= bash "$SCRIPT" 0.15.1 "$DIR/s.md" >/dev/null 2>&1 || true
diff -q "$DIR/s.md" "$DIR/s.before" >/dev/null; check "tagless refusal leaves the file untouched" 0 $?
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7da6538fac
ℹ️ 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".
…rphan check Both bots converged on the same truth: a fence closes only with the same delimiter character, at least the opening run's length, and nothing but spaces after — so a ``` line inside a ```` block, or backticks inside a tilde fence, is content, and toggling on every delimiter run could hide the real Unreleased heading behind a phantom close. One fence automaton is now interpolated into the prose pass and both rewrites, so the judgments and the mutations cannot disagree about what is prose. The abandoned-section refusal also becomes a function called from every branch, not just Unreleased: an unshipped section can no longer hide below a promoted or pending target. The remote fixture ships a tag for each heading it documents, as history would.
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! 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 1 comment.
Suppressed comments (1)
scripts/test-promote-migrating.sh:11
- This comment still describes a “current-version authority” and per-case overrides, but that authority was removed; the only exported seam below is the shipped-release list. Keeping the obsolete explanation makes the fixture setup misleading.
# Pin the current-version authority so the repo's real constant cannot leak
# into scratch-file cases; individual cases override it to probe the gate.
# Pin the shipped-releases authority: the suite must not depend on the
# checkout's real tag state (CI checkouts are shallow and tagless).
The PROMOTE_MIGRATING_RELEASED override rode along into any bump or release that inherited it from the caller's shell: a stale exported list silently displaced the remote as the ordering authority, so a hand-edited rollback could pass --check while a newer tag sat on origin. The seam is now an explicit --released flag that only the self-test passes; the environment is ignored outright, pinned by a fixture-remote case that fails against the env-honoring version. Empty stays fail-closed, and the harness header no longer describes the long-gone current-version authority.
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! 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". |
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
Resolves the release heading from the top of MIGRATING.md and blocks rollbacks by using the remote tag list as the only ordering authority. Old behavior matched any historical “# vX.Y.Z”; new behavior treats the first release heading as current and requires the target to be strictly newer than the newest shipped tag.
Written for commit 19cedba. Summary will update on new commits.