fix(phase4): repair candidate artifact layout and guard Linux acceptance - #75
Merged
Conversation
The first full three-platform candidate run failed in every downstream lane from one root cause: the Linux candidate and channel-image artifacts each mixed `dist/...` and `container/...` upload paths. actions/upload-artifact uses the least-common-ancestor of its inputs as the archive root, so a single `container/` entry made the root the repository root and nested every file under `dist/` on download. Consumers look at the download root, so Linux, Windows, and Phase 2 all reported "candidate archive is missing", and the promotion assembler would have failed the same way on the channel image. Remove the redundant `container/channel-machine.oci.json` from the candidate artifact and `container/channel-machine.oci.sha256` from the channel-image artifact. Both duplicates remain retained elsewhere (the immutable channel image artifact and each release's own container tree); no consumer reads them from these artifacts. The exact candidate and image bytes are unchanged. Also clear the retained self-hosted dress-rehearsal workspace before download so a stale prior candidate can never mask a layout error again. Harden ops/platform-acceptance/linux.sh: it performs a real root install (port 8123, /var/lib/1helm-oci-v1, systemd units), which is only safe on a disposable hosted runner. Refuse unless RUNNER_ENVIRONMENT=github-hosted, no 1Helm/standalone host state already exists, and port 8123 is free. This encodes the prior production-clobber lesson without changing the shipped installer's paths. Tests updated to assert single-rooted uploads, the workspace clear, and the Linux isolation guard. Full `npm run ci` passes. No release, tag, version bump, website deploy, or production change. Co-Authored-By: Claude <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe candidate workflow now excludes OCI metadata and checksum files, clears prior candidate directories, and validates artifact paths. Linux acceptance now requires a GitHub-hosted runner, clean 1Helm state paths, and an unused port 8123. ChangesCandidate workflow and platform acceptance
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 `@ops/platform-acceptance/linux.sh`:
- Around line 40-43: Update the port validation before the root installation in
the Linux acceptance script to fail closed when ss is unavailable or its
inspection command fails. Require ss explicitly, run ss port inspection
separately, handle any nonzero status by printing an error and exiting, and only
then use grep to reject an occupied port 8123.
In `@test/phase4-platform-acceptance.mjs`:
- Around line 118-120: Extend the channelUpload assertions in the phase4
acceptance test to verify the complete metadata contract: match the retained
dist/1Helm-channel-machine-v1-*.json metadata file and assert that the
.oci.sha256 sidecar is absent, while preserving the existing single-root and OCI
tar path checks.
- Around line 121-124: Update the Linux acceptance assertions in the phase4
platform test to require the guarded host-state paths /var/lib/1helm-oci-v1 and
/opt/1helm, in addition to the existing 1helm-standalone check. Keep the current
RUNNER_ENVIRONMENT and port-8123 assertions unchanged so the test covers every
host-state rejection path in the Linux acceptance script.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 51199e68-4992-4e42-bbe5-ae78e83b56c4
📒 Files selected for processing (4)
.github/workflows/candidate.ymlops/platform-acceptance/linux.shtest/phase2-candidate.mjstest/phase4-platform-acceptance.mjs
Comment on lines
+40
to
+43
| if command -v ss >/dev/null 2>&1 && ss -ltn 2>/dev/null | grep -qE '[:.]8123[[:space:]]'; then | ||
| echo "Linux acceptance refuses to run while port 8123 is already in use." >&2 | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Fail closed when port inspection is unavailable.
If ss is absent or returns an error, the if condition is false and the script continues to Line 64, where it performs the root installation. The runner can therefore proceed while port 8123 is occupied. Require ss and handle inspection errors before running grep.
Proposed fix
-if command -v ss >/dev/null 2>&1 && ss -ltn 2>/dev/null | grep -qE '[:.]8123[[:space:]]'; then
+command -v ss >/dev/null 2>&1 \
+ || { echo "Linux acceptance cannot verify port 8123 because ss is unavailable." >&2; exit 1; }
+ports="$(ss -ltn 2>/dev/null)" \
+ || { echo "Linux acceptance cannot verify port 8123." >&2; exit 1; }
+if grep -qE '[:.]8123[[:space:]]' <<<"$ports"; then
echo "Linux acceptance refuses to run while port 8123 is already in use." >&2
exit 1
fiAs per PR objectives, Linux acceptance must verify that port 8123 is available before root installation.
📝 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 command -v ss >/dev/null 2>&1 && ss -ltn 2>/dev/null | grep -qE '[:.]8123[[:space:]]'; then | |
| echo "Linux acceptance refuses to run while port 8123 is already in use." >&2 | |
| exit 1 | |
| fi | |
| command -v ss >/dev/null 2>&1 \ | |
| || { echo "Linux acceptance cannot verify port 8123 because ss is unavailable." >&2; exit 1; } | |
| ports="$(ss -ltn 2>/dev/null)" \ | |
| || { echo "Linux acceptance cannot verify port 8123." >&2; exit 1; } | |
| if grep -qE '[:.]8123[[:space:]]' <<<"$ports"; then | |
| echo "Linux acceptance refuses to run while port 8123 is already in use." >&2 | |
| 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 `@ops/platform-acceptance/linux.sh` around lines 40 - 43, Update the port
validation before the root installation in the Linux acceptance script to fail
closed when ss is unavailable or its inspection command fails. Require ss
explicitly, run ss port inspection separately, handle any nonzero status by
printing an error and exiting, and only then use grep to reject an occupied port
8123.
Comment on lines
+118
to
+120
| const channelUpload = workflow.match(/- name: Retain immutable digest-addressed channel image candidate[\s\S]*?retention-days: 90/)?.[0] || ""; | ||
| assert.doesNotMatch(channelUpload, /container\//); | ||
| assert.match(channelUpload, /dist\/1Helm-channel-machine-v1-\*\.oci\.tar/); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Assert the complete channel-image metadata contract.
The test checks only the OCI tar path. The workflow at .github/workflows/candidate.yml Lines 205-206 retains dist/1Helm-channel-machine-v1-*.json and removes the .oci.sha256 sidecar. Assert both conditions so a metadata regression cannot pass this test.
Proposed assertions
assert.doesNotMatch(channelUpload, /container\//);
assert.match(channelUpload, /dist\/1Helm-channel-machine-v1-\*\.oci\.tar/);
+ assert.match(channelUpload, /dist\/1Helm-channel-machine-v1-\*\.json/);
+ assert.doesNotMatch(channelUpload, /channel-machine\.oci\.sha256/);As per PR objectives, the channel-image artifact must retain its intended single-root layout and metadata.
📝 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
| const channelUpload = workflow.match(/- name: Retain immutable digest-addressed channel image candidate[\s\S]*?retention-days: 90/)?.[0] || ""; | |
| assert.doesNotMatch(channelUpload, /container\//); | |
| assert.match(channelUpload, /dist\/1Helm-channel-machine-v1-\*\.oci\.tar/); | |
| const channelUpload = workflow.match(/- name: Retain immutable digest-addressed channel image candidate[\s\S]*?retention-days: 90/)?.[0] || ""; | |
| assert.doesNotMatch(channelUpload, /container\//); | |
| assert.match(channelUpload, /dist\/1Helm-channel-machine-v1-\*\.oci\.tar/); | |
| assert.match(channelUpload, /dist\/1Helm-channel-machine-v1-\*\.json/); | |
| assert.doesNotMatch(channelUpload, /channel-machine\.oci\.sha256/); |
🤖 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 `@test/phase4-platform-acceptance.mjs` around lines 118 - 120, Extend the
channelUpload assertions in the phase4 acceptance test to verify the complete
metadata contract: match the retained dist/1Helm-channel-machine-v1-*.json
metadata file and assert that the .oci.sha256 sidecar is absent, while
preserving the existing single-root and OCI tar path checks.
Comment on lines
+121
to
+124
| const linuxAccept = read("ops/platform-acceptance/linux.sh"); | ||
| assert.match(linuxAccept, /RUNNER_ENVIRONMENT.*==.*"github-hosted"/); | ||
| assert.match(linuxAccept, /1helm-standalone/); | ||
| assert.match(linuxAccept, /refuses to run while port 8123 is already in use/); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Assert every guarded host-state path.
The test checks only 1helm-standalone. It does not require /var/lib/1helm-oci-v1 or /opt/1helm from ops/platform-acceptance/linux.sh Lines 36-39. A regression could remove either guard while this test still passes.
Proposed assertions
assert.match(linuxAccept, /RUNNER_ENVIRONMENT.*==.*"github-hosted"/);
- assert.match(linuxAccept, /1helm-standalone/);
+ assert.match(linuxAccept, /\/var\/lib\/1helm-oci-v1/);
+ assert.match(linuxAccept, /\/var\/lib\/1helm-standalone/);
+ assert.match(linuxAccept, /\/opt\/1helm/);
assert.match(linuxAccept, /refuses to run while port 8123 is already in use/);As per PR objectives, Linux acceptance must reject existing 1Helm and standalone host state.
📝 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
| const linuxAccept = read("ops/platform-acceptance/linux.sh"); | |
| assert.match(linuxAccept, /RUNNER_ENVIRONMENT.*==.*"github-hosted"/); | |
| assert.match(linuxAccept, /1helm-standalone/); | |
| assert.match(linuxAccept, /refuses to run while port 8123 is already in use/); | |
| const linuxAccept = read("ops/platform-acceptance/linux.sh"); | |
| assert.match(linuxAccept, /RUNNER_ENVIRONMENT.*==.*"github-hosted"/); | |
| assert.match(linuxAccept, /\/var\/lib\/1helm-oci-v1/); | |
| assert.match(linuxAccept, /\/var\/lib\/1helm-standalone/); | |
| assert.match(linuxAccept, /\/opt\/1helm/); | |
| assert.match(linuxAccept, /refuses to run while port 8123 is already in use/); |
🤖 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 `@test/phase4-platform-acceptance.mjs` around lines 121 - 124, Update the Linux
acceptance assertions in the phase4 platform test to require the guarded
host-state paths /var/lib/1helm-oci-v1 and /opt/1helm, in addition to the
existing 1helm-standalone check. Keep the current RUNNER_ENVIRONMENT and
port-8123 assertions unchanged so the test covers every host-state rejection
path in the Linux acceptance script.
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.
Why
The first full three-platform candidate run (30958024284) failed in every downstream lane. Root cause: the Linux candidate and channel-image artifacts each mixed
dist/...andcontainer/...upload paths.actions/upload-artifactroots the archive at the least-common-ancestor of its inputs, so a singlecontainer/entry pushed the root to the repo root and nested every file underdist/on download. Consumers read the download root, so:accept-linux,accept-windows, and the Phase 2deployall reported "exact Linux candidate archive is missing"Changes
container/channel-machine.oci.jsonfrom the candidate artifact andcontainer/channel-machine.oci.sha256from the channel-image artifact. Both duplicates remain retained elsewhere and no consumer reads them from these artifacts. Exact bytes unchanged; no rebuild.ops/platform-acceptance/linux.sh. It does a real root install (port 8123,/var/lib/1helm-oci-v1, systemd units) — only safe on a disposable runner. It now refuses unlessRUNNER_ENVIRONMENT=github-hosted, no 1Helm/standalone host state pre-exists, and port 8123 is free. This encodes the prior production-clobber lesson without changing the shipped installer's hard-coded paths.Out-of-band (already done)
The dedicated Phase 2 rehearsal host (LXC 101) had a stale pre-split installer helper; its
/usr/local/sbin/1helm-candidate-installand boundary validator were updated to match the tracked versions, and its stale inbox was cleared.Verification
node --testphase2/phase4/phase5 — 22/22 passnpm run ci— 175 pass, 2 skipped, 0 failbash -nand YAML parse cleanScope
No release, tag, version bump, website deploy, or production change. Does not alter enablement variables.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Reliability