Build fast previews and trusted candidate delivery - #67
Merged
Conversation
Add preview-only agent guardrails, fast private previews, read-only status and cleanup reports, and a trusted-main dress-rehearsal workflow with verified rollback. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Joseph Yaksich <gitcommit90@users.noreply.github.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds isolated preview and test tooling, read-only delivery reports, trusted candidate artifact generation, and a private Linux dress-rehearsal workflow with fail-closed installation and rollback evidence. ChangesDelivery tooling and dress rehearsal
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CI as CI workflow
participant Build as Candidate build job
participant Manifest as candidate-manifest.mjs
participant Attest as Provenance attestation
participant Runner as Private Phase 2 runner
participant Boundary as candidate-boundary.py
participant Installer as 1helm-candidate-install
CI->>Build: Trigger after successful trusted main push
Build->>Manifest: Create candidate manifest and checksums
Build->>Attest: Generate archive provenance
Runner->>Boundary: Validate downloaded candidate evidence
Runner->>Installer: Invoke root installation boundary
Installer->>Installer: Install candidate and verify health
Installer-->>Runner: Publish installation and rollback status
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Resolve CodeQL's incomplete URL substring sanitization findings in the delivery status test stub. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Joseph Yaksich <gitcommit90@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (9)
scripts/cleanup-report-lib.mjs (1)
56-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAllow dependency injection in
scanBackups.
scanDirectoryacceptslstatImplandreaddirImploverrides.scanBackupscalls the importedreaddirandlstatdirectly.test/cleanup-report.mjstherefore cannot cover the backup scan without touching the real filesystem. Accept the samedependenciesargument for consistency and testability.♻️ Proposed refactor
-async function scanBackups(root) { +async function scanBackups(root, dependencies = {}) { + const lstatImpl = dependencies.lstatImpl || lstat; + const readdirImpl = dependencies.readdirImpl || readdir; const displayPath = "src/server/agent.ts.bak-normal-terminal-<timestamp>"; const scan = emptyScan("Timestamped agent.ts backups", displayPath); const server = join(root, "src", "server"); let entries; - try { entries = await readdir(server, { withFileTypes: true }); } + try { entries = await readdirImpl(server, { withFileTypes: true }); } catch (error) { if (error?.code !== "ENOENT") scan.incomplete = true; return scan; } for (const entry of entries) { if (!isGeneratedAgentBackup(entry.name) || entry.isDirectory()) continue; scan.exists = true; - try { addFile(scan, await lstat(join(server, entry.name))); } catch { scan.incomplete = true; } + try { addFile(scan, await lstatImpl(join(server, entry.name))); } catch { scan.incomplete = true; } } return scan; }Pass the argument through from
collectCleanupReport:-export async function collectCleanupReport(root, now = Date.now()) { +export async function collectCleanupReport(root, now = Date.now(), dependencies = {}) { const paths = await Promise.all([ - scanDirectory(join(root, ".release-tmp"), "Release scratch data", ".release-tmp/"), - scanDirectory(join(root, ".native-test-data"), "Native test data", ".native-test-data/"), - scanBackups(root), + scanDirectory(join(root, ".release-tmp"), "Release scratch data", ".release-tmp/", dependencies), + scanDirectory(join(root, ".native-test-data"), "Native test data", ".native-test-data/", dependencies), + scanBackups(root, dependencies), ]);🤖 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/cleanup-report-lib.mjs` around lines 56 - 72, Update scanBackups to accept the same dependencies argument as scanDirectory, using injected lstatImpl and readdirImpl overrides instead of the imported filesystem functions. Update collectCleanupReport to pass its dependencies through to scanBackups, preserving existing behavior when overrides are absent.test/delivery-status.mjs (1)
35-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the candidate probe health decision.
The suite covers
fixtureProbein detail. It does not cover the health derivation incandidateProbeatscripts/delivery-status-lib.mjslines 250-254. That logic decides whether the private candidate reads ashealthy,unhealthy, oruncertain, and it includes a non-obvious branch on line 252 where a failed install with a successful rollback still reportshealthy. Candidate status is the purpose of this cohort, so this path deserves a direct test.Cover at least three cases: valid evidence with a healthy install and a matching app version; a failed install with a healthy rollback and a matching app version; and an unreadable evidence file with a reachable app.
💚 Proposed test
test("candidate health follows the evidence file and the running app version", async () => { const candidate = { commit: "a".repeat(40), digest: "b".repeat(64), version: "0.0.41", build_identity: "candidate-1-2.1", source_state: "trusted-main", ci: { workflow: "CI", run_id: "1", conclusion: "success" }, }; const evidence = (install, rollback) => JSON.stringify({ schema: 1, kind: "1helm-dress-rehearsal-status", running_candidate: candidate, last_attempt: candidate, previous_candidate: null, install: { ...install, checked_at: "2026-08-04T12:00:00Z" }, rollback: { ...rollback, checked_at: "2026-08-04T12:00:00Z" }, last_rollback: { ...rollback, checked_at: "2026-08-04T12:00:00Z" }, }); const config = { ...DEFAULT_STATUS_CONFIG, candidateUrl: "http://candidate.example:8123", candidateHost: "candidate-host", candidateId: "113", }; const probe = async (stdout, ok = true) => { const report = await collectEnvironmentStatus(config, { fetchImpl: async (url) => { if (url.startsWith(config.candidateUrl)) return response(200, '{"product":"1Helm","version":"0.0.41"}'); if (url.endsWith("/health")) return response(200, '{"ok":true,"product":"1Helm","surface":"website","version":"0.0.41"}'); return response(503, ""); }, runCommand: async () => ({ ok, stdout, timedOut: false }), sourceIdentity: { version: "0.0.41", commit: "abc123", dirty: false }, }); return report.environments.find(({ id }) => id === "candidate"); }; assert.equal((await probe(evidence({ result: "healthy", health: "healthy" }, { result: "not_needed" }))).health, "healthy"); assert.equal((await probe(evidence({ result: "failed", health: "unhealthy" }, { result: "healthy" }))).health, "healthy"); assert.equal((await probe("", false)).health, "uncertain"); });🤖 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/delivery-status.mjs` around lines 35 - 49, Add direct coverage for candidate health derivation in the existing delivery-status tests using collectEnvironmentStatus and the candidate environment result: verify healthy evidence with a matching app version returns healthy, failed installation followed by healthy rollback still returns healthy, and an unreadable evidence file with a reachable app returns uncertain. Reuse the existing configuration and response helpers, and include complete candidate evidence fields required by parseCandidateEvidence.scripts/delivery-status.mjs (1)
16-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueState the candidate pairing requirement in the help text.
statusConfigthrows when only some ofHELM_STATUS_CANDIDATE_URL,HELM_STATUS_CANDIDATE_HOST, andHELM_STATUS_CANDIDATE_IDare set. Seescripts/delivery-status-lib.mjslines 40-42. The fixture entries state this requirement; the candidate entries do not.📝 Proposed fix
- HELM_STATUS_CANDIDATE_URL optional; configure all three candidate values - HELM_STATUS_CANDIDATE_HOST optional local SSH alias for the Proxmox host - HELM_STATUS_CANDIDATE_ID optional local guest ID; evidence read is fixed + HELM_STATUS_CANDIDATE_URL optional; configure all three candidate values + HELM_STATUS_CANDIDATE_HOST local SSH alias for the Proxmox host; required with the other two + HELM_STATUS_CANDIDATE_ID local guest ID; required with the other two; evidence read is fixed🤖 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/delivery-status.mjs` around lines 16 - 18, Update the help text for HELM_STATUS_CANDIDATE_URL, HELM_STATUS_CANDIDATE_HOST, and HELM_STATUS_CANDIDATE_ID to explicitly state that all three values must be configured together, matching the requirement already described for fixture entries and enforced by statusConfig.test/cleanup-report.mjs (1)
17-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for the symlinked or non-directory root.
scanDirectoryhas a distinct branch atscripts/cleanup-report-lib.mjslines 28-33. That branch setsnotDirectoryand stops before enumeration. It is the control that keeps the read-only scan inside the intended paths. No test covers it, so a later change could start following a symlinked root without failing the suite.💚 Proposed test
test("a symlinked or non-directory root is reported without enumeration", async () => { for (const rootStat of [ { isDirectory: () => false, isSymbolicLink: () => false }, { isDirectory: () => true, isSymbolicLink: () => true }, ]) { const scan = await scanDirectory("/generated", "Generated", ".generated/", { lstatImpl: async () => rootStat, readdirImpl: async () => { throw new Error("must not enumerate"); }, }); assert.equal(scan.exists, true); assert.equal(scan.notDirectory, true); assert.equal(scan.incomplete, true); assert.equal(scan.fileCount, 0); assert.match(formatCleanupReport({ checkedAt: new Date().toISOString(), paths: [scan] }), /not a normal directory/); } });🤖 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/cleanup-report.mjs` around lines 17 - 27, Add a test alongside the existing cleanup-report scan tests covering both a non-directory root and a symlinked root. Use scanDirectory with lstatImpl returning each root-stat variant and readdirImpl that fails if called; assert exists, notDirectory, incomplete, and zero fileCount, then verify formatCleanupReport includes “not a normal directory.”test/delivery-governance.mjs (1)
33-42: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a behavioral check for report-only cleanup.
The current test checks source text. The four-name denylist can miss
fs.rm,fs.promises, aliases, child processes, or helper functions. ThereadOnly: trueandremoved: falsematches do not prove that filesystem state remains unchanged. Run cleanup against a temporary fixture and compare files and directories before and after.🤖 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/delivery-governance.mjs` around lines 33 - 42, The test “generated state is ignored and cleanup remains report-only” must verify runtime behavior instead of only scanning source text. Create a temporary fixture with representative files and directories, snapshot their contents and structure, run the cleanup entry points in report-only mode, then assert the fixture is unchanged; retain the existing denylist assertions only if still relevant.scripts/run-test-suite.mjs (1)
5-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrint the exported version constant.
Line 9 hardcodes
3.14.0.scripts/mnemosyne-test-runtime.mjsalready exportsMNEMOSYNE_VERSION. If the pin changes, this message reports a version the suite does not use.♻️ Proposed fix
-import { prepareMnemosyneTestRuntime } from "./mnemosyne-test-runtime.mjs"; +import { MNEMOSYNE_VERSION, prepareMnemosyneTestRuntime } from "./mnemosyne-test-runtime.mjs"; const root = resolve(import.meta.dirname, ".."); const prepared = prepareMnemosyneTestRuntime(root); -process.stdout.write(`Using pinned Mnemosyne 3.14.0 from ${prepared.source}.\n`); +process.stdout.write(`Using pinned Mnemosyne ${MNEMOSYNE_VERSION} from ${prepared.source}.\n`);🤖 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/run-test-suite.mjs` around lines 5 - 11, Update the startup message in the test runner to import and interpolate the exported MNEMOSYNE_VERSION from mnemosyne-test-runtime.mjs instead of hardcoding 3.14.0, while preserving the existing prepared.source and runtime behavior.test/phase1-tools.mjs (1)
11-11: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConvert the file URL with
fileURLToPath.
URL.pathnamekeeps percent-encoding and, on Windows, returns a path such as/C:/repo. If the checkout path contains a space or a non-ASCII character, the encoded value flows intopreviewConfig,selectFastTests, andmnemosyneTestPaths, and the comparison at line 17 fails.♻️ Proposed fix
+import { fileURLToPath } from "node:url"; ... -const root = new URL("..", import.meta.url).pathname; +const root = fileURLToPath(new URL("..", import.meta.url));🤖 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/phase1-tools.mjs` at line 11, Update the root path initialization near the root constant to convert the file URL with Node’s fileURLToPath utility instead of reading URL.pathname, ensuring decoded paths and platform-correct Windows paths are passed to previewConfig, selectFastTests, and mnemosyneTestPaths.scripts/preview-lib.mjs (1)
142-158: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRe-check
closedafter the await and prune stale watchers.
addchecksclosedbeforeawait readdirImpl(...)but not after it. If the returned close function runs while areaddiris pending,addstill creates a watcher and stores it inwatchers. That watcher is never closed, so it keeps the event loop alive andscripts/preview.mjscan hang after Ctrl+C.
scanalso adds watchers for new directories but never removes watchers for deleted directories, so the Map grows during long preview sessions.♻️ Proposed fix
const add = async (directory) => { if (closed || watchers.has(directory)) return; const entries = await readdirImpl(directory, { withFileTypes: true }).catch(() => []); + if (closed || watchers.has(directory)) return; const watcher = watchImpl(directory, (event, filename) => { if (!filename) return; const path = join(directory, String(filename)); onChange(path, event); if (event === "rename") void scan(directory); }); - watcher.on?.("error", (error) => onChange(directory, "watch-error", error)); + watcher.on?.("error", (error) => { + watchers.delete(directory); + watcher.close(); + onChange(directory, "watch-error", error); + }); watchers.set(directory, watcher);🤖 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/preview-lib.mjs` around lines 142 - 158, Update the add function to re-check closed immediately after the awaited readdirImpl call and return before creating or storing a watcher when shutdown has begun. Update scan to reconcile the watchers Map with the currently discovered directory tree, removing and closing watchers for directories that no longer exist while preserving active watchers and recursive additions.scripts/mnemosyne-test-runtime.mjs (1)
65-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove abandoned virtual-environment directories.
Two paths leak disk space under
.test-state/mnemosyne/:
- Line 68 quarantines an invalid cache as
<environmentRoot>.invalid-...and never removes it.- Line 79 creates
<environmentRoot>.install-...for each attempt. A failed attempt hitscontinueat line 83 or line 86 and leaves the directory in place.Each directory holds a complete virtual environment, and
.test-state/is git-ignored, so the growth is silent. Delete the failed attempt root beforecontinue, and remove the previous quarantine directory after a successful replacement.🤖 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/mnemosyne-test-runtime.mjs` around lines 65 - 95, Update the cache preparation flow to remove abandoned virtual-environment directories: retain the quarantine path created for an invalid environment, then delete it after a successful replacement; in the installer loop, remove each prepare-cache attemptRoot before continuing when venv creation, package installation, or pinned validation fails. Preserve disposable-runtime cleanup and successful cache replacement behavior.
🤖 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/canary-plan.md`:
- Around line 34-42: Update the status field list in the canary plan to match
the implemented status.json schema: remove canary role, hypervisor identity,
guest identity, health endpoint, and artifact name, or explicitly identify them
as Phase 0 metadata combined from local configuration. Keep candidate evidence
fields separate from locally configured environment metadata.
In `@ops/dress-rehearsal/1helm-candidate-install`:
- Around line 83-89: Update the success condition in the candidate health-check
block to verify that `/opt/1helm/current` identifies the attempted candidate,
using its digest-named release or validated identity from `/api/setup/status`.
Only set `result`, `health`, and `rollback` to the healthy success values after
this identity check passes; retain the existing install-status, service-active,
and endpoint checks.
In `@ops/dress-rehearsal/candidate-boundary.py`:
- Around line 173-180: Select a single reporting candidate before computing
ci_line, preferring running_candidate and falling back to the relevant
last_attempt record when running_candidate is empty. Read both source_state and
CI fields from that same candidate, while preserving the trusted-main formatting
and local provisioning fallback behavior.
In `@ops/dress-rehearsal/runner-job-started`:
- Around line 5-7: Add a validation alongside the existing GITHUB_WORKFLOW,
GITHUB_JOB, and GITHUB_EVENT_NAME checks that requires GITHUB_WORKFLOW_REF to
identify .github/workflows/candidate.yml at the trusted main ref, rejecting any
other workflow file or ref before proceeding.
In `@scripts/cleanup-report.mjs`:
- Line 22: Add an engines requirement in package.json declaring Node 20.11.0 or
newer, matching the needs of cleanup-report.mjs and the other affected scripts.
Keep the existing import.meta.dirname usage unchanged.
In `@scripts/delivery-status-lib.mjs`:
- Line 346: Update the CI result formatting around the lines.push call to report
“unknown” whenever running is absent or running.source_state is missing, and
only report “not run (local provisioning proof)” when source_state is explicitly
a non-trusted-main value. Ensure parseCandidateEvidence or the surrounding logic
preserves and distinguishes missing source_state rather than treating it as
proof of a local build.
In `@scripts/preview-lib.mjs`:
- Around line 116-126: Update the restart queue logic in changed() so failures
from stopChild or startChild are caught, reported through the class’s existing
callback mechanism, and do not leave this.queue rejected. Ensure subsequent
changed() calls can continue scheduling restarts and close() can still await a
settled queue.
In `@scripts/preview.mjs`:
- Around line 73-79: Update prepareExcalidrawAssets to write preview assets
under a dedicated preview output root rather than public/, and configure the
preview server to serve that same isolated root. Keep the existing Excalidraw
source paths and asset-copy behavior unchanged while ensuring preview execution
cannot overwrite production assets.
In `@scripts/run-fast-tests.mjs`:
- Around line 11-16: Update the exit-code handling after spawnSync in the test
runner so a null result.status, including cases where result.error indicates the
child could not start, sets process.exitCode to a nonzero failure value instead
of 0; preserve the existing signal and normal status handling.
In `@test/delivery-governance.mjs`:
- Around line 12-20: Update the governance assertions in the delivery-governance
test to target the relevant bullet and handoff sections rather than using
independent or unbounded token matches. Add the missing “artifacts” prohibited
boundary and “external system” handoff field, and scope the Phase 2 assertions
to the Phase 2 section so exceptions or unrelated text cannot satisfy them.
In `@test/phase2-candidate.mjs`:
- Around line 56-76: Extend the test case around the existing manifest mutations
in the root boundary test to cover sealed OCI validation: set
manifest.sealed_oci.sha256 to a valid 64-character digest that differs from the
embedded OCI tar, write the manifest, run candidate-boundary.py validate, and
assert a nonzero status with the sealed OCI mismatch error. Preserve the
existing commit and archive SHA-256 mismatch assertions.
- Around line 93-120: Update the test named “candidate workflow and guest
boundary exclude PR code and broad root access” to validate the effective sudo
policy used by sudo -n /usr/local/sbin/1helm-candidate-install instead of only
checking the local sudoersExample string. Add the exact rule to a reviewed
provisioning artifact and validate that artifact with visudo, or inspect the
installed policy directly on the Phase 2 runner, while retaining the existing
protection against broad NOPASSWD: ALL access.
---
Nitpick comments:
In `@scripts/cleanup-report-lib.mjs`:
- Around line 56-72: Update scanBackups to accept the same dependencies argument
as scanDirectory, using injected lstatImpl and readdirImpl overrides instead of
the imported filesystem functions. Update collectCleanupReport to pass its
dependencies through to scanBackups, preserving existing behavior when overrides
are absent.
In `@scripts/delivery-status.mjs`:
- Around line 16-18: Update the help text for HELM_STATUS_CANDIDATE_URL,
HELM_STATUS_CANDIDATE_HOST, and HELM_STATUS_CANDIDATE_ID to explicitly state
that all three values must be configured together, matching the requirement
already described for fixture entries and enforced by statusConfig.
In `@scripts/mnemosyne-test-runtime.mjs`:
- Around line 65-95: Update the cache preparation flow to remove abandoned
virtual-environment directories: retain the quarantine path created for an
invalid environment, then delete it after a successful replacement; in the
installer loop, remove each prepare-cache attemptRoot before continuing when
venv creation, package installation, or pinned validation fails. Preserve
disposable-runtime cleanup and successful cache replacement behavior.
In `@scripts/preview-lib.mjs`:
- Around line 142-158: Update the add function to re-check closed immediately
after the awaited readdirImpl call and return before creating or storing a
watcher when shutdown has begun. Update scan to reconcile the watchers Map with
the currently discovered directory tree, removing and closing watchers for
directories that no longer exist while preserving active watchers and recursive
additions.
In `@scripts/run-test-suite.mjs`:
- Around line 5-11: Update the startup message in the test runner to import and
interpolate the exported MNEMOSYNE_VERSION from mnemosyne-test-runtime.mjs
instead of hardcoding 3.14.0, while preserving the existing prepared.source and
runtime behavior.
In `@test/cleanup-report.mjs`:
- Around line 17-27: Add a test alongside the existing cleanup-report scan tests
covering both a non-directory root and a symlinked root. Use scanDirectory with
lstatImpl returning each root-stat variant and readdirImpl that fails if called;
assert exists, notDirectory, incomplete, and zero fileCount, then verify
formatCleanupReport includes “not a normal directory.”
In `@test/delivery-governance.mjs`:
- Around line 33-42: The test “generated state is ignored and cleanup remains
report-only” must verify runtime behavior instead of only scanning source text.
Create a temporary fixture with representative files and directories, snapshot
their contents and structure, run the cleanup entry points in report-only mode,
then assert the fixture is unchanged; retain the existing denylist assertions
only if still relevant.
In `@test/delivery-status.mjs`:
- Around line 35-49: Add direct coverage for candidate health derivation in the
existing delivery-status tests using collectEnvironmentStatus and the candidate
environment result: verify healthy evidence with a matching app version returns
healthy, failed installation followed by healthy rollback still returns healthy,
and an unreadable evidence file with a reachable app returns uncertain. Reuse
the existing configuration and response helpers, and include complete candidate
evidence fields required by parseCandidateEvidence.
In `@test/phase1-tools.mjs`:
- Line 11: Update the root path initialization near the root constant to convert
the file URL with Node’s fileURLToPath utility instead of reading URL.pathname,
ensuring decoded paths and platform-correct Windows paths are passed to
previewConfig, selectFastTests, and mnemosyneTestPaths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f15fa113-fadb-4758-8926-c0f917048a1a
📒 Files selected for processing (31)
.delivery-status.local.example.github/workflows/candidate.yml.gitignoreAGENTS.mdCLAUDE.mdREADME.mddocs/canary-plan.mddocs/dress-rehearsal.mdops/dress-rehearsal/1helm-candidate-installops/dress-rehearsal/candidate-boundary.pyops/dress-rehearsal/runner-job-startedops/dress-rehearsal/runner.service.override.confpackage.jsonscripts/candidate-manifest.mjsscripts/cleanup-report-lib.mjsscripts/cleanup-report.mjsscripts/delivery-status-lib.mjsscripts/delivery-status.mjsscripts/fast-test-lib.mjsscripts/mnemosyne-test-runtime.mjsscripts/package-linux-host.mjsscripts/preview-lib.mjsscripts/preview.mjsscripts/run-fast-tests.mjsscripts/run-test-suite.mjstest/cleanup-report.mjstest/delivery-governance.mjstest/delivery-status.mjstest/desktop.mjstest/phase1-tools.mjstest/phase2-candidate.mjs
Comment on lines
+34
to
+42
| check, and automatic rollback in the Linux installer. The status record contains: | ||
|
|
||
| - canary role, hypervisor/guest identity, and health endpoint; | ||
| - current version and candidate version; | ||
| - exact source commit plus artifact name and SHA-256 digest; | ||
| - service and application health, check time, result, and any uncertainty; | ||
| - CI workflow/run result and candidate build identity; | ||
| - install health and time; and | ||
| - previous candidate plus rollback result/time. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Align the status field list with the implemented schema.
status.json does not contain the canary role, hypervisor identity, guest identity, health endpoint, or artifact name.
Remove these fields from this list, or state that Phase 0 combines them from local configuration. Keep the candidate evidence fields separate from locally configured environment metadata.
🤖 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/canary-plan.md` around lines 34 - 42, Update the status field list in
the canary plan to match the implemented status.json schema: remove canary role,
hypervisor identity, guest identity, health endpoint, and artifact name, or
explicitly identify them as Phase 0 metadata combined from local configuration.
Keep candidate evidence fields separate from locally configured environment
metadata.
Comment on lines
+83
to
+89
| current_link="$(readlink -f /opt/1helm/current 2>/dev/null || true)" | ||
| if [[ "$install_status" -eq 0 ]] && systemctl is-active --quiet 1helm.service \ | ||
| && curl -fsS http://127.0.0.1:8123/api/setup/status >/dev/null 2>&1; then | ||
| result=healthy | ||
| health=healthy | ||
| rollback=not_needed | ||
| message="Candidate v$version is installed and healthy." |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the installer contract for the release path and success conditions.
fd -a '^install\.sh$' . -0 |
xargs -0 rg -n -C 6 \
'HELM_RELEASE_SHA256|/opt/1helm/current|readlink|ln[[:space:]]+-s|systemctl|8123|setup/status'
# Find whether the health endpoint exposes the active commit, digest, or version.
rg -n -C 5 \
'/api/setup/status|setup/status|release.*digest|build_identity|source.*commit' \
.Repository: gitcommit90/1Helm
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- candidate installer ---'
sed -n '1,125p' ops/dress-rehearsal/1helm-candidate-install
printf '%s\n' '--- candidate installer invocation and record schema ---'
rg -n -C 12 \
'install_status|running_candidate|last_attempt|record|candidate-install|current_link|previous_link' \
ops/dress-rehearsal scriptsRepository: gitcommit90/1Helm
Length of output: 39137
Require candidate identity before recording success.
install_status == 0, an active unit, and any successful response from port 8123 do not prove that /opt/1helm/current points to the attempted candidate. Require the link to match the candidate's digest-named release, or validate the candidate identity from /api/setup/status, before recording running_candidate.
🤖 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/dress-rehearsal/1helm-candidate-install` around lines 83 - 89, Update the
success condition in the candidate health-check block to verify that
`/opt/1helm/current` identifies the attempted candidate, using its digest-named
release or validated identity from `/api/setup/status`. Only set `result`,
`health`, and `rollback` to the healthy success values after this identity check
passes; retain the existing install-status, service-active, and endpoint checks.
Comment on lines
+173
to
+180
| running = status.get("running_candidate") or {} | ||
| previous = status.get("previous_candidate") or {} | ||
| ci = running.get("ci") or (status.get("last_attempt") or {}).get("ci") or {} | ||
| install = status.get("install") or {} | ||
| rollback = status.get("last_rollback") or status.get("rollback") or {} | ||
| ci_line = "not run (local provisioning proof)" if running.get("source_state") != "trusted-main" else ( | ||
| f"{ci.get('workflow', 'unknown')} run {ci.get('run_id', 'unknown')} — {ci.get('conclusion', 'unknown')}" | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use the same candidate record for the CI value and source state.
If the first trusted-main installation fails, running_candidate is empty. The code reads ci from last_attempt but reports not run (local provisioning proof) because it reads source_state from the empty running record.
Select one reporting candidate before formatting both fields.
Proposed fix
def summary(status: dict) -> str:
running = status.get("running_candidate") or {}
previous = status.get("previous_candidate") or {}
- ci = running.get("ci") or (status.get("last_attempt") or {}).get("ci") or {}
+ reported = running or status.get("last_attempt") or {}
+ ci = reported.get("ci") or {}
install = status.get("install") or {}
rollback = status.get("last_rollback") or status.get("rollback") or {}
- ci_line = "not run (local provisioning proof)" if running.get("source_state") != "trusted-main" else (
+ ci_line = "not run (local provisioning proof)" if reported.get("source_state") != "trusted-main" else (
f"{ci.get('workflow', 'unknown')} run {ci.get('run_id', 'unknown')} — {ci.get('conclusion', 'unknown')}"
)📝 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
| running = status.get("running_candidate") or {} | |
| previous = status.get("previous_candidate") or {} | |
| ci = running.get("ci") or (status.get("last_attempt") or {}).get("ci") or {} | |
| install = status.get("install") or {} | |
| rollback = status.get("last_rollback") or status.get("rollback") or {} | |
| ci_line = "not run (local provisioning proof)" if running.get("source_state") != "trusted-main" else ( | |
| f"{ci.get('workflow', 'unknown')} run {ci.get('run_id', 'unknown')} — {ci.get('conclusion', 'unknown')}" | |
| ) | |
| running = status.get("running_candidate") or {} | |
| previous = status.get("previous_candidate") or {} | |
| reported = running or status.get("last_attempt") or {} | |
| ci = reported.get("ci") or {} | |
| install = status.get("install") or {} | |
| rollback = status.get("last_rollback") or status.get("rollback") or {} | |
| ci_line = "not run (local provisioning proof)" if reported.get("source_state") != "trusted-main" else ( | |
| f"{ci.get('workflow', 'unknown')} run {ci.get('run_id', 'unknown')} — {ci.get('conclusion', 'unknown')}" | |
| ) |
🤖 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/dress-rehearsal/candidate-boundary.py` around lines 173 - 180, Select a
single reporting candidate before computing ci_line, preferring
running_candidate and falling back to the relevant last_attempt record when
running_candidate is empty. Read both source_state and CI fields from that same
candidate, while preserving the trusted-main formatting and local provisioning
fallback behavior.
Comment on lines
+5
to
+7
| [[ "${GITHUB_WORKFLOW:-}" == "Candidate dress rehearsal" ]] || { echo "This runner accepts only the candidate workflow." >&2; exit 1; } | ||
| [[ "${GITHUB_JOB:-}" == "deploy" ]] || { echo "This runner accepts only the constrained deployment job." >&2; exit 1; } | ||
| [[ "${GITHUB_EVENT_NAME:-}" == "workflow_run" ]] || { echo "This runner rejects PR and direct-push jobs." >&2; exit 1; } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect all workflows that can target the runner.
fd -e yml -e yaml .github/workflows -0 |
xargs -0 rg -n -C 4 \
'Candidate dress rehearsal|workflow_run|^[[:space:]]*deploy:|runs-on:|1helm-dress-rehearsal'
# Confirm whether the hook validates an exact workflow path/ref.
rg -n -C 3 \
'GITHUB_WORKFLOW(_REF|_SHA)?|candidate\.yml|GITHUB_JOB|GITHUB_EVENT_NAME' \
ops/dress-rehearsalRepository: gitcommit90/1Helm
Length of output: 12706
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- candidate workflow and runner-targeting workflows ---'
find .github/workflows -type f \( -name '*.yml' -o -name '*.yaml' \) -print0 |
xargs -0 grep -n -E -C 5 \
'Candidate dress rehearsal|workflow_run|^[[:space:]]*deploy:|runs-on:|1helm-dress-rehearsal'
printf '%s\n' '--- runner hook ---'
nl -ba ops/dress-rehearsal/runner-job-started | sed -n '1,45p'
printf '%s\n' '--- documentation claim ---'
nl -ba docs/dress-rehearsal.md | sed -n '40,52p'Repository: gitcommit90/1Helm
Length of output: 8756
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- runner hook ---'
sed -n '1,45p' ops/dress-rehearsal/runner-job-started
printf '%s\n' '--- documentation claim ---'
sed -n '40,52p' docs/dress-rehearsal.mdRepository: gitcommit90/1Helm
Length of output: 2384
Authorization Bypass (CWE-863): Incorrect Authorization
Reachability: External
Bind the job to the exact candidate workflow file.
The hook checks the workflow name, job ID, and event type, but not the workflow file identity. A different workflow_run workflow could satisfy these checks and target 1helm-dress-rehearsal-phase2. Reject the job unless GITHUB_WORKFLOW_REF identifies .github/workflows/candidate.yml at the trusted main ref.
🤖 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/dress-rehearsal/runner-job-started` around lines 5 - 7, Add a validation
alongside the existing GITHUB_WORKFLOW, GITHUB_JOB, and GITHUB_EVENT_NAME checks
that requires GITHUB_WORKFLOW_REF to identify .github/workflows/candidate.yml at
the trusted main ref, rejecting any other workflow file or ref before
proceeding.
| process.exit(2); | ||
| } | ||
|
|
||
| const report = await collectCleanupReport(resolve(import.meta.dirname, "..")); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Report the declared Node engine and CI Node versions.
set -euo pipefail
fd -H -t f 'package.json' -d 2 --exec sh -c 'echo "== $1"; jq -r ".engines // \"no engines field\"" "$1"' sh {}
fd -H -t f '^\.nvmrc$|^\.node-version$|^\.tool-versions$' --exec sh -c 'echo "== $1"; cat "$1"' sh {}
fd -H -t f -e yml -e yaml . .github/workflows --exec rg -n 'node-version' {}Repository: gitcommit90/1Helm
Length of output: 254
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== package.json'
cat -n package.json | sed -n '1,120p'
printf '%s\n' '== cleanup-report.mjs'
cat -n scripts/cleanup-report.mjs | sed -n '1,60p'
printf '%s\n' '== delivery-status-lib.mjs context'
cat -n scripts/delivery-status-lib.mjs | sed -n '245,280p'
printf '%s\n' '== workflow Node setup context'
rg -n -C 4 'node-version|setup-node|npm ci|npm install|yarn|pnpm' .github/workflowsRepository: gitcommit90/1Helm
Length of output: 12277
Declare the Node engine requirement.
package.json has no engines field, although both scripts require Node 20.11.0 or newer. Add the requirement or replace import.meta.dirname.
🤖 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/cleanup-report.mjs` at line 22, Add an engines requirement in
package.json declaring Node 20.11.0 or newer, matching the needs of
cleanup-report.mjs and the other affected scripts. Keep the existing
import.meta.dirname usage unchanged.
Comment on lines
+73
to
+79
| async function prepareExcalidrawAssets() { | ||
| const source = join(root, "node_modules", "@excalidraw", "excalidraw", "dist", "prod"); | ||
| const target = join(root, "public", "excalidraw"); | ||
| await mkdir(target, { recursive: true }); | ||
| await copyFile(join(source, "index.css"), join(target, "index.css")); | ||
| await cp(join(source, "fonts"), join(target, "fonts"), { recursive: true, force: true }); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether the production build and server static root use the same public/ outputs.
set -uo pipefail
# Build/start scripts and their output paths.
fd -H -t f 'package.json' -d 1 --exec cat -n {}
# esbuild/tailwind outputs in build scripts.
rg -n --glob 'scripts/**' -e 'public/bundle\.js' -e 'public/app\.css' -e 'public/excalidraw' -e 'outfile'
# Static file root used by the server.
rg -n --glob 'src/server/**' -e '"public"' -e "'public'" -e 'static' -C 3Repository: gitcommit90/1Helm
Length of output: 7470
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- preview structure and relevant code ---'
ast-grep outline scripts/preview.mjs
sed -n '1,150p' scripts/preview.mjs
sed -n '185,215p' scripts/preview.mjs
printf '%s\n' '--- production asset producers and server static root ---'
fd -H -t f . scripts src/server --max-depth 3 | sort
rg -n -C 4 'public/|config\.generatedRoot|static|serveStatic|bundle\.js|app\.css|excalidraw' scripts src/server src README.md package.json
printf '%s\n' '--- preview documentation ---'
rg -n -C 5 'preview|never changes|\.preview-data|npm start' README.mdRepository: gitcommit90/1Helm
Length of output: 44791
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- preview configuration ---'
ast-grep outline scripts/preview-lib.mjs
sed -n '1,220p' scripts/preview-lib.mjs
printf '%s\n' '--- server public-root resolution ---'
sed -n '1,125p' src/server/index.ts
rg -n -C 3 'APP_ROOT|PUBLIC|HELM_APP_ROOT|CTRL_DATA_DIR' src/server/index.ts scripts/preview.mjs scripts/preview-lib.mjs
printf '%s\n' '--- generated-output references and cleanup behavior ---'
rg -n -C 3 'generatedRoot|generated|preview-data|public/bundle\.js|public/app\.css|public/excalidraw|Stable was untouched|rm\(' scripts src README.md package.json .gitignore 2>/dev/null || true
printf '%s\n' '--- deterministic path check ---'
python3 - <<'PY'
from pathlib import Path
import json, re
pkg = json.loads(Path("package.json").read_text())
scripts = pkg["scripts"]
preview = Path("scripts/preview.mjs").read_text()
server = Path("src/server/index.ts").read_text()
production = {
name: [value for value in ("public/bundle.js", "public/app.css", "public/excalidraw")
if value in scripts.get(name, "") or value in Path("scripts").read_text() if False]
for name in ("build:js", "build:css", "build:excalidraw")
}
preview_outputs = sorted(set(re.findall(r'(?:"|`)public/(?:bundle\.js|app\.css|excalidraw)', preview)))
print("production scripts:", {k: v for k, v in production.items()})
print("preview mentions:", preview_outputs)
print("preview writes shared bundle:", 'join(root, "public", "bundle.js")' in preview)
print("preview writes shared excalidraw:", 'join(root, "public", "excalidraw")' in preview)
print("preview writes shared css:", '"public/app.css"' in preview)
print("server derives public root from APP_ROOT:", bool(re.search(r'APP_ROOT.*?PUBLIC|PUBLIC.*?APP_ROOT', server, re.S)))
print("generatedRoot in preview sources:", "generatedRoot" in (preview + Path("scripts/preview-lib.mjs").read_text()))
PYRepository: gitcommit90/1Helm
Length of output: 50373
Use an isolated output directory for preview assets.
The preview writes to the same public/ paths that the production build creates. Stable reads those paths through HELM_APP_ROOT. Running both processes from one checkout lets the preview replace Stable’s assets and leaves modified files after exit. Configure the preview server to serve assets from a separate output root.
🤖 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/preview.mjs` around lines 73 - 79, Update prepareExcalidrawAssets to
write preview assets under a dedicated preview output root rather than public/,
and configure the preview server to serve that same isolated root. Keep the
existing Excalidraw source paths and asset-copy behavior unchanged while
ensuring preview execution cannot overwrite production assets.
Comment on lines
+11
to
+16
| const result = spawnSync(process.execPath, ["--test", ...tests], { | ||
| cwd: root, | ||
| env: { ...process.env, NODE_ENV: "test" }, | ||
| stdio: "inherit", | ||
| }); | ||
| process.exitCode = result.status || (result.signal ? 1 : 0); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Report spawn failures instead of exiting 0.
If spawnSync cannot start the child, it returns status: null, signal: null, and sets result.error. Line 16 then evaluates to 0, so npm run test:fast reports success although no test ran. Inspect result.error and treat a null status as a failure.
🐛 Proposed fix
const result = spawnSync(process.execPath, ["--test", ...tests], {
cwd: root,
env: { ...process.env, NODE_ENV: "test" },
stdio: "inherit",
});
- process.exitCode = result.status || (result.signal ? 1 : 0);
+ if (result.error) throw result.error;
+ process.exitCode = result.status === 0 ? 0 : (result.status ?? 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.
Suggested change
| const result = spawnSync(process.execPath, ["--test", ...tests], { | |
| cwd: root, | |
| env: { ...process.env, NODE_ENV: "test" }, | |
| stdio: "inherit", | |
| }); | |
| process.exitCode = result.status || (result.signal ? 1 : 0); | |
| const result = spawnSync(process.execPath, ["--test", ...tests], { | |
| cwd: root, | |
| env: { ...process.env, NODE_ENV: "test" }, | |
| stdio: "inherit", | |
| }); | |
| if (result.error) throw result.error; | |
| process.exitCode = result.status === 0 ? 0 : (result.status ?? 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/run-fast-tests.mjs` around lines 11 - 16, Update the exit-code
handling after spawnSync in the test runner so a null result.status, including
cases where result.error indicates the child could not start, sets
process.exitCode to a nonzero failure value instead of 0; preserve the existing
signal and normal status handling.
Comment on lines
+12
to
+20
| assert.match(contract, /default delivery mode[\s\S]*PREVIEW ONLY/i); | ||
| assert.match(claudeInstructions, /\[AGENTS\.md\]\(AGENTS\.md\)/); | ||
| assert.match(claudeInstructions, /authoritative delivery contract/i); | ||
| for (const boundary of ["bump versions", "tags", "releases", "deploy", "stable", "production data", "infrastructure", "broaden"]) { | ||
| assert.match(contract, new RegExp(boundary, "i")); | ||
| } | ||
| assert.match(contract, /npm run ci/); | ||
| for (const evidence of ["changed files", "checks run", "risks", "rollback", "stable"]) { | ||
| assert.match(contract, new RegExp(evidence, "i")); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the governance assertions section-aware and complete.
These checks use independent token matches and unbounded [\s\S]* scans. They can pass when required words occur in unrelated sections or when an exception changes the rule. The contract check also omits artifacts from the prohibited boundary list and external system from the handoff fields required by AGENTS.md. Assert the relevant bullet and handoff sections directly, and scope the Phase 2 checks to the Phase 2 section.
Also applies to: 26-30
🤖 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/delivery-governance.mjs` around lines 12 - 20, Update the governance
assertions in the delivery-governance test to target the relevant bullet and
handoff sections rather than using independent or unbounded token matches. Add
the missing “artifacts” prohibited boundary and “external system” handoff field,
and scope the Phase 2 assertions to the Phase 2 section so exceptions or
unrelated text cannot satisfy them.
Comment on lines
+56
to
+76
| test("root boundary rejects digest, source, and sealed OCI mismatches", () => { | ||
| const item = fixture(); | ||
| try { | ||
| const manifestPath = join(item.scratch, "candidate.json"); | ||
| createCandidateManifest({ archivePath: item.archive, outputPath: manifestPath }); | ||
| const validator = join(root, "ops", "dress-rehearsal", "candidate-boundary.py"); | ||
| const output = join(item.scratch, "verified.json"); | ||
| execFileSync("python3", [validator, "validate", manifestPath, item.archive, output]); | ||
| const manifest = JSON.parse(readFileSync(manifestPath, "utf8")); | ||
| manifest.source.commit = "c".repeat(40); | ||
| writeFileSync(manifestPath, JSON.stringify(manifest)); | ||
| let failed = spawnSync("python3", [validator, "validate", manifestPath, item.archive, output], { encoding: "utf8" }); | ||
| assert.notEqual(failed.status, 0); | ||
| assert.match(failed.stderr, /embedded candidate commit mismatch/); | ||
| manifest.source.commit = item.identity.commit; | ||
| manifest.artifact.sha256 = "d".repeat(64); | ||
| writeFileSync(manifestPath, JSON.stringify(manifest)); | ||
| failed = spawnSync("python3", [validator, "validate", manifestPath, item.archive, output], { encoding: "utf8" }); | ||
| assert.notEqual(failed.status, 0); | ||
| assert.match(failed.stderr, /archive SHA-256 mismatch/); | ||
| } finally { rmSync(item.scratch, { recursive: true, force: true }); } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Test the sealed OCI mismatch path.
This test changes manifest.source.commit and manifest.artifact.sha256 only. It never changes manifest.sealed_oci.sha256 or the embedded OCI tar. Add a valid-format sealed OCI digest mismatch and assert that candidate-boundary.py validate rejects it.
🤖 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/phase2-candidate.mjs` around lines 56 - 76, Extend the test case around
the existing manifest mutations in the root boundary test to cover sealed OCI
validation: set manifest.sealed_oci.sha256 to a valid 64-character digest that
differs from the embedded OCI tar, write the manifest, run candidate-boundary.py
validate, and assert a nonzero status with the sealed OCI mismatch error.
Preserve the existing commit and archive SHA-256 mismatch assertions.
Comment on lines
+93
to
+120
| test("candidate workflow and guest boundary exclude PR code and broad root access", () => { | ||
| const workflow = read(".github/workflows/candidate.yml"); | ||
| const helper = read("ops/dress-rehearsal/1helm-candidate-install"); | ||
| const hook = read("ops/dress-rehearsal/runner-job-started"); | ||
| const sudoersExample = "%actions ALL=(root) NOPASSWD: /usr/local/sbin/1helm-candidate-install \"\"\n"; | ||
| assert.match(workflow, /workflow_run:[\s\S]*workflows: \[CI\][\s\S]*branches: \[main\]/); | ||
| assert.match(workflow, /workflow_run\.event == 'push'/); | ||
| assert.match(workflow, /head_repository\.full_name == github\.repository/); | ||
| assert.match(workflow, /runs-on: \[1helm-dress-rehearsal-phase2\]/); | ||
| assert.match(workflow, /github\.sha == github\.event\.workflow_run\.head_sha/); | ||
| assert.match(workflow, /attest-build-provenance@[a-f0-9]{40}/); | ||
| assert.match(workflow, /candidate-download\/candidate-evidence\/candidate\.json/); | ||
| assert.match(workflow, /candidate-download\/candidate-evidence\/provenance\.bundle\.json/); | ||
| assert.match(helper, /--signer-workflow gitcommit90\/1Helm\/\.github\/workflows\/candidate\.yml/); | ||
| assert.match(helper, /--source-ref refs\/heads\/main/); | ||
| assert.match(helper, /--source-digest "\$commit"/); | ||
| assert.match(helper, /--deny-self-hosted-runners/); | ||
| assert.match(helper, /local-proof-authorized/); | ||
| assert.doesNotMatch(helper, /--local-proof/); | ||
| assert.match(helper, /awk -F\/.*!found.*found=1/, "large archive inspection consumes tar output instead of causing SIGPIPE under pipefail"); | ||
| assert.match(helper, /actions\\\.runner[\s\S]*systemd-run[\s\S]*\/usr\/local\/sbin\/1helm-candidate-install/); | ||
| assert.match(helper, /unlink "\$INBOX\/candidate\.json" "\$INBOX\/candidate\.tgz"/); | ||
| assert.match(read("ops/dress-rehearsal/runner.service.override.conf"), /ProtectSystem=strict[\s\S]*ReadWritePaths=.*candidate\/inbox/); | ||
| assert.match(hook, /GITHUB_EVENT_NAME.*workflow_run/); | ||
| assert.match(hook, /run\.get\("event"\) == "push"/); | ||
| assert.doesNotMatch(sudoersExample, /NOPASSWD:\s*ALL/); | ||
| assert.doesNotMatch(workflow, /pull_request:/); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate workflow ---'
cat -n .github/workflows/candidate.yml | sed -n '130,190p'
printf '%s\n' '--- phase2 test ---'
cat -n test/phase2-candidate.mjs | sed -n '80,130p'
printf '%s\n' '--- provisioning and sudo references ---'
rg -n -C 3 'sudoers|NOPASSWD|actions\.runner|systemd-run|candidate-install|sudo ' .github ops test 2>/dev/null || true
printf '%s\n' '--- relevant tracked files ---'
git ls-files | rg '(^|/)(candidate|runner|.*sudo.*|.*provision.*)' || trueRepository: gitcommit90/1Helm
Length of output: 16671
🏁 Script executed:
#!/bin/bash
set -eu
for f in .github/workflows/candidate.yml test/phase2-candidate.mjs ops/dress-rehearsal/runner-job-started ops/dress-rehearsal/runner.service.override.conf; do
if [ -f "$f" ]; then
printf '\n--- %s ---\n' "$f"
cat -n "$f" | sed -n '1,230p'
fi
done
printf '\n--- all sudo policy/provisioning references ---\n'
rg -n -i -C 4 'sudoers|NOPASSWD|visudo|sudo |actions.*runner|runner.*user|provision|1helm-candidate-install' . 2>/dev/null | head -n 500Repository: gitcommit90/1Helm
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate helper ---'
cat -n ops/dress-rehearsal/1helm-candidate-install | sed -n '1,180p'
printf '%s\n' '--- candidate policy references ---'
rg -n -i -C 3 '1helm-candidate-install|candidate.*sudo|sudo.*candidate|NOPASSWD|/etc/sudoers|visudo' \
.github ops docs test site/public/install.sh site/public 2>/dev/null \
| rg -i 'candidate|sudoers|NOPASSWD|visudo' | head -n 300 || true
printf '%s\n' '--- deterministic file-reference check ---'
python3 - <<'PY'
from pathlib import Path
needles = (
"/usr/local/sbin/1helm-candidate-install",
"1helm-candidate-install",
"NOPASSWD",
)
for p in Path(".").rglob("*"):
if not p.is_file() or any(part in {".git", "node_modules", "dist", "build"} for part in p.parts):
continue
try:
text = p.read_text()
except (UnicodeDecodeError, OSError):
continue
hits = [n for n in needles if n in text]
if hits:
print(f"{p}: {', '.join(hits)}")
PYRepository: gitcommit90/1Helm
Length of output: 14348
Validate the effective sudo policy.
The Phase 2 test checks only a hard-coded sudoersExample. It does not validate the policy used by sudo -n /usr/local/sbin/1helm-candidate-install. Add the exact rule to a reviewed provisioning artifact and validate it with visudo, or inspect the effective policy on the Phase 2 runner.
🤖 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/phase2-candidate.mjs` around lines 93 - 120, Update the test named
“candidate workflow and guest boundary exclude PR code and broad root access” to
validate the effective sudo policy used by sudo -n
/usr/local/sbin/1helm-candidate-install instead of only checking the local
sudoersExample string. Add the exact rule to a reviewed provisioning artifact
and validate that artifact with visudo, or inspect the installed policy directly
on the Phase 2 runner, while retaining the existing protection against broad
NOPASSWD: ALL access.
This was referenced Aug 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this changes
This combines delivery-modernization Phases 0–2:
Safety boundaries
Verification
Activation note
The candidate workflow becomes available only after this PR lands on main. Its first trusted-main run will be the GitHub provenance-to-private-deployment activation proof. No stable release is part of this PR.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests