Automate nightly compatibility verification on a standalone VM - #25
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughNightly verification now records failure-aware results, supports rootless Podman, reconciles GitHub issues, and runs through a scheduled non-root systemd service. Compatibility checks use full Foundry versions and system-minor data. Cleanup failures preserve verification results. ChangesNightly Compatibility Verification
Sequence Diagram(s)sequenceDiagram
participant Timer
participant Service
participant NightlyScript
participant VerifyLocal
participant IssueReconciler
participant GitHub
Timer->>Service: Start nightly service
Service->>NightlyScript: Execute verification script
NightlyScript->>VerifyLocal: Verify entries and record results
NightlyScript->>IssueReconciler: Reconcile resolved entries
IssueReconciler->>GitHub: Comment and close or relabel issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
✨ Simplify code
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: 11
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ops/vm/foundry-verify.service`:
- Around line 16-18: Update the service dependency declaration by replacing
Requisite=docker.service with Requires=docker.service, while preserving the
existing After and Wants settings so Docker is started before the service runs.
- Around line 8-12: Update the service setup guidance around the foundry-verify
user to remove Docker group membership as an isolation measure. Require rootless
Docker or execution inside a disposable, isolated VM before enabling the
service, while preserving the non-root service-user setup where applicable.
In `@ops/vm/foundry-verify.timer`:
- Around line 7-14: Update verify-nightly.sh to validate the current
Europe/Berlin time immediately before git push, and skip or defer the push when
it falls within the Mon/Tue/Thu/Fri 07:30–16:30 CEST blackout window. Preserve
pushing outside that window and ensure the cutoff is applied after the
verification step.
- Around line 7-9: Update the timer’s OnCalendar directive to include
Europe/Berlin in the calendar expression, and remove the separate TimeZone
directive. Preserve the existing schedule time and RandomizedDelaySec setting.
In `@ops/vm/verify-nightly.sh`:
- Around line 31-34: Update the verification flow around the npm run command to
capture its exit status instead of masking it with `|| true`; preserve that
status while continuing push and reconciliation, then return the original
nonzero status after all cleanup completes.
- Around line 12-15: Update the lock setup around LOCK_FILE and the exec 200
redirection to use a repository-owned lock path or a systemd-managed runtime
directory instead of the predictable world-writable /tmp path, while preserving
the existing file-descriptor locking behavior.
- Around line 41-43: Move the docker system prune cleanup from the main
execution flow into an EXIT trap in verify-nightly.sh so it runs on every script
exit, including failures during pulls, pushes, or issue reconciliation. Preserve
the existing silent, non-failing cleanup behavior and ensure the trap is
registered before those operations can fail.
In `@scripts/close-resolved-issues.ts`:
- Around line 112-115: Update the failed-entry handling in the issue-processing
flow around entry.status === "failed" so failed issues are not selected again:
remove the existing "verification-required" label when adding
"needs-investigation", or skip issues already carrying "needs-investigation".
Preserve the current failure labeling behavior while preventing repeated
processing and comments.
In `@scripts/verify-local.ts`:
- Around line 361-367: Update the cleanup flow around orchestrator.stopAndRemove
so temporary-data removal always runs even when stopAndRemove rejects. Nest the
tmpDataDir cleanup block in a finally associated with the orchestrator cleanup,
while preserving the existing keepContainer guards and removal behavior.
- Around line 113-130: Update the registry validation in the verify-local flow
to throw an error when registry is not an array instead of replacing it with an
empty array. Ensure execution stops before the entry update and fs.writeFileSync
persistence path, preserving the malformed registry contents.
- Around line 338-355: Update the catch-side registry persistence around
upsertRegistryEntry so status "failed" is recorded only when the collected
failures array contains genuine test failures. Route Docker, Playwright,
report-parsing, and metadata errors through a separate incompatible outcome,
preserving them for --all-pending rather than marking them as failed.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 065bfe67-aa8c-4918-a545-57ef185a2d70
📒 Files selected for processing (7)
ops/vm/foundry-verify.serviceops/vm/foundry-verify.timerops/vm/verify-nightly.shpackage.jsonscripts/close-resolved-issues.tsscripts/monitor-releases.tsscripts/verify-local.ts
| After=network-online.target docker.service | ||
| Wants=network-online.target | ||
| Requisite=docker.service |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
systemd-analyze verify ops/vm/foundry-verify.serviceRepository: TheFehr/foundry-playwright
Length of output: 217
🏁 Script executed:
sed -n '1,120p' ops/vm/foundry-verify.service
printf '\n---\n'
rg -n "foundry-verify|docker.service|timer|install" -S README.md ops docs . || trueRepository: TheFehr/foundry-playwright
Length of output: 27474
Replace Requisite= with Requires= for Docker. Requisite= only works if docker.service is already active, so timer-triggered runs can fail on a VM where Docker is installed but stopped.
🤖 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/vm/foundry-verify.service` around lines 16 - 18, Update the service
dependency declaration by replacing Requisite=docker.service with
Requires=docker.service, while preserving the existing After and Wants settings
so Docker is started before the service runs.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/verify-local.ts`:
- Around line 274-291: Update the Docker branch in the verification flow around
reportPath and runPlaywrightInContainer so PLAYWRIGHT_JSON_OUTPUT_NAME uses the
container-visible /work path with the same report filename, while retaining the
host-absolute reportPath for subsequent report parsing.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3999871a-7463-4978-8523-f29a93b71f78
📒 Files selected for processing (1)
scripts/verify-local.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/docker.ts`:
- Around line 82-90: Replace the 0o777 chmod operations in the directory setup
flow with restrictive permissions that still grant container UID/GID 1000:1000
access to dataDir and cacheDir. Use ownership, group permissions, or ACLs as
appropriate, preserving the existing directory creation behavior and ensuring
sensitive Foundry data is not world-writable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/verify-local.ts (1)
152-170: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not pass Podman’s
keep-idmode to Docker.With
FOUNDRY_PLAYWRIGHT_ROOTLESS=1, Line 170 always passes--userns=keep-idtodocker run. Docker Engine supports onlyhosthere;keep-idis Podman-specific, so rootless Docker aborts before tests run. Detect/plumb the selected runtime and add this option only for Podman. (docs.docker.com)#!/bin/bash set -euo pipefail rg -n -C3 'FOUNDRY_PLAYWRIGHT_ROOTLESS|--userns=keep-id|execFileSync\(' \ scripts/verify-local.ts src/docker.ts ops/vm/verify-nightly.sh # Run on the target VM to identify the implementation behind `docker`. docker version --format '{{.Server.Version}}' docker info --format '{{json .SecurityOptions}}'Also applies to: 207-229
🤖 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/verify-local.ts` around lines 152 - 170, Update the Docker run construction in the function containing getPlaywrightImageTag and execFileSync so --userns=keep-id is added only when the selected container runtime is Podman, never when it is Docker. Detect or plumb the runtime selection into this flow, while preserving the existing rootless behavior for Podman and omitting the flag for Docker.
♻️ Duplicate comments (1)
ops/vm/verify-nightly.sh (1)
40-55: 🩺 Stability & Availability | 🟠 MajorPreserve verification failures through the blackout path.
This remains unresolved from the previous review: Line [43] masks every nonzero verification exit, including setup/authentication/runtime failures that never record per-entry results. Additionally, Line [55] exits
0during blackout, so a captured failure would still be reported as successful. Capture the status and return it after cleanup, including the blackout path.Proposed fix
+verify_status=0 npm run verify:local -- --all-pending --docker --update-registry --record-failures --git-commit || true +verify_status=$? ... - exit 0 + exit "$verify_status" ... +exit "$verify_status"🤖 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/vm/verify-nightly.sh` around lines 40 - 55, Preserve the verification command’s exit status instead of unconditionally masking it with `|| true`: capture the status after `npm run verify:local` while allowing cleanup to continue. Update the blackout branch to perform its existing local cleanup, then return the captured verification status rather than always exiting successfully; likewise propagate that status after the normal reconciliation and push flow.
🧹 Nitpick comments (1)
src/docker.test.ts (1)
39-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlso assert the
--usermapping.These tests cover only
--userns=keep-id; they do not protect the primary ownership contract introduced by--user ${process.getuid()}:${process.getgid()}. Add an assertion for the expected UID/GID in both configurations.🤖 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 `@src/docker.test.ts` around lines 39 - 54, Update both tests for DockerFoundryOrchestrator.getRunCommand to assert the command includes the expected --user UID/GID mapping using process.getuid() and process.getgid(), while preserving the existing --userns=keep-id assertions for default and rootless configurations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/monitor-releases.ts`:
- Around line 152-171: Update the incompatibility-note generation to preserve
the complete FVTT version by using the existing `fvtt` value directly instead of
extracting only its major component with `fvtt.split(".")[0]`. Keep the newly
added full-version compatibility comparison in `isCompatibleWithFvtt` unchanged.
In `@src/docker.ts`:
- Around line 93-96: Update copyToContainer() to remove the hard-coded chown -R
1000:1000 step, or replace it with an operation compatible with the configured
runtime identity used by getRunCommand(). Ensure copied files remain usable by
the calling host user without requiring root privileges.
---
Outside diff comments:
In `@scripts/verify-local.ts`:
- Around line 152-170: Update the Docker run construction in the function
containing getPlaywrightImageTag and execFileSync so --userns=keep-id is added
only when the selected container runtime is Podman, never when it is Docker.
Detect or plumb the runtime selection into this flow, while preserving the
existing rootless behavior for Podman and omitting the flag for Docker.
---
Duplicate comments:
In `@ops/vm/verify-nightly.sh`:
- Around line 40-55: Preserve the verification command’s exit status instead of
unconditionally masking it with `|| true`: capture the status after `npm run
verify:local` while allowing cleanup to continue. Update the blackout branch to
perform its existing local cleanup, then return the captured verification status
rather than always exiting successfully; likewise propagate that status after
the normal reconciliation and push flow.
---
Nitpick comments:
In `@src/docker.test.ts`:
- Around line 39-54: Update both tests for
DockerFoundryOrchestrator.getRunCommand to assert the command includes the
expected --user UID/GID mapping using process.getuid() and process.getgid(),
while preserving the existing --userns=keep-id assertions for default and
rootless configurations.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5c3522cd-62b3-4376-9144-c13f1ecba28e
📒 Files selected for processing (9)
ops/vm/foundry-verify.serviceops/vm/foundry-verify.timerops/vm/verify-nightly.shscripts/close-resolved-issues.tsscripts/monitor-releases.tsscripts/verify-local.tssrc/docker.test.tssrc/docker.tsverified-versions.json
🚧 Files skipped from review as they are similar to previous changes (2)
- ops/vm/foundry-verify.timer
- scripts/close-resolved-issues.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/verify-local.ts (1)
281-319: 🗄️ Data Integrity & Integration | 🟠 MajorDo not treat a nonzero Playwright exit as stable when a report exists.
If Playwright exits with an error but leaves a JSON report containing no
failed/timedOutspecs,execErroris ignored and execution proceeds to the stable registry write. Re-throwexecErrorwhenever no genuine test failures were extracted.Proposed fix
if (fs.existsSync(reportPath)) { const report = JSON.parse(fs.readFileSync(reportPath, "utf8")); failures = extractFailures(report); fs.unlinkSync(reportPath); } else if (execError) { throw execError; } + + if (execError && failures.length === 0) { + throw execError; + }🤖 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/verify-local.ts` around lines 281 - 319, The Playwright result handling must not ignore execError merely because a JSON report exists. Update the logic following the Docker/non-Docker execution around execError so it rethrows execError whenever no genuine failed or timedOut specs are extracted, while preserving report-based failure handling when such specs are present and only allowing the stable registry write on a successful or genuinely reported test result.
♻️ Duplicate comments (1)
src/docker.ts (1)
107-110: 🩺 Stability & Availability | 🟠 MajorThe
copyToContainer()permission issue remains unresolved.Because the container runs with
--user <host uid>:<gid>,docker execinherits a non-root user.docker cpcommonly creates destination files as root, so thischownruns without permission and can leave copied files unusable. Use archive-mode copying and remove the chown, or execute the ownership fix through a supported privileged path.Also applies to: 199-204
🤖 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 `@src/docker.ts` around lines 107 - 110, Update copyToContainer() to avoid relying on chown through docker exec as the non-root container user: use archive-mode copying so destination files retain the intended ownership, then remove the ineffective ownership-fix step and any related permission assumptions in the directory setup flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ops/vm/verify-nightly.sh`:
- Around line 46-47: Update the failure path in the nightly verification wrapper
so recorded failures in verified-versions.json are committed before the
subsequent pull/push. Either change the verify-local handling of --git-commit to
commit failure records as well, or explicitly stage and commit only
verified-versions.json after the verification command fails, preserving the
existing status propagation and push flow.
---
Outside diff comments:
In `@scripts/verify-local.ts`:
- Around line 281-319: The Playwright result handling must not ignore execError
merely because a JSON report exists. Update the logic following the
Docker/non-Docker execution around execError so it rethrows execError whenever
no genuine failed or timedOut specs are extracted, while preserving report-based
failure handling when such specs are present and only allowing the stable
registry write on a successful or genuinely reported test result.
---
Duplicate comments:
In `@src/docker.ts`:
- Around line 107-110: Update copyToContainer() to avoid relying on chown
through docker exec as the non-root container user: use archive-mode copying so
destination files retain the intended ownership, then remove the ineffective
ownership-fix step and any related permission assumptions in the directory setup
flow.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7a064e94-dd9a-4e39-9c6a-729794afeee9
📒 Files selected for processing (5)
ops/vm/verify-nightly.shscripts/monitor-releases.tsscripts/verify-local.tssrc/docker.test.tssrc/docker.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/monitor-releases.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/docker.ts (1)
107-110: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftDon’t assume
cp -apreserves usable ownership here. The existing-directory path still skips any ownership fixup, anddocker cp -abehaves differently across runtimes: Docker preserves source UID/GID, while Podman maps ownership to the container’s primary UID/GID. That can leave copied files unwritable by the non-root process; add an explicit ownership/access check for existing dirs and copied inputs.🤖 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 `@src/docker.ts` around lines 107 - 110, Update the directory setup flow near getRunCommand() so it does not assume bind-mount ownership or cp -a preservation is usable. For existing directories and copied inputs, explicitly verify ownership and write access for the non-root runtime user, applying the required ownership fixup when the check fails while preserving the current behavior for correctly accessible paths.scripts/verify-local.ts (3)
281-339: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRequire a fresh, structurally valid report before recording the outcome.
The new guard only handles stale reports with zero extracted failures. If a prior report contains failures and the current process crashes before writing a new report, those stale failures reach Line 435 and permanently record
status: "failed", preventing--all-pendingretries. Conversely, a zero-exit run with no report—or a JSON object without the expectedsuitesarray—is accepted as success.Remove the report before starting, use a unique per-run path, and require a valid report before extracting failures.
Proposed fix
const reportPath = path.join(process.cwd(), `.playwright-report-${version}.json`); +fs.rmSync(reportPath, { force: true }); ... - if (fs.existsSync(reportPath)) { - const report = JSON.parse(fs.readFileSync(reportPath, "utf8")); + if (!fs.existsSync(reportPath)) { + throw execError ?? new Error("Playwright produced no JSON report"); + } + const report = JSON.parse(fs.readFileSync(reportPath, "utf8")); + if (!report || !Array.isArray(report.suites)) { + throw new Error("Playwright produced an invalid JSON report"); + }Also applies to: 435-460
🤖 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/verify-local.ts` around lines 281 - 339, Update the test execution flow around reportPath, execError, and extractFailures to remove any existing report before starting and use a unique per-run report path. After execution, only parse the report when it exists and has the expected structural suites array; otherwise propagate execError or treat the run as unsuccessful rather than recording success. Ensure stale or malformed reports cannot populate failures or allow a failed/crashed run to be recorded as status "failed" before --all-pending retries.
341-348: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate
.foundry_metadata.jsonbefore using it.
JSON.parseis assigned directly tometa; a valid partial file such as{}causes later dereferences ofmeta.system.versionormeta.modules.filterto throw. The latter occurs inside the failure handler and can mask the original test failure, preventing the intended result or registry update. Since the file is deleted only after execution, interrupted runs can also leave stale metadata.Parse into
unknown, validate the required shape, clear stale metadata before execution, and treat invalid metadata as infrastructure failure rather than dereferencing it.Also applies to: 383-390, 440-460
🤖 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/verify-local.ts` around lines 341 - 348, Harden the metadata handling around metaPath and the verify-local execution flow: remove any stale .foundry_metadata.json before tests run, parse newly written contents as unknown, and validate the required system.version and modules shape before assigning to meta. Treat missing, malformed, or invalid metadata as an infrastructure failure while preserving the original test failure and preventing dereferences in the failure handler and registry-update path.
312-317: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAvoid shell interpolation in the Playwright call
playwrightArgsis copied fromprocess.argvand concatenated into theexecSyncstring, so a quoted flag value can inject shell metacharacters into the inner shell. Switch toexecFileSync("npx", ["playwright", "test", ...testFiles, "--workers=1", "--reporter=line,json", ...playwrightArgs], ...)instead.🤖 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/verify-local.ts` around lines 312 - 317, Replace the string-based execSync Playwright invocation with execFileSync, passing "npx" and the Playwright command arguments as separate array elements in the existing order, including testFiles and playwrightArgs. Preserve the current stdio and environment options while avoiding shell interpolation.Source: Linters/SAST tools
🤖 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 `@src/docker.ts`:
- Around line 193-202: Update copyToContainer() to replace interpolated execSync
Docker commands with execFileSync("docker", ...) argument arrays, including the
mkdir and docker cp operations. Pass containerName, localPath, and containerPath
as separate arguments so shell metacharacters are not interpreted, while
preserving the existing stdio and Docker options.
---
Outside diff comments:
In `@scripts/verify-local.ts`:
- Around line 281-339: Update the test execution flow around reportPath,
execError, and extractFailures to remove any existing report before starting and
use a unique per-run report path. After execution, only parse the report when it
exists and has the expected structural suites array; otherwise propagate
execError or treat the run as unsuccessful rather than recording success. Ensure
stale or malformed reports cannot populate failures or allow a failed/crashed
run to be recorded as status "failed" before --all-pending retries.
- Around line 341-348: Harden the metadata handling around metaPath and the
verify-local execution flow: remove any stale .foundry_metadata.json before
tests run, parse newly written contents as unknown, and validate the required
system.version and modules shape before assigning to meta. Treat missing,
malformed, or invalid metadata as an infrastructure failure while preserving the
original test failure and preventing dereferences in the failure handler and
registry-update path.
- Around line 312-317: Replace the string-based execSync Playwright invocation
with execFileSync, passing "npx" and the Playwright command arguments as
separate array elements in the existing order, including testFiles and
playwrightArgs. Preserve the current stdio and environment options while
avoiding shell interpolation.
In `@src/docker.ts`:
- Around line 107-110: Update the directory setup flow near getRunCommand() so
it does not assume bind-mount ownership or cp -a preservation is usable. For
existing directories and copied inputs, explicitly verify ownership and write
access for the non-root runtime user, applying the required ownership fixup when
the check fails while preserving the current behavior for correctly accessible
paths.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 87a1442a-4f66-44bb-b6a8-7616d1c7efd5
📒 Files selected for processing (2)
scripts/verify-local.tssrc/docker.ts
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/docker.ts (1)
156-176: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRemove shell construction from the remaining Docker operations.
getRunCommand()interpolatescontainerName,envPath, mount paths, andversioninto a string executed byexecSyncat Line 144. Quotes do not protect embedded quotes or command substitutions, so a configuration-derived value can execute shell syntax. Migrate this and the otherexecSyncDocker calls in this class toexecFileSync("docker", [...args]); fixing onlycopyToContainer()leaves the command-injection path open.🤖 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 `@src/docker.ts` around lines 156 - 176, Replace shell-string construction in getRunCommand and every other Docker operation in the class with execFileSync("docker", [...args]) using separate argument values for containerName, envPath, mount paths, version, and flags. Remove shell quoting and command joining while preserving the existing Docker arguments and runtime behavior across all operations.Source: Linters/SAST tools
scripts/verify-local.ts (1)
342-361: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFail when the required JSON report is absent.
If Playwright exits successfully but does not create the requested report, this path falls through as a pass and can persist a
stableentry using default/unknown metadata. Treat a missing report as an infrastructure error regardless of exit status.Proposed fix
- } else if (execError) { - // Playwright failed to start or crashed without producing a report. - throw execError; + } else { + throw ( + execError ?? + new Error(`Playwright completed without producing JSON report: ${reportPath}`) + ); }🤖 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/verify-local.ts` around lines 342 - 361, The report-handling branch around isPlaywrightReport must treat a missing reportPath as an infrastructure failure even when execError is unset. Update the else branch to throw an appropriate missing-report error, while preserving execError propagation when Playwright fails without producing a report.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/verify-local.ts`:
- Around line 368-376: Update the metadata parsing around isCapturedMetadata to
catch JSON.parse failures and fall back to the existing unknown metadata
behavior. Strengthen isCapturedMetadata to require a string system.id and ensure
every module has string id and version fields before assigning rawMeta to meta.
Preserve cleanup and malformed-metadata handling without allowing invalid values
into the summary or registry.
In `@src/docker.ts`:
- Around line 107-118: Update ensureWritableDir to validate the directory
itself, not only its children: include dir in ownership repair and verify it has
W_OK and X_OK access for the current process. Ensure this validation runs for
both dataDir and cacheDir before container startup, while preserving the
existing best-effort repair behavior.
- Around line 240-246: Update copyToContainer so ownership handling is
runtime-specific instead of relying on docker cp -a: preserve the existing
Podman behavior while ensuring Docker-copied files are owned by the container
user and remain writable. Use the configured runtime information and existing
container identity symbols to normalize ownership before or during the copy, and
add coverage for both Docker and Podman ownership behavior.
---
Outside diff comments:
In `@scripts/verify-local.ts`:
- Around line 342-361: The report-handling branch around isPlaywrightReport must
treat a missing reportPath as an infrastructure failure even when execError is
unset. Update the else branch to throw an appropriate missing-report error,
while preserving execError propagation when Playwright fails without producing a
report.
In `@src/docker.ts`:
- Around line 156-176: Replace shell-string construction in getRunCommand and
every other Docker operation in the class with execFileSync("docker", [...args])
using separate argument values for containerName, envPath, mount paths, version,
and flags. Remove shell quoting and command joining while preserving the
existing Docker arguments and runtime behavior across all operations.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bac8ff35-7bfe-4aba-81d7-9dbb0636a47b
📒 Files selected for processing (2)
scripts/verify-local.tssrc/docker.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/docker.ts (1)
198-235: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUnhandled exception if the directory itself isn't readable.
fixOwnership(dir)gracefully catches a failedchownSyncintounfixable, but the subsequentfs.readdirSync(dir)at Line 220 has no such guard. Ifdiris owned by a different uid with no group/other read+execute bits — exactly the "switching automation users" scenario this function's own comments call out as having happened in practice —readdirSyncthrows an uncaught, raw fs error instead of the clear, actionable diagnostic built at Lines 230-234.🐛 Proposed fix
fixOwnership(dir); - for (const entry of fs.readdirSync(dir)) { - fixOwnership(path.join(dir, entry)); - } + try { + for (const entry of fs.readdirSync(dir)) { + fixOwnership(path.join(dir, entry)); + } + } catch { + unfixable.add(dir); + }🤖 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 `@src/docker.ts` around lines 198 - 235, Update ensureWritableDir, specifically the directory-entry iteration after fixOwnership(dir), to catch readdirSync failures and record dir in unfixable instead of allowing a raw filesystem exception to escape. Preserve the existing actionable error construction and continue checking accessibility so the final diagnostic identifies the inaccessible directory.
🧹 Nitpick comments (1)
scripts/verify-local.ts (1)
342-369: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMalformed-report handling is asymmetric with the metadata parsing added below.
JSON.parse(fs.readFileSync(reportPath, ...))at Line 343 isn't wrapped in try/catch, unlike the.foundry_metadata.jsonparsing added at Lines 376-389. A truncated/corrupt report JSON throws uncaught here, skipsfs.unlinkSync(reportPath)(leaking the report file), and falls through to the outer catch — which is still safe (failures stays empty, so it won't be misrecorded as "failed"), but the error message is a rawSyntaxErrorinstead of the clear "malformed report" diagnostic this code is designed to produce.♻️ Proposed hardening
if (fs.existsSync(reportPath)) { - const rawReport: unknown = JSON.parse(fs.readFileSync(reportPath, "utf8")); - fs.unlinkSync(reportPath); - const validReport = isPlaywrightReport(rawReport); - if (validReport) { - failures = extractFailures(rawReport); - } + const rawContent = fs.readFileSync(reportPath, "utf8"); + fs.unlinkSync(reportPath); + let validReport = false; + try { + const rawReport: unknown = JSON.parse(rawContent); + validReport = isPlaywrightReport(rawReport); + if (validReport) { + failures = extractFailures(rawReport); + } + } catch { + validReport = false; + } if (!validReport || (failures.length === 0 && execError)) {🤖 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/verify-local.ts` around lines 342 - 369, Wrap the report JSON parsing in the report-handling block around JSON.parse, ensuring reportPath is removed even when parsing fails. Convert parse failures into the existing malformed-report diagnostic, while preserving execError precedence and the current valid-report failure extraction flow.
🤖 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.
Outside diff comments:
In `@src/docker.ts`:
- Around line 198-235: Update ensureWritableDir, specifically the
directory-entry iteration after fixOwnership(dir), to catch readdirSync failures
and record dir in unfixable instead of allowing a raw filesystem exception to
escape. Preserve the existing actionable error construction and continue
checking accessibility so the final diagnostic identifies the inaccessible
directory.
---
Nitpick comments:
In `@scripts/verify-local.ts`:
- Around line 342-369: Wrap the report JSON parsing in the report-handling block
around JSON.parse, ensuring reportPath is removed even when parsing fails.
Convert parse failures into the existing malformed-report diagnostic, while
preserving execError precedence and the current valid-report failure extraction
flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bb5da8cd-d936-46fe-a58d-071085def53c
📒 Files selected for processing (3)
scripts/verify-local.tssrc/docker.test.tssrc/docker.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/docker.test.ts
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/docker.ts`:
- Around line 250-255: Update stopAndRemove to stop swallowing cleanup errors:
ignore only the expected “container not found” case, while propagating daemon,
permission, and other Docker failures so callers retain the temporary bind mount
when removal is unsuccessful. Preserve the existing stop-then-remove sequence
and use the execFileSync error details to distinguish the not-found condition.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bd5d9890-ef7d-4ca5-80c1-10ec5c89d4eb
📒 Files selected for processing (3)
scripts/verify-local.tssrc/docker.test.tssrc/docker.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/docker.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/verify-local.ts (1)
469-478: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve the pending registry key when metadata is unavailable.
A passing run with missing/malformed metadata writes
systemMinor: "unknown"instead of the target’ssystemMinor. The original pending entry remains unresolved and will be verified repeatedly.Proposed fix
const realModules = meta.modules.filter((m) => m.id !== "fake-module"); +const registrySystemVersion = + installedSystemVersion !== "unknown" + ? installedSystemVersion + : (systemVersion ?? "unknown"); +const registrySystemMinor = + installedSystemVersion !== "unknown" + ? minorOf(installedSystemVersion) + : (systemMinor ?? minorOf(registrySystemVersion)); upsertRegistryEntry({ fvtt: version, system: meta.system.id, - systemMinor: minorOf(installedSystemVersion), - systemVersion: installedSystemVersion, + systemMinor: registrySystemMinor, + systemVersion: registrySystemVersion,🤖 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/verify-local.ts` around lines 469 - 478, Update the upsertRegistryEntry call in the local verification flow to preserve the pending registry entry’s target systemMinor when metadata is missing or malformed, rather than deriving or writing "unknown". Reuse the target/pending entry’s systemMinor while keeping the existing metadata-derived value for valid metadata and leaving the remaining registry fields 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 `@scripts/verify-local.ts`:
- Around line 521-539: Handle failures from temporary data-directory removal in
the cleanup block around tmpDataDir and fs.rmSync so they cannot override the
verification result or abort an --all-pending batch. Track the removal failure
similarly to cleanupFailed, catch and log the error with the directory path, and
preserve the existing behavior of skipping removal when cleanupFailed or
keepContainer is set.
---
Outside diff comments:
In `@scripts/verify-local.ts`:
- Around line 469-478: Update the upsertRegistryEntry call in the local
verification flow to preserve the pending registry entry’s target systemMinor
when metadata is missing or malformed, rather than deriving or writing
"unknown". Reuse the target/pending entry’s systemMinor while keeping the
existing metadata-derived value for valid metadata and leaving the remaining
registry fields unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b8e54a52-36e1-4581-9bd4-df7e68e0690e
📒 Files selected for processing (3)
scripts/verify-local.tssrc/cli/index.tssrc/docker.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/verify-local.ts (1)
474-487: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winResolve captured metadata before writing the stable entry.
isCapturedMetadataaccepts any string version, butminorOfreturns"unknown"for incomplete or invalid values. The check at Lines [474]-[477] recognizes only the literal"unknown", so a value such as"garbage"can writesystemMinor: "unknown"and orphan the pending entry. When metadata is missing, Lines [483] and [487] also persistsystemVersionand notes as"unknown"instead of the target version.Compute one validated
resolvedSystemVersion, then derive both the registry key and persisted fields from it.Proposed fix
+const installedMinor = minorOf(installedSystemVersion); +const hasValidInstalledVersion = + installedSystemVersion !== "unknown" && installedMinor !== "unknown"; +const resolvedSystemVersion = hasValidInstalledVersion + ? installedSystemVersion + : (systemVersion ?? "unknown"); const resolvedSystemMinor = - installedSystemVersion !== "unknown" - ? minorOf(installedSystemVersion) - : (systemMinor ?? "unknown"); + hasValidInstalledVersion + ? installedMinor + : (systemMinor ?? minorOf(resolvedSystemVersion)); ... - systemVersion: installedSystemVersion, + systemVersion: resolvedSystemVersion, ... - notes: `Verified locally with ${meta.system.id} v${installedSystemVersion}.`, + notes: `Verified locally with ${meta.system.id} v${resolvedSystemVersion}.`,Also applies to: 603-610
🤖 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/verify-local.ts` around lines 474 - 487, Update the stable-entry construction around resolvedSystemMinor and the corresponding flow near the alternate occurrence so one validated resolvedSystemVersion is computed first: use the captured version only when it passes metadata validation, otherwise fall back to the target version. Derive resolvedSystemMinor from that value, use it for the registry key, and persist it in systemVersion and the verification notes instead of allowing invalid or unknown captured metadata to orphan the pending entry.
🤖 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.
Outside diff comments:
In `@scripts/verify-local.ts`:
- Around line 474-487: Update the stable-entry construction around
resolvedSystemMinor and the corresponding flow near the alternate occurrence so
one validated resolvedSystemVersion is computed first: use the captured version
only when it passes metadata validation, otherwise fall back to the target
version. Derive resolvedSystemMinor from that value, use it for the registry
key, and persist it in systemVersion and the verification notes instead of
allowing invalid or unknown captured metadata to orphan the pending entry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 24ccac8d-6e32-4e67-8c34-e80117b1f680
📒 Files selected for processing (2)
scripts/verify-local.tssrc/cli/index.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/verify-local.ts`:
- Around line 475-492: In scripts/verify-local.ts lines 475-492, only fall back
to the requested system version when buildManifestUrl() supplied a manifest URL;
otherwise keep resolvedSystemVersion and resolvedSystemMinor as "unknown" rather
than creating a stable entry for an unpinned latest-version run. In
scripts/verify-local.ts lines 513-515, derive systemMinor from the resolved
captured version, and leave the entry pending instead of marking it failed when
neither a captured version nor an explicit pin establishes the system version.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b4050d02-975e-464c-a831-0c15f29a9ac7
📒 Files selected for processing (1)
scripts/verify-local.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
ops/vm/foundry-verify.service (2)
31-34: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake Git push authentication explicit.
gh ssh-key add --type signingregisters a signing key. It does not configure the Git remote used bygit push. Documentgh auth setup-gitafter HTTPS authentication, or document a separate SSH authentication key. Then rungit push --dry-runasfoundry-verify. GitHub CLI documents these as separate operations. (cli.github.com)Proposed documentation addition
# `gh ssh-key add ~/.ssh/foundry-verify-signing.pub --type signing --title "foundry-verify"`. +# For an HTTPS Git remote, also run: +# gh auth setup-git +# git push --dry-run🤖 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/vm/foundry-verify.service` around lines 31 - 34, Update the setup instructions around the `gh ssh-key add --type signing` command to explicitly configure Git push authentication, preferably by documenting `gh auth setup-git` after HTTPS authentication. Also instruct users to run `git push --dry-run` as `foundry-verify`, while keeping the signing-key registration steps separate.
30-30: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a preflight test for unattended SSH signing.
The nightly path creates signed commits with
--git-commit. Add a temporary-repository test that runs asfoundry-verify, after applying the signing configuration, and fails ifgit commit --allow-emptyorgit verify-commit HEADfails. Ensuressh-keygenis installed.🤖 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/vm/foundry-verify.service` at line 30, Add a preflight test in the foundry-verify service after applying the SSH signing configuration: verify ssh-keygen is installed, create a temporary repository with the required signing setup, run as foundry-verify, and fail the service if either git commit --allow-empty or git verify-commit HEAD fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ops/vm/foundry-verify.service`:
- Line 28: Update the git configuration setup comment near the existing
user.email command to also configure user.name with a stable bot identity,
ensuring both values are set before git commit runs for a fresh foundry-verify
account.
- Line 24: Create the ~/.ssh directory with mode 700 before the ssh-keygen
command in the signing-key setup instructions. Add the directory creation step
immediately before the command so key generation works when the parent directory
is absent.
---
Nitpick comments:
In `@ops/vm/foundry-verify.service`:
- Around line 31-34: Update the setup instructions around the `gh ssh-key add
--type signing` command to explicitly configure Git push authentication,
preferably by documenting `gh auth setup-git` after HTTPS authentication. Also
instruct users to run `git push --dry-run` as `foundry-verify`, while keeping
the signing-key registration steps separate.
- Line 30: Add a preflight test in the foundry-verify service after applying the
SSH signing configuration: verify ssh-keygen is installed, create a temporary
repository with the required signing setup, run as foundry-verify, and fail the
service if either git commit --allow-empty or git verify-commit HEAD fails.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2ffd8cf1-b921-41d6-b473-f2bd21a82a33
📒 Files selected for processing (2)
ops/vm/foundry-verify.servicescripts/verify-local.ts
Replaces manual verify:local runs with a standalone VM (rootless Podman,
no docker-group/root-equivalent access, since it executes third-party
FoundryVTT/system/module content on every run) that consumes "pending"
entries the cloud Monitor Releases workflow files as GitHub issues, runs
the Docker-based verification suite nightly via a systemd timer, and
pushes results back - closing the loop by reconciling verified-versions.json
against open issues (scripts/close-resolved-issues.ts).
Key pieces:
- ops/vm/{foundry-verify.service,foundry-verify.timer,verify-nightly.sh}:
the systemd unit/timer/wrapper script, scheduled well clear of both this
host's backup-job CPU peaks and the Mon/Tue/Thu/Fri 07:30-16:30 CEST push
blackout, with disk-usage guards, file locking, and a same-day blackout
re-check immediately before push as defense in depth.
- scripts/verify-local.ts: containerizes the Playwright test client itself
(not just the Foundry server), adds --record-failures/--git-commit for
unattended runs, and - after several rounds of hardening - only ever
trusts a system version for the registry from validated captured
metadata or an actually-used manifest pin, never a bare requested
version that might not reflect what was actually installed.
- scripts/close-resolved-issues.ts (new): reconciles resolved registry
entries against open "verification-required" issues.
- scripts/monitor-releases.ts: fixed to compare full version strings
(not just major integers) and to keep checking newer builds within an
already-stable Foundry generation, instead of freezing on the first
build that went stable.
- src/docker.ts: DockerFoundryOrchestrator runs containers with --user
matching the calling process (fixing a uid:gid bind-mount mismatch) and
optionally --userns=keep-id under a real Podman runtime (detected, not
assumed) for rootless support; all Docker invocations use execFileSync
array args rather than shell-interpolated strings.
- foundry-verify's commits are SSH-signed (dedicated passphrase-less key,
used only for signing, never repo auth).
Built and iteratively hardened via live dry runs directly on the
provisioned VM (192.168.6.92) and multiple automated code-review passes,
not just local testing.
166324f to
5258bd3
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
scripts/verify-local.ts (4)
623-631: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueValidate
foundryinisCapturedMetadata.The predicate returns
value is CapturedMetadata, but it never checksfoundry.CapturedMetadatadeclaresfoundry: string. After line 388 assigns the parsed value, TypeScript treatsmeta.foundryas astringeven when the file omits it or supplies a number. No current reader usesmeta.foundry, so this is latent. Add the check so the predicate matches the declared type.Proposed fix
function isCapturedMetadata(value: unknown): value is CapturedMetadata { if (typeof value !== "object" || value === null) return false; const v = value as Record<string, unknown>; + if (typeof v["foundry"] !== "string") return false; const sys = v["system"];🤖 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/verify-local.ts` around lines 623 - 631, Update isCapturedMetadata to validate that the top-level foundry property is a string before returning true, alongside the existing system and modules checks, so the CapturedMetadata type guard matches its declared shape.
192-202: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the positional parameter list with an options object.
verifyVersionnow takes nine positional parameters. Four consecutive booleans (isDocker,updateRegistry,recordFailures,keepContainer) and two consecutivestring | undefinedparameters follow each other. A swapped argument at the call site compiles without error and silently changes behavior. An options object makes each value explicit.🤖 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/verify-local.ts` around lines 192 - 202, Update verifyVersion to accept a single options object instead of its nine positional parameters, using named properties for system, modules, systemVersion, systemMinor, isDocker, updateRegistry, recordFailures, and keepContainer. Update every call site to pass the corresponding named values and adjust the function body to read from the options object.
35-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare one
minorOfimplementation.
scripts/monitor-releases.tscontains an identicalminorOf. Both scripts define the minor-grouping rule that the registry key depends on. If one copy changes, registry keys silently diverge. Move the function into a shared module and import it in both scripts.🤖 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/verify-local.ts` around lines 35 - 38, Extract the duplicated minorOf implementation from scripts/verify-local.ts and scripts/monitor-releases.ts into a shared module, then import and use that shared function in both scripts. Preserve the existing major.minor or "unknown" behavior so registry keys continue using one consistent grouping rule.
507-542: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared system-version resolution.
Lines 517-523 repeat the logic of lines 476-481, and line 511 repeats the
fake-modulefilter of line 466. Both paths must agree on what counts as a trustworthy system version. If one copy changes, the success path and the failure path key entries differently. Extract one helper that takesmeta,system, andsystemVersionand returns the resolved version, then call it from both paths.🤖 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/verify-local.ts` around lines 507 - 542, Extract the duplicated system-version resolution into a shared helper accepting meta, system, and systemVersion, then use it in both the success and failure paths so they apply identical trust rules. Also centralize the realModules filtering that excludes “fake-module” and reuse it from both paths, preserving the existing unresolved-version behavior.src/docker.ts (1)
30-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider deduplicating
isPodmanRuntime().
scripts/verify-local.tsdefines an identicalisPodmanRuntime()(same implementation, same doc-comment intent) independently of this exported function. Two copies of Podman-detection logic can silently drift if one is updated without the other, e.g., a broadened detection regex fixed in only one place.Export this function as the single source of truth and import it from
scripts/verify-local.tsinstead of duplicating the implementation.🤖 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 `@src/docker.ts` around lines 30 - 42, Use the exported isPodmanRuntime() from src/docker.ts as the single Podman-detection implementation. Remove the duplicate function and related detection logic from scripts/verify-local.ts, import the shared symbol there, and preserve its existing callers and behavior.src/docker.test.ts (1)
48-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd coverage for the other newly hardened
DockerFoundryOrchestratorbehavior.These three tests cover
getRunCommand()'s rootless/Podman branch well.ensureWritableDir(),stopAndRemove(), andcopyToContainer()insrc/docker.tswere also substantially reworked in this PR (ownership repair,no such containerdetection, archive-copy ownership verification) after multiple past reviews found critical bugs in exactly this logic (silently swallowed cleanup errors, world-writable directories, shell-injected copy commands). None of that logic has test coverage in this file.Add tests that mock
fs(forensureWritableDir's stat/chown/access paths) andexecFileSync(forstopAndRemove's "no such container" vs. real-failure branching, andcopyToContainer's ownership-mismatch error) to lock in the fixed behavior against regression.🤖 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 `@src/docker.test.ts` around lines 48 - 84, Extend src/docker.test.ts with focused coverage for DockerFoundryOrchestrator.ensureWritableDir, stopAndRemove, and copyToContainer. Mock fs to exercise ensureWritableDir stat, chown, and access behavior; mock execFileSync to verify stopAndRemove ignores only “no such container” failures while propagating real failures, and copyToContainer rejects ownership mismatches. Preserve the existing rootless getRunCommand tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/close-resolved-issues.ts`:
- Around line 95-129: Wrap the processing body for each entry in the resolved
loop with a try/catch so failures from githubRequest calls do not abort
subsequent entries. Log the affected issue or entry and the caught error in the
catch block, then continue iterating through the remaining entries while
preserving the existing success and status-specific behavior.
- Around line 89-93: Update the openIssues retrieval around the githubRequest
call to paginate the verification-required open-issue query, requesting
successive pages with the existing per_page limit until a response contains
fewer than 100 issues. Accumulate every page into one combined list, then use
that complete list for the existing search and close flow.
In `@src/docker.ts`:
- Around line 198-245: Update the fixOwnership helper inside ensureWritableDir
to include fs.statSync(entryPath) within its existing try/catch. When stat or
ownership correction fails, add entryPath to unfixable so dangling or removed
entries produce the function’s intended diagnostic instead of escaping as a raw
filesystem error.
---
Nitpick comments:
In `@scripts/verify-local.ts`:
- Around line 623-631: Update isCapturedMetadata to validate that the top-level
foundry property is a string before returning true, alongside the existing
system and modules checks, so the CapturedMetadata type guard matches its
declared shape.
- Around line 192-202: Update verifyVersion to accept a single options object
instead of its nine positional parameters, using named properties for system,
modules, systemVersion, systemMinor, isDocker, updateRegistry, recordFailures,
and keepContainer. Update every call site to pass the corresponding named values
and adjust the function body to read from the options object.
- Around line 35-38: Extract the duplicated minorOf implementation from
scripts/verify-local.ts and scripts/monitor-releases.ts into a shared module,
then import and use that shared function in both scripts. Preserve the existing
major.minor or "unknown" behavior so registry keys continue using one consistent
grouping rule.
- Around line 507-542: Extract the duplicated system-version resolution into a
shared helper accepting meta, system, and systemVersion, then use it in both the
success and failure paths so they apply identical trust rules. Also centralize
the realModules filtering that excludes “fake-module” and reuse it from both
paths, preserving the existing unresolved-version behavior.
In `@src/docker.test.ts`:
- Around line 48-84: Extend src/docker.test.ts with focused coverage for
DockerFoundryOrchestrator.ensureWritableDir, stopAndRemove, and copyToContainer.
Mock fs to exercise ensureWritableDir stat, chown, and access behavior; mock
execFileSync to verify stopAndRemove ignores only “no such container” failures
while propagating real failures, and copyToContainer rejects ownership
mismatches. Preserve the existing rootless getRunCommand tests.
In `@src/docker.ts`:
- Around line 30-42: Use the exported isPodmanRuntime() from src/docker.ts as
the single Podman-detection implementation. Remove the duplicate function and
related detection logic from scripts/verify-local.ts, import the shared symbol
there, and preserve its existing callers and behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3dab3707-f66e-4569-aca6-acb1bb1aca1f
📒 Files selected for processing (11)
ops/vm/foundry-verify.serviceops/vm/foundry-verify.timerops/vm/verify-nightly.shpackage.jsonscripts/close-resolved-issues.tsscripts/monitor-releases.tsscripts/verify-local.tssrc/cli/index.tssrc/docker.test.tssrc/docker.tsverified-versions.json
🚧 Files skipped from review as they are similar to previous changes (7)
- ops/vm/foundry-verify.timer
- package.json
- ops/vm/foundry-verify.service
- verified-versions.json
- ops/vm/verify-nightly.sh
- scripts/monitor-releases.ts
- src/cli/index.ts
- scripts/close-resolved-issues.ts: per-entry try/catch so one issue's API failure (deleted issue, rate limit, transient network error) doesn't abort reconciliation for every other independent entry; paginate the open-issues query instead of silently missing anything past the first 100 open verification-required issues. - src/docker.ts: fixOwnership()'s fs.statSync call is now inside its own try/catch, so a stat failure (dangling entry, race with a concurrent process) folds into ensureWritableDir's own actionable diagnostic instead of escaping as a raw exception. - scripts/verify-local.ts: isCapturedMetadata now validates the declared foundry: string field too, matching its own CapturedMetadata interface (no behavioral impact - meta.foundry is never read - but the guard should actually validate what it claims to). - scripts/verify-local.ts: verifyVersion() takes a single options object instead of 9 positional parameters, removing a real footgun for future edits (same-typed positional args aren't distinguished by the compiler). - Deduplicated minorOf into a new scripts/version-utils.ts, shared by verify-local.ts and monitor-releases.ts - the two copies had actually drifted: monitor-releases.ts's version lacked the "unknown" fallback entirely, producing "X.undefined" instead of "unknown" for a malformed version string. - scripts/verify-local.ts: extracted the system-version resolution logic (the manifestUrl-gated fallback) and fake-module filtering into shared helpers (resolveVerifiedSystemVersion, filterRealModules) used by both the success and failure registry-write paths - this exact duplication is why an earlier fix to this logic had to be applied twice and could have drifted apart again on a future edit to only one copy. - src/docker.test.ts: added focused coverage for ensureWritableDir (real temp filesystem - creation, already-correctly-owned, and a chmod-000 directory that can't be fixed), stopAndRemove (Docker's and Podman's differing "no such container" phrasings tolerated, a real daemon-unreachable failure propagated), and copyToContainer (matching vs. mismatched post-copy ownership) - these three methods had zero coverage despite being the subject of most of this review thread's actual bugs. Skipped: a claimed duplicate Podman-detection implementation in verify-local.ts doesn't exist - it already imports and reuses the shared isPodmanRuntime from src/docker.ts.
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/docker.ts (2)
293-302: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect the comment: only
docker execis used.The comment says "via an ephemeral container or exec (if running)", but the code only runs
docker exec. The call therefore requires a running container and fails otherwise. Update the comment to describe the actual behavior.Proposed change
- // Ensure destination directory exists via an ephemeral container or exec (if running) + // Ensure the destination directory exists. This requires the container to + // be running, since it uses `docker exec`.🤖 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 `@src/docker.ts` around lines 293 - 302, Update the comment immediately above the docker exec call to state that the destination directory is created using docker exec in the running container. Remove the inaccurate reference to an ephemeral container or conditional execution, and note that this operation requires the configured container to be running.
120-137: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a timeout to the image pull calls.
execFileSyncwaits without limit by default. If a registry stalls,docker pullblocks the nightly run for an unbounded time. Pass an explicittimeoutso a stalled pull fails fast. The missing-image pull must still fail the run, and the update pull already tolerates failures.Proposed change
if (!imageExists) { console.log(`[DockerOrchestrator] Image ${image} not found locally. Pulling...`); - execFileSync("docker", ["pull", image], { stdio: "inherit" }); + execFileSync("docker", ["pull", image], { stdio: "inherit", timeout: PULL_TIMEOUT_MS }); } else { console.log(`[DockerOrchestrator] Image ${image} already exists locally.`); // Optional: try to pull to update, but ignore failures try { console.log(`[DockerOrchestrator] Attempting to update image ${image}...`); - execFileSync("docker", ["pull", image], { stdio: "ignore" }); + execFileSync("docker", ["pull", image], { stdio: "ignore", timeout: PULL_TIMEOUT_MS }); } catch {🤖 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 `@src/docker.ts` around lines 120 - 137, Add an explicit timeout option to both docker pull calls in the image-loading flow, using the existing missing-image branch and update branch around imageExists. Keep failures from the missing-image pull propagating to fail the run, while preserving the update pull’s existing catch-and-warn 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 `@scripts/verify-local.ts`:
- Around line 375-400: Update the report validation flow around
isPlaywrightReport and extractFailures to count executed specs via a countSpecs
traversal, and reject reports with zero specs as infrastructure failures. Keep
parse errors handled as malformed reports, but perform the empty-report failure
check after the JSON.parse catch so that validation errors are not swallowed;
preserve execError precedence and existing failure extraction.
In `@src/docker.test.ts`:
- Around line 137-150: Update the inaccessible-directory test around
callEnsureWritableDir so it no longer relies on chmod 000, which is ineffective
for root and Windows runners. Mock or otherwise simulate the filesystem EACCES
failure encountered by ensureWritableDir, while preserving the assertion for the
clear “isn't writable/accessible” error and existing cleanup.
In `@src/docker.ts`:
- Around line 321-330: Update the ownership validation around actualOwner so
directory copies are checked recursively, validating every copied entry’s
ownership against expectedOwner rather than only containerPath. Preserve the
existing regular-file validation and error reporting, and use the existing
Docker execution flow to walk entries when localPath is a directory.
---
Nitpick comments:
In `@src/docker.ts`:
- Around line 293-302: Update the comment immediately above the docker exec call
to state that the destination directory is created using docker exec in the
running container. Remove the inaccurate reference to an ephemeral container or
conditional execution, and note that this operation requires the configured
container to be running.
- Around line 120-137: Add an explicit timeout option to both docker pull calls
in the image-loading flow, using the existing missing-image branch and update
branch around imageExists. Keep failures from the missing-image pull propagating
to fail the run, while preserving the update pull’s existing catch-and-warn
behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fceb4230-856f-4cd4-8b1f-2ffc5742545c
📒 Files selected for processing (12)
ops/vm/foundry-verify.serviceops/vm/foundry-verify.timerops/vm/verify-nightly.shpackage.jsonscripts/close-resolved-issues.tsscripts/monitor-releases.tsscripts/verify-local.tsscripts/version-utils.tssrc/cli/index.tssrc/docker.test.tssrc/docker.tsverified-versions.json
🚧 Files skipped from review as they are similar to previous changes (7)
- package.json
- ops/vm/foundry-verify.service
- ops/vm/foundry-verify.timer
- ops/vm/verify-nightly.sh
- verified-versions.json
- src/cli/index.ts
- scripts/monitor-releases.ts
- scripts/verify-local.ts: a well-formed Playwright report showing zero specs executed (empty test-file match, config issue) was silently recorded as a pass with zero failures - no evidence anything was actually verified. Added countSpecs() alongside the existing extractFailures() traversal; a zero count is now treated the same as a malformed report (execError takes precedence if present, otherwise a clear "zero specs executed" infrastructure-failure message). - src/docker.ts: copyToContainer()'s post-copy ownership verification only checked the single top-level containerPath, never the nested contents of a directory copy - a regression that only got the top-level directory's ownership right (but not files underneath it) would go undetected. Now recursively verifies every entry via `find ... -exec stat` when the copied path is a directory (confirmed live against real Docker with a nested test directory), falling back to the original single-stat check for a plain file. - src/docker.ts: added a 5-minute timeout to both docker pull calls in start() - previously a hung pull (bad network/registry issue) would block silently with no clear diagnosis, other than the systemd unit's blunt 6h TimeoutStartSec eventually killing everything. - src/docker.ts: fixed a stale comment claiming the destination directory is created "via an ephemeral container or exec (if running)" - it's unconditionally a docker exec against the already-running container. - src/docker.test.ts: replaced the ensureWritableDir permission-failure test's reliance on chmod 0o000 (unreliable when run as root, e.g. common CI containers, or on Windows) with a mocked EACCES on readdirSync/ accessSync, so the test is deterministic regardless of who/where it runs. Updated the existing copyToContainer tests to use real temp files (needed since the fix above now calls fs.statSync on localPath) and added directory-copy test cases (matching and mismatched nested ownership).
1 participant
Summary
verified-versions.json's pending entries: a systemd timer on a standalone VM (no Actions runner involved) can runverify:local --all-pendingunattended and push results, instead of a human running it by hand off the issues the Monitor Releases workflow files.--record-failures(verify-local.ts): genuine test failures now get written as a"failed"registry status, distinct from the pre-check"incompatible", so--all-pendingstops silently retrying the same broken combo every night.verify-local.ts— the fix(cli): clean up temp data dirs and use shared cache #23 cleanup fix only landed in the public CLI, not this internal script, so every Docker-based run left its full/datadir behind indefinitely.close-resolved-issues.ts: closes/relabelsverification-requiredissues once their entry leavespending(stable/incompatible → close with a comment; failed → relabelneeds-investigation).ops/vm/: wrapper script + systemd unit/timer templates for the VM. Scheduled nightly at 02:00 Europe/Berlin, deliberately withoutPersistent=trueso a missed run waits for the next night instead of a reboot-triggered catch-up landing inside the push blackout window.Test plan
npm run build,npm run lint,npm run format,npm run testall passops/vm/*on the actual VM and do a live dry run against a real pending entrySummary by CodeRabbit