refactor(worker): share the platform browser opener; add the graph artifact dir (issue #54)

admin/src/read-model.mjs

@@ -18,7 +18,7 @@
 import * as nodeFs from "node:fs";
 import { join, delimiter, sep } from "node:path";
 import { execFileSync } from "node:child_process";
-import { defaultLogsDir, defaultSandboxDir, CHAIN_DEPTH_MAX_DEFAULT, CHAIN_MAX_PER_JOB_DEFAULT } from "@edgehero/pi-dispatch/config";
+import { defaultLogsDir, defaultSandboxDir, defaultGraphDir, CHAIN_DEPTH_MAX_DEFAULT, CHAIN_MAX_PER_JOB_DEFAULT } from "@edgehero/pi-dispatch/config";
 import { settingsFilePath, readOverlay, writeOverlay, KNOWN_KEYS } from "@edgehero/pi-dispatch/runtime-settings";
 import { sanitizeJobId } from "@edgehero/pi-dispatch/run-history";
 import { dayKey, weekKey, monthKey } from "@edgehero/pi-dispatch/budget";
@@ -99,6 +99,11 @@ export function resolvePaths(env = process.env) {
     // GRAPH view exists to remove.
     chainDepthMax: parseNonNegInt(env.PI_CHAIN_DEPTH_MAX, CHAIN_DEPTH_MAX_DEFAULT),
     chainMaxPerJob: parseNonNegInt(env.PI_CHAIN_MAX_PER_JOB, CHAIN_MAX_PER_JOB_DEFAULT),
+    // Where the graph HTML artifact lands (issue #54): the worker's own temp-dir default, imported
+    // like defaultLogsDir/defaultSandboxDir above, so the admin and any future worker consumer agree
+    // on the path without loadConfig. Deliberately NOT logsDir -- that directory's filename shape is
+    // contract (INT-RUN-HISTORY-FILE-CONTRACT).
+    graphDir: env.PI_GRAPH_DIR || defaultGraphDir(env),
   };
 }
 

admin/test/fixtures/worker-import-probe.ts

@@ -5,11 +5,14 @@ import { settingsFilePath } from "@edgehero/pi-dispatch/runtime-settings";
 // test, rather than at the first /dispatch graph in a bundled install.
 import { selectEntries, keepOnlyDeclaredSkills } from "@edgehero/pi-dispatch/materialize";
 import { aiTriggerAllows } from "@edgehero/pi-dispatch/flow-gate";
-import { CHAIN_DEPTH_MAX_DEFAULT } from "@edgehero/pi-dispatch/config";
+import { CHAIN_DEPTH_MAX_DEFAULT, defaultGraphDir } from "@edgehero/pi-dispatch/config";
+import { openBrowser } from "@edgehero/pi-dispatch/open-browser";
 
 export const ok =
   typeof settingsFilePath === "function" &&
   typeof selectEntries === "function" &&
   typeof keepOnlyDeclaredSkills === "function" &&
   typeof aiTriggerAllows === "function" &&
+  typeof defaultGraphDir === "function" &&
+  typeof openBrowser === "function" &&
   Number.isInteger(CHAIN_DEPTH_MAX_DEFAULT);

admin/test/read-model.test.mjs

@@ -146,6 +146,8 @@ test("resolvePaths reads env with safe defaults and never calls loadConfig", ()
     // Pinned so the sandbox default does not drag the OS temp dir into this equality
     // (REQ-RESURRECTABLE-SANDBOX); the defaulting itself is asserted in its own test below.
     PI_SANDBOX_DIR: "/sbx",
+    // Pinned for the same temp-dir reason; the default is asserted in its own test below.
+    PI_GRAPH_DIR: "/g",
   });
   assert.deepEqual(p, {
     valkeyUrl: "redis://h:1",
@@ -163,11 +165,18 @@ test("resolvePaths reads env with safe defaults and never calls loadConfig", ()
     schedulerStallMax: 2,
     chainDepthMax: 1,
     chainMaxPerJob: 2,
+    graphDir: "/g",
     pauseWindowsPath: "./pause-windows.json",
     subscriptionsPath: "/subs.json",
   });
 });
 
+test("resolvePaths resolves the graph dir from PI_GRAPH_DIR with the worker's temp default", () => {
+  assert.equal(resolvePaths({ PI_GRAPH_DIR: "/x/graphs" }).graphDir, "/x/graphs");
+  assert.ok(resolvePaths({}).graphDir.endsWith("/pi-dispatch/graph"), "the default is the worker-owned temp path, never cwd");
+  assert.ok(resolvePaths({ PI_GRAPH_DIR: "" }).graphDir.endsWith("/pi-dispatch/graph"), "empty falls back like every other path here");
+});
+
 test("resolvePaths falls back to defaults on empty env (no worker config required)", () => {
   const p = resolvePaths({});
   assert.equal(p.valkeyUrl, "redis://127.0.0.1:6379");

worker/package.json

@@ -45,6 +45,7 @@
     "./exit-code": "./src/exit-code.mjs",
     "./flow-gate": "./src/flow-gate.mjs",
     "./materialize": "./src/materialize.mjs",
+    "./open-browser": "./src/open-browser.mjs",
     "./git-dirty": "./src/git-dirty.mjs",
     "./queue": "./src/queue.mjs",
     "./connection": "./src/connection.mjs",

worker/src/config.mjs

@@ -269,6 +269,16 @@ export function defaultLogsDir() {
 	return `${process.env.TMPDIR ?? process.env.TEMP ?? "/tmp"}/pi-dispatch/logs`.replace(/\\/g, "/");
 }
 
+export function defaultGraphDir(env = process.env) {
+	// Under the OS temp dir by default, beside logs/ and jobs/ -- the admin's graph HTML artifact
+	// (issue #54) is host-side display output on the defaultLogsDir doctrine, and deliberately NOT
+	// inside logsDir: INT-RUN-HISTORY-FILE-CONTRACT names that directory's filename shape, and a
+	// stray .html beside the sidecars would widen a contract for a file that is not a record.
+	// Overridable with PI_GRAPH_DIR; exported so the admin resolves the same default without
+	// loadConfig, like defaultSandboxDir above.
+	return `${env.TMPDIR ?? env.TEMP ?? "/tmp"}/pi-dispatch/graph`.replace(/\\/g, "/");
+}
+
 export function defaultSettingsFile() {
 	// Under the OS temp dir by default. Holds the runtime-tunable settings overlay shared with the admin
 	// extension (INT-CONFIG-OVERLAY-CONTRACT); a worker-owned path that never enters the container env

worker/src/github-app-setup.mjs

@@ -32,13 +32,13 @@
  * Everything side-effecting is injected (fetch, the listener, the browser opener, prompt, fs, clock),
  * defaulting to the real thing — the up.mjs convention — so the whole flow is testable offline.
  */
-import { spawn as nodeSpawn } from "node:child_process";
 import { createSign } from "node:crypto";
 import { chmodSync, existsSync, readFileSync, renameSync, statSync, writeFileSync } from "node:fs";
 import { hostname } from "node:os";
 import { join } from "node:path";
 import { parseArgs } from "node:util";
 import { updateEnvFile } from "./env-file.mjs";
+import { openBrowser as defaultOpenBrowser } from "./open-browser.mjs";
 import { defaultPrompt } from "./up.mjs";
 
 const API_ROOT = "https://api.github.com";
@@ -503,20 +503,6 @@ async function defaultListen(pageFor) {
 	};
 }
 
-/**
- * Best-effort platform browser opener. ALWAYS paired with the URL printed to the terminal — a
- * headless or SSH'd operator has no opener that works, and the printed URL pasted into any browser
- * (on the right machine, or through the port-forward the wizard suggests) is the real contract; the
- * spawn is only a convenience on top. Failures are swallowed for the same reason.
- */
-function defaultOpenBrowser(url) {
-	const [cmd, args] =
-		process.platform === "darwin" ? ["open", [url]] : process.platform === "win32" ? ["cmd", ["/c", "start", "", url]] : ["xdg-open", [url]];
-	try {
-		const child = nodeSpawn(cmd, args, { stdio: "ignore", detached: true });
-		child.on("error", () => {});
-		child.unref();
-	} catch {
-		// No opener on this host — the printed URL carries the flow.
-	}
-}
+// The best-effort platform opener moved to its own module (issue #54) so the admin's graph export
+// shares this one reviewed argv table instead of hand-copying it; the print-the-URL-first doctrine
+// lives in its docstring and is unchanged here.

worker/src/open-browser.mjs

@@ -0,0 +1,24 @@
+import { spawn as nodeSpawn } from "node:child_process";
+
+/**
+ * Best-effort platform browser opener. ALWAYS paired with the URL printed to the terminal -- a
+ * headless or SSH'd operator has no opener that works, and the printed URL pasted into any browser
+ * (on the right machine, or through the port-forward the caller suggests) is the real contract; the
+ * spawn is only a convenience on top. Failures are swallowed for the same reason.
+ *
+ * One module on purpose (issue #54): the GitHub App wizard and the admin's graph export both open a
+ * browser, and two hand-copies of platform-opener argv is exactly the drift class the repo's mirror
+ * tests exist to prevent. `spawn` and `platform` are injectable so the argv table is testable
+ * without launching anything.
+ */
+export function openBrowser(url, { spawn = nodeSpawn, platform = process.platform } = {}) {
+	const [cmd, args] =
+		platform === "darwin" ? ["open", [url]] : platform === "win32" ? ["cmd", ["/c", "start", "", url]] : ["xdg-open", [url]];
+	try {
+		const child = spawn(cmd, args, { stdio: "ignore", detached: true });
+		child.on("error", () => {});
+		child.unref();
+	} catch {
+		// No opener on this host -- the printed URL carries the flow.
+	}
+}

worker/test/config.test.mjs

@@ -1,7 +1,7 @@
 import assert from "node:assert/strict";
 import { delimiter } from "node:path";
 import { test } from "node:test";
-import { CHAIN_DEPTH_MAX_DEFAULT, CHAIN_MAX_PER_JOB_DEFAULT, configError, globalExtensionsEnabled, loadConfig, loadGitLabAuth } from "../src/config.mjs";
+import { CHAIN_DEPTH_MAX_DEFAULT, CHAIN_MAX_PER_JOB_DEFAULT, configError, defaultGraphDir, globalExtensionsEnabled, loadConfig, loadGitLabAuth } from "../src/config.mjs";
 import { FORGES, FORGE_KINDS } from "../src/forges.mjs";
 
 test("loads conservative defaults with an empty-ish env", () => {
@@ -34,6 +34,15 @@ test("AI-trigger / chaining knobs default conservatively", () => {
 	assert.deepEqual(c.dispatchRunRoots, []);
 });
 
+test("defaultGraphDir is the worker-owned temp path, beside logs/ and jobs/, never inside logsDir", () => {
+	// NOT logsDir on purpose: INT-RUN-HISTORY-FILE-CONTRACT names that directory's filename shape,
+	// and a stray .html beside the sidecars would widen a contract for a file that is not a record.
+	assert.equal(defaultGraphDir({ TMPDIR: "/t" }), "/t/pi-dispatch/graph");
+	assert.equal(defaultGraphDir({ TEMP: "C:\\Temp" }), "C:/Temp/pi-dispatch/graph", "backslashes normalise like the sibling defaults");
+	assert.equal(defaultGraphDir({}), "/tmp/pi-dispatch/graph");
+	assert.ok(!defaultGraphDir({}).includes("/logs"), "never inside the run-history directory");
+});
+
 test("the exported chain-cap defaults are the literals loadConfig uses (issue #54)", () => {
 	// The admin's resolvePaths imports these so the graph never states a cap the worker does not
 	// enforce. The literal assertions beside the loadConfig ones make widening either a reviewed edit:

worker/test/open-browser.test.mjs

@@ -0,0 +1,52 @@
+import assert from "node:assert/strict";
+import { test } from "node:test";
+import { openBrowser } from "../src/open-browser.mjs";
+
+/** A recording child the spawn fake returns; `errorHandlers` proves the swallow listener is attached. */
+function fakeChild() {
+	const child = { errorHandlers: [], unrefCalled: false };
+	child.on = (event, cb) => {
+		if (event === "error") child.errorHandlers.push(cb);
+		return child;
+	};
+	child.unref = () => {
+		child.unrefCalled = true;
+	};
+	return child;
+}
+
+test("openBrowser spawns the exact per-platform argv, detached and ignored", () => {
+	// The argv table is the module (the GitHub App wizard shipped it first; the graph export reuses
+	// it), so it is pinned literally per platform -- the up.mjs exact-argv doctrine.
+	const cases = [
+		["darwin", "open", ["https://x"]],
+		["win32", "cmd", ["/c", "start", "", "https://x"]],
+		["linux", "xdg-open", ["https://x"]],
+	];
+	for (const [platform, cmd, args] of cases) {
+		const calls = [];
+		const child = fakeChild();
+		openBrowser("https://x", { platform, spawn: (...a) => (calls.push(a), child) });
+		assert.equal(calls.length, 1, platform);
+		assert.deepEqual(calls[0], [cmd, args, { stdio: "ignore", detached: true }], platform);
+		assert.equal(child.errorHandlers.length, 1, "the async error path must be swallowed, or a missing opener crashes the process later");
+		assert.equal(child.unrefCalled, true, "the child must not hold the event loop open");
+	}
+});
+
+test("openBrowser swallows a synchronous spawn failure -- the printed URL carries the flow", () => {
+	assert.doesNotThrow(() =>
+		openBrowser("https://x", {
+			platform: "linux",
+			spawn: () => {
+				throw new Error("ENOENT: no xdg-open");
+			},
+		}),
+	);
+});
+
+test("openBrowser's swallowed error handler is inert when invoked", () => {
+	const child = fakeChild();
+	openBrowser("https://x", { platform: "darwin", spawn: () => child });
+	assert.doesNotThrow(() => child.errorHandlers[0](new Error("spawn open ENOENT")));
+});