lab: a read-only forge account of its own, and no credential left on disk after a stop (hq issue 624, review 196)

The lab's credential came from the package team, which also writes the @novox packages, and the
branches a run clones are not reviewed yet. The lab now contributes {"access": "read"} to its
npm-package-registry requirement, and gitea's provisioner puts such a consumer on a team of its own,
readers: code read on the org's repositories, package read, no write, and off the package team. The
build agent, which publishes, stays on packages.

A stop's SIGTERM to the run's process group killed the wrapper before it removed the credential file.
The run's script now removes it as soon as its clones are done and traps every way it ends, and a run's
start and lab_check remove what a crash or a reboot left.

modules/gitea/client.ts

@@ -905,6 +905,15 @@ export class GiteaAdmin {
     GiteaAdmin.fail(`/teams/${teamId}/members/${username}`, res);
   }
 
+  /** Take a user off a team; one not on it is already off. */
+  async removeUserFromTeam(teamId: number, username: string): Promise<void> {
+    const res = await this.request(`/teams/${teamId}/members/${encodeURIComponent(username)}`, {
+      method: "DELETE",
+    });
+    if (res.status === 204 || res.status === 200 || res.status === 404) return;
+    GiteaAdmin.fail(`/teams/${teamId}/members/${username}`, res);
+  }
+
   /**
    * Whether a consumer's user logs in with exactly this password and is still a member of the
    * package team. Read-only: the password is checked as the consumer presents it, basic auth on the

modules/gitea/package.json

@@ -5,7 +5,7 @@
   "type": "module",
   "private": true,
   "scripts": {
-    "build": "tsc client.ts token.ts index.ts provisioner/index.ts tools/index.ts --module NodeNext --moduleResolution NodeNext --target ES2022 --outDir dist",
+    "build": "tsc client.ts token.ts index.ts provisioner/index.ts provisioner/teams.ts tools/index.ts --module NodeNext --moduleResolution NodeNext --target ES2022 --outDir dist",
     "test": "npm run build && node --test --experimental-strip-types 'test/*.test.ts'"
   },
   "dependencies": {

modules/gitea/provisioner/index.ts

@@ -11,8 +11,8 @@
 //
 // The `npm-package-registry` interface: a consumer authenticates to the npm registry at
 // `/api/packages/novox/npm/` with basic auth, as `as` with the password the mesh minted, and can
-// read and write packages under the `@novox` scope. The registry's npm owner is the gitea org
-// `novox`; a consumer is a gitea *user* placed on that org's package team.
+// read packages under the `@novox` scope, and write them unless it asked for read only (teams.ts). The
+// registry's npm owner is the gitea org `novox`; a consumer is a gitea *user* on one of that org's teams.
 //
 // **The user name and password are the mesh's, not the provisioner's (ADR 0048).** The mesh derives
 // the login and hands it to both ends, and mints the password. gitea creates a user under exactly
@@ -24,11 +24,9 @@
 
 import { runProvisioner, type Provision } from "@novox/mesh-sdk/provisioner";
 import { GiteaAdmin } from "../client.js";
+import { grant, holds } from "./teams.js";
 
-// The npm registry owner: a gitea org named `novox`, whose package team every consumer joins so it
-// can read and write packages under the `@novox` scope (ADR 0076).
-const ORG = "novox";
-const PACKAGE_TEAM = "packages";
+// Which team a consumer joins — `packages` to publish, `readers` to read only — is teams.ts's.
 
 const gitea = GiteaAdmin.fromEnv();
 
@@ -38,19 +36,10 @@ const gitea = GiteaAdmin.fromEnv();
 // module needs the mesh to say where each provision's file is — not yet possible, and not faked.
 runProvisioner("npm-package-registry", {
   async create(p: Provision): Promise<void> {
-    // The org and its package team are the same for every consumer; ensuring them per-create is
-    // idempotent and needs no separate bootstrap step.
-    await gitea.ensureOrg(ORG);
-    const teamId = await gitea.ensureTeam(ORG, PACKAGE_TEAM, true);
-    // The user carries the consumer's login and the mesh's minted password, set every run so a
-    // rotation takes. Membership of the package team is what grants read+write on packages.
-    //
-    // The address is gitea's own convention for one that is not real: its email validation
-    // requires a dotted domain, so `@localhost` was refused at create — the fault that had this
-    // grant retrying for a day — while `@noreply.localhost` is the shape gitea itself gives
-    // hidden addresses.
-    await gitea.ensureUser(p.as, p.password, `${p.as}@noreply.localhost`);
-    await gitea.addUserToTeam(teamId, p.as);
+    // The org and its teams are ensured per create: idempotent, and no separate bootstrap step. The
+    // user carries the consumer's login and the mesh's minted password, set every run so a rotation
+    // takes; membership of its team is what grants it what it may do.
+    await grant(gitea, p);
   },
 
   async remove(p: { as: string }): Promise<void> {
@@ -60,6 +49,6 @@ runProvisioner("npm-package-registry", {
   // Asked every minute by the harness: whether the backend still holds this consumer exactly as
   // the mesh gave it, so a login lost behind the provisioner's back is made again (novox/hq issue 120).
   async holds(p: Provision): Promise<boolean> {
-    return gitea.holdsTeamMember(ORG, PACKAGE_TEAM, p.as, p.password);
+    return holds(gitea, p);
   },
 });

modules/gitea/provisioner/teams.ts

@@ -0,0 +1,64 @@
+// Which team of the forge's npm owner a consumer of `npm-package-registry` joins, and the per-consumer
+// half of the provisioner around it — pure of the forge's address, so it is tested with a stand-in.
+//
+// **Two teams, by what the consumer said it needs** (novox/hq issue 624). A consumer that publishes —
+// the build agent, whose builds publish the SDK and every TypeScript module's packages — joins `packages`:
+// code read on the org's repositories and write on its packages. A consumer that contributes
+// `{"access": "read"}` joins `readers`: code read and package read, and no write. The lab is the first: it
+// clones branches that are not yet reviewed and runs their code beside its credential, and code that is
+// not reviewed must not be able to publish the mesh's packages with it.
+
+/** The npm registry's owner: the gitea org every consumer's packages live under (ADR 0076). */
+export const ORG = "novox";
+/** The team whose members read and write the org's packages. */
+export const PACKAGE_TEAM = "packages";
+/** The team whose members read the org's code and packages, and write nothing. */
+export const READ_TEAM = "readers";
+
+/** The team a consumer joins, from what it contributed; an access it may not ask for is refused. */
+export function teamFor(values: Readonly<Record<string, unknown>> | undefined): { team: string; packageWrite: boolean } {
+  const access = values?.access;
+  if (access === undefined || access === "write") return { team: PACKAGE_TEAM, packageWrite: true };
+  if (access === "read") return { team: READ_TEAM, packageWrite: false };
+  throw new Error(`npm-package-registry: access ${JSON.stringify(access)} is not "read" or "write"`);
+}
+
+/** What the adapter asks of the forge's admin API (client.ts GiteaAdmin). */
+export interface Forge {
+  ensureOrg(org: string): Promise<void>;
+  ensureTeam(org: string, team: string, packageWrite: boolean): Promise<number>;
+  ensureUser(username: string, password: string, email: string): Promise<void>;
+  addUserToTeam(teamId: number, username: string): Promise<void>;
+  removeUserFromTeam(teamId: number, username: string): Promise<void>;
+  holdsTeamMember(org: string, team: string, username: string, password: string): Promise<boolean>;
+}
+
+export interface Consumer {
+  readonly as: string;
+  readonly password: string;
+  readonly values: Readonly<Record<string, unknown>>;
+}
+
+/**
+ * grant makes the consumer's user, with exactly the mesh's password, a member of its one team. A consumer
+ * that asks for read only is also taken out of the package team, so one that asked for write before and
+ * read now loses the write at once.
+ */
+export async function grant(forge: Forge, p: Consumer): Promise<void> {
+  const { team, packageWrite } = teamFor(p.values);
+  await forge.ensureOrg(ORG);
+  const teamId = await forge.ensureTeam(ORG, team, packageWrite);
+  // The address is gitea's own convention for one that is not real: its email validation requires a
+  // dotted domain, so `@localhost` was refused at create, while `@noreply.localhost` is the shape gitea
+  // itself gives hidden addresses.
+  await forge.ensureUser(p.as, p.password, `${p.as}@noreply.localhost`);
+  await forge.addUserToTeam(teamId, p.as);
+  if (!packageWrite) {
+    await forge.removeUserFromTeam(await forge.ensureTeam(ORG, PACKAGE_TEAM, true), p.as);
+  }
+}
+
+/** holds is whether the forge still holds the consumer as the mesh gave it: its login, in its team. */
+export async function holds(forge: Forge, p: Consumer): Promise<boolean> {
+  return forge.holdsTeamMember(ORG, teamFor(p.values).team, p.as, p.password);
+}

modules/gitea/test/teams.test.ts

@@ -0,0 +1,56 @@
+import { test } from "node:test";
+import assert from "node:assert/strict";
+
+import { grant, holds, PACKAGE_TEAM, READ_TEAM, teamFor, type Forge } from "../provisioner/teams.ts";
+
+// Defends novox/hq issue 624: the lab reads the forge with a credential of its own that can write no
+// package. A consumer that contributes {"access": "read"} joins a team with code and package read only.
+
+function aForge(): { forge: Forge; did: string[] } {
+  const did: string[] = [];
+  const teams: Record<string, number> = {};
+  const forge: Forge = {
+    async ensureOrg(org) { did.push(`org ${org}`); },
+    async ensureTeam(org, team, packageWrite) {
+      did.push(`team ${org}/${team} packages:${packageWrite ? "write" : "read"}`);
+      return (teams[team] ??= Object.keys(teams).length + 1);
+    },
+    async ensureUser(username) { did.push(`user ${username}`); },
+    async addUserToTeam(id, username) { did.push(`add ${username} to ${id}`); },
+    async removeUserFromTeam(id, username) { did.push(`remove ${username} from ${id}`); },
+    async holdsTeamMember(org, team, username) { did.push(`holds ${username} in ${org}/${team}`); return true; },
+  };
+  return { forge, did };
+}
+
+test("a consumer that asks nothing, or write, publishes: the package team", () => {
+  assert.deepEqual(teamFor({}), { team: PACKAGE_TEAM, packageWrite: true });
+  assert.deepEqual(teamFor({ access: "write" }), { team: PACKAGE_TEAM, packageWrite: true });
+});
+
+test("a consumer that asks to read joins the readers, who write no package", () => {
+  assert.deepEqual(teamFor({ access: "read" }), { team: READ_TEAM, packageWrite: false });
+  assert.throws(() => teamFor({ access: "admin" }), /not "read" or "write"/);
+});
+
+test("a reader is made, put on the readers team, and taken off the package team", async () => {
+  const { forge, did } = aForge();
+  await grant(forge, { as: "lab-shanks", password: "pw", values: { access: "read" } });
+  assert.deepEqual(did, [
+    "org novox",
+    "team novox/readers packages:read",
+    "user lab-shanks",
+    "add lab-shanks to 1",
+    "team novox/packages packages:write",
+    "remove lab-shanks from 2",
+  ]);
+  did.length = 0;
+  await holds(forge, { as: "lab-shanks", password: "pw", values: { access: "read" } });
+  assert.deepEqual(did, ["holds lab-shanks in novox/readers"]);
+});
+
+test("a publisher is made and put on the package team, as before", async () => {
+  const { forge, did } = aForge();
+  await grant(forge, { as: "build-agent-ace", password: "pw", values: {} });
+  assert.deepEqual(did, ["org novox", "team novox/packages packages:write", "user build-agent-ace", "add build-agent-ace to 1"]);
+});

