From 5bd40fa4061d9aa7fe619f6641c35993478ab9cf Mon Sep 17 00:00:00 2001 From: jochen Date: Tue, 13 Oct 2026 14:40:06 +0200 Subject: [PATCH 1/2] lab_check: work in a fresh directory each time, and name what an earlier check left (hq issue 624) lab_check cloned into one fixed path, /check, after an rm -rf whose result it ignored; a tree an earlier check left that the lab's account could not remove made every check fail on "destination path already exists". Each check now works in a fresh check-* directory it removes when done, and clears what earlier checks left, naming any path it could not remove. --- modules/lab/test/runs.test.ts | 32 ++++++++++++++++++++++++++++++++ modules/lab/tools/index.ts | 26 +++++++++++++++++--------- modules/lab/tools/runs.ts | 33 ++++++++++++++++++++++++++++++++- 3 files changed, 81 insertions(+), 10 deletions(-) diff --git a/modules/lab/test/runs.test.ts b/modules/lab/test/runs.test.ts index 816d1baa..441af105 100644 --- a/modules/lab/test/runs.test.ts +++ b/modules/lab/test/runs.test.ts @@ -194,3 +194,35 @@ test("the lab asks the package registry for read only", () => { assert.ok(m.requires.includes("npm-package-registry")); assert.equal(m.contributes?.["npm-package-registry"]?.access, "read"); }); + +// lab_check failed for ever on "destination path '/check' already exists": it cloned into one fixed +// path and ignored whether removing the last one worked (novox/hq issue 624). Each check now works in a +// fresh directory, and what earlier checks left is cleared or named. +import { checkDir, clearChecks } from "../tools/runs.ts"; + +test("each check gets a fresh directory of its own", () => { + const work = mkdtempSync(join(tmpdir(), "lab-work-")); + const a = checkDir(work); + const b = checkDir(work); + assert.notEqual(a, b); + assert.ok(a.startsWith(join(work, "check-")) && existsSync(a) && existsSync(b)); + rmSync(work, { recursive: true, force: true }); +}); + +test("what earlier checks left is cleared, and what cannot be is named rather than tripped over", () => { + const work = mkdtempSync(join(tmpdir(), "lab-work-")); + for (const d of ["check/src", "check-setup/mesh-lab", "check-abc", "run-x", "keep"]) mkdirSync(join(work, d), { recursive: true }); + // A tree this account cannot empty: a directory it may not write, with something in it. + mkdirSync(join(work, "check-stuck", "locked"), { recursive: true }); + writeFileSync(join(work, "check-stuck", "locked", "file"), "x"); + chmodSync(join(work, "check-stuck", "locked"), 0o555); + const said = clearChecks(work); + chmodSync(join(work, "check-stuck", "locked"), 0o755); + for (const gone of ["check", "check-setup", "check-abc"]) assert.equal(existsSync(join(work, gone)), false, gone); + assert.ok(existsSync(join(work, "run-x")) && existsSync(join(work, "keep"))); + if (process.getuid?.() !== 0) { + assert.equal(said.length, 1); + assert.match(said[0] as string, /check-stuck, left by an earlier check, could not be removed/); + } + rmSync(work, { recursive: true, force: true }); +}); diff --git a/modules/lab/tools/index.ts b/modules/lab/tools/index.ts index ce665cbf..d13d5cc7 100644 --- a/modules/lab/tools/index.ts +++ b/modules/lab/tools/index.ts @@ -8,7 +8,7 @@ import { mkdtempSync, rmSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { - cleanCredentials, credentialArgs, forgeCredential, installPlan, listRuns, NPM_REPOSITORIES, readStatus, REPOSITORIES, running, start, + checkDir, cleanCredentials, clearChecks, credentialArgs, forgeCredential, installPlan, listRuns, NPM_REPOSITORIES, readStatus, REPOSITORIES, running, start, stop, tail, writeCredential, } from "./runs.js"; @@ -30,12 +30,21 @@ export function getLabTools(env: NodeJS.ProcessEnv): ToolDefinition[] { run: async () => { const forge = forgeOf(); if (!forge) return { ok: false, output: "the lab's forge is not set: settings for lab, {\"forge\": \"\"}" }; - const dir = `${work}/check`; - spawnSync("rm", ["-rf", dir]); - const clone = spawnSync("git", ["clone", "--quiet", "--depth", "1", `${forge}/novox/mesh-lab.git`, dir], { encoding: "utf8" }); - if (clone.status !== 0) return { ok: false, output: clone.stderr }; - spawnSync("npm", ["ci", "--no-audit", "--no-fund", "--loglevel=error"], { cwd: dir, encoding: "utf8" }); - const check = spawnSync("node", ["--experimental-strip-types", "src/cli.ts", "check"], { cwd: dir, encoding: "utf8" }); + // What earlier checks left is cleared, and what could not be is said; this check works in a fresh + // directory of its own either way, and removes it when it is done. + const leftovers = clearChecks(work); + const dir = checkDir(work); + let check: ReturnType; + let problems: string[]; + try { + const clone = spawnSync("git", ["clone", "--quiet", "--depth", "1", `${forge}/novox/mesh-lab.git`, dir], { encoding: "utf8" }); + if (clone.status !== 0) return { ok: false, output: [String(clone.stderr).trim(), ...leftovers].join("\n") }; + spawnSync("npm", ["ci", "--no-audit", "--no-fund", "--loglevel=error"], { cwd: dir, encoding: "utf8" }); + check = spawnSync("node", ["--experimental-strip-types", "src/cli.ts", "check"], { cwd: dir, encoding: "utf8" }); + problems = setupProblems(forge, `${dir}/setup`, credentialOf(forge)); + } finally { + rmSync(dir, { recursive: true, force: true }); + } // **And what a run would check out and install, before a run finds out** (novox/hq issue 624): every // repository it clones answers on the forge, and every one it installs has a lock file where the run // installs from. @@ -43,9 +52,8 @@ export function getLabTools(env: NodeJS.ProcessEnv): ToolDefinition[] { // by a running run is not. const busy = running(work); const left = cleanCredentials(work, busy ? `${work}/.${busy.id}.git-credentials` : undefined); - const problems = setupProblems(forge, `${work}/check-setup`, credentialOf(forge)); const removed = left.length > 0 ? `removed ${left.length} forge credential(s) a run left behind: ${left.join(", ")}` : ""; - const output = [`${check.stdout}${check.stderr}`.trim(), removed, ...problems].filter(Boolean).join("\n"); + const output = [`${check.stdout}${check.stderr}`.trim(), removed, ...leftovers, ...problems].filter(Boolean).join("\n"); return { ok: check.status === 0 && problems.length === 0, output }; }, }, diff --git a/modules/lab/tools/runs.ts b/modules/lab/tools/runs.ts index debc0bc0..9d06f18f 100644 --- a/modules/lab/tools/runs.ts +++ b/modules/lab/tools/runs.ts @@ -7,7 +7,7 @@ // for it, and says it was lost rather than pretending it is still going. import { spawn } from "node:child_process"; -import { existsSync, mkdirSync, readFileSync, readdirSync, rmSync, writeFileSync } from "node:fs"; +import { existsSync, mkdirSync, mkdtempSync, readFileSync, readdirSync, rmSync, writeFileSync } from "node:fs"; import { join } from "node:path"; /** @@ -137,6 +137,37 @@ export function cleanCredentials(work: string, keep?: string): string[] { return removed; } +/** + * checkDir is a directory of lab_check's own for one check, fresh each time (novox/hq issue 624). The check + * cloned into the one fixed path `/check`, removing it first and ignoring whether that worked; a tree + * an earlier check left that this account could not remove — one cloned as another account before the + * work directory was the operator's — made every check fail on "destination path already exists". + */ +export function checkDir(work: string): string { + mkdirSync(work, { recursive: true }); + return mkdtempSync(join(work, "check-")); +} + +/** + * clearChecks removes what earlier checks left in the work directory — the fixed `check` and `check-setup` + * of before, and any `check-*` — and says, for each it could not remove, which and why, so a check reports + * it rather than tripping over it. + */ +export function clearChecks(work: string): string[] { + if (!existsSync(work)) return []; + const said: string[] = []; + for (const name of readdirSync(work)) { + if (name !== "check" && name !== "check-setup" && !name.startsWith("check-")) continue; + const path = join(work, name); + try { + rmSync(path, { recursive: true, force: true }); + } catch (err) { + said.push(`${path}, left by an earlier check, could not be removed: ${(err as Error).message}`); + } + } + return said; +} + /** The arguments that have git offer the credential in that file, and only that one. */ export function credentialArgs(path: string | undefined): string { if (!path) return ""; From dd39d4ace74b6a592cfbc726db297e2b7bf42d37 Mon Sep 17 00:00:00 2001 From: jochen Date: Tue, 13 Oct 2026 14:51:18 +0200 Subject: [PATCH 2/2] lab: name the bound credential in the tools bundle's env, and skip the locked-tree test as root (hq issue 624, reviews 198, 199) The lab's tools read their bound npm-package-registry credential, root's at 0600 on the machine. The controller gives the runtime's account exactly the files a tools bundle names in its env (ADR 0192), as gitea and umami are given theirs, so the bundle now names the credential and its binding there, and no other credential changes hands. The merge check runs as root, where no directory mode stops a removal: the test that makes an unremovable tree now skips there before it makes anything, and a second test clears leftovers whoever runs it. --- modules/lab/module.json | 4 +++- modules/lab/test/runs.test.ts | 35 ++++++++++++++++++++++++++++++----- 2 files changed, 33 insertions(+), 6 deletions(-) diff --git a/modules/lab/module.json b/modules/lab/module.json index 5406579d..c9c14770 100644 --- a/modules/lab/module.json +++ b/modules/lab/module.json @@ -104,7 +104,9 @@ ], "env": { "MESH_LAB_WORK": "${dir:work}", - "MESH_LAB_ENV_FILE": "${dir:state}/lab.env" + "MESH_LAB_ENV_FILE": "${dir:state}/lab.env", + "MESH_LAB_FORGE_BINDING": "${dir:state}/package-registry.json", + "MESH_LAB_FORGE_SECRET": "${dir:state}/package-registry.secret" } } ] diff --git a/modules/lab/test/runs.test.ts b/modules/lab/test/runs.test.ts index 441af105..f87bf3d2 100644 --- a/modules/lab/test/runs.test.ts +++ b/modules/lab/test/runs.test.ts @@ -209,7 +209,11 @@ test("each check gets a fresh directory of its own", () => { rmSync(work, { recursive: true, force: true }); }); -test("what earlier checks left is cleared, and what cannot be is named rather than tripped over", () => { +// As root nothing is locked against removal, so the unremovable tree cannot be made: skipped there, before +// it makes anything (the merge check runs as root). +test("what earlier checks left is cleared, and what cannot be is named rather than tripped over", { + skip: process.getuid?.() === 0 ? "running as root, which no directory mode stops from removing anything" : false, +}, () => { const work = mkdtempSync(join(tmpdir(), "lab-work-")); for (const d of ["check/src", "check-setup/mesh-lab", "check-abc", "run-x", "keep"]) mkdirSync(join(work, d), { recursive: true }); // A tree this account cannot empty: a directory it may not write, with something in it. @@ -220,9 +224,30 @@ test("what earlier checks left is cleared, and what cannot be is named rather th chmodSync(join(work, "check-stuck", "locked"), 0o755); for (const gone of ["check", "check-setup", "check-abc"]) assert.equal(existsSync(join(work, gone)), false, gone); assert.ok(existsSync(join(work, "run-x")) && existsSync(join(work, "keep"))); - if (process.getuid?.() !== 0) { - assert.equal(said.length, 1); - assert.match(said[0] as string, /check-stuck, left by an earlier check, could not be removed/); - } + assert.equal(said.length, 1); + assert.match(said[0] as string, /check-stuck, left by an earlier check, could not be removed/); rmSync(work, { recursive: true, force: true }); }); + +// And as root too: what earlier checks left is cleared. +test("leftover checks are cleared, whoever runs the check", () => { + const work = mkdtempSync(join(tmpdir(), "lab-work-")); + for (const d of ["check/src", "check-setup/mesh-lab", "check-abc", "run-x"]) mkdirSync(join(work, d), { recursive: true }); + assert.deepEqual(clearChecks(work), []); + for (const gone of ["check", "check-setup", "check-abc"]) assert.equal(existsSync(join(work, gone)), false, gone); + assert.ok(existsSync(join(work, "run-x"))); + rmSync(work, { recursive: true, force: true }); +}); + +// The lab's tools read the bound credential, so the runtime is given that file by the tools bundle's env +// (ADR 0192), as gitea and umami are given theirs: the runtime's account may then read exactly it, and no +// other credential changes hands (issue 624). +test("the lab's tools bundle names its bound credential and binding in its env", () => { + const m = JSON.parse(readFileSync(join(import.meta.dirname, "..", "module.json"), "utf8")) as { + secrets: Record; binds: Record; + build: { artifacts: { name: string; env?: Record }[] }; + }; + const env = m.build.artifacts.find((a) => a.name === "code")?.env ?? {}; + assert.equal(env.MESH_LAB_FORGE_SECRET, m.secrets["npm-package-registry"]); + assert.equal(env.MESH_LAB_FORGE_BINDING, m.binds["npm-package-registry"]); +});