From 5258bd3f7702e6140b2b547993988850f6d6c449 Mon Sep 17 00:00:00 2001 From: Philipp Fehr Date: Fri, 31 Jul 2026 20:11:25 +0200 Subject: [PATCH 1/3] feat: automate nightly compatibility verification on a standalone VM 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. --- ops/vm/foundry-verify.service | 52 ++++ ops/vm/foundry-verify.timer | 20 ++ ops/vm/verify-nightly.sh | 66 +++++ package.json | 1 + scripts/close-resolved-issues.ts | 132 +++++++++ scripts/monitor-releases.ts | 44 +-- scripts/verify-local.ts | 447 ++++++++++++++++++++++++++----- src/cli/index.ts | 24 +- src/docker.test.ts | 61 ++++- src/docker.ts | 231 +++++++++++++--- verified-versions.json | 6 +- 11 files changed, 954 insertions(+), 130 deletions(-) create mode 100644 ops/vm/foundry-verify.service create mode 100644 ops/vm/foundry-verify.timer create mode 100755 ops/vm/verify-nightly.sh create mode 100644 scripts/close-resolved-issues.ts diff --git a/ops/vm/foundry-verify.service b/ops/vm/foundry-verify.service new file mode 100644 index 0000000..3ad2fe5 --- /dev/null +++ b/ops/vm/foundry-verify.service @@ -0,0 +1,52 @@ +# Install: /etc/systemd/system/foundry-verify.service +# Then: systemctl daemon-reload && systemctl enable --now foundry-verify.timer +# +# Credentials (FOUNDRY_USERNAME/PASSWORD/ADMIN_KEY) live in +# /opt/foundry-playwright/.env and the push/issue token lives in `gh`'s own +# config (`gh auth login`) — nothing is passed via this unit. +# +# Runs as a dedicated non-root user rather than root, with NO docker-group +# membership - containers run rootless via Podman (see below), since this +# box fetches and executes third-party FoundryVTT/system/module content on +# every run, and docker-group access is root-equivalent. Before enabling: +# sudo useradd --system --create-home --shell /usr/sbin/nologin foundry-verify +# sudo apt install podman podman-docker uidmap slirp4netns +# sudo usermod --add-subuids 200000-265535 --add-subgids 200000-265535 foundry-verify +# sudo loginctl enable-linger foundry-verify +# sudo chown -R foundry-verify:foundry-verify /opt/foundry-playwright +# (the .env file - which must set FOUNDRY_PLAYWRIGHT_ROOTLESS=1 - and +# gh/git credentials need to be set up as that user too) +# +# podman-docker makes the `docker` binary a Podman wrapper, so nothing in +# this repo needs to know it isn't talking to real Docker. +# +# Commits are SSH-signed. One-time setup (as foundry-verify): +# ssh-keygen -t ed25519 -N "" -C "foundry-verify commit signing" -f ~/.ssh/foundry-verify-signing +# git config --global gpg.format ssh +# git config --global user.signingkey ~/.ssh/foundry-verify-signing.pub +# git config --global commit.gpgsign true +# git config --global user.email "+@users.noreply.github.com" +# echo " $(cat ~/.ssh/foundry-verify-signing.pub)" > ~/.ssh/allowed_signers +# git config --global gpg.ssh.allowedSignersFile ~/.ssh/allowed_signers +# Then register the public key on GitHub as a *Signing key* (not an +# authentication key - it's never used for repo access, only for the +# cryptographic signature) under the account matching that noreply email: +# `gh ssh-key add ~/.ssh/foundry-verify-signing.pub --type signing --title "foundry-verify"`. +# No passphrase, since this runs unattended with no agent/terminal - the +# key's blast radius if leaked is "forge signed commits as this identity", +# not repo or account access, so this is an acceptable tradeoff here. + +[Unit] +Description=Foundry Playwright nightly compatibility verification +After=network-online.target +Wants=network-online.target +# No Requires=docker.service: containers run via rootless Podman, which is +# daemonless - there's no system-wide service for this unit to depend on. + +[Service] +Type=oneshot +User=foundry-verify +Group=foundry-verify +WorkingDirectory=/opt/foundry-playwright +ExecStart=/opt/foundry-playwright/ops/vm/verify-nightly.sh +TimeoutStartSec=6h diff --git a/ops/vm/foundry-verify.timer b/ops/vm/foundry-verify.timer new file mode 100644 index 0000000..dc99103 --- /dev/null +++ b/ops/vm/foundry-verify.timer @@ -0,0 +1,20 @@ +# Install: /etc/systemd/system/foundry-verify.timer + +[Unit] +Description=Run Foundry Playwright compatibility verification nightly + +[Timer] +# 20:00 CEST: ~5h clear of the host's ~01:00-01:30/04:30-06:00 CEST backup-job +# CPU peaks (from a week of RRD history on the PBS VM sharing this host), and +# ~11.5h clear of the next weekday's 07:30 CEST push blackout - both wider +# margins than a 02:00 slot would give. +OnCalendar=*-*-* 20:00:00 Europe/Berlin +RandomizedDelaySec=600 +# Deliberately no Persistent=true: a missed run should be skipped, not caught +# up. Persistent=true fires as soon as the timer becomes active again (e.g. +# right at boot) regardless of the time of day, which could land the git push +# inside the Mon/Tue/Thu/Fri 07:30-16:30 CEST blackout this schedule exists to +# avoid. Waiting for the next 20:00 keeps that guarantee unconditional. + +[Install] +WantedBy=timers.target diff --git a/ops/vm/verify-nightly.sh b/ops/vm/verify-nightly.sh new file mode 100755 index 0000000..d230652 --- /dev/null +++ b/ops/vm/verify-nightly.sh @@ -0,0 +1,66 @@ +#!/usr/bin/env bash +set -euo pipefail + +# Nightly compatibility verification, run by foundry-verify.timer on the VM. +# +# Consumes "pending" rows written by the cloud Monitor Releases workflow, +# runs the Docker-based verification suite against each, and pushes the +# result back. Requires FOUNDRY_USERNAME/PASSWORD/ADMIN_KEY in .env and a +# `gh auth login`'d token with repo scope, both local to this VM only. + +REPO_DIR="${FP_REPO_DIR:-/opt/foundry-playwright}" +# Repo-owned rather than /tmp: a world-writable, predictable path there is +# open to a symlink pre-creation attack from another local user; this path is +# only writable by foundry-verify itself. +LOCK_FILE="$REPO_DIR/.verify-nightly.lock" +MAX_DISK_USED_PCT=85 + +cd "$REPO_DIR" + +exec 200>"$LOCK_FILE" +if ! flock -n 200; then + echo "[verify-nightly] Another run is still in progress, exiting." + exit 0 +fi + +# Foundry-version-tagged images are reused across runs and left alone; this +# only clears dangling layers/containers. Runs via EXIT trap rather than as a +# last step so it still fires if an earlier command (disk guard, git push, +# reconciliation) aborts the script under `set -e`. +trap 'docker system prune -f >/dev/null 2>&1 || true' EXIT + +used_pct=$(df --output=pcent "$REPO_DIR" | tail -1 | tr -dc '0-9') +if [ "$used_pct" -gt "$MAX_DISK_USED_PCT" ]; then + echo "[verify-nightly] Disk usage at ${used_pct}%, aborting run." >&2 + exit 1 +fi + +git pull --rebase --autostash + +# Don't let a genuine test failure abort the script — --record-failures already +# writes it to the registry as "failed" so it stops being retried; we still want +# to push whatever did succeed and reconcile issues either way. Capture the +# real exit status instead of masking it, so it still surfaces at the end +# (e.g. to systemd/monitoring) rather than always reporting success. +verify_status=0 +npm run verify:local -- --all-pending --docker --update-registry --record-failures --git-commit || + verify_status=$? + +git pull --rebase --autostash + +# The 20:00 CEST schedule is chosen to sit well clear of the Mon/Tue/Thu/Fri +# 07:30-16:30 CEST push blackout, but this system user's git isn't wired to +# the global pre-push hook that enforces it interactively - so check again +# here, right before the one action (the push) that actually leaves the VM. +tz_day=$(TZ="Europe/Berlin" date +%u) +tz_hm=$((10#$(TZ="Europe/Berlin" date +%H%M))) +if [[ "$tz_day" =~ ^[1245]$ ]] && [ "$tz_hm" -ge 730 ] && [ "$tz_hm" -lt 1630 ]; then + echo "[verify-nightly] Within the Mon/Tue/Thu/Fri 07:30-16:30 Europe/Berlin push blackout; leaving results committed locally and skipping push/reconciliation for tonight." >&2 + exit "$verify_status" +fi + +git push + +npm run close-resolved-issues + +exit "$verify_status" diff --git a/package.json b/package.json index ee34e8b..bd45477 100644 --- a/package.json +++ b/package.json @@ -36,6 +36,7 @@ "format": "oxfmt . --check", "format:fix": "oxfmt .", "verify:local": "tsx scripts/verify-local.ts", + "close-resolved-issues": "tsx scripts/close-resolved-issues.ts", "release": "release-it", "prepublishOnly": "npm run build", "prepare": "husky" diff --git a/scripts/close-resolved-issues.ts b/scripts/close-resolved-issues.ts new file mode 100644 index 0000000..67034cb --- /dev/null +++ b/scripts/close-resolved-issues.ts @@ -0,0 +1,132 @@ +import "dotenv/config"; +import fs from "fs"; +import path from "path"; +import { execSync } from "child_process"; + +/** + * Reconciles verification-required GitHub issues against verified-versions.json. + * + * Run after a verification pass: any entry that has left "pending" gets its + * matching issue commented on and closed (or, for "failed", relabeled + * needs-investigation so a human looks at the real regression instead of it + * being silently retried forever). + */ + +interface RegistryEntry { + fvtt: string; + system: string; + systemVersion: string; + status: "stable" | "pending" | "incompatible" | "failed"; + notes: string; +} + +interface GhIssue { + number: number; + title: string; +} + +function getGithubToken(): string { + try { + const token = execSync("gh auth token", { + encoding: "utf8", + stdio: ["pipe", "pipe", "pipe"], + }).trim(); + if (token) return token; + } catch { + console.warn("[close-resolved-issues] gh not available or not logged in."); + } + const envToken = process.env.GITHUB_TOKEN || process.env.GH_TOKEN; + if (envToken) return envToken; + throw new Error( + "No GitHub token available (`gh auth token` failed and GITHUB_TOKEN/GH_TOKEN are unset).", + ); +} + +function repoSlug(): string { + const pkg = JSON.parse(fs.readFileSync(path.join(process.cwd(), "package.json"), "utf8")); + const url: string = pkg.repository?.url ?? ""; + const m = url.match(/github\.com[/:]([^/]+)\/([^/.]+?)(\.git)?$/); + if (!m) + throw new Error(`Could not determine owner/repo from package.json repository.url: "${url}"`); + return `${m[1]}/${m[2]}`; +} + +async function githubRequest( + token: string, + method: string, + urlPath: string, + body?: unknown, +): Promise { + const res = await fetch(`https://api.github.com${urlPath}`, { + method, + headers: { + Authorization: `Bearer ${token}`, + Accept: "application/vnd.github.v3+json", + "User-Agent": "foundry-playwright/close-resolved-issues", + ...(body ? { "Content-Type": "application/json" } : {}), + }, + body: body ? JSON.stringify(body) : undefined, + }); + if (!res.ok) { + throw new Error(`GitHub API ${method} ${urlPath} failed: ${res.status} ${await res.text()}`); + } + return (await res.json()) as T; +} + +async function run() { + const registryPath = path.join(process.cwd(), "verified-versions.json"); + const registry: RegistryEntry[] = JSON.parse(fs.readFileSync(registryPath, "utf8")); + const resolved = registry.filter((e) => e.status !== "pending"); + + if (resolved.length === 0) { + console.log("[close-resolved-issues] No resolved entries to reconcile."); + return; + } + + const token = getGithubToken(); + const repo = repoSlug(); + + const openIssues = await githubRequest( + token, + "GET", + `/repos/${repo}/issues?labels=verification-required&state=open&per_page=100`, + ); + + for (const entry of resolved) { + const title = `Verification Required: FVTT ${entry.fvtt} + ${entry.system} v${entry.systemVersion}`; + const issue = openIssues.find((i) => i.title === title); + if (!issue) continue; + + const outcome = + entry.status === "stable" + ? `✅ Verified stable.\n\n${entry.notes}` + : entry.status === "incompatible" + ? `❌ Confirmed incompatible.\n\n${entry.notes}` + : `⚠️ Automated verification failed and needs investigation.\n\n${entry.notes}`; + + console.log(`[close-resolved-issues] #${issue.number}: ${title} -> ${entry.status}`); + await githubRequest(token, "POST", `/repos/${repo}/issues/${issue.number}/comments`, { + body: outcome, + }); + + if (entry.status === "failed") { + await githubRequest(token, "POST", `/repos/${repo}/issues/${issue.number}/labels`, { + labels: ["needs-investigation"], + }); + // Drop the label this query selects on, or a "failed" entry (which + // stays "failed" forever - it's not re-verified by --all-pending) + // would get re-commented and re-labeled every single night. + await githubRequest( + token, + "DELETE", + `/repos/${repo}/issues/${issue.number}/labels/verification-required`, + ); + } else { + await githubRequest(token, "PATCH", `/repos/${repo}/issues/${issue.number}`, { + state: "closed", + }); + } + } +} + +run(); diff --git a/scripts/monitor-releases.ts b/scripts/monitor-releases.ts index f7410ba..e8cfb93 100644 --- a/scripts/monitor-releases.ts +++ b/scripts/monitor-releases.ts @@ -17,7 +17,7 @@ interface RegistryEntry { systemMinor: string; systemVersion: string; modules?: { id: string; version: string }[]; - status: "stable" | "pending" | "incompatible"; + status: "stable" | "pending" | "incompatible" | "failed"; timestamp: string; notes: string; } @@ -121,8 +121,8 @@ function buildManifestUrl(systemId: string, version: string): string | null { } interface CompatRange { - minimum?: number; - maximum?: number; + minimum?: string; + maximum?: string; } const compatCache = new Map(); @@ -139,8 +139,8 @@ function fetchCompatRange(systemId: string, version: string): CompatRange { const manifest = JSON.parse(json) as { compatibility?: Record }; const compat = manifest.compatibility ?? {}; const result: CompatRange = {}; - if (compat["minimum"]) result.minimum = parseInt(String(compat["minimum"]).split(".")[0], 10); - if (compat["maximum"]) result.maximum = parseInt(String(compat["maximum"]).split(".")[0], 10); + if (compat["minimum"]) result.minimum = String(compat["minimum"]); + if (compat["maximum"]) result.maximum = String(compat["maximum"]); compatCache.set(key, result); return result; } catch { @@ -149,15 +149,26 @@ function fetchCompatRange(systemId: string, version: string): CompatRange { } } +// A bare-major maximum (e.g. "14") means "compatible through all of 14.x" - +// normalize it to an exclusive ceiling at the next major so a full version +// compare against e.g. "14.360.0" doesn't wrongly treat it as exceeding "14". +// A bare-major minimum needs no such adjustment: compareVersions already +// treats missing components as 0, so "14" naturally floors at 14.0.0. +function normalizeMaximum(bound: string): string { + const parts = bound.split("."); + if (parts.length > 1) return bound; + return `${parseInt(parts[0]!, 10) + 1}.0.0`; +} + function isCompatibleWithFvtt( systemId: string, systemVersion: string, fvttVersion: string, ): boolean { - const fvttMajor = parseInt(fvttVersion.split(".")[0], 10); const { minimum, maximum } = fetchCompatRange(systemId, systemVersion); - if (minimum !== undefined && fvttMajor < minimum) return false; - if (maximum !== undefined && fvttMajor > maximum) return false; + if (minimum !== undefined && compareVersions(fvttVersion, minimum) < 0) return false; + if (maximum !== undefined && compareVersions(fvttVersion, normalizeMaximum(maximum)) >= 0) + return false; return true; } @@ -186,19 +197,22 @@ async function run() { ...new Set(registry.filter((e) => e.status === "stable").map((e) => e.fvtt)), ]; - // Include new Foundry generation if no stable row exists for it yet. - // A pending/incompatible row is not enough — keep checking until something is verified. + // Always include the current latest build alongside every historically- + // stable one, not just as a one-time gate for a brand-new generation - + // otherwise, once a generation's first build goes stable, later patches + // within that same generation (e.g. 14.360 -> 14.365) never get checked + // again, silently missing any system that bumps its minimum FVTT build + // requirement past the one we happen to be pinned on. Old stable rows + // for superseded builds are untouched history (registry key includes + // fvtt, so a new build just adds new rows). const majorFoundry = foundryLatest.split(".")[0]; const hasGenerationStable = registry.some( (e) => e.status === "stable" && e.fvtt.startsWith(`${majorFoundry}.`), ); - const fvttToCheck = hasGenerationStable - ? stableFvttVersions - : [...stableFvttVersions, foundryLatest]; - if (!hasGenerationStable) { console.log(`[monitor] New Foundry generation detected: ${foundryLatest}`); } + const fvttToCheck = [...new Set([...stableFvttVersions, foundryLatest])]; console.log( `[monitor] FVTT latest: ${foundryLatest} | Checking ${fvttToCheck.length} version(s)`, @@ -238,7 +252,7 @@ async function run() { systemVersion: latestPatch, status: "incompatible", timestamp: new Date().toISOString(), - notes: `System declares compatibility ${rangeNote}; incompatible with FVTT ${fvtt.split(".")[0]}.`, + notes: `System declares compatibility ${rangeNote}; incompatible with FVTT ${fvtt}.`, }); updated = true; continue; diff --git a/scripts/verify-local.ts b/scripts/verify-local.ts index 1a9dda1..80c8c3c 100644 --- a/scripts/verify-local.ts +++ b/scripts/verify-local.ts @@ -2,7 +2,7 @@ import { execSync, execFileSync } from "child_process"; import "dotenv/config"; import path from "path"; import fs from "fs"; -import { DockerFoundryOrchestrator } from "../src/docker.js"; +import { DockerFoundryOrchestrator, isPodmanRuntime } from "../src/docker.js"; import { Command } from "commander"; /** @@ -32,6 +32,11 @@ function extractVersionTag(tag: string, systemId: string): string | null { return null; } +function minorOf(version: string): string { + const [major, minor] = version.split("."); + return major && minor ? `${major}.${minor}` : "unknown"; +} + function compareVersions(a: string, b: string): number { const ap = a.split(".").map(Number); const bp = b.split(".").map(Number); @@ -90,13 +95,109 @@ async function resolveLatestPatch(systemId: string, minor: string): Promise[]).findIndex( + (e) => + e["fvtt"] === entry.fvtt && + e["system"] === entry.system && + e["systemMinor"] === entry.systemMinor, + ); + if (entryIdx !== -1) { + registry[entryIdx] = entry; + } else { + registry.push(entry); + } + + fs.writeFileSync(registryPath, JSON.stringify(registry, null, 2)); +} + +function getPlaywrightImageTag(): string { + const pkgPath = path.join(process.cwd(), "node_modules", "@playwright", "test", "package.json"); + const { version } = JSON.parse(fs.readFileSync(pkgPath, "utf8")) as { version: string }; + return `mcr.microsoft.com/playwright:v${version}-noble`; +} + +/** + * Runs the Playwright test suite inside Microsoft's official Playwright image + * instead of on the host. Keeps the host OS entirely out of Playwright's + * browser/dependency support matrix. Only explicitly listed env vars are + * forwarded (not the full host environment, which would clobber the + * container's own PATH/HOME) and secret values are never placed in argv — + * `-e KEY` (no value) makes docker forward it from its own process env. + */ +function runPlaywrightInContainer( + testFiles: string[], + playwrightArgs: string[], + containerEnv: Record, + rootless: boolean, +): void { + const image = getPlaywrightImageTag(); + const envFlags = Object.entries(containerEnv) + .filter(([, v]) => v !== undefined) + .flatMap(([k]) => ["-e", k]); + + execFileSync( + "docker", + [ + "run", + "--rm", + "--network", + "host", + "--user", + `${process.getuid!()}:${process.getgid!()}`, + // See DockerOrchestratorConfig.rootless / isPodmanRuntime in + // src/docker.ts - --userns=keep-id is Podman-specific syntax, so only + // add it when the docker binary is actually Podman under the hood. + ...(rootless && isPodmanRuntime() ? ["--userns=keep-id"] : []), + "-e", + "HOME=/tmp", + ...envFlags, + "-v", + `${process.cwd()}:/work`, + "-w", + "/work", + image, + "npx", + "playwright", + "test", + ...testFiles, + "--workers=1", + "--reporter=line,json", + ...playwrightArgs, + ], + { stdio: "inherit", env: { ...process.env, ...containerEnv } }, + ); +} + async function verifyVersion( version: string, system: string, modules: string[], systemVersion: string | undefined, + systemMinor: string | undefined, isDocker: boolean, updateRegistry: boolean, + recordFailures: boolean, keepContainer: boolean, ): Promise<{ success: boolean; failures: string[] }> { console.log( @@ -104,12 +205,19 @@ async function verifyVersion( ); let foundryUrl = process.env.FOUNDRY_URL || "http://localhost:30000"; + const rootless = process.env.FOUNDRY_PLAYWRIGHT_ROOTLESS === "1"; let orchestrator: DockerFoundryOrchestrator | null = null; + let tmpDataDir: string | null = null; let failures: string[] = []; + let meta = { + foundry: version, + system: { id: system, version: "unknown" }, + modules: [] as { id: string; version: string }[], + }; try { if (isDocker) { - const tmpDataDir = path.join( + tmpDataDir = path.join( process.cwd(), ".foundry_test_data", `.foundry_data_tmp_${version}_${Date.now()}`, @@ -119,6 +227,7 @@ async function verifyVersion( version: version, adminKey: process.env.FOUNDRY_ADMIN_KEY || "password", dataDir: tmpDataDir, + rootless, }); // Inject all local modules from e2e/ into the container @@ -169,46 +278,128 @@ async function verifyVersion( (a) => a.startsWith("--ui") || a.startsWith("--headed") || a.startsWith("--debug"), ); - const testFiles = ["e2e/verify.spec.ts", "e2e/user-management.spec.ts"].join(" "); - const reportPath = path.join(process.cwd(), `.playwright-report-${version}.json`); + const testFiles = ["e2e/verify.spec.ts", "e2e/user-management.spec.ts"]; + // Unique per run (not just per version), and removed up front - a + // previous run at this same path that crashed before reaching its own + // cleanup could otherwise leave a stale report behind for this run to + // misread as its own results. + const reportPath = path.join( + process.cwd(), + `.playwright-report-${version}-${Date.now()}-${process.pid}.json`, + ); + fs.rmSync(reportPath, { force: true }); + const metaPath = path.join(process.cwd(), ".foundry_metadata.json"); + fs.rmSync(metaPath, { force: true }); let execError: Error | null = null; try { - execSync( - `npx playwright test ${testFiles} --workers=1 --reporter=line,json ${playwrightArgs.join(" ")}`, - { - stdio: "inherit", - env: { ...env, PLAYWRIGHT_JSON_OUTPUT_NAME: reportPath }, - }, - ); + if (isDocker) { + // The container mounts process.cwd() at /work, so the path we hand + // to Playwright's own JSON reporter (running inside the container) + // must be rewritten relative to that mount point - the host's + // absolute reportPath doesn't exist inside the container's + // filesystem at all. + const reportPathInContainer = `/work/${path.relative(process.cwd(), reportPath)}`; + runPlaywrightInContainer( + testFiles, + playwrightArgs, + { + FOUNDRY_URL: env["FOUNDRY_URL"], + FOUNDRY_VERSION: env["FOUNDRY_VERSION"], + FOUNDRY_SYSTEM_ID: env["FOUNDRY_SYSTEM_ID"], + FOUNDRY_UI_ADAPTER: env["FOUNDRY_UI_ADAPTER"], + FOUNDRY_MODULE_IDS: env["FOUNDRY_MODULE_IDS"], + FOUNDRY_SYSTEM_MANIFEST: env["FOUNDRY_SYSTEM_MANIFEST"], + FOUNDRY_ADMIN_KEY: process.env.FOUNDRY_ADMIN_KEY, + FOUNDRY_ADMIN_PASSWORD: process.env.FOUNDRY_ADMIN_PASSWORD, + FOUNDRY_USERNAME: process.env.FOUNDRY_USERNAME, + FOUNDRY_PASSWORD: process.env.FOUNDRY_PASSWORD, + FOUNDRY_LICENSE_KEY: process.env.FOUNDRY_LICENSE_KEY, + PLAYWRIGHT_JSON_OUTPUT_NAME: reportPathInContainer, + }, + rootless, + ); + } else { + execFileSync( + "npx", + [ + "playwright", + "test", + ...testFiles, + "--workers=1", + "--reporter=line,json", + ...playwrightArgs, + ], + { + stdio: "inherit", + env: { ...env, PLAYWRIGHT_JSON_OUTPUT_NAME: reportPath }, + }, + ); + } } catch (e) { execError = e as Error; } if (fs.existsSync(reportPath)) { - const report = JSON.parse(fs.readFileSync(reportPath, "utf8")); - failures = extractFailures(report); + const rawContent = fs.readFileSync(reportPath, "utf8"); fs.unlinkSync(reportPath); + let validReport = false; + try { + const rawReport: unknown = JSON.parse(rawContent); + if (isPlaywrightReport(rawReport)) { + validReport = true; + failures = extractFailures(rawReport); + } + } catch { + // Corrupted/truncated report - fold into the same "malformed" + // diagnostic below instead of letting a raw JSON.parse error + // escape and override execError precedence. + validReport = false; + } + if (!validReport || (failures.length === 0 && execError)) { + // Either the report doesn't have the expected shape (corrupted or + // unexpected content) or the process genuinely failed despite a + // clean-looking report - in both cases, don't silently treat this + // as success just because some report file exists. + throw ( + execError ?? + new Error(`Malformed Playwright report at ${reportPath}: missing "suites" array.`) + ); + } } else if (execError) { // Playwright failed to start or crashed without producing a report. throw execError; + } else { + // Exited "successfully" but produced no report at all - no evidence + // any test actually ran. Don't silently treat that as a pass. + throw new Error( + `Playwright exited successfully but produced no report at ${reportPath} - treating as an infrastructure failure.`, + ); } - if (failures.length > 0) { - throw new Error(`Verification failed with ${failures.length} test failures.`); - } - - // Capture versions for the report + // Capture versions for the report (best-effort — the metadata test may have + // run and written this even if a later test in the same run failed). console.log("[verifyVersion] Capturing system and module versions..."); - let meta = { - foundry: version, - system: { id: system, version: "unknown" }, - modules: [] as { id: string; version: string }[], - }; - - const metaPath = path.join(process.cwd(), ".foundry_metadata.json"); if (fs.existsSync(metaPath)) { - meta = JSON.parse(fs.readFileSync(metaPath, "utf8")); + const rawContent = fs.readFileSync(metaPath, "utf8"); fs.unlinkSync(metaPath); + try { + const rawMeta: unknown = JSON.parse(rawContent); + if (isCapturedMetadata(rawMeta)) { + meta = rawMeta; + } else { + console.warn( + `[verifyVersion] Ignoring malformed ${metaPath} - missing expected system.id/version/modules shape; keeping "unknown" version metadata.`, + ); + } + } catch (e) { + console.warn( + `[verifyVersion] Failed to parse ${metaPath} (${(e as Error).message}); keeping "unknown" version metadata.`, + ); + } + } + + if (failures.length > 0) { + throw new Error(`Verification failed with ${failures.length} test failures.`); } console.log(`--- Verification Successful for ${version} ---`); @@ -272,53 +463,119 @@ async function verifyVersion( // Registry update — key is (fvtt, system, systemMinor) if (updateRegistry) { - console.log(`Updating verified-versions.json for ${version}...`); - const registryPath = path.join(process.cwd(), "verified-versions.json"); - let registry = JSON.parse(fs.readFileSync(registryPath, "utf8")); - - if (!Array.isArray(registry)) { - console.warn("Registry is not an array. Performing migration..."); - registry = []; - } - const realModules = meta.modules.filter((m) => m.id !== "fake-module"); - const [sysMajor, sysMinor] = installedSystemVersion.split("."); - const systemMinor = `${sysMajor}.${sysMinor}`; - - const entry = { - fvtt: version, - system: meta.system.id, - systemMinor, - systemVersion: installedSystemVersion, - modules: realModules.length > 0 ? realModules : undefined, - status: "stable" as const, - timestamp: new Date().toISOString(), - notes: `Verified locally with ${meta.system.id} v${installedSystemVersion}.`, - }; - - const entryIdx = (registry as Record[]).findIndex( - (e) => - e["fvtt"] === version && - e["system"] === meta.system.id && - e["systemMinor"] === systemMinor, - ); - if (entryIdx !== -1) { - registry[entryIdx] = entry; + // installedSystemVersion can still be "unknown" here even on a passing + // run (metadata missing or rejected by isCapturedMetadata). Only trust + // a fallback to the originally-requested systemVersion when it was + // actually pinned via a manifest URL this run (manifestUrl, + // buildManifestUrl only supports dnd5e/pf2e) - for any other system, + // or no version requested at all, Foundry just installs whatever + // "latest" its own resolver picks, which may have no relation to + // systemVersion at all. Recording it anyway would fabricate a + // "verified" claim for a version we never actually pinned or observed. + const resolvedSystemVersion = + installedSystemVersion !== "unknown" + ? installedSystemVersion + : manifestUrl + ? (systemVersion ?? "unknown") + : "unknown"; + + if (resolvedSystemVersion === "unknown") { + console.warn( + `[verifyVersion] Cannot determine the installed system version for ${version} (metadata missing/invalid and no manifest pin this run) - skipping registry update rather than recording an unverifiable "stable" entry.`, + ); } else { - registry.push(entry); + console.log(`Updating verified-versions.json for ${version}...`); + upsertRegistryEntry({ + fvtt: version, + system: meta.system.id, + systemMinor: minorOf(resolvedSystemVersion), + systemVersion: resolvedSystemVersion, + modules: realModules.length > 0 ? realModules : undefined, + status: "stable", + timestamp: new Date().toISOString(), + notes: `Verified locally with ${meta.system.id} v${resolvedSystemVersion}.`, + }); + console.log("Registry updated successfully."); } - - fs.writeFileSync(registryPath, JSON.stringify(registry, null, 2)); - console.log("Registry updated successfully."); } return { success: true, failures: [] }; } catch (error: unknown) { console.error(`--- Verification Failed for ${version} ---`); console.error((error as Error).message); + + if (updateRegistry && recordFailures && failures.length > 0) { + // Only genuine test failures land here - Docker/Playwright/report-parsing/ + // metadata errors fall through below, since "failed" is permanent (never + // retried by --all-pending) and an infra hiccup isn't a real incompatibility. + const realModules = meta.modules.filter((m) => m.id !== "fake-module"); + // A genuine failure almost always means the metadata-capture test never + // ran, so meta.system.version is still its "unknown" default. Same + // manifestUrl-gated fallback as the success path above (recomputed - + // manifestUrl there is out of scope in this catch block): only trust + // systemVersion when it was actually pinned via a manifest this run. + const manifestUrl = systemVersion ? buildManifestUrl(system, systemVersion) : null; + const resolvedSystemVersion = + meta.system.version !== "unknown" + ? meta.system.version + : manifestUrl + ? (systemVersion ?? "unknown") + : "unknown"; + + if (resolvedSystemVersion === "unknown") { + console.log( + `Not recording a failure entry for ${version}: cannot determine which system version was actually tested (metadata missing/invalid and no manifest pin this run). Leaving the entry pending so --all-pending retries it.`, + ); + } else { + console.log(`Recording failure in verified-versions.json for ${version}...`); + upsertRegistryEntry({ + fvtt: version, + system: meta.system.id || system, + systemMinor: minorOf(resolvedSystemVersion), + systemVersion: resolvedSystemVersion, + modules: realModules.length > 0 ? realModules : undefined, + status: "failed", + timestamp: new Date().toISOString(), + notes: `Automated verification failed: ${failures.join("; ")}`, + }); + console.log("Registry updated with failure entry."); + } + } else if (updateRegistry && recordFailures) { + console.log( + `Not recording a failure entry for ${version}: no test failures were collected, so this looks like an infrastructure error rather than a real incompatibility. Leaving the entry pending so --all-pending retries it.`, + ); + } + return { success: false, failures }; } finally { + let cleanupFailed = false; if (orchestrator && !keepContainer) { - await orchestrator.stopAndRemove(); + try { + await orchestrator.stopAndRemove(); + } catch (e) { + // A real cleanup failure (not just "container didn't exist" - + // stopAndRemove() already tolerates that) - don't let this override + // the actual verification result above, or crash the rest of an + // --all-pending batch. Retain tmpDataDir instead of removing it out + // from under a container that may still be running. + cleanupFailed = true; + console.error( + `[verifyVersion] Failed to clean up the Docker container: ${(e as Error).message}. Retaining ${tmpDataDir} for inspection.`, + ); + } + } + if (tmpDataDir && !keepContainer && !cleanupFailed) { + console.log(`Cleaning up temporary data directory: ${tmpDataDir}`); + try { + fs.rmSync(tmpDataDir, { recursive: true, force: true }); + } catch (e) { + // Same reasoning as the container-cleanup catch above - don't let + // this override the actual verification result or crash the rest + // of an --all-pending batch. + console.error( + `[verifyVersion] Failed to remove temporary data directory ${tmpDataDir}: ${(e as Error).message}`, + ); + } } } } @@ -343,6 +600,36 @@ interface PlaywrightReport { suites?: PlaywrightSuite[]; } +function isPlaywrightReport(value: unknown): value is PlaywrightReport { + return ( + typeof value === "object" && + value !== null && + Array.isArray((value as { suites?: unknown }).suites) + ); +} + +interface CapturedMetadata { + foundry: string; + system: { id: string; version: string }; + modules: { id: string; version: string }[]; +} + +function isModuleEntry(value: unknown): value is { id: string; version: string } { + if (typeof value !== "object" || value === null) return false; + const v = value as Record; + return typeof v["id"] === "string" && typeof v["version"] === "string"; +} + +function isCapturedMetadata(value: unknown): value is CapturedMetadata { + if (typeof value !== "object" || value === null) return false; + const v = value as Record; + const sys = v["system"]; + if (typeof sys !== "object" || sys === null) return false; + const sysRecord = sys as Record; + if (typeof sysRecord["id"] !== "string" || typeof sysRecord["version"] !== "string") return false; + return Array.isArray(v["modules"]) && v["modules"].every(isModuleEntry); +} + function extractFailures(report: PlaywrightReport): string[] { const failures: string[] = []; @@ -368,6 +655,7 @@ interface VerifyTarget { version: string; system: string; systemVersion?: string; + systemMinor?: string; modules: string[]; } @@ -397,7 +685,16 @@ program ) .option("--all", "Verify all pairings (pending and stable) in the registry", false) .option("--update-registry", "Update verified-versions.json on successful verification", false) - .option("--git-commit", "Automatically commit changes on success", false) + .option( + "--record-failures", + "On genuine verification failure, write a 'failed' status entry to the registry so --all-pending stops retrying it. Only takes effect with --update-registry.", + false, + ) + .option( + "--git-commit", + "Automatically commit registry/report changes whenever they exist, regardless of pass/fail", + false, + ) .option( "--keep-container", "Do not stop and remove the Docker container after verification", @@ -410,6 +707,10 @@ program console.log("Building library..."); execSync("npm run build", { stdio: "inherit" }); + if (options.recordFailures && !options.updateRegistry) { + console.warn("[verify] --record-failures has no effect without --update-registry; ignoring."); + } + const modules = options.modules ? options.modules.split(",").map((m: string) => m.trim()) : []; let targets: VerifyTarget[] = []; @@ -426,6 +727,7 @@ program version: e["fvtt"] as string, system: e["system"] as string, systemVersion: e["systemVersion"] as string | undefined, + systemMinor: e["systemMinor"] as string | undefined, modules: Array.isArray(e["modules"]) ? (e["modules"] as Record[]).map( (m: Record) => m["id"] as string, @@ -444,6 +746,7 @@ program system: e["system"] as string, // Don't pin systemVersion for re-verify: let installSystem handle already-installed // systems; the registry update records whatever version is actually installed. + systemMinor: e["systemMinor"] as string | undefined, modules: Array.isArray(e["modules"]) ? (e["modules"] as Record[]).map( (m: Record) => m["id"] as string, @@ -466,7 +769,15 @@ program systemVersion = await resolveLatestPatch(options.system, options.systemMinor); } - targets = [{ version: versionArg, system: options.system, systemVersion, modules }]; + targets = [ + { + version: versionArg, + system: options.system, + systemVersion, + systemMinor: options.systemMinor, + modules, + }, + ]; } if (targets.length === 0) { @@ -482,8 +793,10 @@ program target.system, target.modules, target.systemVersion, + target.systemMinor, options.docker, options.updateRegistry, + options.recordFailures, options.keepContainer, ); const sysLabel = target.systemVersion @@ -517,9 +830,9 @@ program } }); - if (allPassed && changedFiles.length > 0) { - const verifiedKeys = results.map((r) => r.key).join(", "); - const commitMsg = `chore(registry): verify ${verifiedKeys}`; + if (changedFiles.length > 0) { + const summary = results.map((r) => `${r.key} [${r.success ? "PASS" : "FAIL"}]`).join(", "); + const commitMsg = `chore(registry): verify ${summary}`; if (options.gitCommit) { console.log(`\n--- Auto-committing changes ---`); diff --git a/src/cli/index.ts b/src/cli/index.ts index bc06f3e..a064d0c 100644 --- a/src/cli/index.ts +++ b/src/cli/index.ts @@ -112,12 +112,30 @@ program console.error(`Error: ${(error as Error).message}`); process.exit(1); } finally { + let cleanupFailed = false; if (orchestrator) { - await orchestrator.stopAndRemove(); + try { + await orchestrator.stopAndRemove(); + } catch (e) { + // A real cleanup failure (not just "container didn't exist" - + // stopAndRemove() already tolerates that). Retain tmpDataDir + // instead of removing it out from under a container that may + // still be running. + cleanupFailed = true; + console.error( + `[CLI] Failed to clean up the Docker container: ${(e as Error).message}. Retaining ${tmpDataDir} for inspection.`, + ); + } } - if (tmpDataDir) { + if (tmpDataDir && !cleanupFailed) { console.log(`[CLI] Cleaning up temporary data directory: ${tmpDataDir}`); - fs.rmSync(tmpDataDir, { recursive: true, force: true }); + try { + fs.rmSync(tmpDataDir, { recursive: true, force: true }); + } catch (e) { + console.error( + `[CLI] Failed to remove temporary data directory ${tmpDataDir}: ${(e as Error).message}`, + ); + } } } }); diff --git a/src/docker.test.ts b/src/docker.test.ts index 8f22067..8eacf33 100644 --- a/src/docker.test.ts +++ b/src/docker.test.ts @@ -1,8 +1,17 @@ -import { describe, it, expect } from "vitest"; +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { execFileSync } from "child_process"; import { DockerFoundryOrchestrator } from "./docker.js"; import path from "path"; +vi.mock("child_process", () => ({ execFileSync: vi.fn() })); + +const expectedUserFlag = [`--user`, `${process.getuid!()}:${process.getgid!()}`]; + describe("DockerFoundryOrchestrator", () => { + beforeEach(() => { + vi.mocked(execFileSync).mockReset(); + }); + it("generates the correct docker run command", () => { const orchestrator = new DockerFoundryOrchestrator({ version: "12.327", @@ -13,14 +22,14 @@ describe("DockerFoundryOrchestrator", () => { }); const envPath = ".env.test"; - const command = orchestrator.getRunCommand(envPath); + const command = orchestrator.getRunCommand(envPath).join(" "); - expect(command).toContain("docker run -d"); + expect(command).toContain("run -d"); expect(command).toContain("--name test-foundry"); expect(command).toContain("-p 30001:30000"); - expect(command).toContain(`--env-file "${path.resolve(envPath)}"`); - expect(command).toContain(`-v "${path.resolve("/tmp/data")}:/data"`); - expect(command).toContain(`-v "${path.resolve("/tmp/cache")}:/data/container_cache"`); + expect(command).toContain(`--env-file ${path.resolve(envPath)}`); + expect(command).toContain(`-v ${path.resolve("/tmp/data")}:/data`); + expect(command).toContain(`-v ${path.resolve("/tmp/cache")}:/data/container_cache`); expect(command).toContain("ghcr.io/felddy/foundryvtt:12.327"); }); @@ -29,13 +38,51 @@ describe("DockerFoundryOrchestrator", () => { version: "11.315", }); - const command = orchestrator.getRunCommand(".env"); + const command = orchestrator.getRunCommand(".env").join(" "); expect(command).toContain("-p 30000:30000"); expect(command).toContain("--name foundry-playwright-11-315"); expect(command).toContain("ghcr.io/felddy/foundryvtt:11.315"); }); + it("omits --userns=keep-id by default", () => { + const orchestrator = new DockerFoundryOrchestrator({ + version: "12.327", + }); + const command = orchestrator.getRunCommand(".env"); + expect(command).toEqual(expect.arrayContaining(expectedUserFlag)); + expect(command).not.toContain("--userns=keep-id"); + // rootless defaults to false, so the runtime-detection shell-out should + // never even happen. + expect(execFileSync).not.toHaveBeenCalled(); + }); + + it("adds --userns=keep-id when rootless is set and the runtime is Podman", () => { + vi.mocked(execFileSync).mockReturnValue( + "Emulate Docker CLI using podman.\npodman version 5.7.0\n", + ); + const orchestrator = new DockerFoundryOrchestrator({ + version: "12.327", + rootless: true, + }); + const command = orchestrator.getRunCommand(".env"); + expect(command).toEqual(expect.arrayContaining(expectedUserFlag)); + expect(command).toContain("--userns=keep-id"); + }); + + it("omits --userns=keep-id when rootless is set but the runtime is real Docker", () => { + // --userns=keep-id is Podman-specific syntax; real (including rootless) + // Docker doesn't understand it and would fail outright if it were added. + vi.mocked(execFileSync).mockReturnValue("Docker version 27.3.1, build ce12230\n"); + const orchestrator = new DockerFoundryOrchestrator({ + version: "12.327", + rootless: true, + }); + const command = orchestrator.getRunCommand(".env"); + expect(command).toEqual(expect.arrayContaining(expectedUserFlag)); + expect(command).not.toContain("--userns=keep-id"); + }); + it("respects maxPortRetries in config", () => { const orchestrator = new DockerFoundryOrchestrator({ version: "12.327", diff --git a/src/docker.ts b/src/docker.ts index cf5fa16..b4b4c74 100644 --- a/src/docker.ts +++ b/src/docker.ts @@ -1,4 +1,4 @@ -import { execSync } from "child_process"; +import { execFileSync } from "child_process"; import path from "path"; import fs from "fs"; import net from "net"; @@ -15,6 +15,30 @@ export interface DockerOrchestratorConfig { cacheDir?: string; containerName?: string; envFile?: string; + /** + * Set this when the `docker` binary is actually a rootless Podman install + * (e.g. via the `podman-docker` package). Rootless container runtimes only + * identity-map container UID 0 back to the invoking host user - any other + * UID (including the `--user`-forced host UID below) otherwise maps + * through an arbitrary /etc/subuid subordinate range instead, breaking + * bind-mount ownership. `--userns=keep-id` (Podman-specific) fixes that. + * Default false: no behavior change for a plain rootful Docker install. + */ + rootless?: boolean; +} + +/** + * Whether the `docker` binary is actually a Podman install (e.g. via the + * `podman-docker` package). `--userns=keep-id` is Podman-specific syntax - + * real Docker (including rootless Docker) doesn't understand it, so the + * `rootless` config option must only add it when this is true. + */ +export function isPodmanRuntime(): boolean { + try { + return /podman/i.test(execFileSync("docker", ["--version"], { encoding: "utf8" })); + } catch { + return false; + } } /** @@ -37,6 +61,7 @@ export class DockerFoundryOrchestrator { containerName: config.containerName || `foundry-playwright-${config.version.replace(/\./g, "-")}`, envFile: config.envFile || ".env", + rootless: config.rootless ?? false, }; } @@ -79,36 +104,43 @@ export class DockerFoundryOrchestrator { this.config.port = availablePort; } - // 3. Ensure directories exist - if (!fs.existsSync(this.config.dataDir)) fs.mkdirSync(this.config.dataDir, { recursive: true }); - if (!fs.existsSync(this.config.cacheDir)) - fs.mkdirSync(this.config.cacheDir, { recursive: true }); + // 3. Ensure directories exist and are actually writable by this process. + // dataDir is normally freshly created per run, so this rarely matters + // there, but cacheDir is persistent across runs and can carry over + // top-level entries owned by a different uid (e.g. from before a --user + // fix, or a different automation user) - getRunCommand()'s --user + // override only controls what NEW writes are owned by, it does nothing + // for files that already exist with the wrong owner. Detect and + // best-effort fix that here, rather than finding out ~15 minutes into a + // container run via a cryptic "Permission denied" (this exact scenario + // has happened in practice). + this.ensureWritableDir(this.config.dataDir); + this.ensureWritableDir(this.config.cacheDir); // 4. Pull image if missing const image = `ghcr.io/felddy/foundryvtt:${this.config.version}`; - const imageExists = execSync(`docker images -q ${image}`, { encoding: "utf8" }).trim() !== ""; + const imageExists = + execFileSync("docker", ["images", "-q", image], { encoding: "utf8" }).trim() !== ""; if (!imageExists) { console.log(`[DockerOrchestrator] Image ${image} not found locally. Pulling...`); - execSync(`docker pull ${image}`, { stdio: "inherit" }); + execFileSync("docker", ["pull", image], { stdio: "inherit" }); } 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}...`); - execSync(`docker pull ${image}`, { stdio: "ignore" }); + execFileSync("docker", ["pull", image], { stdio: "ignore" }); } catch { console.warn(`[DockerOrchestrator] Failed to update image ${image}, using local version.`); } } // 5. Run container - const dockerCmd = this.getRunCommand(envPath); - console.log( `[DockerOrchestrator] Executing: docker run -d --name ${this.config.containerName} ... (using --env-file for security)`, ); - execSync(dockerCmd, { stdio: "inherit" }); + execFileSync("docker", this.getRunCommand(envPath), { stdio: "inherit" }); // 6. Wait for healthy await this.waitForReady(); @@ -117,21 +149,99 @@ export class DockerFoundryOrchestrator { } /** - * Generates the docker run command. + * Generates the arguments for `docker run` (excluding the "docker" binary + * itself) as an array, for execFileSync - never shell-joined, so none of + * these values (container name, resolved paths, version-derived image tag) + * can be interpreted as shell metacharacters. * @internal */ - getRunCommand(envPath: string): string { + getRunCommand(envPath: string): string[] { const image = `ghcr.io/felddy/foundryvtt:${this.config.version}`; return [ - "docker run -d", - `--name ${this.config.containerName}`, - "--restart always", - `-p ${this.config.port}:30000`, - `--env-file "${path.resolve(envPath)}"`, - `-v "${path.resolve(this.config.dataDir)}:/data"`, - `-v "${path.resolve(this.config.cacheDir)}:/data/container_cache"`, + "run", + "-d", + "--name", + this.config.containerName, + "--restart", + "always", + "-p", + `${this.config.port}:30000`, + "--env-file", + path.resolve(envPath), + // Foundry defaults to running internally as uid:gid 1000:1000, which + // won't generally match the host user owning the bind mount (e.g. a + // dedicated automation user). The image supports overriding this via + // Docker's own --user (see felddy/foundryvtt-docker discussion #1197), + // matched here to whichever user is actually running this - so + // everything Foundry writes to the bind mounts is owned by that same + // user from the start, with no permission mismatch to reconcile. + "--user", + `${process.getuid!()}:${process.getgid!()}`, + ...(this.config.rootless && isPodmanRuntime() ? ["--userns=keep-id"] : []), + "-v", + `${path.resolve(this.config.dataDir)}:/data`, + "-v", + `${path.resolve(this.config.cacheDir)}:/data/container_cache`, image, - ].join(" "); + ]; + } + + /** + * Ensures a bind-mount directory exists and its top-level entries are + * actually owned by the current process, best-effort fixing any that + * aren't (only possible when this process already has permission to - + * e.g. after switching automation users, it typically won't). Only checks + * top-level entries, matching the known shape of this cache directory + * (a handful of files, not deeply nested) rather than a full recursive + * walk of potentially large cached content. + */ + private ensureWritableDir(dir: string): void { + if (!fs.existsSync(dir)) { + fs.mkdirSync(dir, { recursive: true }); + return; + } + const uid = process.getuid!(); + const gid = process.getgid!(); + const unfixable = new Set(); + + const fixOwnership = (entryPath: string) => { + if (fs.statSync(entryPath).uid === uid) return; + try { + fs.chownSync(entryPath, uid, gid); + } catch { + unfixable.add(entryPath); + } + }; + + // The directory itself, not just its contents - readdirSync can succeed + // on a directory this process doesn't own if group/other bits allow it, + // which isn't the same as being able to write new entries into it. + fixOwnership(dir); + let entries: string[] = []; + try { + entries = fs.readdirSync(dir); + } catch { + // Can't even list it (e.g. restrictive mode bits on a directory owned + // by a different uid that chownSync above also couldn't fix) - fold + // into the same diagnostic below rather than letting a raw fs + // exception escape this function. + unfixable.add(dir); + } + for (const entry of entries) { + fixOwnership(path.join(dir, entry)); + } + + try { + fs.accessSync(dir, fs.constants.W_OK | fs.constants.X_OK); + } catch { + unfixable.add(dir); + } + + if (unfixable.size > 0) { + throw new Error( + `[DockerOrchestrator] ${dir} isn't writable/accessible by the current user (uid ${uid}) and this process lacks permission to fix it: ${[...unfixable].join(", ")}. This can happen after switching automation users or removing elevated privileges. Fix manually, e.g.: sudo chown -R ${uid}:${gid} "${dir}"`, + ); + } } /** @@ -139,10 +249,30 @@ export class DockerFoundryOrchestrator { */ stopAndRemove() { console.log(`[DockerOrchestrator] Cleaning up container ${this.config.containerName}...`); - try { - execSync(`docker stop ${this.config.containerName}`, { stdio: "ignore" }); - execSync(`docker rm ${this.config.containerName}`, { stdio: "ignore" }); - } catch {} + for (const args of [ + ["stop", this.config.containerName], + ["rm", this.config.containerName], + ]) { + try { + execFileSync("docker", args, { stdio: ["ignore", "ignore", "pipe"] }); + } catch (e) { + // The container simply not existing yet is expected (this runs as + // pre-cleanup before every start()) - both Docker ("No such + // container: X") and Podman ("no container with name or ID X + // found: no such container") phrase it differently but always + // include "no such container". Anything else (daemon down, + // permission denied, a container that won't stop) is a real + // failure callers need to know about, since they may otherwise + // proceed to remove a bind-mounted data dir a container is still + // using. + const stderr = (e as { stderr?: Buffer | string }).stderr?.toString() ?? ""; + if (!/no such container/i.test(stderr)) { + throw new Error( + `[DockerOrchestrator] Failed to ${args[0]} container ${this.config.containerName}: ${stderr || (e as Error).message}`, + ); + } + } + } } /** @@ -152,17 +282,48 @@ export class DockerFoundryOrchestrator { console.log( `[DockerOrchestrator] Copying ${localPath} to ${this.config.containerName}:${containerPath}`, ); + const uid = process.getuid!(); + const gid = process.getgid!(); + const expectedOwner = `${uid}:${gid}`; + // Ensure destination directory exists via an ephemeral container or exec (if running) - execSync(`docker exec ${this.config.containerName} mkdir -p ${path.dirname(containerPath)}`, { - stdio: "inherit", - }); - execSync(`docker cp ${localPath} ${this.config.containerName}:${containerPath}`, { - stdio: "inherit", - }); - // Fix permissions - execSync(`docker exec ${this.config.containerName} chown -R 1000:1000 ${containerPath}`, { - stdio: "inherit", - }); + // (docker exec defaults to the same identity getRunCommand() configured + // via --user, so this directory is already owned by that identity.) + // Array-form execFileSync avoids shell interpretation of localPath/ + // containerPath/containerName entirely (no `sh -c`, no metacharacters). + execFileSync( + "docker", + ["exec", this.config.containerName, "mkdir", "-p", path.dirname(containerPath)], + { stdio: "inherit" }, + ); + execFileSync( + "docker", + ["cp", "-a", localPath, `${this.config.containerName}:${containerPath}`], + { + stdio: "inherit", + }, + ); + + // Archive mode (-a) has been verified, directly, to attribute ownership + // to the container's own --user-configured identity under both real + // Docker and rootless Podman - but that's an emergent behavior, not a + // documented contract, so verify the actual result explicitly rather + // than silently trusting it. If it ever doesn't hold (a different + // Docker/Podman version), fail clearly instead of leaving files that + // Foundry can't read/write: this process runs docker exec as the + // container's own non-root identity (matching getRunCommand()'s + // --user), so it has no privilege to chown the file after the fact + // either - there's no fixup to fall back to here. + const actualOwner = execFileSync( + "docker", + ["exec", this.config.containerName, "stat", "-c", "%u:%g", containerPath], + { encoding: "utf8" }, + ).trim(); + if (actualOwner !== expectedOwner) { + throw new Error( + `[DockerOrchestrator] Copied ${containerPath} is owned by ${actualOwner}, not the expected ${expectedOwner} - archive-mode copy didn't attribute ownership as expected on this Docker/Podman version.`, + ); + } } private async waitForReady(): Promise { diff --git a/verified-versions.json b/verified-versions.json index 64abe8b..5c1d4b9 100644 --- a/verified-versions.json +++ b/verified-versions.json @@ -130,8 +130,8 @@ "system": "pf2e", "systemMinor": "8.3", "systemVersion": "8.3.0", - "status": "pending", - "timestamp": "2026-07-07T07:54:16.728Z", - "notes": "Automated detection. Run verification: `npm run verify:local -- --docker --version 14.360.0 --system pf2e --system-minor 8.3 --update-registry --git-commit`" + "status": "incompatible", + "timestamp": "2026-07-28T18:26:32.343Z", + "notes": "System declares compatibility minimum: 14.361; incompatible with FVTT 14.360.0 (one build below the required minimum). The earlier \"failed\"/rate-limit diagnoses were misdiagnoses of this same genuine incompatibility - monitor-releases.ts's compatibility check only compared major-version integers and missed the build-level minimum bump, and separately never re-checked newer 14.x builds once 14.360.0 went stable for this generation. Both fixed." } ] From 3d57825a198f43506975814ea89b45a7ca359663 Mon Sep 17 00:00:00 2001 From: Philipp Fehr Date: Fri, 31 Jul 2026 20:44:42 +0200 Subject: [PATCH 2/3] fix: harden reconciliation/ownership edge cases, dedupe drifted logic - 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. --- scripts/close-resolved-issues.ts | 80 ++++++++++++-------- scripts/monitor-releases.ts | 6 +- scripts/verify-local.ts | 122 ++++++++++++++++++------------- scripts/version-utils.ts | 4 + src/docker.test.ts | 103 ++++++++++++++++++++++++++ src/docker.ts | 6 +- 6 files changed, 231 insertions(+), 90 deletions(-) create mode 100644 scripts/version-utils.ts diff --git a/scripts/close-resolved-issues.ts b/scripts/close-resolved-issues.ts index 67034cb..b65fb25 100644 --- a/scripts/close-resolved-issues.ts +++ b/scripts/close-resolved-issues.ts @@ -86,45 +86,61 @@ async function run() { const token = getGithubToken(); const repo = repoSlug(); - const openIssues = await githubRequest( - token, - "GET", - `/repos/${repo}/issues?labels=verification-required&state=open&per_page=100`, - ); + // Paginate - a single page could silently miss issues once there are more + // than 100 open verification-required issues at once. + const openIssues: GhIssue[] = []; + for (let page = 1; ; page++) { + const batch = await githubRequest( + token, + "GET", + `/repos/${repo}/issues?labels=verification-required&state=open&per_page=100&page=${page}`, + ); + openIssues.push(...batch); + if (batch.length < 100) break; + } for (const entry of resolved) { const title = `Verification Required: FVTT ${entry.fvtt} + ${entry.system} v${entry.systemVersion}`; - const issue = openIssues.find((i) => i.title === title); - if (!issue) continue; - - const outcome = - entry.status === "stable" - ? `✅ Verified stable.\n\n${entry.notes}` - : entry.status === "incompatible" - ? `❌ Confirmed incompatible.\n\n${entry.notes}` - : `⚠️ Automated verification failed and needs investigation.\n\n${entry.notes}`; + try { + const issue = openIssues.find((i) => i.title === title); + if (!issue) continue; - console.log(`[close-resolved-issues] #${issue.number}: ${title} -> ${entry.status}`); - await githubRequest(token, "POST", `/repos/${repo}/issues/${issue.number}/comments`, { - body: outcome, - }); + const outcome = + entry.status === "stable" + ? `✅ Verified stable.\n\n${entry.notes}` + : entry.status === "incompatible" + ? `❌ Confirmed incompatible.\n\n${entry.notes}` + : `⚠️ Automated verification failed and needs investigation.\n\n${entry.notes}`; - if (entry.status === "failed") { - await githubRequest(token, "POST", `/repos/${repo}/issues/${issue.number}/labels`, { - labels: ["needs-investigation"], + console.log(`[close-resolved-issues] #${issue.number}: ${title} -> ${entry.status}`); + await githubRequest(token, "POST", `/repos/${repo}/issues/${issue.number}/comments`, { + body: outcome, }); - // Drop the label this query selects on, or a "failed" entry (which - // stays "failed" forever - it's not re-verified by --all-pending) - // would get re-commented and re-labeled every single night. - await githubRequest( - token, - "DELETE", - `/repos/${repo}/issues/${issue.number}/labels/verification-required`, + + if (entry.status === "failed") { + await githubRequest(token, "POST", `/repos/${repo}/issues/${issue.number}/labels`, { + labels: ["needs-investigation"], + }); + // Drop the label this query selects on, or a "failed" entry (which + // stays "failed" forever - it's not re-verified by --all-pending) + // would get re-commented and re-labeled every single night. + await githubRequest( + token, + "DELETE", + `/repos/${repo}/issues/${issue.number}/labels/verification-required`, + ); + } else { + await githubRequest(token, "PATCH", `/repos/${repo}/issues/${issue.number}`, { + state: "closed", + }); + } + } catch (e) { + // One entry's GitHub API call failing (rate limit, deleted issue, + // transient network error) shouldn't abort reconciliation for every + // other independent entry in this run. + console.error( + `[close-resolved-issues] Failed to reconcile "${title}": ${(e as Error).message}`, ); - } else { - await githubRequest(token, "PATCH", `/repos/${repo}/issues/${issue.number}`, { - state: "closed", - }); } } } diff --git a/scripts/monitor-releases.ts b/scripts/monitor-releases.ts index e8cfb93..0a1fff6 100644 --- a/scripts/monitor-releases.ts +++ b/scripts/monitor-releases.ts @@ -2,6 +2,7 @@ import "dotenv/config"; import fs from "fs"; import path from "path"; import { execSync } from "child_process"; +import { minorOf } from "./version-utils.js"; /** * Release Monitoring Script @@ -49,11 +50,6 @@ function extractVersion(tag: string, systemId: string): string | null { return null; } -function minorOf(version: string): string { - const [major, minor] = version.split("."); - return `${major}.${minor}`; -} - function compareVersions(a: string, b: string): number { const ap = a.split(".").map(Number); const bp = b.split(".").map(Number); diff --git a/scripts/verify-local.ts b/scripts/verify-local.ts index 80c8c3c..43a33c0 100644 --- a/scripts/verify-local.ts +++ b/scripts/verify-local.ts @@ -4,6 +4,7 @@ import path from "path"; import fs from "fs"; import { DockerFoundryOrchestrator, isPodmanRuntime } from "../src/docker.js"; import { Command } from "commander"; +import { minorOf } from "./version-utils.js"; /** * Local Verification Script @@ -32,11 +33,6 @@ function extractVersionTag(tag: string, systemId: string): string | null { return null; } -function minorOf(version: string): string { - const [major, minor] = version.split("."); - return major && minor ? `${major}.${minor}` : "unknown"; -} - function compareVersions(a: string, b: string): number { const ap = a.split(".").map(Number); const bp = b.split(".").map(Number); @@ -58,6 +54,31 @@ function buildManifestUrl(systemId: string, version: string): string | null { } } +/** + * Applies one consistent trust rule for both the success and failure + * registry-write paths: only ever record a system version we actually know, + * either from validated captured metadata, or from the originally-requested + * systemVersion when it was actually pinned via a manifest URL this run + * (manifestUrl only supports dnd5e/pf2e - for any other system, or no + * version requested at all, Foundry just installs whatever "latest" its own + * resolver picks, which may have no relation to the requested version at + * all). Returns "unknown" when neither source establishes it. + */ +function resolveVerifiedSystemVersion( + capturedVersion: string, + manifestUrl: string | null, + requestedSystemVersion: string | undefined, +): string { + if (capturedVersion !== "unknown") return capturedVersion; + return manifestUrl ? (requestedSystemVersion ?? "unknown") : "unknown"; +} + +function filterRealModules( + modules: { id: string; version: string }[], +): { id: string; version: string }[] { + return modules.filter((m) => m.id !== "fake-module"); +} + function getGithubAuthHeader(): string { try { const token = execSync("gh auth token", { @@ -189,17 +210,29 @@ function runPlaywrightInContainer( ); } +interface VerifyVersionOptions { + system: string; + modules: string[]; + systemVersion: string | undefined; + isDocker: boolean; + updateRegistry: boolean; + recordFailures: boolean; + keepContainer: boolean; +} + async function verifyVersion( version: string, - system: string, - modules: string[], - systemVersion: string | undefined, - systemMinor: string | undefined, - isDocker: boolean, - updateRegistry: boolean, - recordFailures: boolean, - keepContainer: boolean, + options: VerifyVersionOptions, ): Promise<{ success: boolean; failures: string[] }> { + const { + system, + modules, + systemVersion, + isDocker, + updateRegistry, + recordFailures, + keepContainer, + } = options; console.log( `\n--- Verifying Version: ${version} (System: ${system}${systemVersion ? ` v${systemVersion}` : ""}, Modules: ${modules.join(", ") || "none"}) ---`, ); @@ -463,22 +496,12 @@ async function verifyVersion( // Registry update — key is (fvtt, system, systemMinor) if (updateRegistry) { - const realModules = meta.modules.filter((m) => m.id !== "fake-module"); - // installedSystemVersion can still be "unknown" here even on a passing - // run (metadata missing or rejected by isCapturedMetadata). Only trust - // a fallback to the originally-requested systemVersion when it was - // actually pinned via a manifest URL this run (manifestUrl, - // buildManifestUrl only supports dnd5e/pf2e) - for any other system, - // or no version requested at all, Foundry just installs whatever - // "latest" its own resolver picks, which may have no relation to - // systemVersion at all. Recording it anyway would fabricate a - // "verified" claim for a version we never actually pinned or observed. - const resolvedSystemVersion = - installedSystemVersion !== "unknown" - ? installedSystemVersion - : manifestUrl - ? (systemVersion ?? "unknown") - : "unknown"; + const realModules = filterRealModules(meta.modules); + const resolvedSystemVersion = resolveVerifiedSystemVersion( + installedSystemVersion, + manifestUrl, + systemVersion, + ); if (resolvedSystemVersion === "unknown") { console.warn( @@ -508,19 +531,15 @@ async function verifyVersion( // Only genuine test failures land here - Docker/Playwright/report-parsing/ // metadata errors fall through below, since "failed" is permanent (never // retried by --all-pending) and an infra hiccup isn't a real incompatibility. - const realModules = meta.modules.filter((m) => m.id !== "fake-module"); - // A genuine failure almost always means the metadata-capture test never - // ran, so meta.system.version is still its "unknown" default. Same - // manifestUrl-gated fallback as the success path above (recomputed - - // manifestUrl there is out of scope in this catch block): only trust - // systemVersion when it was actually pinned via a manifest this run. + const realModules = filterRealModules(meta.modules); + // manifestUrl is recomputed here since the one from the try block above + // is out of scope in this catch block - same resolution rule either way. const manifestUrl = systemVersion ? buildManifestUrl(system, systemVersion) : null; - const resolvedSystemVersion = - meta.system.version !== "unknown" - ? meta.system.version - : manifestUrl - ? (systemVersion ?? "unknown") - : "unknown"; + const resolvedSystemVersion = resolveVerifiedSystemVersion( + meta.system.version, + manifestUrl, + systemVersion, + ); if (resolvedSystemVersion === "unknown") { console.log( @@ -623,6 +642,7 @@ function isModuleEntry(value: unknown): value is { id: string; version: string } function isCapturedMetadata(value: unknown): value is CapturedMetadata { if (typeof value !== "object" || value === null) return false; const v = value as Record; + if (typeof v["foundry"] !== "string") return false; const sys = v["system"]; if (typeof sys !== "object" || sys === null) return false; const sysRecord = sys as Record; @@ -788,17 +808,15 @@ program const results: { key: string; success: boolean; failures: string[] }[] = []; for (const target of targets) { - const result = await verifyVersion( - target.version, - target.system, - target.modules, - target.systemVersion, - target.systemMinor, - options.docker, - options.updateRegistry, - options.recordFailures, - options.keepContainer, - ); + const result = await verifyVersion(target.version, { + system: target.system, + modules: target.modules, + systemVersion: target.systemVersion, + isDocker: options.docker, + updateRegistry: options.updateRegistry, + recordFailures: options.recordFailures, + keepContainer: options.keepContainer, + }); const sysLabel = target.systemVersion ? `${target.system} v${target.systemVersion}` : target.system; diff --git a/scripts/version-utils.ts b/scripts/version-utils.ts new file mode 100644 index 0000000..f6cdc45 --- /dev/null +++ b/scripts/version-utils.ts @@ -0,0 +1,4 @@ +export function minorOf(version: string): string { + const [major, minor] = version.split("."); + return major && minor ? `${major}.${minor}` : "unknown"; +} diff --git a/src/docker.test.ts b/src/docker.test.ts index 8eacf33..bfb19c6 100644 --- a/src/docker.test.ts +++ b/src/docker.test.ts @@ -2,11 +2,21 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; import { execFileSync } from "child_process"; import { DockerFoundryOrchestrator } from "./docker.js"; import path from "path"; +import fs from "fs"; +import os from "os"; vi.mock("child_process", () => ({ execFileSync: vi.fn() })); const expectedUserFlag = [`--user`, `${process.getuid!()}:${process.getgid!()}`]; +function callEnsureWritableDir(orchestrator: DockerFoundryOrchestrator, dir: string): void { + (orchestrator as unknown as { ensureWritableDir: (d: string) => void }).ensureWritableDir(dir); +} + +function dockerErrorWithStderr(stderr: string): Error & { stderr: string } { + return Object.assign(new Error("Command failed"), { stderr }); +} + describe("DockerFoundryOrchestrator", () => { beforeEach(() => { vi.mocked(execFileSync).mockReset(); @@ -100,4 +110,97 @@ describe("DockerFoundryOrchestrator", () => { const config = (orchestrator as unknown as { config: { maxPortRetries: number } }).config; expect(config.maxPortRetries).toBe(10); }); + + describe("ensureWritableDir (real filesystem, not mocked)", () => { + let tmpBase: string; + + beforeEach(() => { + tmpBase = fs.mkdtempSync(path.join(os.tmpdir(), "fp-docker-test-")); + }); + + it("creates the directory if it doesn't exist", () => { + const dir = path.join(tmpBase, "new-subdir"); + const orchestrator = new DockerFoundryOrchestrator({ version: "1.0.0" }); + expect(fs.existsSync(dir)).toBe(false); + callEnsureWritableDir(orchestrator, dir); + expect(fs.existsSync(dir)).toBe(true); + fs.rmSync(tmpBase, { recursive: true, force: true }); + }); + + it("does not throw when the directory and its contents are already owned by the current user", () => { + fs.writeFileSync(path.join(tmpBase, "file.txt"), "hi"); + const orchestrator = new DockerFoundryOrchestrator({ version: "1.0.0" }); + expect(() => callEnsureWritableDir(orchestrator, tmpBase)).not.toThrow(); + fs.rmSync(tmpBase, { recursive: true, force: true }); + }); + + it("throws a clear, actionable error when the directory can't be made writable/accessible", () => { + const dir = path.join(tmpBase, "locked"); + fs.mkdirSync(dir); + fs.chmodSync(dir, 0o000); + const orchestrator = new DockerFoundryOrchestrator({ version: "1.0.0" }); + try { + expect(() => callEnsureWritableDir(orchestrator, dir)).toThrow( + /isn't writable\/accessible/, + ); + } finally { + fs.chmodSync(dir, 0o700); + fs.rmSync(tmpBase, { recursive: true, force: true }); + } + }); + }); + + describe("stopAndRemove", () => { + it("tolerates a container that doesn't exist yet (Docker's error phrasing)", () => { + vi.mocked(execFileSync).mockImplementation(() => { + throw dockerErrorWithStderr("Error response from daemon: No such container: x\n"); + }); + const orchestrator = new DockerFoundryOrchestrator({ version: "1.0.0", containerName: "x" }); + expect(() => orchestrator.stopAndRemove()).not.toThrow(); + }); + + it("tolerates a container that doesn't exist yet (Podman's error phrasing)", () => { + vi.mocked(execFileSync).mockImplementation(() => { + throw dockerErrorWithStderr( + 'Error: no container with name or ID "x" found: no such container\n', + ); + }); + const orchestrator = new DockerFoundryOrchestrator({ version: "1.0.0", containerName: "x" }); + expect(() => orchestrator.stopAndRemove()).not.toThrow(); + }); + + it("propagates a real cleanup failure instead of swallowing it", () => { + vi.mocked(execFileSync).mockImplementation(() => { + throw dockerErrorWithStderr( + "Cannot connect to the Docker daemon. Is the docker daemon running?\n", + ); + }); + const orchestrator = new DockerFoundryOrchestrator({ version: "1.0.0", containerName: "x" }); + expect(() => orchestrator.stopAndRemove()).toThrow(/Failed to stop container x/); + }); + }); + + describe("copyToContainer", () => { + const expectedOwner = `${process.getuid!()}:${process.getgid!()}`; + + it("succeeds when the copied file's ownership matches the configured identity", () => { + vi.mocked(execFileSync) + .mockReturnValueOnce("") // mkdir -p + .mockReturnValueOnce("") // cp -a + .mockReturnValueOnce(`${expectedOwner}\n`); // stat + const orchestrator = new DockerFoundryOrchestrator({ version: "1.0.0", containerName: "x" }); + expect(() => orchestrator.copyToContainer("/local/path", "/container/path")).not.toThrow(); + }); + + it("throws when the copied file's ownership doesn't match the configured identity", () => { + vi.mocked(execFileSync) + .mockReturnValueOnce("") // mkdir -p + .mockReturnValueOnce("") // cp -a + .mockReturnValueOnce("0:0\n"); // stat - unexpectedly root-owned + const orchestrator = new DockerFoundryOrchestrator({ version: "1.0.0", containerName: "x" }); + expect(() => orchestrator.copyToContainer("/local/path", "/container/path")).toThrow( + /didn't attribute ownership as expected/, + ); + }); + }); }); diff --git a/src/docker.ts b/src/docker.ts index b4b4c74..5488455 100644 --- a/src/docker.ts +++ b/src/docker.ts @@ -205,10 +205,14 @@ export class DockerFoundryOrchestrator { const unfixable = new Set(); const fixOwnership = (entryPath: string) => { - if (fs.statSync(entryPath).uid === uid) return; try { + if (fs.statSync(entryPath).uid === uid) return; fs.chownSync(entryPath, uid, gid); } catch { + // Covers both a failed chown AND a failed stat (e.g. a dangling + // symlink, or the entry vanishing between readdirSync and here) - + // either way, fold into the same diagnostic below instead of + // letting a raw fs exception escape this function. unfixable.add(entryPath); } }; From 2ad40b54921e95deb6b0f9ce50435a6918d4103a Mon Sep 17 00:00:00 2001 From: Philipp Fehr Date: Fri, 31 Jul 2026 21:32:43 +0200 Subject: [PATCH 3/3] fix: reject zero-spec reports, verify directory copies recursively - 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). --- scripts/verify-local.ts | 40 ++++++++++++++++++++++---- src/docker.test.ts | 55 +++++++++++++++++++++++++++++++---- src/docker.ts | 63 +++++++++++++++++++++++++++++++---------- 3 files changed, 133 insertions(+), 25 deletions(-) diff --git a/scripts/verify-local.ts b/scripts/verify-local.ts index 43a33c0..739b271 100644 --- a/scripts/verify-local.ts +++ b/scripts/verify-local.ts @@ -376,11 +376,13 @@ async function verifyVersion( const rawContent = fs.readFileSync(reportPath, "utf8"); fs.unlinkSync(reportPath); let validReport = false; + let specCount = 0; try { const rawReport: unknown = JSON.parse(rawContent); if (isPlaywrightReport(rawReport)) { validReport = true; failures = extractFailures(rawReport); + specCount = countSpecs(rawReport); } } catch { // Corrupted/truncated report - fold into the same "malformed" @@ -388,16 +390,32 @@ async function verifyVersion( // escape and override execError precedence. validReport = false; } - if (!validReport || (failures.length === 0 && execError)) { - // Either the report doesn't have the expected shape (corrupted or - // unexpected content) or the process genuinely failed despite a - // clean-looking report - in both cases, don't silently treat this - // as success just because some report file exists. + if (!validReport) { + // The report doesn't have the expected shape (corrupted or + // unexpected content) - don't silently treat this as success just + // because some report file exists. throw ( execError ?? new Error(`Malformed Playwright report at ${reportPath}: missing "suites" array.`) ); } + if (specCount === 0) { + // A well-formed report with zero specs (e.g. a test-file pattern + // matched nothing, or a config issue skipped everything) is no + // evidence anything was actually verified - don't record it as a + // pass just because it happens to show zero failures too. + throw ( + execError ?? + new Error( + `Playwright report at ${reportPath} shows zero specs executed - treating as an infrastructure failure.`, + ) + ); + } + if (failures.length === 0 && execError) { + // The process genuinely failed despite an otherwise clean-looking + // report. + throw execError; + } } else if (execError) { // Playwright failed to start or crashed without producing a report. throw execError; @@ -671,6 +689,18 @@ function extractFailures(report: PlaywrightReport): string[] { return failures; } +function countSpecs(report: PlaywrightReport): number { + let count = 0; + + function traverse(suite: PlaywrightSuite) { + if (suite.suites) suite.suites.forEach(traverse); + if (suite.specs) count += suite.specs.length; + } + + if (report.suites) report.suites.forEach(traverse); + return count; +} + interface VerifyTarget { version: string; system: string; diff --git a/src/docker.test.ts b/src/docker.test.ts index bfb19c6..d50ad55 100644 --- a/src/docker.test.ts +++ b/src/docker.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, vi, beforeEach } from "vitest"; +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; import { execFileSync } from "child_process"; import { DockerFoundryOrchestrator } from "./docker.js"; import path from "path"; @@ -135,16 +135,24 @@ describe("DockerFoundryOrchestrator", () => { }); it("throws a clear, actionable error when the directory can't be made writable/accessible", () => { + // Mocked rather than chmod 0o000 - permission bits don't reliably + // produce EACCES when running as root (common in CI containers) or + // on Windows, so this needs to work regardless of who/where it runs. const dir = path.join(tmpBase, "locked"); fs.mkdirSync(dir); - fs.chmodSync(dir, 0o000); + const eacces = (): never => { + throw Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" }); + }; + const readdirSpy = vi.spyOn(fs, "readdirSync").mockImplementation(eacces as never); + const accessSpy = vi.spyOn(fs, "accessSync").mockImplementation(eacces as never); const orchestrator = new DockerFoundryOrchestrator({ version: "1.0.0" }); try { expect(() => callEnsureWritableDir(orchestrator, dir)).toThrow( /isn't writable\/accessible/, ); } finally { - fs.chmodSync(dir, 0o700); + readdirSpy.mockRestore(); + accessSpy.mockRestore(); fs.rmSync(tmpBase, { recursive: true, force: true }); } }); @@ -182,6 +190,21 @@ describe("DockerFoundryOrchestrator", () => { describe("copyToContainer", () => { const expectedOwner = `${process.getuid!()}:${process.getgid!()}`; + let tmpBase: string; + let localFile: string; + let localDir: string; + + beforeEach(() => { + tmpBase = fs.mkdtempSync(path.join(os.tmpdir(), "fp-docker-test-")); + localFile = path.join(tmpBase, "file.txt"); + fs.writeFileSync(localFile, "hi"); + localDir = path.join(tmpBase, "dir"); + fs.mkdirSync(localDir); + }); + + afterEach(() => { + fs.rmSync(tmpBase, { recursive: true, force: true }); + }); it("succeeds when the copied file's ownership matches the configured identity", () => { vi.mocked(execFileSync) @@ -189,7 +212,7 @@ describe("DockerFoundryOrchestrator", () => { .mockReturnValueOnce("") // cp -a .mockReturnValueOnce(`${expectedOwner}\n`); // stat const orchestrator = new DockerFoundryOrchestrator({ version: "1.0.0", containerName: "x" }); - expect(() => orchestrator.copyToContainer("/local/path", "/container/path")).not.toThrow(); + expect(() => orchestrator.copyToContainer(localFile, "/container/path")).not.toThrow(); }); it("throws when the copied file's ownership doesn't match the configured identity", () => { @@ -198,7 +221,29 @@ describe("DockerFoundryOrchestrator", () => { .mockReturnValueOnce("") // cp -a .mockReturnValueOnce("0:0\n"); // stat - unexpectedly root-owned const orchestrator = new DockerFoundryOrchestrator({ version: "1.0.0", containerName: "x" }); - expect(() => orchestrator.copyToContainer("/local/path", "/container/path")).toThrow( + expect(() => orchestrator.copyToContainer(localFile, "/container/path")).toThrow( + /didn't attribute ownership as expected/, + ); + }); + + it("verifies every entry recursively when the copied path is a directory", () => { + vi.mocked(execFileSync) + .mockReturnValueOnce("") // mkdir -p + .mockReturnValueOnce("") // cp -a + .mockReturnValueOnce( + `${expectedOwner} /container/path\n${expectedOwner} /container/path/nested.txt\n`, + ); // find + stat + const orchestrator = new DockerFoundryOrchestrator({ version: "1.0.0", containerName: "x" }); + expect(() => orchestrator.copyToContainer(localDir, "/container/path")).not.toThrow(); + }); + + it("throws when any nested entry's ownership doesn't match, not just the top-level directory", () => { + vi.mocked(execFileSync) + .mockReturnValueOnce("") // mkdir -p + .mockReturnValueOnce("") // cp -a + .mockReturnValueOnce(`${expectedOwner} /container/path\n0:0 /container/path/nested.txt\n`); // find + stat + const orchestrator = new DockerFoundryOrchestrator({ version: "1.0.0", containerName: "x" }); + expect(() => orchestrator.copyToContainer(localDir, "/container/path")).toThrow( /didn't attribute ownership as expected/, ); }); diff --git a/src/docker.ts b/src/docker.ts index 5488455..d52f70f 100644 --- a/src/docker.ts +++ b/src/docker.ts @@ -121,16 +121,20 @@ export class DockerFoundryOrchestrator { const image = `ghcr.io/felddy/foundryvtt:${this.config.version}`; const imageExists = execFileSync("docker", ["images", "-q", image], { encoding: "utf8" }).trim() !== ""; + // A hung pull (bad network/registry issue) would otherwise block silently + // until whatever much longer timeout wraps this whole process - fail + // clearly and quickly instead. + const PULL_TIMEOUT_MS = 5 * 60 * 1000; 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 { console.warn(`[DockerOrchestrator] Failed to update image ${image}, using local version.`); } @@ -290,11 +294,13 @@ export class DockerFoundryOrchestrator { const gid = process.getgid!(); const expectedOwner = `${uid}:${gid}`; - // Ensure destination directory exists via an ephemeral container or exec (if running) - // (docker exec defaults to the same identity getRunCommand() configured - // via --user, so this directory is already owned by that identity.) - // Array-form execFileSync avoids shell interpretation of localPath/ - // containerPath/containerName entirely (no `sh -c`, no metacharacters). + // Creates the destination directory inside the already-running container + // via `docker exec` - this requires the configured container to already + // be running (no ephemeral container is involved). docker exec defaults + // to the same identity getRunCommand() configured via --user, so this + // directory is already owned by that identity. Array-form execFileSync + // avoids shell interpretation of localPath/containerPath/containerName + // entirely (no `sh -c`, no metacharacters). execFileSync( "docker", ["exec", this.config.containerName, "mkdir", "-p", path.dirname(containerPath)], @@ -317,15 +323,42 @@ export class DockerFoundryOrchestrator { // Foundry can't read/write: this process runs docker exec as the // container's own non-root identity (matching getRunCommand()'s // --user), so it has no privilege to chown the file after the fact - // either - there's no fixup to fall back to here. - const actualOwner = execFileSync( - "docker", - ["exec", this.config.containerName, "stat", "-c", "%u:%g", containerPath], - { encoding: "utf8" }, - ).trim(); - if (actualOwner !== expectedOwner) { + // either - there's no fixup to fall back to here. Directory copies are + // verified recursively (every entry, not just the top-level path), + // since a partial-ownership regression on nested content wouldn't be + // caught by only checking containerPath itself. + const mismatches = fs.statSync(localPath).isDirectory() + ? execFileSync( + "docker", + [ + "exec", + this.config.containerName, + "find", + containerPath, + "-exec", + "stat", + "-c", + "%u:%g %n", + "{}", + "+", + ], + { encoding: "utf8" }, + ) + .trim() + .split("\n") + .filter((line) => line.length > 0 && !line.startsWith(`${expectedOwner} `)) + : (() => { + const actualOwner = execFileSync( + "docker", + ["exec", this.config.containerName, "stat", "-c", "%u:%g", containerPath], + { encoding: "utf8" }, + ).trim(); + return actualOwner === expectedOwner ? [] : [`${actualOwner} ${containerPath}`]; + })(); + + if (mismatches.length > 0) { throw new Error( - `[DockerOrchestrator] Copied ${containerPath} is owned by ${actualOwner}, not the expected ${expectedOwner} - archive-mode copy didn't attribute ownership as expected on this Docker/Podman version.`, + `[DockerOrchestrator] Copied path(s) not owned by the expected ${expectedOwner}: ${mismatches.join(", ")} - archive-mode copy didn't attribute ownership as expected on this Docker/Podman version.`, ); } }