feat: automatically bump version on release - #102

@claude review this

Claude finished @carlos-granados's task in 2m 7s —— View job

Claude Code Review

• Load required tools
• Read the PR diff (.github/workflows/release.yml)
• Analyze release workflow logic
• Post review feedback

Nice direction — automating the version bump removes a manual step that's easy to forget. That said, the current design has a few significant issues around what commit the tag actually points to vs. what commit produces the release binaries, plus a few smaller robustness concerns. Details below.

Major issues

1. Tag ↔ release-binary divergence (correctness)

bump-version checks out main, appends a new commit chore: update version number to X, and all build jobs then use that new SHA. But the tag that triggered the workflow still points to the pre-bump commit. Concretely, after a release you end up with:

• git checkout v0.3.0 → #define XDEBUG_VERSION "0.2.0" (stale).
• The .so/.dll in the GitHub Release → reports 0.3.0.
• The chore: update version number to 0.3.0 commit sits on main, untagged.

Anyone building from source at v0.3.0 (distros, packagers, PECL-style users) will produce binaries whose version string disagrees with the tag they cloned. This is the exact problem the PR is trying to solve, just moved elsewhere.

Options, roughly in order of cleanliness:

• (preferred) Flip the process: run the bump as a workflow_dispatch job that opens a PR (or commits+tags on main), so the tag itself points to the bumped commit. The release workflow then just builds ${{ github.sha }} and needs no push permission.
• If you keep the current shape, force-move the tag to the bump commit (git tag -f v… && git push -f origin v…) so tag and binaries stay in sync. This is destructive but at least keeps history consistent.
• At minimum, document the divergence prominently in the release notes template.

2. bump-version uses main HEAD, not the tagged commit

