fix(tag-release): use Git Data API so commits and tags auto-sign#56
Merged
Conversation
These tests are intentionally failing — they pin the contract that the Git Data API rewrite of tag-release.yml will consume. The manifest must be a unique path set so POST /git/trees does not receive duplicate tree[] entries. Refs cross-agent-reviews#12.
…phase pass
Code-quality review found that Test 4 (no cross-run pollution) and Test 2
(unique-paths in mixed run) could pass for the wrong reason during the
red phase if /tmp/bump.modified didn't exist:
- Test 4: `! grep -q server.json /tmp/bump.modified` returned true on a
missing file (grep fails, ! flips), masking that the script wasn't
creating the manifest at all. Add explicit `[ -f /tmp/bump.modified ]`
guard before the negated grep.
- Test 2: `actual=$(sort /tmp/bump.modified)` swallowed the missing-file
error into an empty `actual`, producing a content-mismatch message
that hid the real problem. Switch to `< /tmp/bump.modified` so the
shell-level open fails loudly with No such file or directory.
Also clarify Test 3's confusing inline comment about idempotent rerun.
Refs cross-agent-reviews#12.
Manifest of modified paths consumed by tag-release.yml's Git Data API rewrite. De-duped via sort -u so multi-entry-same-file configs (two path_expr entries against one file) produce one path, not two — which would create duplicate tree[] entries in POST /git/trees. Refs cross-agent-reviews#12.
Mirror the manifest-emission changes from scripts/bump-version-files.sh into the inline copy embedded in tag-release.yml, satisfying the inline-sync invariant validated by scripts/check-inline-sync.sh. Refs cross-agent-reviews#12.
Static checks that the rewritten Bump and Tag steps do not regress to local-state reads (which would reintroduce the silent-overwrite race the API rewrite is designed to prevent) and that bot-signing-disabling identity fields stay out of API payloads. Phase B-only checks (GITHUB_SHA usage, no identity fields) skip on the current Phase A baseline and activate after the workflow rewrite lands. Refs cross-agent-reviews#12.
Replace `git commit && git push origin HEAD:main` with a sequence of gh api calls: blobs → tree → commit → PATCH ref. The commit's parent is BASE_SHA=$GITHUB_SHA (the runner's checkout SHA), not a fresh read of main, to avoid the silent-overwrite race documented in spec §2. The commit payload omits author/committer fields so GitHub auto-signs with its bot key (spec §3.3) — satisfying the required_signatures branch ruleset on consumer repos like cross-agent-reviews without needing bypass-actor entries. The exit-2 branch (no bump created) reads main HEAD via API and exports TAG_TARGET_SHA for the tag step. Fixes cross-agent-reviews#12.
Code-quality review against GitHub's REST API docs revealed the plan and
spec had inherited a wrong jq path: GET /repos/.../git/commits/{sha}
returns the raw Git commit object with `verification` at the TOP level,
not under `.commit.verification` (that nesting only exists on the higher-
level GET /repos/.../commits/{sha} endpoint).
With the wrong path, $VERIFIED would always be the literal string "null"
and the acceptance-criterion-#2 check would always fail with exit 1
AFTER the bump commit had already been PATCH'd onto main — leaving a
dangling commit and no tag.
Fix: keep the low-level /git/commits/{sha} endpoint (consistent with the
PARENT_TREE_SHA call earlier in the same step) but use .verification.*
jq paths instead. Add a comment documenting the gotcha so future
maintainers don't reintroduce it.
Refs cross-agent-reviews#12.
Replace `git tag -a && git push origin <tag>` with POST /git/tags + POST /git/refs. Tag points at TAG_TARGET_SHA exported by the prior Bump step (new commit SHA in the happy path; current main HEAD via API in the no-bump case). Tag payload omits the tagger field so GitHub auto-signs the tag object with its bot key when possible. The verification.verified status is reported in the workflow step summary so the canary run can record acceptance criterion #3 (signed) vs #4 (unsigned-fallback) per spec §6. Fixes cross-agent-reviews#12.
Three review findings on the signed-commit release path: - Finding 2: the case-0 PATCH to refs/heads/main now runs *after* the verification check, not before. A commit object is addressable by SHA the instant POST /git/commits returns, so verification needs no ref update first. Verifying first means a verification failure leaves main untouched — no dangling unverified commit for a later rerun to pick up. - Finding 1: the no-bump (exit 2) path now tags GITHUB_SHA — the commit "Compute release" semver-analyzed — instead of a live GET /git/ref/heads/main read. The live read raced with concurrent pushes (spec §2) and could tag an unanalyzed commit. GITHUB_SHA also covers rerun-after-partial-failure: a fresh dispatch resolves it to current main HEAD anyway. - Finding 4: manifest path is now configurable via BUMP_MODIFIED_FILE (default unchanged) so concurrent script/test runs don't clobber a shared /tmp file. The bats suite pins a per-test path; the inline copy stays in sync.
Finding 3: the parent-SHA invariant test previously only checked that GITHUB_SHA appeared somewhere in the bump step — a stray echo would satisfy it. It now asserts the full load-bearing chain: GITHUB_SHA -> BASE_SHA -> jq --arg parent -> commit payload. Adds a companion guard (Finding 1 regression): the bump step must not perform a live `GET .../git/ref/heads/` read. The singular-`ref` GET form is forbidden; the legitimate fast-forward PATCH uses the plural `git/refs/heads/main` and is unaffected.
The tag step comment claimed case 2 reads refs/heads/main via GET /git/ref/heads/main; the implementation exports GITHUB_SHA (the semver-analyzed commit). Correct the comment so it cannot mislead a maintainer into reintroducing the live-ref race. Add a RECOVERY NOTE next to the main-advancing PATCH: after a partial failure (bump pushed, tag not yet created), GitHub's "Re-run failed jobs" 422s because it recreates a sibling bump commit; recovery is a fresh workflow_dispatch. Documents review Finding 1 as an accepted, signposted limitation. Refs #56
The RECOVERY NOTE claimed a fresh workflow_dispatch unconditionally takes the no-bump path. That holds only if the recovery run reuses the original bump and tag-prefix inputs: "Compute release" derives the version from the last tag (unchanged — the tag was never created), so an explicit bump override or a different tag-prefix recomputes a different version and produces a second, wrong bump commit. Correct the comment to state the input-reuse requirement and why, and surface the same guidance at runtime: case 0 (the only path that advances main) now prints a partial-failure recovery hint into the step summary, visible on a failed run without reading the workflow. Refs #56
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
1 participant
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.
Summary
tag-release.yml's bump-commit step to use the GitHub Git Data API (POST /git/blobs→POST /git/trees→POST /git/commits→PATCH /git/refs/heads/main). The commit's parent isBASE_SHA=$GITHUB_SHA, not a fresh read ofmain, to avoid the silent-overwrite race documented in the spec.POST /git/tags+POST /git/refsagainst either the new bump commit's SHA ormain's current HEAD (via API, never local checkout state).author/committer/taggerfields so GitHub auto-signs with its bot/web-flow key — satisfying therequired_signaturesbranch ruleset on consumer repos without bypass-actor entries./tmp/bump.modifiedmanifest emission (unique path set) toscripts/bump-version-files.shso the API path knows which files to put in the tree.The verification check uses the low-level
GET /git/commits/{sha}endpoint with jq path.verification.verified(top-level), not the higher-level/commits/{sha}endpoint with.commit.verification.verified— a distinction that bit us during review and is now documented in-line.See the design notes in the spec authored alongside this PR (kept locally per the project's docs/ workflow; not committed) for the full reasoning behind the design invariant, race-detection backstop, and acceptance criteria.
Test plan
bash scripts/check-inline-sync.shpassesbash scripts/lint-workflow-call.shpassesbats tests/bump-version-files.bats— 50/50 (existing 46 + 4 new manifest tests)bats tests/tag-release-invariants.bats— 4/4bats tests/suite — 92/92python3 yaml.safe_load(...)git config user.*,git tag -a, orgit push originpatterns remain intag-release.ymlfix:+ 3 ×test:, 0 ×feat:(no inadvertent minor-bump trigger)v2.5.3-rc1(manual —release.ymlisworkflow_callonly, not push-driven), pincross-agent-reviewsto RC SHA, run Tag Release oncross-agent-reviewsviaworkflow_dispatch, verify acceptance criteria deps: bump step-security/harden-runner from 2.16.0 to 2.16.1 #1–fix: use PR author instead of github.actor for bot detection #7 from spec §6v2.5.3from RC SHA (manual replay ofrelease.yml's two steps — floatingv2/v2.5tag updates +gh release create v2.5.3), bumpcross-agent-reviewstov2.5.3finalRefs j7an/cross-agent-reviews#12. Not auto-closed on merge:
cross-agent-reviewsmust still adopt the resultingv2.5.3tag release (the two unchecked post-merge items above) before #12 is fully resolved.