chore: rolling promotion dev -> main (consolidated release-chain fix) - #2649
Closed
namastex888 wants to merge 6 commits into
Closed
namastex888 wants to merge 6 commits into
namastex888 wants to merge 6 commits into
Conversation
The generic SLSA provenance embeds the workflow_run that TRIGGERED the release, whose head is the commit CI ran on — one before the [auto-version] bump the tarballs are built from. Nothing asserted that this value is actually threaded through version.yml -> release.yml -> sign-attest.yml + release-publish.yml, so dropping it from any single 'with:' block would silently re-break exact verification (or hard-fail on an empty TRIGGER_SHA) exactly as it did on 2026-07-25. Pins the full chain, the verifier comparing the trigger commit rather than source_sha, and the per-channel descriptor comparison.
The previous fix wired trigger_sha into sign-attest.yml's native-predicate step (line 340), whose script never reads it, while the step that actually runs 'release-generic-provenance.sh verify-exact' had no TRIGGER_SHA at all. verify_automated_event requires a well-formed one on dev/homolog and exits 2 without it — so the next dev release would have died at the same place with the same silent exit, a fourth consecutive broken release. The wiring test did not catch it because it used a whole-file toContain(), which the misplaced value satisfied. It now resolves the step that consumes each value and asserts the env of THAT step; verified to fail against the misplacement and pass once corrected. - CRITICAL: TRIGGER_SHA into the 'Verify SLSA provenance (self-check)' env; dead assignment removed from the native-predicate step - dead TRIGGER_SHA removed from release-publish prepare-delivery-evidence (no consumer); the live consumer in the security gate is untouched - dev version derivation uses max+1 instead of count+1, so a burned build number cannot collide, and a tag collision is now a hard error instead of being reported as a benign race that drops the release with a green run
fix(release): deliver TRIGGER_SHA to the verifying step + consolidated chain fixes
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughRelease workflows now allocate development versions from the highest existing tag suffix and report tag collisions explicitly. Package and plugin manifests move to ChangesRelease integrity
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/version.yml:
- Around line 183-190: The local version allocator in scripts/version.ts must
use the highest numeric matching tag suffix plus one, rather than counting
matching tags. Update the generator’s tag lookup and BUILD_NUMBER calculation to
ignore gaps and select the next unused suffix, keeping its version format and
other generation behavior unchanged.
- Around line 322-329: Update the collision check in the git push failure
handling to report a tag collision only when push_output identifies
refs/tags/v${VERSION}; do not treat a generic “cannot lock ref” mentioning
refs/heads/dev as a tag collision. Preserve the existing error message and exit
behavior when the release tag is confirmed to collide.
In `@scripts/release-docs.test.ts`:
- Around line 504-510: Strengthen the assertions in the test covering verifyStep
and gateStep to match the complete TRIGGER_SHA assignment, including the
expected `${{ inputs.trigger_sha }}` value, rather than checking each substring
independently. Ensure both the signing verification step and the publish gate
validate the binding itself.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 723d66fa-b322-42f6-b498-101da7d89a39
📒 Files selected for processing (10)
.claude-plugin/marketplace.json.github/workflows/release-publish.yml.github/workflows/sign-attest.yml.github/workflows/version.ymlpackage.jsonplugins/genie/.claude-plugin/plugin.jsonplugins/genie/.codex-plugin/plugin.jsonplugins/genie/package.jsonplugins/hermes-genie/plugin.yamlscripts/release-docs.test.ts
💤 Files with no reviewable changes (1)
- .github/workflows/release-publish.yml
Comment on lines
+183
to
+190
| # Highest existing build number + 1, never the count: a gap (a tag | ||
| # deleted, or a build number burned by a failed release) would make | ||
| # count+1 collide with an existing tag, and the push failure below is | ||
| # reported as a benign race, silently dropping the release. | ||
| HIGHEST=$(git tag --list "v${PREFIX}.${TODAY}.*" \ | ||
| | sed -n "s|^v${PREFIX}\.${TODAY}\.\([0-9][0-9]*\)$|\1|p" \ | ||
| | sort -n | tail -1) | ||
| BUILD_NUMBER=$(( ${HIGHEST:-0} + 1 )) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep the local allocator aligned with this max-plus-one rule.
scripts/version.ts still counts matching tags, so a missing suffix can make local generation select an existing version while this workflow selects the next free one. Update that generator to compute the highest numeric suffix too, or centralize allocation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/version.yml around lines 183 - 190, The local version
allocator in scripts/version.ts must use the highest numeric matching tag suffix
plus one, rather than counting matching tags. Update the generator’s tag lookup
and BUILD_NUMBER calculation to ignore gaps and select the next unused suffix,
keeping its version format and other generation behavior unchanged.
Comment on lines
+322
to
+329
| if ! push_output=$(git push --atomic origin "HEAD:refs/heads/dev" "refs/tags/v${VERSION}" 2>&1); then | ||
| printf '%s\n' "$push_output" >&2 | ||
| # A tag collision is a version-derivation defect, not a race: exiting | ||
| # 0 here would drop the release with a green run and no diagnosis. | ||
| if printf '%s' "$push_output" | grep -qiE 'already exists|cannot lock ref|tag .* exists'; then | ||
| echo "::error ::release-version.tag-collision v${VERSION} already exists; version derivation picked a used build number" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Restrict ref-lock collision detection to the release tag.
cannot lock ref can refer to refs/heads/dev; this converts the intended benign branch-advance skip into a hard tag-collision failure. Require that the output identifies refs/tags/v${VERSION} before reporting a tag collision.
Proposed fix
- if printf '%s' "$push_output" | grep -qiE 'already exists|cannot lock ref|tag .* exists'; then
+ if printf '%s' "$push_output" | grep -qiE "refs/tags/v${VERSION}" &&
+ printf '%s' "$push_output" | grep -qiE 'already exists|cannot lock ref|tag .* exists'; then📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if ! push_output=$(git push --atomic origin "HEAD:refs/heads/dev" "refs/tags/v${VERSION}" 2>&1); then | |
| printf '%s\n' "$push_output" >&2 | |
| # A tag collision is a version-derivation defect, not a race: exiting | |
| # 0 here would drop the release with a green run and no diagnosis. | |
| if printf '%s' "$push_output" | grep -qiE 'already exists|cannot lock ref|tag .* exists'; then | |
| echo "::error ::release-version.tag-collision v${VERSION} already exists; version derivation picked a used build number" | |
| exit 1 | |
| fi | |
| if ! push_output=$(git push --atomic origin "HEAD:refs/heads/dev" "refs/tags/v${VERSION}" 2>&1); then | |
| printf '%s\n' "$push_output" >&2 | |
| # A tag collision is a version-derivation defect, not a race: exiting | |
| # 0 here would drop the release with a green run and no diagnosis. | |
| if printf '%s' "$push_output" | grep -qiE "refs/tags/v${VERSION}" && | |
| printf '%s' "$push_output" | grep -qiE 'already exists|cannot lock ref|tag .* exists'; then | |
| echo "::error ::release-version.tag-collision v${VERSION} already exists; version derivation picked a used build number" | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/version.yml around lines 322 - 329, Update the collision
check in the git push failure handling to report a tag collision only when
push_output identifies refs/tags/v${VERSION}; do not treat a generic “cannot
lock ref” mentioning refs/heads/dev as a tag collision. Preserve the existing
error message and exit behavior when the release tag is confirmed to collide.
Comment on lines
+504
to
+510
| const verifyStep = stepEnvFor(signAttest, 'release-generic-provenance.sh verify-exact'); | ||
| expect(verifyStep).toContain('TRIGGER_SHA:'); | ||
| expect(verifyStep).toContain('${{ inputs.trigger_sha }}'); | ||
|
|
||
| const gateStep = stepEnvFor(publish, 'generic_expected_sha'); | ||
| expect(gateStep).toContain('TRIGGER_SHA:'); | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the TRIGGER_SHA binding itself.
These independent substring checks pass if TRIGGER_SHA is assigned the wrong value while another field in the step contains ${{ inputs.trigger_sha }}. Match the complete assignment, including in the publish gate.
Proposed fix
- expect(verifyStep).toContain('TRIGGER_SHA:');
- expect(verifyStep).toContain('${{ inputs.trigger_sha }}');
+ expect(verifyStep).toMatch(/TRIGGER_SHA:\s*\$\{\{ inputs\.trigger_sha \}\}/);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/release-docs.test.ts` around lines 504 - 510, Strengthen the
assertions in the test covering verifyStep and gateStep to match the complete
TRIGGER_SHA assignment, including the expected `${{ inputs.trigger_sha }}`
value, rather than checking each substring independently. Ensure both the
signing verification step and the publish gate validate the binding itself.
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
2 participants
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.
Promotes the consolidated release-chain fix (#2648,
58500117) tomain.This is the promotion that matters. The release chain executes from
main—sign-attest.yml,release-publish.ymland friends are always read fromrefs/heads/main, never fromdev. So the fix has no effect until this lands, and the dev release firing right now will still fail.What it carries
TRIGGER_SHAreaches the env of the step that actually runsrelease-generic-provenance.sh verify-exact. The prior fix put it in the native-predicate step, whose script never reads it;verify_automated_eventrequires it on dev/homolog andexit 2s silently without it. Without this, the next dev release dies at the same place for the fourth time.TRIGGER_SHAremoved fromrelease-publish.ymlprepare-delivery-evidence(no consumer); the live consumer in the security gate is untouched.max+1instead ofcount+1, and a tag collision is now a hard error rather than being reported as a benign race that drops a release with a green run.mainagain.Audit basis
A full static audit of the chain at
main@93243cf0covered secrets, environment gates, verification-script satisfiability, permissions, the App-token push path, previous-release state and orphaned tags: 1 must-fix for dev (fixed here), 0 for stable.Known risk after merge
PR #2624 grew
release-publish.ymlfrom 3 jobs to 10. Seven publish-side jobs have never executed — the last run to reach publish used the old 3-job workflow. The next dev release is their first production run, including a 4-platform native Codex dogfood. Static analysis found no unsatisfiable assertion in them, but treat that run as a shakeout of unexercised surface, not as verification of this fix.Summary by CodeRabbit
Release
Bug Fixes
Tests