L44: ref: main. If any commit lands on main between the tag being pushed and this workflow starting (dependabot, another merge, etc.), the release ships those extra commits. The release binaries would then contain code the release-maker never intended to ship. Consider ref: ${{ github.sha }} (the tag's commit) instead, then push the bump onto whatever branch you want tagged.

3. Fork/non-canonical branch is wrong for downstream jobs

L51–55: on a non-canonical repo the script early-returns with sha=$(git rev-parse HEAD), but at that point HEAD is main from the fork — not the tag that triggered the workflow. All the build-* jobs will then check that SHA out instead of the tagged commit, so fork releases build from main rather than from what the user tagged. Should be sha=${{ github.sha }} (or $GITHUB_SHA) in that early-return branch.

Medium issues

4. No handling for branch-protected main

git push origin HEAD:main with GITHUB_TOKEN will fail if main requires PRs / status checks / signed commits, and the whole release pipeline aborts. Worth either: (a) catching the failure and continuing the release without the bump commit, (b) documenting the required protection exemption for github-actions[bot], or (c) switching to the PR-based flow from issue #1.

5. Race on the push itself

If anything else pushes to main between the checkout (L42) and the push (L73), git push non-fast-forwards and the job fails. A retry loop with rebase (git pull --rebase origin main then re-push) would make this bulletproof; alternatively accept the race since releases are rare.

6. Commit made with GITHUB_TOKEN won't trigger workflows

By design, commits pushed with GITHUB_TOKEN don't fire other workflows. If you have a push-branch CI on main you rely on to validate every commit, this one will silently skip it. Usually fine for a mechanical version bump, but worth being aware of.

Minor / nits

• L45 fetch-depth: 0 — unnecessary for a single-file edit + commit; the default shallow fetch works. Not harmful, just a wasted git clone.
• L64 sed: the pattern uses hardcoded 4 spaces in the replacement (XDEBUG_VERSION "…"), which happens to match php_xdebug.h:21 today. If the whitespace ever changes (tab, different count) the replace silently becomes a no-op and the whole downstream release ships the old version. Consider capturing and reusing the original whitespace, e.g.:

  sed -i -E "s/^(#define XDEBUG_VERSION[[:space:]]+)\"[^\"]*\"/\1\"${VERSION}\"/" php_xdebug.h

  Also worth a sanity check that the sed actually matched — currently a typo/mismatch results in git diff --quiet succeeding and the "already at version" branch running silently.
• L58 regex: -[0-9A-Za-z.-]+ is fine for semver pre-release tags, but doesn't accept build metadata (+…). Probably intentional — worth confirming.
• Step id push (L49): the step both bumps and pushes; either rename to bump or split into two steps for readability. steps.push.outputs.sha reads oddly when no push happened (fork/non-version-tag paths).
• Release job (L771) doesn't declare needs: bump-version — it inherits transitively via the build jobs, which works but is easy to break during future refactors. Explicit needs is cheap insurance.

Suggested overall shape

If you want to keep this in one workflow, the smallest change that resolves the correctness concerns is:

1. Bump php_xdebug.h on the tagged commit, not main.
2. Force-move the tag to the new commit so v0.3.0 == binaries shipped.
3. Fast-forward main to the new commit (or open a PR if protected).

If you'd rather avoid rewriting tags, the PR-based flow (workflow_dispatch with a version input → opens a bump PR → after merge, a follow-up job creates the tag on the merge commit) is cleaner but requires reworking how you kick off releases.

Happy to sketch either alternative if useful — Fix this →.
· Branch: feat/automatically-bump-version

@claude I modified the workflow to do a pre-release step. Can you do a new review?

Claude finished @carlos-granados's task in 1m 46s —— View job

Claude Code Review — v2 (post-rework)

• Read the new .github/workflows/prepare-release.yml
• Diff against previous approach; confirm which prior issues were addressed
• Look for new issues introduced by the split
• Post review feedback

Big improvement — splitting this out into a manually-dispatched pre-release workflow that bumps + tags in one commit resolves the core correctness problem from the last review: the tag now points at the same commit the release binaries are built from. Nice.

What the rework fixed

• ✅ Tag ↔ binary agreement. The tag is created after the bump commit, so git checkout 0.3.0 and the shipped .so/.dll now agree on the version string.
• ✅ release.yml no longer needs push permission or the fork-branch early-return dance — it just builds ${{ github.sha }} from the tag ref.
• ✅ No accidental main-drift. Since the release-maker triggers this deliberately, unrelated commits landing on main mid-release aren't picked up automatically anymore.
• ✅ Added version-format validation and a duplicate-tag guard up front — good fail-fast.

Issues remaining

1. sed can silently no-op (medium)

L40:

sed -i -E "s/^#define XDEBUG_VERSION[[:space:]]+\"[^\"]*\"/#define XDEBUG_VERSION    \"${VERSION}\"/" php_xdebug.h

If the pattern doesn't match (rename, whitespace change, someone reformats the header), sed exits 0, the file is unchanged, git diff --quiet succeeds, and the script prints "php_xdebug.h already at version X, nothing to commit" — then goes on to tag anyway. You now have a v0.3.0 tag pointing at code that says XDEBUG_VERSION "0.2.0", exactly the failure mode this PR is trying to prevent.

The "already at version" branch should positively verify the file contains the target version, not just infer it from a no-op diff:

if ! grep -qE "^#define XDEBUG_VERSION[[:space:]]+\"${VERSION}\"" php_xdebug.h; then
    echo "::error::sed did not update XDEBUG_VERSION to ${VERSION} — pattern likely no longer matches"
    exit 1
fi

Put this right after the sed (before the git diff --quiet check), so both the "bumped" and "already correct" paths pass through it.

2. Hardcoded whitespace in replacement (minor)

Same L40 — the replacement uses four literal spaces (XDEBUG_VERSION "…"). Matches php_xdebug.h:21 today, but if the file ever uses a tab or a different count the diff will contain a spurious whitespace change every release. Trivial to preserve with a capture group:

sed -i -E "s/^(#define XDEBUG_VERSION[[:space:]]+)\"[^\"]*\"/\1\"${VERSION}\"/" php_xdebug.h

3. Push to main will fail if main is branch-protected (medium, situational)

git push origin HEAD:main (L50) using GITHUB_TOKEN will be rejected if main requires PRs / reviews / status checks / signed commits. If protection is currently off, no immediate issue — worth deciding whether to (a) exempt github-actions[bot] in the branch protection rule, (b) switch to a PR-based flow, or (c) leave a note in the workflow so future-you knows why it broke when protection gets added.

4. Race on push (low)

If anything else lands on main between L18 (checkout) and L50 (push), the push non-fast-forwards and the whole prep aborts (no tag created — recoverable, just annoying). Since this is manually triggered and rare, probably fine to accept, but a small git pull --rebase && git push retry loop would eliminate it.

5. Tag-existence check is local-only (low)

L31: git rev-parse "refs/tags/${VERSION}" only sees tags that actions/checkout fetched. With fetch-depth: 0 this normally includes tags, but if you ever slim the checkout it silently regresses. Consider git ls-remote --exit-code --tags origin "refs/tags/${VERSION}" for a remote-authoritative check.

Nits

• fetch-depth: 0 (L21) is heavier than needed for a single-file edit — the default shallow fetch works, though tags need fetch-tags: true. Not a real cost issue.
• Tag naming. Existing tags in the repo are 0.1.0 / 0.2.0 (no v prefix), and the input description matches that — good, just calling it out so the convention doesn't drift.
• Job if (L16) restricts to php-debugger/php-debugger. Good — forks won't push tags upstream by accident when someone hits "Run workflow" on their fork.

Suggested minimal patch

Just addressing (1) and (2) — the two that turn into shipped-wrong-binaries if they trigger:

sed -i -E "s/^(#define XDEBUG_VERSION[[:space:]]+)\"[^\"]*\"/\1\"${VERSION}\"/" php_xdebug.h

if ! grep -qE "^#define XDEBUG_VERSION[[:space:]]+\"${VERSION}\"" php_xdebug.h; then
    echo "::error::Failed to set XDEBUG_VERSION to ${VERSION} in php_xdebug.h"
    exit 1
fi

Fix this →

· Branch: feat/automatically-bump-version

@claude all fixed, let's do a new review

Claude finished @carlos-granados's task in 1m 24s —— View job

Claude Code Review — v3 (post-rework)

• Load required tools
• Read the current .github/workflows/prepare-release.yml
• Diff against v2; verify fixes from prior review
• Look for any new issues
• Post updated review

All five issues from the v2 review are addressed cleanly. This is in good shape to merge.

v2 fixes — verified

Bonus: fetch-tags: false (L27) is a nice explicit cleanup now that the tag check is remote.

Small remaining observations (none blocking)

1. Rebase in a shallow clone. L26 is fetch-depth: 1, and the retry loop does git fetch origin main && git rebase origin/main. In practice this usually works — git fetch deepens the shallow history enough to establish reachability when needed — but if it ever misbehaves on a busy main, deepening the initial checkout (e.g. fetch-depth: 20) would be a bulletproof fix. Very low probability; noting only for future debuggability.

2. Retry loop doesn't git rebase --abort on failure. If the concurrent push on main touched php_xdebug.h, the rebase will halt mid-conflict and the next loop iteration's git rebase origin/main will refuse ("rebase in progress"). The only realistic way to hit this is two concurrent version bumps, which shouldn't happen. Cheap belt-and-braces:

   git rebase origin/main || { git rebase --abort; continue; }

3. Tag-then-push isn't atomic. If the commit push succeeds but the tag push fails at L77, the bump commit is on main without its tag. Rerunning the workflow with the same version recovers correctly (sed no-op → grep passes → git diff --quiet true → tag current HEAD → push), so this is self-healing. No action needed, just worth knowing the recovery path.

Nit

• L67 error message: "after ${attempt} attempts" — at the point that branch fires, $attempt is 5, but the actual retry count includes the initial try and 4 rebase attempts. Reads fine either way; no change necessary.

Verdict

LGTM. The correctness-critical concerns (sed silent no-op, tag/binary mismatch, tag-existence race with remote) are all handled. Remaining items are edge-cases that are either self-recovering or extremely unlikely in practice.
· Branch: feat/automatically-bump-version

@claude new fixes, please another review

Claude finished @carlos-granados's task in 1m 4s —— View job

Claude Code Review — v4 (post-rework)

• Load required tools
• Read the current .github/workflows/prepare-release.yml
• Diff against v3; verify fixes from prior review
• Look for any new issues
• Post updated review

All non-blocking items from the v3 review are now either fixed or explicitly documented as intentional. Nothing new introduced. LGTM to merge.

v3 observations — status

Small observations (none blocking)

1. Redundant continue at L72 (nit only). The continue is the last statement in the loop body, so control would flow to the next iteration anyway. Harmless — arguably even a nice bit of explicitness signaling intent. No change needed.

2. Push-then-rebase ordering is right, but attempt 5 skips a final rebase-then-push. The loop is 5 push attempts with 4 rebases interleaved. On attempt 5, if the push fails, we bail immediately without a final rebase attempt. That's the correct behavior in practice (5 consecutive push failures ≈ genuine problem worth surfacing), just noting it so a future reader doesn't "fix" it into an infinite-retry loop.

3. No monotonicity check on the version input. Nothing stops someone from dispatching with 0.1.5 after 0.3.0 has shipped. The duplicate-tag guard would catch reusing an existing version, but not going backwards to a new-but-earlier version. Release-maker discipline covers this; not worth automating unless it becomes a real footgun.

Verdict

Ship it. The correctness-critical paths (sed silent no-op, tag/binary mismatch, tag-existence race, mid-rebase conflict wedge, self-healing tag-push recovery) are all handled. Remaining items are either documented tradeoffs or purely stylistic.
· Branch: feat/automatically-bump-version

@claude I did some changes after Copilot review, can you take a new look?

Claude finished @carlos-granados's task in 1m 54s —— View job

Claude Code Review — v5 (post-Copilot review)

• Load required tools
• Read the current .github/workflows/prepare-release.yml
• Diff against v4; verify fixes addressing Copilot's feedback
• Look for any new issues
• Post updated review

All four Copilot findings are addressed cleanly, and the fix for the "always tag HEAD" issue is more careful than Copilot suggested — it uses git blame to locate the actual bump commit, which correctly handles the self-healing rerun scenario. LGTM to merge.

Copilot's review — status

Verifying the new blame logic holds up

The new tag-resolution is the most substantive change; walking the paths:

• Happy path (sed changes the file, single push succeeds) — HEAD is the bump commit; blame returns HEAD's SHA; tag matches HEAD. ✅
• Retry-with-rebase path — the rebase produces a new SHA for the bump commit (different parent). Blame runs against the post-rebase file, so it returns the correct new SHA. Concurrent commits that landed on main sit before the bump in linear history, and the tag correctly points at the version-bump-inclusive commit. ✅
• Self-healing rerun (previous run committed but tag-push failed) — fresh checkout has the previous bump commit in history; sed no-ops; git diff --quiet skips commit; blame finds the previous run's bump SHA and tags it. This is exactly the behavior Copilot flagged as missing. ✅

Additional invariants verified:

• Only one #define XDEBUG_VERSION line exists in php_xdebug.h:21, so grep ... | head -1 is unambiguous.
• The grep -qE on L51 guarantees the line exists before L86 runs, so LINE is never empty and git blame -L "${LINE},${LINE}" never gets a malformed range.
• git blame -l outputs full 40-char SHAs (not abbreviated), so git tag "${VERSION}" "${BUMP_SHA}" won't be ambiguous.

Very minor observations (none blocking)

1. Rerun-tags-an-old-commit corner case. If someone (in some unrelated past commit) had ever set XDEBUG_VERSION to a value that later becomes the target release version, git blame would find that old commit and tag it rather than a fresh bump commit. Requires the version line to already contain the target value on entry — vanishingly unlikely in practice, and honestly the release-maker should notice the "already at version" log message and investigate. Not worth defending against.

2. head -1 vs head -n 1. POSIX-purism nit; GNU head on ubuntu-latest accepts both. No change needed.

Verdict

Ship it. Every correctness-critical concern raised across the four review rounds is now handled — including the subtle "tag the bump commit, not HEAD" fix. Nice iteration.

· Branch: feat/automatically-bump-version