Merge pull request #15 from japer-technology/copilot/ensure-lifecycle-are-rock-solid

Harden lifecycle scripts: exit-code contract, EOF safety, path guard, atomic writes

Author: ericmourant

.github-minimum-intelligence/lifecycle/agent.ts

@@ -113,6 +113,14 @@ const repo = process.env.GITHUB_REPOSITORY!;
 const defaultBranch = event.repository?.default_branch ?? "main";
 
 // The issue number is present on both the `issues` and `issue_comment` payloads.
+// Guard explicitly so a misconfigured workflow trigger (e.g. a non-issue event)
+// fails with a clear message instead of an opaque TypeError.
+if (!event.issue || typeof event.issue.number !== "number") {
+  throw new Error(
+    `Event payload for "${eventName}" has no issue number. ` +
+    `agent.ts only supports "issues" and "issue_comment" events.`
+  );
+}
 const issueNumber: number = event.issue.number;
 
 // Read the committed `.pi` defaults and pass them explicitly to the runtime.
@@ -464,4 +472,9 @@ async function main() {
   }
 }
 
-main();
+main().catch((err: unknown) => {
+  // Fail the workflow step with a readable error instead of relying on the
+  // runtime's unhandled-rejection behaviour (which varies between versions).
+  console.error(err instanceof Error ? err.message : String(err));
+  process.exit(1);
+});

.github-minimum-intelligence/lifecycle/local-chat.test.ts

@@ -9,6 +9,10 @@
  *      `.github-minimum-intelligence/.github-minimum-intelligence/state/`.
  *   2. `--list` must succeed (exit 0) regardless of cwd.
  *   3. `--rm` of an unknown ref must exit 2 (user error), not 1.
+ *   4. Exit-code contract: user errors (unknown thread, invalid/taken alias)
+ *      exit 2; environment problems exit 1.
+ *   5. EOF safety: closed stdin (non-TTY / Ctrl-D) must never hang an
+ *      interactive prompt.
  */
 
 import { describe, expect, test } from "bun:test";
@@ -20,12 +24,17 @@ import { join, resolve } from "path";
 const MI_DIR = resolve(import.meta.dir, "..");
 const CHAT_SCRIPT = join(MI_DIR, "lifecycle", "local-chat.ts");
 
-function runChat(args: string[], cwd: string) {
+// Hard ceiling for each spawned runner: an EOF-handling regression would
+// otherwise hang the suite forever.
+const SPAWN_TIMEOUT_MS = 30_000;
+
+function runChat(args: string[], cwd: string, extraEnv: Record<string, string | undefined> = {}) {
   // process.execPath points at the currently running Bun, regardless of PATH.
   return spawnSync(process.execPath, ["run", CHAT_SCRIPT, ...args], {
     cwd,
     encoding: "utf-8",
-    env: { ...process.env, NO_COLOR: "1" },
+    timeout: SPAWN_TIMEOUT_MS,
+    env: { ...process.env, NO_COLOR: "1", ...extraEnv },
   });
 }
 
@@ -57,4 +66,50 @@ describe("local-chat regression tests", () => {
     // would indicate the runner mistook missing state for a config problem.
     expect([0, 2]).toContain(result.status);
   });
+
+  test("--thread with an unknown ref exits 2 (user error)", () => {
+    const result = runChat(
+      ["--thread", "no-such-thread-abc", "hello"],
+      MI_DIR,
+      { OPENAI_API_KEY: "test-key", OPENAI_BASE_URL: undefined, LOCAL_LLM_BASE_URL: undefined },
+    );
+    expect(result.status).toBe(2);
+    expect(result.stdout).toContain("Unknown thread");
+  });
+
+  test("--new with an invalid (pure-digit) alias exits 2 (user error)", () => {
+    const result = runChat(["--new", "--name", "12345"], MI_DIR);
+    expect(result.status).toBe(2);
+    expect(result.stdout).toContain("Invalid name");
+  });
+
+  test("--new with a taken alias exits 2; --rm cleans it up", () => {
+    const alias = `gmi-test-${Date.now()}-${process.pid}`;
+    try {
+      const first = runChat(["--new", "--name", alias], MI_DIR);
+      expect(first.status).toBe(0);
+      const dup = runChat(["--new", "--name", alias], MI_DIR);
+      expect(dup.status).toBe(2);
+      expect(dup.stdout).toContain("already taken");
+    } finally {
+      const rm = runChat(["--rm", alias], MI_DIR);
+      expect(rm.status).toBe(0);
+    }
+  });
+
+  test("missing API key with closed stdin exits cleanly without hanging", () => {
+    // With no key and stdin at EOF, the guided-recovery prompt must resolve
+    // (treating EOF as "quit") rather than waiting forever on input.
+    const result = runChat([], MI_DIR, {
+      OPENAI_API_KEY: undefined,
+      ANTHROPIC_API_KEY: undefined,
+      OPENAI_BASE_URL: undefined,
+      LOCAL_LLM_BASE_URL: undefined,
+      LOCAL_PROVIDER: undefined,
+      LOCAL_MODEL: undefined,
+    });
+    // Must terminate (no timeout) and quit cleanly from the recovery guide.
+    expect(result.signal).toBeNull();
+    expect(result.status).toBe(0);
+  });
 });

.github-minimum-intelligence/lifecycle/local-chat.ts

@@ -90,7 +90,7 @@ import {
   mkdirSync, readdirSync, statSync, unlinkSync, renameSync,
   openSync, closeSync,
 } from "fs";
-import { resolve, join, basename } from "path";
+import { resolve, join, basename, sep } from "path";
 import { networkInterfaces } from "os";
 import { createInterface } from "readline";
 import { execFileSync, execSync } from "child_process";
@@ -246,6 +246,13 @@ const say = {
   },
 };
 
+// ─── Error taxonomy ───────────────────────────────────────────────────────────
+// UserError marks problems caused by user input (unknown thread, taken alias,
+// malformed args).  The top-level handler maps it to exit code 2, keeping the
+// documented contract: 0 success, 1 environment problem, 2 user error.
+
+class UserError extends Error {}
+
 // ─── Thread record schema ─────────────────────────────────────────────────────
 
 type Thread = {
@@ -273,7 +280,12 @@ function readThread(id: number): Thread | null {
 }
 
 function writeThread(t: Thread): void {
-  writeFileSync(threadPath(t.id), JSON.stringify(t, null, 2) + "\n");
+  // Atomic write: write to a temp file then rename over the target so a crash
+  // mid-write can never leave a truncated/corrupt thread record behind.
+  const target = threadPath(t.id);
+  const tmp = `${target}.tmp-${process.pid}`;
+  writeFileSync(tmp, JSON.stringify(t, null, 2) + "\n");
+  renameSync(tmp, target);
 }
 
 function listThreads(): Thread[] {
@@ -297,15 +309,15 @@ function listThreads(): Thread[] {
 function allocateThread(name: string | null): Thread {
   if (name !== null) {
     if (!ALIAS_PATTERN.test(name)) {
-      throw new Error(
+      throw new UserError(
         `Invalid name "${name}". Must start with a letter and contain only ` +
         `letters, digits, "_" or "-" (max 64 chars). Pure-digit names are ` +
         `reserved for IDs.`
       );
     }
     for (const existing of listThreads()) {
       if (existing.name === name) {
-        throw new Error(
+        throw new UserError(
           `Thread name "${name}" already taken by thread #${existing.id}.`
         );
       }
@@ -507,7 +519,10 @@ function getRepoName(): string {
 function safePath(userPath: string): string | null {
   const root = getRepoRoot();
   const resolved = resolve(root, userPath);
-  if (!resolved.startsWith(root)) return null;
+  // Require the repo root itself or a path under `root + sep`; a bare
+  // startsWith(root) check would wrongly accept sibling dirs like
+  // "/repo-evil" when root is "/repo".
+  if (resolved !== root && !resolved.startsWith(root + sep)) return null;
   return resolved;
 }
 
@@ -578,6 +593,34 @@ rejected (no auto-create on typos). Aliases must start with a letter.`
   );
 }
 
+// ─── EOF-safe readline questions ──────────────────────────────────────────────
+
+/**
+ * Build an EOF-safe `ask` function for a readline interface.  Resolves with
+ * the user's answer, or `null` when the input reaches EOF (Ctrl-D / closed
+ * non-TTY stdin) — readline never invokes the question callback in that case,
+ * which would otherwise leave the promise (and the process) hanging forever.
+ * The `pending` hand-off guarantees each promise settles exactly once even if
+ * the close event and the question callback race.  Questions must be asked
+ * serially (await each answer before asking the next), which is how every
+ * call site in this file uses it; concurrent questions would overwrite the
+ * single pending resolver.
+ */
+function makeAsk(rl: ReturnType<typeof createInterface>): (q: string) => Promise<string | null> {
+  let pending: ((v: string | null) => void) | null = null;
+  rl.on("close", () => {
+    const p = pending; pending = null;
+    if (p) p(null);
+  });
+  return (q: string) => new Promise((res) => {
+    pending = res;
+    rl.question(q, (a: string) => {
+      const p = pending; pending = null;
+      if (p) p(a ?? "");
+    });
+  });
+}
+
 /**
  * Interactive launcher shown when `bun run chat` is invoked with no args.
  * Lists existing threads and lets the user pick by row number, press Enter
@@ -644,10 +687,12 @@ async function interactiveStart(provider: string, model: string, thinking: strin
   console.log("");
 
   const rl = createInterface({ input: process.stdin, output: process.stdout, terminal: true });
-  const ask = (q: string): Promise<string> => new Promise((res) => rl.question(q, res));
+  const ask = makeAsk(rl);
   try {
     while (true) {
-      const raw = (await ask("    Select> ")).trim();
+      const answer = await ask("    Select> ");
+      if (answer === null) return null; // EOF — treat as quit.
+      const raw = answer.trim();
       if (raw === "q" || raw === "Q" || raw === "/exit" || raw === "/quit") {
         return null;
       }
@@ -974,9 +1019,14 @@ async function runTurn(
             "and risk binding the wrong session to this thread."
           );
         }
-        created.sort((a, b) =>
-          statSync(join(sessionsDir, b)).mtimeMs - statSync(join(sessionsDir, a)).mtimeMs
-        );
+        created.sort((a, b) => {
+          // A concurrent runner may delete/rotate a session file between the
+          // snapshot diff and this sort; treat vanished files as oldest.
+          const mtime = (f: string): number => {
+            try { return statSync(join(sessionsDir, f)).mtimeMs; } catch { return 0; }
+          };
+          return mtime(b) - mtime(a);
+        });
         sessionPath = join(sessionsDir, created[0]);
       }
 
@@ -1019,6 +1069,7 @@ function cmdRemove(ref: string): void {
   const t = resolveThreadRef(ref);
   if (!t) {
     say.warn(`No thread matching "${ref}".`, "Use `--list` to see existing threads.");
+    process.exitCode = 2; // user error per the documented exit-code contract
     return;
   }
   unlinkSync(threadPath(t.id));
@@ -1116,7 +1167,9 @@ async function repl(initial: Thread, rt: RuntimeState): Promise<void> {
   console.log("");
 
   const rl = createInterface({ input: process.stdin, output: process.stdout, terminal: true });
-  const ask = (q: string): Promise<string> => new Promise((res) => rl.question(q, res));
+  // EOF-safe question wrapper: resolves null on Ctrl-D / closed stdin so the
+  // REPL exits cleanly instead of hanging on an unanswerable question.
+  const ask = makeAsk(rl);
 
   function prompt(): string {
     const branch = getGitBranch();
@@ -1147,7 +1200,9 @@ async function repl(initial: Thread, rt: RuntimeState): Promise<void> {
   try {
     // eslint-disable-next-line no-constant-condition
     while (true) {
-      const line = (await ask(prompt())).trim();
+      const answer = await ask(prompt());
+      if (answer === null) break; // EOF — end the session cleanly.
+      const line = answer.trim();
       if (!line) continue;
 
       // ─── Exit ─────────────────────────────────────────────────────────────
@@ -1549,7 +1604,7 @@ async function repl(initial: Thread, rt: RuntimeState): Promise<void> {
         // eslint-disable-next-line no-constant-condition
         while (true) {
           const more = await ask("  ... ");
-          if (more.trim() === "") break;
+          if (more === null || more.trim() === "") break;
           lines.push(more);
         }
         const full = lines.join("\n").trim();
@@ -1588,9 +1643,9 @@ type RuntimeCfg = { provider: string; model: string; thinking: string | undefine
 async function promptLine(question: string): Promise<string> {
   const rl = createInterface({ input: process.stdin, output: process.stdout, terminal: true });
   try {
-    return await new Promise<string>((res) => {
-      rl.question(question, (a: string) => res(a ?? ""));
-    });
+    // makeAsk resolves null on EOF (Ctrl-D / closed stdin); map that to ""
+    // so callers can treat it as "user backed out".
+    return (await makeAsk(rl)(question)) ?? "";
   } catch {
     return "";
   } finally {
@@ -1813,9 +1868,13 @@ async function main(): Promise<void> {
 
   let cfg: RuntimeCfg = resolveRuntimeConfig();
 
+  // Pure allocation (`--new` without a prompt or thread ref) never contacts a
+  // model, so it must work without an API key.
+  const allocationOnly = args.newThread && !args.prompt && !args.threadRef;
+
   // ── Validate config BEFORE creating threads ─────────────────────────────
   // (so quitting from the guide doesn't leave orphan thread #1 behind.)
-  if (!isLocalProvider(cfg.provider)) {
+  if (!allocationOnly && !isLocalProvider(cfg.provider)) {
     const keyName = PROVIDER_KEY_MAP[cfg.provider];
     if (keyName && !process.env[keyName]) {
       const updated = await guideMissingApiKey(cfg);
@@ -1859,6 +1918,7 @@ async function main(): Promise<void> {
           "Use `--list` to see existing threads, or `--new` to create one. " +
           "Closed-world: unknown refs are never auto-created."
         );
+        process.exitCode = 2; // user error per the documented exit-code contract
         return;
       }
     }
@@ -1894,5 +1954,14 @@ async function main(): Promise<void> {
   await repl(activeThread, rt);
 }
 
-main();
-
+main().catch((err: unknown) => {
+  // Top-level error handler: honour the documented exit-code contract
+  // (1 = environment problem, 2 = user error) and print a readable message
+  // instead of an unhandled-rejection stack trace.
+  say.error(
+    err instanceof UserError ? "Invalid request" : "Startup failed",
+    err instanceof Error ? err.message : String(err),
+  );
+  cleanup();
+  process.exit(err instanceof UserError ? 2 : 1);
+});