fix(release): prove one staged candidate set before publication - #391
Conversation
Summary by CodeRabbit
WalkthroughThe release workflow binds jobs to an immutable commit, stages a four-platform candidate, validates metadata and binaries, performs first-use acceptance, runs negative controls, and records readiness evidence before human-authorized publication. ChangesRelease candidate workflow
Estimated code review effort: 5 (Critical) | ~90 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
CI budget acknowledgementThe PR planner selected the release-package, workflow-policy, and CLI-product risk packs: 11 lanes at an estimated ~165 LEM, placing this in the hard band. The spend is justified because this PR changes the exact release artifact trust boundary, platform smoke scripts, workflow policy surface, and cross-platform integration proof together. Splitting the workflow from the scripts/tests would create an intermediate contract that cannot prove the bytes it selects and would run overlapping release/policy lanes multiple times. Skipped nightly, coverage, mutation, and release-only lanes are not claimed as executed proof. A real explicit-tag run remains required before #245 can close. |
EffortlessSteven
left a comment
EffortlessSteven
left a comment
There was a problem hiding this comment.
[P1] Make the staged-candidate success path pass receipt validation
Failure mode:
The exact-head hosted Shiplog Rust Small Result run for this PR fails staged_candidate_smoke_uses_local_bundle_and_emits_receipts: report validate --path "$latest_run/intake.report.json" --receipts rejects the generated report because reports.json points at .../out/merge_.../out/merge_.../intake.report.json while the validated file is .../out/merge_.../intake.report.json.
Why here:
The new staged-candidate smoke contract makes this validation a required release gate at scripts/release-install-smoke.sh:258, so the PR's exact hosted gate is currently red even though the other three new tests pass.
Fix direction:
Align the receipt path contract used by the cold-start command and report validate --receipts—either emit the canonical run-relative path expected by the validator or resolve an already run-rooted path without prepending out/<run> again. Keep the regression in release_candidate_smoke so this exact candidate path stays green.
Validation:
Rerun cargo test -p shiplog --test release_candidate_smoke --locked and the routed Shiplog Rust Small Result check on the updated exact head.
Confidence:
High; reproduced in hosted run 30899824202, job 91961327124.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/release.yml (1)
304-316: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAlign candidate retention with the receipt retention.
The staged candidate set expires after 5 days. The readiness receipt at Line 732 is retained for 30 days and records
candidate_artifact_idandcandidate_artifact_digest. For 25 of those days the receipt names bytes that no longer exist, so a human cannot re-verify the digest before authorizing publication.Set
retention-dayshere to match the receipt, or shorten the receipt retention.🤖 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/release.yml around lines 304 - 316, Update the retention-days setting in the “Retain exact staged release candidates” step to 30 days, matching the readiness receipt retention and preserving candidate artifacts for the full verification window.
🤖 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/release.yml:
- Around line 516-526: Add an exact-commit verification step immediately after
the checkout step in the job containing “Checkout release scripts,” comparing
git rev-parse HEAD with needs.release-preflight.outputs.release_sha and failing
on mismatch. Keep the existing artifact download and release-install-smoke.sh
flow unchanged.
- Around line 286-296: Update the release candidate metadata generation around
RELEASE_CANDIDATE.txt to derive asset_count from the expected asset array
defined earlier in the workflow instead of hardcoding 4. Ensure the computed
count is written to the asset_count field so validators and smoke tests reflect
every configured target.
- Around line 254-257: Update every actions/download-artifact reference in the
release workflow, including the steps named “Download all binary artifacts” and
the other listed usages, to the immutable commit SHA
3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c and retain a # v8 comment on each
reference.
- Around line 228-230: Update the release workflow step’s shell commands so both
handoff messages, including the statement that Swarm cannot create a GitHub
release, are appended to GITHUB_STEP_SUMMARY rather than only printed to stdout.
Preserve the existing message text and ordering.
In `@apps/shiplog/tests/release_candidate_smoke.rs`:
- Around line 101-107: In the release-candidate smoke test, update the
filesystem operations around the Unix permission setup and the additional sites
near lines 209, 229, 231, 253, and 255 to attach context before propagating
errors. Use static .context descriptions for fixed operations and .with_context
closures with the relevant path for dynamic operations, covering metadata,
set_permissions, and other bare filesystem ? calls while preserving the existing
behavior.
- Around line 246-248: Update scripts/release-install-smoke.ps1 to emit the
distinct “candidate binary failed --version” signal when the candidate command
exits with a nonzero $LASTEXITCODE, while retaining “unexpected version output”
for successful commands with mismatched output. Then remove the #[cfg(unix)]
gate from staged_candidate_smoke_rejects_broken_command_with_matching_checksum
so the same rejection assertion runs on Windows.
- Around line 16-21: Move CandidateFixture and candidate_fixture() out of
release_candidate_smoke.rs into the reusable shiplog-testkit::fixtures module or
an appropriate release-scenario submodule. Preserve the existing
release-candidate metadata and bundle setup, then update the smoke test imports
and usages to reference the shared fixture implementation.
In `@scripts/release-install-smoke.ps1`:
- Around line 66-73: Rename the local $matches collection in the candidate
bundle lookup to a non-automatic variable, and update its Count, error message,
and index references consistently; leave the Assert-CandidateManifest -notmatch
usage unchanged.
In `@scripts/release-install-smoke.sh`:
- Around line 259-264: Normalize `binary_path` to an absolute path before the
subshell changes directories, matching the existing candidate-path normalization
pattern near the release smoke setup. Ensure the invocation in the
`cold_start_dir` subshell uses that absolute `binary_path` while preserving the
current validation arguments.
- Around line 119-134: Update the Bash manifest validation before the existing
schema_version, release_tag, source_sha, and asset_count assertions to detect
and reject duplicate manifest keys, matching the parsing behavior in the
PowerShell validator. Ensure each required field appears exactly once while
preserving the current field-value checks and error handling.
- Around line 188-192: Update the checksum lookup in the SHA256SUMS verification
block to match both flat filenames and directory-prefixed entries, reusing the
basename-matching behavior from the PowerShell smoke script. Escape the asset
value before using it in the awk regular expression, while preserving the
existing missing-entry failure behavior.
---
Outside diff comments:
In @.github/workflows/release.yml:
- Around line 304-316: Update the retention-days setting in the “Retain exact
staged release candidates” step to 30 days, matching the readiness receipt
retention and preserving candidate artifacts for the full verification window.
🪄 Autofix
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3dd83058-a03d-4c42-9260-fb97321a663a
📒 Files selected for processing (4)
.github/workflows/release.ymlapps/shiplog/tests/release_candidate_smoke.rsscripts/release-install-smoke.ps1scripts/release-install-smoke.sh
EffortlessSteven
left a comment
EffortlessSteven
left a comment
There was a problem hiding this comment.
[P1] Assert the negative-control checkout resolves the preflight SHA
Failure mode:
The release-candidate-negative-controls job checks out ${{ needs.release-preflight.outputs.release_tag }} and immediately runs scripts/release-install-smoke.sh, but never compares git rev-parse HEAD with ${{ needs.release-preflight.outputs.release_sha }}. If the tag resolution changes between preflight and this job, the negative controls can execute a different release-script revision while reporting evidence for the preflight candidate.
Why here:
This PR's core contract is one immutable exact-tag candidate set. Every job that executes release scripts must prove the checkout identity, not only the artifact manifest identity.
Fix direction:
Add the same exact-commit assertion used by the other release jobs immediately after the checkout:
test "$(git rev-parse HEAD)" = "${{ needs.release-preflight.outputs.release_sha }}".
Retain the existing staged-artifact and negative-control assertions.
Validation:
Exercise a tag/ref mismatch fixture or equivalent workflow proof and confirm the negative-control job fails before invoking either smoke script.
Confidence:
High; inspected current head 595e6152c3370c85a069caf9f12b5264c2d5c1ce, .github/workflows/release.yml:516-520.
EffortlessSteven
added
the
full-ci
ChatGPT Codex Connector
|
@codex review Please review the current final head, especially exact tag/source/artifact binding, complete four-platform bundle validation, failure controls, matrix consumption of staged bytes, and the terminal readiness aggregate. |
![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: 46fd16c469
ℹ️ 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".
| git fetch --force --depth=1 origin "refs/tags/$tag:refs/tags/$tag" | ||
| git checkout --detach "$tag" |
There was a problem hiding this comment.
Failure mode: On a tag-push run, this force-fetch resolves the tag from its current remote value instead of binding it to the triggering github.sha; if the tag is moved after the webhook is created but before preflight executes, the workflow can build and certify a different commit while still reporting success for the original tag event. Why here: every downstream comparison uses the newly resolved release_sha, so none detects this initial substitution. Fix direction: for push events, require the peeled tag commit to equal the event SHA (while retaining current-tag resolution for approved manual dispatches). Validation: trigger a run and retarget the tag before this fetch, then verify preflight rejects the mismatch. Confidence: high. This is particularly important because tag and release operations remain source-repository authority.
AGENTS.md reference: AGENTS.md:L14-L17
Useful? React with 👍 / 👎.
|
@codex address that feedback Address the current workflow-only findings without broadening scope:
Do not move the release fixture to |
Summary
Testing
|
80b7dd3 to
27ed2a4
Compare
|
Droid encountered an error —— View job Droid is reviewing code and running a security check… |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@scripts/release-install-smoke.sh`:
- Around line 185-188: Normalize both source SHA values to lowercase before the
comparison in the release smoke script, reusing the existing lower_hex
normalization used elsewhere. Update the corresponding comparison in the
PowerShell script as well, preserving the existing mismatch error and exit
behavior.
🪄 Autofix
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1e389ed9-35c9-4477-b1c6-39b290ada800
📒 Files selected for processing (5)
.github/workflows/release.ymlapps/shiplog/tests/release_candidate_smoke.rspolicy/workflow-allowlist.tomlscripts/release-install-smoke.ps1scripts/release-install-smoke.sh
| [[ "$source_sha" == "$expected_source_sha" ]] || { | ||
| echo "candidate manifest is not bound to source commit $expected_source_sha" >&2 | ||
| exit 1 | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Normalize case before the source_sha comparison.
Line 127 accepts SHIPLOG_RELEASE_SOURCE_SHA in either case, but Line 185 compares it literally against the manifest value. An uppercase input then fails against a lowercase manifest value even though both identify the same commit. The file already added lower_hex for this reason at Lines 215 and 274. scripts/release-install-smoke.ps1 Line 145 has the same case-sensitive comparison.
♻️ Proposed refactor
- [[ "$source_sha" == "$expected_source_sha" ]] || {
+ [[ "$(lower_hex "$source_sha")" == "$(lower_hex "$expected_source_sha")" ]] || {
echo "candidate manifest is not bound to source commit $expected_source_sha" >&2
exit 1
}📝 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.
| [[ "$source_sha" == "$expected_source_sha" ]] || { | |
| echo "candidate manifest is not bound to source commit $expected_source_sha" >&2 | |
| exit 1 | |
| } | |
| [[ "$(lower_hex "$source_sha")" == "$(lower_hex "$expected_source_sha")" ]] || { | |
| echo "candidate manifest is not bound to source commit $expected_source_sha" >&2 | |
| exit 1 | |
| } |
🤖 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-install-smoke.sh` around lines 185 - 188, Normalize both
source SHA values to lowercase before the comparison in the release smoke
script, reusing the existing lower_hex normalization used elsewhere. Update the
corresponding comparison in the PowerShell script as well, preserving the
existing mismatch error and exit behavior.
1 participant
Final exact-head proof
Current consolidated head:
27ed2a4bed4d6c8cb826d8168c9fa2922f7e1591Observed locally on that exact head:
cargo fmt --all -- --check— passbash -n scripts/release-install-smoke.sh— passcargo clippy -p shiplog --all-targets --locked -- -D warnings— passcargo test -p shiplog --test release_candidate_smoke --locked— 9 passedcargo xtask check-workflows --mode blocking-allowlist— passcargo xtask check-file-policy --mode blocking-allowlist— passcargo xtask ci-small— not verified; the lane timed out with an orphaned stalled process and was terminatedThe focused suite now covers success/receipt retention, incomplete and duplicate/unknown manifests, workflow identity, checksum mismatch, flat checksum entries, broken executables, and the tag-push/negative-control workflow contract. CandidateFixture remains intentionally private to this integration target.
The exact-tag/source-writer live release proof remains out of scope here; #245 stays open until the source counterpart and an authorized non-public exact-tag run provide observed evidence.
What changed Implements the still-open staged-artifact portion of #245 in the authoritative development repository. ### One exact candidate set -
release-preflightresolves both the explicit semver tag and its exact 40-character commit SHA; - every build and downstream job proves it checked out that exact commit; -Stage Release Candidatesassembles exactly one copy of all four platform binaries, deterministicSHA256SUMS.txt, and a boundedRELEASE_CANDIDATE.txtmanifest containing tag, source SHA, repository, workflow run identity, asset count, and checksum-manifest digest; - the retainedverified-release-candidatesworkflow artifact exposes its artifact id/digest and manifest hashes to downstream jobs. ### Acceptance consumes staged bytes - Linux, macOS Intel, macOS Apple Silicon, and Windows first-use jobs now wait for candidate staging and download the sameverified-release-candidatesartifact; - Bash and PowerShell smoke scripts acceptSHIPLOG_RELEASE_CANDIDATE_DIRplus exactSHIPLOG_RELEASE_SOURCE_SHA, require the complete four-binary bundle, validate manifest identity/checksums, and exercise the local candidate from an empty directory with provider credentials cleared; - public-release download mode remains available for post-publication verification; - each platform uploads produced receipts and diagnostics even on failure. ### Fail-closed release authority - a Linux negative-control job corrupts the staged binary without changing checksums and proves the gate rejects it; - a second control replaces the binary with a failing executable, updates checksums consistently, and proves command execution still rejects it; - terminalRelease Candidate Readyrequires preflight, staging, release validation, all four first-use lanes, both negative controls, and release-mode tests; - it emits a durable receipt with exact tag/source/artifact/manifests and states that publication still requires explicit source-side human authority. ### Ordinary PR proof A new cross-platform integration target synthesizes the complete four-platform candidate bundle from the cargo-built test binary and drives the real Bash or PowerShell smoke script. It proves: - local staged-candidate success and retained first-use receipts; - checksum corruption fails before execution; and - on Unix, a broken executable with a matching checksum still fails the command boundary. ## Why PR #273 correctly added structural validation for all five first-use receipt artifacts, but explicitly left candidate staging, exact uploaded-byte consumption, four-lane publication gating, and rollback outside scope. The current workflow allowed first-use jobs to race asset upload and the smoke scripts fetched release URLs independently, so the jobs were not bound to one workflow-staged candidate set. This PR closes that implementation gap on the swarm verification surface. It does not add release-write authority to swarm. ## Review map -.github/workflows/release.yml— exact tag binding, candidate assembly, matrix consumption, diagnostics, negative controls, and terminal authority. -scripts/release-install-smoke.sh— complete local candidate-bundle mode plus public release mode, manifest/checksum validation, and explicit command-failure diagnostics. -scripts/release-install-smoke.ps1— Windows parity for the same contract. -apps/shiplog/tests/release_candidate_smoke.rs— normal-PR execution proof for success, checksum failure, and command failure. ## Expected proofbash bash -n scripts/release-install-smoke.sh pwsh -NoProfile -Command '$null = [scriptblock]::Create((Get-Content scripts/release-install-smoke.ps1 -Raw))' cargo fmt --all -- --check cargo clippy -p shiplog --all-targets --locked -- -D warnings cargo test -p shiplog --test release_candidate_smoke --locked cargo xtask check-workflows --mode blocking-allowlist cargo xtask check-file-policy --mode blocking-allowlist cargo xtask ci-small git diff --checkA real release proof additionally requires one explicit tag/owner-approved workflow run whose four first-use jobs consume the same staged artifact and whoseRelease Candidate Readyjob is green. ## Source handoff.github/workflows/release.ymlis deliberately source-owned governance. After this shared implementation is merged and promoted, canonical source must port the bounded source-writer counterpart: draft GitHub Release creation/upload may remain source-only, but its four acceptance lanes must consume this same staged candidate set and final publication must remain human-authorized. ## Claim boundary This prepares and proves release candidates inshiplog-swarm. It does not create or publish a GitHub Release, publish crates.io, push or move a tag, sign/notarize artifacts, update Homebrew/Scoop, change the current promotion state, or move release authority fromEffortlessMetrics/shiplog. ## Rollback Revert the squash merge. Public release download mode remains unchanged; no tag, release, crate, package channel, or source branch is mutated by this PR. Refs #245. Keep #245 open until the source writer counterpart lands and one exact explicit-tag run records all four target results plus a greenRelease Candidate Readyreceipt.