modules/gitea/tsconfig.json

@@ -8,5 +8,5 @@
     "skipLibCheck": true,
     "noEmit": true
   },
-  "include": ["client.ts", "token.ts", "pulls.ts", "protection.ts", "index.ts", "provisioner/index.ts", "tools/index.ts"]
+  "include": ["client.ts", "token.ts", "pulls.ts", "protection.ts", "index.ts", "provisioner/index.ts", "provisioner/teams.ts", "tools/index.ts"]
 }

modules/lab/module.json

@@ -8,6 +8,11 @@
   "requires": [
     "npm-package-registry"
   ],
+  "contributes": {
+    "npm-package-registry": {
+      "access": "read"
+    }
+  },
   "binds": {
     "npm-package-registry": "${dir:state}/package-registry.json"
   },

modules/lab/test/runs.test.ts

@@ -1,6 +1,6 @@
 import { test } from "node:test";
 import assert from "node:assert/strict";
-import { chmodSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs";
+import { chmodSync, existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs";
 import { spawnSync } from "node:child_process";
 import { tmpdir } from "node:os";
 import { join } from "node:path";
@@ -149,3 +149,48 @@ test("git presents the stored credential to the forge, and only when one is give
   assert.doesNotMatch(other.stdout, /password=pw/);
   rmSync(dir, { recursive: true, force: true });
 });
+
+// Review 196 on mesh-catalog #272: the credential file outlived a stop, because SIGTERM to the run's
+// process group killed the wrapper before its cleanup. It is now removed right after the clones, on
+// every way the script ends, and at the next run's start and in lab_check for what a crash left.
+import { cleanCredentials, credentialGuard } from "../tools/runs.ts";
+
+test("a stopped script removes the credential, whatever ends it", () => {
+  for (const signal of ["SIGTERM", "SIGINT", "SIGHUP"] as const) {
+    const dir = mkdtempSync(join(tmpdir(), "lab-guard-"));
+    const file = join(dir, ".run-x.git-credentials");
+    writeFileSync(file, "http://u:p@h\n", { mode: 0o600 });
+    const ran = spawnSync("bash", ["-c", `${credentialGuard(file)}\nkill -${signal.slice(3)} $$\nsleep 5`], { encoding: "utf8", timeout: 10_000 });
+    assert.notEqual(ran.status, 0, `${signal}: the script was not stopped`);
+    assert.equal(existsSync(file), false, `${signal} left the credential on disk`);
+    rmSync(dir, { recursive: true, force: true });
+  }
+});
+
+test("the script removes the credential as soon as its clones are done", () => {
+  const s = script("/work", "run-x", "http://forge", ["t.test.ts"], {}, "/work/.run-x.git-credentials");
+  const lastClone = s.lastIndexOf("clone --quiet");
+  const removed = s.indexOf("rm -f '/work/.run-x.git-credentials'", lastClone);
+  const building = s.indexOf("npm ci");
+  assert.ok(lastClone > 0 && removed > lastClone && removed < building, "not removed between the clones and the build");
+  assert.match(s, /trap 'rm -f '\\''\/work\/\.run-x\.git-credentials'\\''' EXIT/);
+});
+
+test("credentials a crashed run left are removed, but not the running one's", () => {
+  const work = mkdtempSync(join(tmpdir(), "lab-work-"));
+  for (const n of [".run-a.git-credentials", ".run-b.git-credentials", "run-a", "other"]) writeFileSync(join(work, n), "x");
+  const removed = cleanCredentials(work, join(work, ".run-b.git-credentials"));
+  assert.deepEqual(removed, [join(work, ".run-a.git-credentials")]);
+  assert.ok(existsSync(join(work, ".run-b.git-credentials")) && existsSync(join(work, "run-a")) && existsSync(join(work, "other")));
+  rmSync(work, { recursive: true, force: true });
+});
+
+// The lab asks the forge for read only (review 196): its credential is for clones of branches nobody has
+// reviewed yet, and must not be able to publish the mesh's packages.
+test("the lab asks the package registry for read only", () => {
+  const m = JSON.parse(readFileSync(join(import.meta.dirname, "..", "module.json"), "utf8")) as {
+    requires: unknown[]; contributes?: Record<string, Record<string, unknown>>;
+  };
+  assert.ok(m.requires.includes("npm-package-registry"));
+  assert.equal(m.contributes?.["npm-package-registry"]?.access, "read");
+});

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 {
-  credentialArgs, forgeCredential, installPlan, listRuns, NPM_REPOSITORIES, readStatus, REPOSITORIES, running, start,
+  cleanCredentials, credentialArgs, forgeCredential, installPlan, listRuns, NPM_REPOSITORIES, readStatus, REPOSITORIES, running, start,
   stop, tail, writeCredential,
 } from "./runs.js";
 
@@ -39,8 +39,13 @@ export function getLabTools(env: NodeJS.ProcessEnv): ToolDefinition[] {
         // **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.
+        // A credential a stopped, crashed or rebooted run left behind is removed, and said; one still in use
+        // 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 output = [`${check.stdout}${check.stderr}`.trim(), ...problems].filter(Boolean).join("\n");
+        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");
         return { ok: check.status === 0 && problems.length === 0, output };
       },
     },

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, writeFileSync } from "node:fs";
+import { existsSync, mkdirSync, readFileSync, readdirSync, rmSync, writeFileSync } from "node:fs";
 import { join } from "node:path";
 
 /**
@@ -106,6 +106,37 @@ export function writeCredential(path: string, line: string | undefined): string
   return path;
 }
 
+/**
+ * credentialGuard is the shell that removes the credential however the script ends — its end, an error,
+ * or a stop's SIGTERM to the run's process group, which kills the wrapper before its own cleanup runs
+ * (review 196 on mesh-catalog #272). The script also removes it as soon as its clones are done.
+ */
+export function credentialGuard(path: string | undefined): string {
+  if (!path) return "";
+  const rm = `rm -f ${shellQuote(path)}`;
+  return `trap ${shellQuote(rm)} EXIT
+trap ${shellQuote(`${rm}; exit 143`)} TERM
+trap ${shellQuote(`${rm}; exit 130`)} INT
+trap ${shellQuote(`${rm}; exit 129`)} HUP`;
+}
+
+/**
+ * cleanCredentials removes every run's credential file left in the work directory but the one named — what
+ * a crash or a reboot left behind, which no trap could remove. Answers what it removed.
+ */
+export function cleanCredentials(work: string, keep?: string): string[] {
+  if (!existsSync(work)) return [];
+  const removed: string[] = [];
+  for (const name of readdirSync(work)) {
+    if (!/^\.run-.*\.git-credentials$/.test(name)) continue;
+    const path = join(work, name);
+    if (path === keep) continue;
+    rmSync(path, { force: true });
+    removed.push(path);
+  }
+  return removed;
+}
+
 /** 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 "";
@@ -195,9 +226,11 @@ export function script(work: string, id: string, forge: string, tests: string[],
   return `set -euo pipefail
 # A repository the forge will not show fails its clone at once, rather than waiting on a prompt.
 export GIT_TERMINAL_PROMPT=0
+${credentialGuard(credential)}
 cd ${shellQuote(dir)}
 ${setState("checking-out")}
 ${clones}
+${credential ? `# The forge credential is for the clones, and nothing after them runs with it on disk.\nrm -f ${shellQuote(credential)}` : ""}
 node -e ${shellQuote(
     `const fs=require("fs");const f=${JSON.stringify(join(dir, "status.json"))};const s=JSON.parse(fs.readFileSync(f,"utf8"));s.commits=Object.fromEntries(fs.readFileSync(${JSON.stringify(join(dir, "commits.txt"))},"utf8").trim().split("\\n").map(l=>l.split(" ")));fs.writeFileSync(f,JSON.stringify(s,null,2))`,
   )}
@@ -236,6 +269,8 @@ export function start(work: string, forge: string, tests: string[], refs: Record
   mkdirSync(dir, { recursive: true });
   const status: RunStatus = { id, state: "checking-out", started: new Date().toISOString(), tests, refs };
   writeFileSync(statusPath(work, id), JSON.stringify(status, null, 2));
+  // What a crashed or rebooted run left behind goes before this run writes its own.
+  cleanCredentials(work);
   const credentials = writeCredential(credentialPath(work, id), credential);
   writeFileSync(join(dir, "run.sh"), script(work, id, forge, tests, refs, credentials), { mode: 0o700 });