Automate cross-platform candidate acceptance - #70
Conversation
Retain exact Mac and Linux candidate artifacts, run fail-closed platform acceptance, and assemble promotion evidence only after all supported lanes pass. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Joseph Yaksich <gitcommit90@users.noreply.github.com>
📝 WalkthroughWalkthroughThe PR adds Phase 4 Linux, macOS, and Windows candidate acceptance. It builds and validates exact artifacts, records normalized evidence, enforces trusted workflow identities, and gates promotion on a complete platform matrix. ChangesPhase 4 acceptance and promotion
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CandidateWorkflow
participant BuildJobs
participant AcceptanceJobs
participant EvidenceScripts
participant PromotionAssembly
CandidateWorkflow->>BuildJobs: build exact candidate artifacts
BuildJobs->>AcceptanceJobs: download artifacts and provenance
AcceptanceJobs->>EvidenceScripts: run platform checks and write evidence
EvidenceScripts->>PromotionAssembly: provide normalized acceptance records
PromotionAssembly->>CandidateWorkflow: assemble promotion bundle
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 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: 10
🧹 Nitpick comments (12)
ops/platform-acceptance/windows.ps1 (1)
9-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
$Outputis assigned and never used.The Linux and macOS lanes fail fast when
HELM_ACCEPTANCE_OUTPUTis missing (ops/platform-acceptance/linux.shline 8). This lane only stores the value. Either refuse a missing value, or drop the variable.♻️ Proposed fix
-$Output = $env:HELM_ACCEPTANCE_OUTPUT +if (-not $env:HELM_ACCEPTANCE_OUTPUT) { throw 'Windows acceptance refused: acceptance output is required' }🤖 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/windows.ps1` at line 9, Update the Windows acceptance setup around the $Output assignment so a missing HELM_ACCEPTANCE_OUTPUT is handled consistently with the Linux and macOS lanes: either validate and fail when the environment variable is absent, or remove the unused assignment entirely.Source: Linters/SAST tools
ops/platform-acceptance/runner-job-started.ps1 (1)
22-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename
$eventto avoid the PowerShell automatic variable.PowerShell reserves
$Eventfor event-subscriber actions. PSScriptAnalyzer flags the assignment. Rename the local variable to keep the script analyzer clean.♻️ Proposed rename
-$event = Get-Content -LiteralPath $env:GITHUB_EVENT_PATH -Raw | ConvertFrom-Json -$run = $event.workflow_run -if ($event.repository.full_name -ne $env:GITHUB_REPOSITORY -or +$payload = Get-Content -LiteralPath $env:GITHUB_EVENT_PATH -Raw | ConvertFrom-Json +$run = $payload.workflow_run +if ($payload.repository.full_name -ne $env:GITHUB_REPOSITORY -or🤖 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/runner-job-started.ps1` at line 22, Rename the local $event variable assigned from GITHUB_EVENT_PATH to a non-reserved name, then update every reference to it in runner-job-started.ps1 so the script behavior remains unchanged and PSScriptAnalyzer no longer flags the assignment.Source: Linters/SAST tools
ops/platform-acceptance/runner-job-started.sh (1)
8-12: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
expected_labelis assigned but never used.Both branches set
expected_labeland no later code reads it. The guard works only because the*)branch exits. Either compare the label against the runner's actual labels, or drop the variable and keep the job allowlist.♻️ Proposed simplification
-case "${GITHUB_JOB:-}" in - build-macos) expected_label=1helm-macos-phase4 ;; - accept-macos) expected_label=1helm-macos-phase4 ;; - *) echo "Phase 4 runner refused job ${GITHUB_JOB:-missing}." >&2; exit 1 ;; -esac +case "${GITHUB_JOB:-}" in + build-macos|accept-macos) ;; + *) echo "Phase 4 runner refused job ${GITHUB_JOB:-missing}." >&2; exit 1 ;; +esacNote that
test/phase4-platform-acceptance.mjslines 149-150 only assert that the stringsbuild-macosandaccept-macosappear, so this change keeps the tests passing.🤖 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/runner-job-started.sh` around lines 8 - 12, Remove the unused expected_label assignments from the case statement in runner-job-started.sh and retain the build-macos and accept-macos allowlist branches with the existing refusal behavior for all other jobs.Source: Linters/SAST tools
ops/platform-acceptance/macos.sh (1)
40-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winA failed run leaves
Application Supportbehind and wedges the next run.
cleanupremoves$DATA_ROOTonly whencompletedis 1. Lines 9-10 and 67 refuse to start when that directory exists. After any mid-script failure the dedicated Mac needs a manual reset before the lane can run again. If the retention is deliberate for forensics, state that in the comment and document the reset step; otherwise move the residue to a timestamped path so later runs stay unblocked.🤖 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/macos.sh` around lines 40 - 48, The cleanup function currently preserves DATA_ROOT after failed runs, blocking subsequent executions. Update cleanup to remove or relocate DATA_ROOT on failure as well as success, preferably moving retained residue to a timestamped path, while preserving the existing successful-run cleanup behavior and ensuring the next run is not blocked.ops/platform-acceptance/linux.sh (1)
10-13: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRead the manifest once instead of starting four Node processes.
Lines 10-13 start
nodefour times to read four fields of the same JSON file. One invocation can emit all four values.♻️ Proposed refactor
-VERSION="$(node -p 'JSON.parse(require("fs").readFileSync(process.argv[1])).version' "$MANIFEST")" -DIGEST="$(node -p 'JSON.parse(require("fs").readFileSync(process.argv[1])).artifact.sha256' "$MANIFEST")" -COMMIT="$(node -p 'JSON.parse(require("fs").readFileSync(process.argv[1])).source.commit' "$MANIFEST")" -CI_RUN="$(node -p 'JSON.parse(require("fs").readFileSync(process.argv[1])).ci.run_id' "$MANIFEST")" +read -r VERSION DIGEST COMMIT CI_RUN < <(node -p ' + const m = JSON.parse(require("fs").readFileSync(process.argv[1], "utf8")); + [m.version, m.artifact.sha256, m.source.commit, m.ci.run_id].join(" "); +' "$MANIFEST")🤖 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 10 - 13, Refactor the manifest parsing around VERSION, DIGEST, COMMIT, and CI_RUN to invoke Node only once, parse MANIFEST once, and emit all four values for assignment to the existing shell variables. Preserve each variable’s current manifest field mapping and downstream names.test/phase4-platform-acceptance.mjs (2)
60-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the removed check id instead of hard-coding it.
Line 61 removes the first Windows check. Line 65 then asserts that
non_elevated_installis blocked. The test only passes whilenon_elevated_installstays first inPLATFORM_CHECKS.windows. Reorder that list and the assertion becomes wrong without any behavior change.♻️ Proposed refactor
const missing = fixture("windows"); +const removed = missing.checks[0].id; missing.checks = missing.checks.slice(1); delete missing.result; const blocked = normalizePlatformEvidence(missing); assert.equal(blocked.result, "blocked"); -assert.equal(blocked.checks.find((item) => item.id === "non_elevated_install").result, "blocked"); +assert.equal(blocked.checks.find((item) => item.id === removed).result, "blocked");🤖 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 60 - 66, Update the test around normalizePlatformEvidence to capture the id of the check removed by missing.checks.slice(1), then use that derived id in the blocked-result assertion instead of hard-coding "non_elevated_install". Keep the existing aggregate-result validation unchanged.
9-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeclare the minimum Node version.
CI uses Node 22, but
package.jsondeclares no minimum version. Addengines.nodewith a supported version of at least20.11.0, becauseimport.meta.dirnameis used here.🤖 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` at line 9, Add an engines.node declaration to package.json specifying Node.js 20.11.0 or newer, matching the import.meta.dirname usage in phase4-platform-acceptance.mjs. Leave the existing package metadata and scripts unchanged.scripts/promotion-lib.mjs (1)
206-215: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winName the macOS roles explicitly instead of using
role !== "linux_tgz".The
elsebranch treats every non-Linux role as a signed macOS artifact. IfSTABLE_ARTIFACT_ROLESgains another role, that role inherits the macOS signing requirements and fails with a misleading blocker. Bind the branch to the mac roles.♻️ Proposed refactor
- if (role !== "linux_tgz") { + if (role === "mac_dmg" || role === "mac_updater_zip") { add(blockers, provenance.value?.builder === "dedicated-macos"🤖 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/promotion-lib.mjs` around lines 206 - 215, The non-Linux branch in the provenance validation currently applies macOS signing checks to every future role. Replace the role !== "linux_tgz" condition with an explicit check for the supported macOS roles in the surrounding role configuration, keeping the existing dedicated-mac builder and signing validations unchanged.scripts/candidate-promotion-skeleton.mjs (1)
111-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winIterate the shared platform contract instead of a hardcoded list.
scripts/promotion-lib.mjsline 236 iteratesObject.keys(PLATFORM_CHECKS). This loop hardcodes the same three platforms. If a platform is added toPLATFORM_CHECKS, this assembly step skips its evidence and still produces a bundle, and the omission only surfaces at the later promotion gate.♻️ Proposed refactor
-import { platformEvidenceBlockers } from "./platform-acceptance-lib.mjs"; +import { PLATFORM_CHECKS, platformEvidenceBlockers } from "./platform-acceptance-lib.mjs";-for (const platform of ["macos", "linux", "windows"]) { +for (const platform of Object.keys(PLATFORM_CHECKS)) {🤖 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/candidate-promotion-skeleton.mjs` at line 111, Update the platform iteration in the candidate-promotion assembly flow to use the shared PLATFORM_CHECKS contract, matching the Object.keys(PLATFORM_CHECKS) iteration in promotion-lib.mjs, instead of hardcoding macos, linux, and windows. Ensure newly registered platforms are included in the assembled evidence bundle automatically.test/phase3-promotion.mjs (1)
128-128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd negative cases for the new promotion blockers.
The fixture now satisfies every new macOS check, so the happy path is covered. The new fail-closed controls have no negative coverage. Add cases that mutate the bundle and assert the expected blocker, at minimum:
- change
mac-candidate.jsonbuilder.runner_nameso it differs frommacos-acceptance.jsonrunner.name, and assert the "runner does not match the dedicated Mac builder" blocker.- drop
gatekeeperfrom a mac provenance record, and assert the signing evidence blocker.- change the mac manifest
run_attempt, and assert the candidate run identity blocker.Do you want me to draft these test cases?
🤖 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/phase3-promotion.mjs` at line 128, Extend the promotion tests around the fixture records in phase3-promotion.mjs with negative cases that clone and mutate the bundle while preserving the existing happy path. Add assertions for mismatched macOS builder and acceptance runner names, missing gatekeeper evidence in a mac provenance record, and mismatched mac manifest run_attempt, verifying each expected blocker message..github/workflows/candidate.yml (2)
153-153: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeclare the self-hosted labels for actionlint.
actionlint rejects
1helm-macos-phase4(Lines 153, 337),1helm-windows-phase4(Line 391), and1helm-dress-rehearsal-phase2(Line 234) as unknown labels. Add the labels to anactionlint.yamlconfig so workflow linting passes.self-hosted-runner: labels: - 1helm-macos-phase4 - 1helm-windows-phase4 - 1helm-dress-rehearsal-phase2🤖 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/candidate.yml at line 153, Add an actionlint.yaml configuration defining the self-hosted-runner labels 1helm-macos-phase4, 1helm-windows-phase4, and 1helm-dress-rehearsal-phase2, so the runs-on values in the workflow are recognized during linting.Source: Linters/SAST tools
186-229: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueClean
dist/before the Mac build to avoid uploading stale artifacts.The Mac lane runs on a persistent self-hosted runner, so
dist/survives between workflow runs. The upload paths at lines 223-224 use version-agnostic wildcards. If an earlier run left old build outputs, the upload step would include them in the artifact even though the currentpackage.jsonversion has changed.Although the
accept-macosworkflow constructs exact filenames using the manifest's version field and thus would not use stale files, removing build outputs beforenpm run package:dmg:releasekeeps the uploaded artifact clean and simplifies auditing.Proposed clean step
- name: Verify checkout and install exact dependencies without privilege shell: bash run: | set -euo pipefail test "$(git rev-parse HEAD)" = "${{ github.event.workflow_run.head_sha }}" test -z "$(git status --porcelain)" test "$(uname -s)-$(uname -m)" = Darwin-arm64 + rm -rf dist PUPPETEER_SKIP_DOWNLOAD=1 npm ci🤖 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/candidate.yml around lines 186 - 229, Clean the existing dist/ build-output directory before the Mac packaging step runs, using the workflow’s shell setup and preserving the directory creation behavior needed by later manifest generation. Place the cleanup immediately before npm run package:dmg:release so stale artifacts cannot match the wildcard upload paths, while leaving the identity and upload steps unchanged.
🤖 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 `@docs/phase4-platform-acceptance.md`:
- Line 134: Update the “Linux hosted runners” phrase in the Phase 4 platform
acceptance documentation to “Linux-hosted runners,” preserving the surrounding
text.
In `@ops/platform-acceptance/macos.sh`:
- Around line 78-79: Update the shutdown checks near the wait loops in the macOS
acceptance script, including all three occurrences around the 1Helm termination
proofs, so a still-running process explicitly exits the script with failure
instead of relying on an inverted pgrep command under set -e. Preserve the
existing polling behavior and success path once the process has stopped.
In `@ops/platform-acceptance/windows.ps1`:
- Around line 91-92: Wrap the post-install health probe around $cleanHealth in
the same retry pattern used by the block near lines 166-172, retrying failed or
unavailable requests until the service startup grace period is exhausted before
calling Refuse. Preserve the existing status-code and needs_setup validation
once a response is obtained, and apply the same retry behavior to the analogous
health probe near line 127.
- Around line 12-15: Add validation for the $Version variable after Refuse is
defined and before the first command that interpolates $Version into bash -lc.
Validate that $Version conforms to an expected format or is not empty, rejecting
invalid values using the Refuse mechanism. Do not add redundant format
validation for $Digest since the archive hash comparison already constrains it.
In `@scripts/candidate-promotion-skeleton.mjs`:
- Around line 51-64: Update the mac manifest validation around the existing
builder checks to require a non-empty mac.builder.runner_name, matching the
presence rule used by promotion-lib.mjs. Ensure this validation rejects missing
or empty runner_name before the assembly comparison with the macOS acceptance
evidence, preventing undefined-to-undefined matches while preserving the
existing runner identity binding.
In `@scripts/macos-acceptance-evidence.mjs`:
- Line 21: Add byte-count validation in scripts/macos-acceptance-evidence.mjs
and scripts/windows-acceptance-evidence.mjs, checking the macOS DMG, updater
ZIP, and HELM_CANDIDATE_ARCHIVE file sizes against their corresponding manifest
metadata before accepting them. Preserve the existing exact-file hashing checks
and reject any size mismatch.
- Line 37: Update the output handling around writeFileSync so every run enforces
0o600 permissions even when the target already exists; remove the resolved
output file before writing, or use a symlink-safe creation path that explicitly
sets permissions, while preserving the current output location and JSON content.
In `@scripts/platform-acceptance-lib.mjs`:
- Around line 80-91: Update normalizeOutcome to require both before_sha256 and
after_sha256 when result is "passed" and digest evidence is expected, rejecting
missing or undefined values before returning the normalized outcome. Preserve
nullable digest handling for non-passed outcomes and ensure the failure occurs
during outcome normalization rather than being deferred to
platformEvidenceBlockers.
In `@scripts/promotion-lib.mjs`:
- Around line 179-182: Anchor run-attempt validation to the trusted workflow
input: in scripts/promotion-lib.mjs lines 179-182, use options.runAttempt as
expected.runAttempt and compare the candidate run record against it instead of
deriving the expectation from runRecord. In
scripts/candidate-promotion-skeleton.mjs lines 113-114, read and validate
GITHUB_RUN_ATTEMPT alongside workflowRunId, require mac.candidate.run_attempt to
match it, and pass it as runAttempt to platformEvidenceBlockers.
In `@scripts/windows-acceptance-evidence.mjs`:
- Line 40: Update the evidence-output flow around writeFileSync to store
windows-acceptance.json in an ACL-protected directory instead of relying on the
ineffective mode option. Apply an explicit Windows ACL to that directory, verify
the ACL before any upload or retention step, and preserve the existing
configurable HELM_ACCEPTANCE_OUTPUT path behavior.
---
Nitpick comments:
In @.github/workflows/candidate.yml:
- Line 153: Add an actionlint.yaml configuration defining the self-hosted-runner
labels 1helm-macos-phase4, 1helm-windows-phase4, and
1helm-dress-rehearsal-phase2, so the runs-on values in the workflow are
recognized during linting.
- Around line 186-229: Clean the existing dist/ build-output directory before
the Mac packaging step runs, using the workflow’s shell setup and preserving the
directory creation behavior needed by later manifest generation. Place the
cleanup immediately before npm run package:dmg:release so stale artifacts cannot
match the wildcard upload paths, while leaving the identity and upload steps
unchanged.
In `@ops/platform-acceptance/linux.sh`:
- Around line 10-13: Refactor the manifest parsing around VERSION, DIGEST,
COMMIT, and CI_RUN to invoke Node only once, parse MANIFEST once, and emit all
four values for assignment to the existing shell variables. Preserve each
variable’s current manifest field mapping and downstream names.
In `@ops/platform-acceptance/macos.sh`:
- Around line 40-48: The cleanup function currently preserves DATA_ROOT after
failed runs, blocking subsequent executions. Update cleanup to remove or
relocate DATA_ROOT on failure as well as success, preferably moving retained
residue to a timestamped path, while preserving the existing successful-run
cleanup behavior and ensuring the next run is not blocked.
In `@ops/platform-acceptance/runner-job-started.ps1`:
- Line 22: Rename the local $event variable assigned from GITHUB_EVENT_PATH to a
non-reserved name, then update every reference to it in runner-job-started.ps1
so the script behavior remains unchanged and PSScriptAnalyzer no longer flags
the assignment.
In `@ops/platform-acceptance/runner-job-started.sh`:
- Around line 8-12: Remove the unused expected_label assignments from the case
statement in runner-job-started.sh and retain the build-macos and accept-macos
allowlist branches with the existing refusal behavior for all other jobs.
In `@ops/platform-acceptance/windows.ps1`:
- Line 9: Update the Windows acceptance setup around the $Output assignment so a
missing HELM_ACCEPTANCE_OUTPUT is handled consistently with the Linux and macOS
lanes: either validate and fail when the environment variable is absent, or
remove the unused assignment entirely.
In `@scripts/candidate-promotion-skeleton.mjs`:
- Line 111: Update the platform iteration in the candidate-promotion assembly
flow to use the shared PLATFORM_CHECKS contract, matching the
Object.keys(PLATFORM_CHECKS) iteration in promotion-lib.mjs, instead of
hardcoding macos, linux, and windows. Ensure newly registered platforms are
included in the assembled evidence bundle automatically.
In `@scripts/promotion-lib.mjs`:
- Around line 206-215: The non-Linux branch in the provenance validation
currently applies macOS signing checks to every future role. Replace the role
!== "linux_tgz" condition with an explicit check for the supported macOS roles
in the surrounding role configuration, keeping the existing dedicated-mac
builder and signing validations unchanged.
In `@test/phase3-promotion.mjs`:
- Line 128: Extend the promotion tests around the fixture records in
phase3-promotion.mjs with negative cases that clone and mutate the bundle while
preserving the existing happy path. Add assertions for mismatched macOS builder
and acceptance runner names, missing gatekeeper evidence in a mac provenance
record, and mismatched mac manifest run_attempt, verifying each expected blocker
message.
In `@test/phase4-platform-acceptance.mjs`:
- Around line 60-66: Update the test around normalizePlatformEvidence to capture
the id of the check removed by missing.checks.slice(1), then use that derived id
in the blocked-result assertion instead of hard-coding "non_elevated_install".
Keep the existing aggregate-result validation unchanged.
- Line 9: Add an engines.node declaration to package.json specifying Node.js
20.11.0 or newer, matching the import.meta.dirname usage in
phase4-platform-acceptance.mjs. Leave the existing package metadata and scripts
unchanged.
🪄 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: 467ae0c8-0480-4b96-9a90-78ade541a5e4
📒 Files selected for processing (24)
.github/workflows/candidate.ymldocs/dress-rehearsal.mddocs/phase4-platform-acceptance.mddocs/release-lifecycle.mdops/platform-acceptance/linux.shops/platform-acceptance/macos.shops/platform-acceptance/runner-job-started.ps1ops/platform-acceptance/runner-job-started.shops/platform-acceptance/windows.ps1package.jsonscripts/candidate-matrix-status.mjsscripts/candidate-promotion-skeleton.mjsscripts/linux-acceptance-evidence.mjsscripts/mac-candidate-manifest.mjsscripts/macos-acceptance-evidence.mjsscripts/pending-acceptance-evidence.mjsscripts/platform-acceptance-evidence.mjsscripts/platform-acceptance-lib.mjsscripts/promotion-lib.mjsscripts/run-test-suite.mjsscripts/windows-acceptance-evidence.mjssite/public/install.ps1test/phase3-promotion.mjstest/phase4-platform-acceptance.mjs
| uses the scoped product uninstaller for the exact `1helm-phase4` distribution | ||
| and separately unregisters only its named unrelated test control. If teardown | ||
| is interrupted, leave the runner disabled and inspect both exact names before | ||
| retrying. Linux hosted runners are ephemeral; the Phase 2 private runner and |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Hyphenate the compound modifier.
Change Linux hosted runners to Linux-hosted runners at Line 134.
🧰 Tools
🪛 LanguageTool
[grammar] ~134-~134: Use a hyphen to join words.
Context: ... both exact names before retrying. Linux hosted runners are ephemeral; the Phase ...
(QB_NEW_EN_HYPHEN)
🤖 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 `@docs/phase4-platform-acceptance.md` at line 134, Update the “Linux hosted
runners” phrase in the Phase 4 platform acceptance documentation to
“Linux-hosted runners,” preserving the surrounding text.
Source: Linters/SAST tools
| for _ in {1..30}; do pgrep -x 1Helm >/dev/null || break; sleep 1; done | ||
| ! pgrep -x 1Helm >/dev/null |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
! pgrep ... never fails the script, so the quit check is not enforced.
Bash exempts a command whose return value is inverted with ! from set -e. If 1Helm is still running after the wait loop, line 79 returns 1 and the script continues. The same pattern appears on lines 133 and 148, so none of the three shutdown proofs can block the lane.
🐛 Proposed fix (apply to lines 79, 133, and 148)
for _ in {1..30}; do pgrep -x 1Helm >/dev/null || break; sleep 1; done
-! pgrep -x 1Helm >/dev/null
+if pgrep -x 1Helm >/dev/null; then echo "1Helm did not quit." >&2; exit 1; fi📝 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.
| for _ in {1..30}; do pgrep -x 1Helm >/dev/null || break; sleep 1; done | |
| ! pgrep -x 1Helm >/dev/null | |
| for _ in {1..30}; do pgrep -x 1Helm >/dev/null || break; sleep 1; done | |
| if pgrep -x 1Helm >/dev/null; then echo "1Helm did not quit." >&2; exit 1; fi |
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 79-79: This ! is not on a condition and skips errexit. Use && exit 1 instead, or make sure $? is checked.
(SC2251)
🤖 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/macos.sh` around lines 78 - 79, Update the shutdown
checks near the wait loops in the macOS acceptance script, including all three
occurrences around the 1Helm termination proofs, so a still-running process
explicitly exits the script with failure instead of relying on an inverted pgrep
command under set -e. Preserve the existing polling behavior and success path
once the process has stopped.
Source: Linters/SAST tools
| $Commit = [string]$Manifest.source.commit | ||
| $Version = [string]$Manifest.version | ||
| $Digest = [string]$Manifest.artifact.sha256 | ||
| $CiRun = [string]$Manifest.ci.run_id |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- file outline ---'
ast-grep outline ops/platform-acceptance/windows.ps1 --view expanded || true
printf '%s\n' '--- relevant source ---'
sed -n '1,190p' ops/platform-acceptance/windows.ps1
printf '%s\n' '--- manifest and command references ---'
rg -n -C 3 '\$Version|\$Digest|bash -lc|Manifest|function Refuse|Refuse ' ops/platform-acceptance/windows.ps1Repository: gitcommit90/1Helm
Length of output: 27403
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- acceptance callers and manifest producers ---'
rg -n -C 4 'windows\.ps1|HELM_CANDIDATE_MANIFEST|candidate-manifest|artifact\.sha256|trusted-main|HELM_EXPECTED_COMMIT|HELM_EXPECTED_CI_RUN_ID' .github ops scripts site 2>/dev/null || true
printf '%s\n' '--- nearby workflow files ---'
find .github ops -maxdepth 4 -type f \( -name '*.yml' -o -name '*.yaml' -o -name '*.ps1' -o -name '*.mjs' -o -name '*.js' \) -print | sortRepository: gitcommit90/1Helm
Length of output: 37384
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate manifest validation ---'
sed -n '1,125p' scripts/candidate-manifest.mjs
printf '%s\n' '--- candidate manifest call-site inputs ---'
sed -n '90,112p' .github/workflows/candidate.yml
printf '%s\n' '--- deterministic validation probe ---'
python3 - <<'PY'
import re
patterns = {
"proposed_version": re.compile(r"^\d+\.\d+\.\d+$"),
"proposed_digest": re.compile(r"^[a-f0-9]{64}$"),
}
cases = [
("1.2.3", "a"*64),
("1.2.3' ; id; #", "a"*64),
("1.2.3", "a"*63 + "'"),
]
for version, digest in cases:
print({
"version": version,
"digest": digest,
"version_passes": bool(patterns["proposed_version"].fullmatch(version)),
"digest_passes": bool(patterns["proposed_digest"].fullmatch(digest)),
})
PYRepository: gitcommit90/1Helm
Length of output: 6998
Command Injection (CWE-78): Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection')
Reachability: Internal · Exploitability: Difficult
Validate $Version before interpolating it into bash -lc. The candidate manifest producer validates the version, but this script does not enforce that boundary. The archive hash comparison already constrains $Digest, so its proposed format check is redundant. Add the version check after Refuse is defined and before the first affected command.
🤖 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/windows.ps1` around lines 12 - 15, Add validation for
the $Version variable after Refuse is defined and before the first command that
interpolates $Version into bash -lc. Validate that $Version conforms to an
expected format or is not empty, rejecting invalid values using the Refuse
mechanism. Do not add redundant format validation for $Digest since the archive
hash comparison already constrains it.
| $cleanHealth = Invoke-WebRequest -Uri 'http://localhost:8123/api/setup/status' -UseBasicParsing -TimeoutSec 10 | ||
| if ($cleanHealth.StatusCode -ne 200 -or -not (($cleanHealth.Content | ConvertFrom-Json).needs_setup)) { Refuse 'candidate clean install did not expose localhost onboarding health' } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Add a retry loop around the post-install health probe.
site/public/install.ps1 does not fail when http://localhost:8123 has not answered yet; it only prints a warning (lines 467-471 of that file). Line 91 then makes a single request with a 10 second timeout. If the service is still starting, Invoke-WebRequest throws and the lane blocks for a timing reason, not a product defect. Line 127 has the same shape. Reuse the retry pattern from lines 166-172.
🐛 Proposed fix
+function Wait-Health([int] $Attempts = 60) {
+ for ($i = 0; $i -lt $Attempts; $i++) {
+ try {
+ $r = Invoke-WebRequest -Uri 'http://localhost:8123/api/setup/status' -UseBasicParsing -TimeoutSec 5
+ if ($r.StatusCode -eq 200) { return $r }
+ } catch { }
+ Start-Sleep -Seconds 2
+ }
+ return $null
+}
-$cleanHealth = Invoke-WebRequest -Uri 'http://localhost:8123/api/setup/status' -UseBasicParsing -TimeoutSec 10
-if ($cleanHealth.StatusCode -ne 200 -or -not (($cleanHealth.Content | ConvertFrom-Json).needs_setup)) { Refuse 'candidate clean install did not expose localhost onboarding health' }
+$cleanHealth = Wait-Health
+if ($null -eq $cleanHealth -or -not (($cleanHealth.Content | ConvertFrom-Json).needs_setup)) { Refuse 'candidate clean install did not expose localhost onboarding health' }📝 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.
| $cleanHealth = Invoke-WebRequest -Uri 'http://localhost:8123/api/setup/status' -UseBasicParsing -TimeoutSec 10 | |
| if ($cleanHealth.StatusCode -ne 200 -or -not (($cleanHealth.Content | ConvertFrom-Json).needs_setup)) { Refuse 'candidate clean install did not expose localhost onboarding health' } | |
| function Wait-Health([int] $Attempts = 60) { | |
| for ($i = 0; $i -lt $Attempts; $i++) { | |
| try { | |
| $r = Invoke-WebRequest -Uri 'http://localhost:8123/api/setup/status' -UseBasicParsing -TimeoutSec 5 | |
| if ($r.StatusCode -eq 200) { return $r } | |
| } catch { } | |
| Start-Sleep -Seconds 2 | |
| } | |
| return $null | |
| } | |
| $cleanHealth = Wait-Health | |
| if ($null -eq $cleanHealth -or -not (($cleanHealth.Content | ConvertFrom-Json).needs_setup)) { Refuse 'candidate clean install did not expose localhost onboarding health' } |
🤖 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/windows.ps1` around lines 91 - 92, Wrap the
post-install health probe around $cleanHealth in the same retry pattern used by
the block near lines 166-172, retrying failed or unavailable requests until the
service startup grace period is exhausted before calling Refuse. Preserve the
existing status-code and needs_setup validation once a response is obtained, and
apply the same retry behavior to the analogous health probe near line 127.
| const macManifestPath = join(macSource, "candidate-evidence", "mac-candidate.json"); | ||
| const mac = JSON.parse(readFileSync(macManifestPath, "utf8")); | ||
| if (mac?.schema !== 1 || mac?.kind !== "1helm-macos-candidate" || mac?.repository !== STABLE_REPOSITORY | ||
| || mac?.ref !== "refs/heads/main" || mac?.commit !== commit || mac?.version !== version | ||
| || mac?.candidate?.workflow !== "Candidate dress rehearsal" || mac?.candidate?.workflow_path !== ".github/workflows/candidate.yml" | ||
| || mac?.candidate?.event !== "workflow_run" || String(mac?.candidate?.run_id) !== workflowRunId | ||
| || !/^\d+$/.test(String(mac?.candidate?.run_attempt || "")) || String(mac?.source_ci?.run_id) !== ciRunId | ||
| || mac?.source_ci?.conclusion !== "success" || mac?.signing?.identity !== "developer-id-application" | ||
| || mac?.signing?.notarization !== "accepted" || mac?.signing?.stapling !== "validated" | ||
| || mac?.signing?.gatekeeper !== "accepted" || mac?.builder?.type !== "dedicated-self-hosted" | ||
| || mac?.builder?.runner_label !== "1helm-macos-phase4" || mac?.builder?.os !== "macOS" | ||
| || mac?.builder?.architecture !== "ARM64") { | ||
| throw new Error("Mac candidate manifest does not bind signed/notarized bytes to the exact candidate identity"); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Require a non-empty builder.runner_name so the runner match cannot be skipped.
The manifest check does not validate mac.builder.runner_name. If the manifest omits builder.runner_name and the macOS acceptance evidence omits runner.name, then line 115 compares undefined !== undefined, which is false, and no blocker is pushed. platformEvidenceBlockers validates runner.labels and runner.job only, so nothing restores the binding. The bundle is then assembled with macOS acceptance recorded as accepted while it is not bound to the signing builder.
scripts/promotion-lib.mjs line 98 rejects an absent runner_name, so the later gate still blocks. Add the same presence requirement at assembly time.
🛡️ Proposed fix
|| mac?.builder?.runner_label !== "1helm-macos-phase4" || mac?.builder?.os !== "macOS"
- || mac?.builder?.architecture !== "ARM64") {
+ || mac?.builder?.architecture !== "ARM64"
+ || !/^[A-Za-z0-9][A-Za-z0-9 ._:/@+()`#-`]{0,255}$/.test(String(mac?.builder?.runner_name || ""))) {
throw new Error("Mac candidate manifest does not bind signed/notarized bytes to the exact candidate identity");
}Also applies to: 115-117
🤖 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/candidate-promotion-skeleton.mjs` around lines 51 - 64, Update the
mac manifest validation around the existing builder checks to require a
non-empty mac.builder.runner_name, matching the presence rule used by
promotion-lib.mjs. Ensure this validation rejects missing or empty runner_name
before the assembly comparison with the macOS acceptance evidence, preventing
undefined-to-undefined matches while preserving the existing runner identity
binding.
| architecture: env.RUNNER_ARCH, dedicated: true, production_data: false, | ||
| }, | ||
| runner: { name: env.RUNNER_NAME, labels: ["1helm-macos-phase4"], job: env.GITHUB_JOB }, | ||
| artifacts: manifest.artifacts, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 15 'HELM_MAC_CANDIDATE_DOWNLOAD|HELM_CANDIDATE_ARCHIVE|sha256sum|shasum|Get-FileHash|stat|macos-acceptance-evidence|windows-acceptance-evidence' .github ops scriptsRepository: gitcommit90/1Helm
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- target generators ---'
cat -n scripts/macos-acceptance-evidence.mjs
cat -n scripts/windows-acceptance-evidence.mjs
printf '%s\n' '--- relevant acceptance references ---'
rg -n -C 8 \
'macos-acceptance-evidence|windows-acceptance-evidence|HELM_MAC_CANDIDATE_DOWNLOAD|HELM_CANDIDATE_ARCHIVE|manifest\.artifacts|candidate\.artifact|sha256sum|shasum|Get-FileHash|stat' \
.github ops scripts \
-g '*.yml' -g '*.yaml' -g '*.mjs' -g '*.js' -g '*.sh' -g '*.ps1' -g '*.psm1' \
--glob '!scripts/1helm-oci-runtime-*'Repository: gitcommit90/1Helm
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- macOS acceptance script ---'
sed -n '1,180p' ops/platform-acceptance/macos.sh
printf '%s\n' '--- Windows acceptance setup and generator inputs ---'
sed -n '1,220p' ops/platform-acceptance/windows.ps1
sed -n '1,80p' scripts/windows-acceptance-evidence.mjs
printf '%s\n' '--- candidate manifest and macOS artifact definitions ---'
rg -n -C 12 \
'mac-candidate\.json|candidate-evidence|artifacts\s*:|artifact\.sha256|artifact\.bytes|HELM_MAC_CANDIDATE_DOWNLOAD|HELM_CANDIDATE_MANIFEST' \
scripts ops .github \
-g '*.mjs' -g '*.js' -g '*.sh' -g '*.ps1' -g '*.yml' -g '*.yaml' \
--glob '!scripts/1helm-oci-runtime-*' \
--glob '!ops/platform-acceptance/macos.sh' \
--glob '!ops/platform-acceptance/windows.ps1'Repository: gitcommit90/1Helm
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
checks = {
"ops/platform-acceptance/macos.sh": {
"generator": r'node "\$ROOT/scripts/macos-acceptance-evidence\.mjs"',
"hashes": r'shasum -a 256 "\$(DMG|ZIP)"',
"stats": r'(stat|wc -c).*(DMG|ZIP)|(?:DMG|ZIP).*(stat|wc -c)',
},
"ops/platform-acceptance/windows.ps1": {
"generator": r"windows-acceptance-evidence\.mjs",
"hashes": r"Get-FileHash \$Archive -Algorithm SHA256",
"stats": r"(Get-Item \$Archive|\.Length|stat|bytes)",
},
}
for name, patterns in checks.items():
text = Path(name).read_text()
print(name)
for label, pattern in patterns.items():
hits = [i + 1 for i, line in enumerate(text.splitlines()) if re.search(pattern, line, re.I)]
print(f" {label}: {hits or 'none'}")
PYRepository: gitcommit90/1Helm
Length of output: 410
Bind accepted-file sizes to artifact metadata.
Both scripts hash the exact candidate files. Neither compares file sizes with declared metadata.
Add byte-count checks for the macOS DMG, updater ZIP, and HELM_CANDIDATE_ARCHIVE against their manifest values.
📍 Affects 2 files
scripts/macos-acceptance-evidence.mjs#L21-L21(this comment)scripts/windows-acceptance-evidence.mjs#L25-L25
🤖 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/macos-acceptance-evidence.mjs` at line 21, Add byte-count validation
in scripts/macos-acceptance-evidence.mjs and
scripts/windows-acceptance-evidence.mjs, checking the macOS DMG, updater ZIP,
and HELM_CANDIDATE_ARCHIVE file sizes against their corresponding manifest
metadata before accepting them. Preserve the existing exact-file hashing checks
and reject any size mismatch.
| recovery: { ...pass("Both isolated launches quit cleanly and transient app copies were removed."), before_sha256: env.HELM_STATE_BEFORE_SHA256, after_sha256: env.HELM_STATE_AFTER_SHA256 }, | ||
| notes: ["This is real Apple Silicon signature, ticket, Gatekeeper, launch, loopback, and state evidence."], | ||
| }); | ||
| writeFileSync(resolve(env.HELM_ACCEPTANCE_OUTPUT || "macos-acceptance.json"), `${JSON.stringify(evidence, null, 2)}\n`, { mode: 0o600 }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- scripts/macos-acceptance-evidence.mjs ---'
sed -n '1,80p' scripts/macos-acceptance-evidence.mjs
printf '%s\n' '--- output-path and invocation references ---'
rg -n -C 3 'macos-acceptance\.json|HELM_ACCEPTANCE_OUTPUT|macos-acceptance-evidence' .github scripts README.md 2>/dev/null || true
printf '%s\n' '--- standalone Node behavior probe ---'
node - <<'JS'
'use strict';
const fs = require('node:fs');
const os = require('node:os');
const path = require('node:path');
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'fs-mode-probe-'));
const file = path.join(dir, 'existing.json');
fs.writeFileSync(file, 'old\n', { mode: 0o644 });
const before = fs.statSync(file).mode & 0o777;
fs.writeFileSync(file, 'new\n', { mode: 0o600 });
const after = fs.statSync(file).mode & 0o777;
console.log(JSON.stringify({ before: before.toString(8), after: after.toString(8) }));
JSRepository: gitcommit90/1Helm
Length of output: 10435
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- macOS workflow job ---'
sed -n '340,390p' .github/workflows/candidate.yml
printf '%s\n' '--- macOS acceptance runner script ---'
sed -n '1,240p' ops/platform-acceptance/macos.sh
printf '%s\n' '--- standalone Node behavior probe with chmod-established existing mode ---'
node - <<'JS'
'use strict';
const fs = require('node:fs');
const os = require('node:os');
const path = require('node:path');
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'fs-mode-probe-'));
const file = path.join(dir, 'existing.json');
fs.writeFileSync(file, 'old\n', { mode: 0o600 });
fs.chmodSync(file, 0o644);
const before = fs.statSync(file).mode & 0o777;
fs.writeFileSync(file, 'new\n', { mode: 0o600 });
const after = fs.statSync(file).mode & 0o777;
console.log(JSON.stringify({ before: before.toString(8), after: after.toString(8) }));
JSRepository: gitcommit90/1Helm
Length of output: 11094
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- macOS job definition and runner selection ---'
sed -n '318,350p' .github/workflows/candidate.yml
printf '%s\n' '--- pending evidence output behavior ---'
sed -n '1,90p' scripts/pending-acceptance-evidence.mjs
printf '%s\n' '--- output cleanup and permission handling references ---'
rg -n -C 2 'chmod|unlink|rm .*acceptance|acceptance\.json|HELM_ACCEPTANCE_OUTPUT' ops scripts .github/workflows/candidate.ymlRepository: gitcommit90/1Helm
Length of output: 25179
Security Misconfiguration (CWE-732): Incorrect Permission Assignment for Critical Resource
Reachability: Internal
Reachability path
● Entry
scripts/windows-acceptance-evidence.mjs:27
pass
│
▼
● Sink
scripts/macos-acceptance-evidence.mjs
Set restrictive permissions on existing output files.
writeFileSync(..., { mode: 0o600 }) does not change permissions when macos-acceptance.json already exists. Remove the output before each run, or use a symlink-safe create-and-permission-setting path.
🤖 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/macos-acceptance-evidence.mjs` at line 37, Update the output handling
around writeFileSync so every run enforces 0o600 permissions even when the
target already exists; remove the resolved output file before writing, or use a
symlink-safe creation path that explicitly sets permissions, while preserving
the current output location and JSON content.
| function normalizeOutcome(value, label, allowed) { | ||
| if (!value || typeof value !== "object") throw new Error(`${label} outcome is missing`); | ||
| const result = String(value.result || ""); | ||
| if (!allowed.includes(result)) throw new Error(`${label} outcome is invalid`); | ||
| return { | ||
| result, | ||
| checked_at: exact(value.checked_at, ISO_TIME, `${label} timestamp`), | ||
| summary: safeText(value.summary, `${label} summary`), | ||
| before_sha256: value.before_sha256 == null ? null : exact(value.before_sha256, HEX64, `${label} before digest`), | ||
| after_sha256: value.after_sha256 == null ? null : exact(value.after_sha256, HEX64, `${label} after digest`), | ||
| }; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject a passed outcome that carries no state digests.
value.before_sha256 == null is also true for undefined. If HELM_STATE_BEFORE_SHA256 or HELM_STATE_AFTER_SHA256 is unset, the digests silently become null while result stays "passed". normalizePlatformEvidence then derives result: "passed", and scripts/platform-acceptance-evidence.mjs exits 0. The unmeasured state preservation is only caught later by platformEvidenceBlockers at Lines 238-239, which reports it as a promotion blocker instead of a lane failure.
Require both digests when the outcome result is "passed" and digest evidence is expected.
🔒 Proposed fail-closed digest requirement
-function normalizeOutcome(value, label, allowed) {
+function normalizeOutcome(value, label, allowed, { requireDigests = false } = {}) {
if (!value || typeof value !== "object") throw new Error(`${label} outcome is missing`);
const result = String(value.result || "");
if (!allowed.includes(result)) throw new Error(`${label} outcome is invalid`);
+ if (requireDigests && result === "passed" && (value.before_sha256 == null || value.after_sha256 == null)) {
+ throw new Error(`${label} claimed a pass without before/after digests`);
+ }
return {- const statePreservation = normalizeOutcome(input.state_preservation, `${platform} state preservation`, ["passed", "failed", "blocked"]);
+ const statePreservation = normalizeOutcome(input.state_preservation, `${platform} state preservation`, ["passed", "failed", "blocked"], { requireDigests: true });🤖 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/platform-acceptance-lib.mjs` around lines 80 - 91, Update
normalizeOutcome to require both before_sha256 and after_sha256 when result is
"passed" and digest evidence is expected, rejecting missing or undefined values
before returning the normalized outcome. Preserve nullable digest handling for
non-passed outcomes and ensure the failure occurs during outcome normalization
rather than being deferred to platformEvidenceBlockers.
| if (runRecord) { | ||
| expected.runAttempt = String(runRecord.value?.run_attempt || ""); | ||
| validateRun(runRecord.value, expected, blockers); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Anchor the candidate run attempt to a trusted input. Both files take the expected run attempt from evidence that the candidate run itself produced, so the check proves only that the records agree with each other. runId and ciRunId are anchored to caller or environment inputs; run attempt is not. A complete, self-consistent evidence set from a different attempt of the same run therefore passes.
scripts/promotion-lib.mjs#L179-L182: accept the expected run attempt fromoptions(likeoptions.runId) and comparerunRecord.value?.run_attemptagainst it, instead of assigningexpected.runAttemptfrom that record and only format-checking it at line 116.scripts/candidate-promotion-skeleton.mjs#L113-L114: readGITHUB_RUN_ATTEMPT, validate it with/^\d+$/alongsideworkflowRunIdat lines 20-27, requiremac.candidate.run_attemptto equal it, and pass that value asrunAttempttoplatformEvidenceBlockers.
🛡️ Proposed fix for scripts/candidate-promotion-skeleton.mjs
+const runAttempt = String(env.GITHUB_RUN_ATTEMPT || "");
if (!env.HELM_CANDIDATE_DOWNLOAD || !env.HELM_MAC_CANDIDATE_DOWNLOAD || !env.HELM_REHEARSAL_EVIDENCE
|| !env.HELM_LINUX_ACCEPTANCE_EVIDENCE || !env.HELM_MAC_ACCEPTANCE_EVIDENCE
|| !env.HELM_WINDOWS_ACCEPTANCE_EVIDENCE || !env.HELM_ACCEPTANCE_CONTENT
- || !env.HELM_PROMOTION_OUTPUT || !/^\d+$/.test(workflowRunId) || !/^\d+$/.test(ciRunId)) {
+ || !env.HELM_PROMOTION_OUTPUT || !/^\d+$/.test(workflowRunId) || !/^\d+$/.test(ciRunId)
+ || !/^\d+$/.test(runAttempt)) {- || !/^\d+$/.test(String(mac?.candidate?.run_attempt || "")) || String(mac?.source_ci?.run_id) !== ciRunId
+ || String(mac?.candidate?.run_attempt) !== runAttempt || String(mac?.source_ci?.run_id) !== ciRunId const blockers = platformEvidenceBlockers(value, { platform, commit, version, runId: workflowRunId,
- runAttempt: String(mac.candidate.run_attempt), ciRunId, artifacts });
+ runAttempt, ciRunId, artifacts });📍 Affects 2 files
scripts/promotion-lib.mjs#L179-L182(this comment)scripts/candidate-promotion-skeleton.mjs#L113-L114
🤖 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/promotion-lib.mjs` around lines 179 - 182, Anchor run-attempt
validation to the trusted workflow input: in scripts/promotion-lib.mjs lines
179-182, use options.runAttempt as expected.runAttempt and compare the candidate
run record against it instead of deriving the expectation from runRecord. In
scripts/candidate-promotion-skeleton.mjs lines 113-114, read and validate
GITHUB_RUN_ATTEMPT alongside workflowRunId, require mac.candidate.run_attempt to
match it, and pass it as runAttempt to platformEvidenceBlockers.
| recovery: { ...pass("Scoped uninstall removed target state while preserving the unrelated distribution."), before_sha256: env.HELM_STATE_BEFORE_SHA256, after_sha256: env.HELM_STATE_AFTER_SHA256 }, | ||
| notes: ["Windows publishes no artifact and has no signing claim; this record binds behavior to the exact Linux TGZ.", "The exact candidate used the documented snapshot-assisted equivalent, not a claimed in-job Windows reboot."], | ||
| }); | ||
| writeFileSync(resolve(env.HELM_ACCEPTANCE_OUTPUT || "windows-acceptance.json"), `${JSON.stringify(evidence, null, 2)}\n`, { mode: 0o600 }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate script map ---'
ast-grep outline scripts/windows-acceptance-evidence.mjs
printf '%s\n' '--- candidate script ---'
cat -n scripts/windows-acceptance-evidence.mjs
printf '%s\n' '--- references to the output file and script ---'
rg -n -C 3 'windows-acceptance|windows-acceptance-evidence|HELM_ACCEPTANCE_OUTPUT' .Repository: gitcommit90/1Helm
Length of output: 13786
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Windows acceptance runner ---'
cat -n ops/platform-acceptance/windows.ps1 | sed -n '1,230p'
printf '%s\n' '--- candidate workflow Windows job and artifact handling ---'
rg -n -C 8 'accept-windows:|HELM_ACCEPTANCE_OUTPUT|Upload .*acceptance|windows-acceptance|actions/upload-artifact|retention-days' .github/workflows/candidate.yml
printf '%s\n' '--- relevant ACL and evidence handling ---'
rg -n -C 4 'icacls|Get-Acl|Set-Acl|AccessRule|ACL|provisioning-evidence|acceptance evidence|artifact' ops scripts .github/workflows/candidate.ymlRepository: gitcommit90/1Helm
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
script = Path("scripts/windows-acceptance-evidence.mjs").read_text()
runner = Path("ops/platform-acceptance/windows.ps1").read_text()
workflow = Path(".github/workflows/candidate.yml").read_text()
write = re.search(
r'writeFileSync\(resolve\(env\.HELM_ACCEPTANCE_OUTPUT \|\| "([^"]+)"\).*?mode:\s*(0o[0-7]+)',
script,
re.S,
)
assert write, "Windows evidence write was not found"
default_name, mode = write.groups()
job = workflow[workflow.index(" accept-windows:"):workflow.index(" assemble-promotion:")]
assert "HELM_ACCEPTANCE_OUTPUT: windows-acceptance.json" in job
assert re.search(r'path:\s*windows-acceptance\.json', job)
assert "if: always()" in job
acl_terms = re.findall(r'(?im)^\s*.*(?:icacls|Get-Acl|Set-Acl|AccessRule|New-Object\s+Security\.AccessControl).*$', runner + "\n" + job)
print({
"defaultOutput": default_name,
"requestedMode": mode,
"workflowUploadsSamePath": True,
"uploadRunsAlways": True,
"aclOperationsInBoundedPath": acl_terms,
})
PYRepository: gitcommit90/1Helm
Length of output: 319
🌐 Web query:
Node.js fs writeFileSync Windows file mode owner group other permissions documentation
💡 Result:
In Node.js, the mode option—which specifies file permissions (owner, group, other)—is not supported on Windows [1][2]. While the fs.writeFileSync method and other file system APIs allow you to pass a mode option (typically as an octal integer), this parameter is ignored when running on Windows [1][3][4]. Furthermore, the traditional Unix-style permission system (distinguishing between owner, group, and others) is not implemented on Windows [4]. On Windows, only the write permission can be modified, and access control is instead managed through Access Control Lists (ACLs), which Node.js does not directly manipulate via the mode option [4][5][6]. For portable applications, you should be aware that specifying a mode will have no effect on Windows, and Node.js will not raise an error when the option is provided [1][4]. If your application requires specific permission management on Windows, you will need to use platform-specific tools or native modules to interact with Windows ACLs directly [4][5].
Citations:
- 1: https://nodejs.org/api/fs.html
- 2: https://github.com/nodejs/node/blob/main/doc/api/fs.md
- 3: https://nodejs.org/docs/latest-v24.x/api/fs.html
- 4: https://nodejs.org/download/release/v19.5.0/docs/api/fs.html
- 5: https://nodejs.org/docs/latest-v23.x/api/fs.html
- 6: https://nodejs.org/docs/v22.14.0/api/fs.html
Security Misconfiguration (CWE-732): Incorrect Permission Assignment for Critical Resource
Reachability: Internal · Exploitability: Difficult
Reachability path
● Entry
scripts/macos-acceptance-evidence.mjs:23
pass
│
▼
● Sink
scripts/windows-acceptance-evidence.mjs
Apply an explicit Windows ACL to the evidence output.
mode: 0o600 is ignored on Windows. Store windows-acceptance.json in an ACL-protected directory and verify the ACL before upload or 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 `@scripts/windows-acceptance-evidence.mjs` at line 40, Update the
evidence-output flow around writeFileSync to store windows-acceptance.json in an
ACL-protected directory instead of relying on the ineffective mode option. Apply
an explicit Windows ACL to that directory, verify the ACL before any upload or
retention step, and preserve the existing configurable HELM_ACCEPTANCE_OUTPUT
path behavior.
1 participant
Outcome
Phase 4 automates fail-closed Linux, macOS, and Windows candidate acceptance and feeds exact retained evidence into Stable promotion.
Current honest blockers
No safe dedicated Mac or Windows runner was available, so no hardware evidence is claimed. The workflow remains blocked until those dedicated machines and credentials are provisioned. Stable cannot publish without them.
Verification
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests