From 4d5eeda46db3a2ca6bf718632fb2a404f0a2e7e6 Mon Sep 17 00:00:00 2001 From: Aleksandar Grbic Date: Tue, 23 Jun 2026 08:40:22 +0200 Subject: [PATCH] fix(cli): bound the --notify hook through the shared runner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit runNotify spawned `sh -c cmd` with a bare `await proc.exited` — no timeout, no abort — bypassing the shared command runner whose whole purpose is uniform kill-timeout. A notifier that hangs (curl to a dead host with no --max-time, a stray `read` on stdin) wedged the run forever at the finish line of an unattended/cron greenfield build, contradicting its own docstring ("best-effort: never changes the run's exit code" — an infinite hang means no exit code at all). - Add an `env` option to runShellCommand/runArgvCommand (REPLACES inherited env), so a command routed through the runner can still inject $TSFORGE_STATUS. - Route runNotify through runShellCommand with NOTIFY_TIMEOUT_MS (30s), forwarding output via onChunk. Timeout is an optional param for testability. - Tests: shared-runner custom env; runNotify is timeout-bounded (flip-confirmed). - docs(manifest): fix stale lib/fs paths (scope.ts -> scope/scope.ts), note the --notify route + env semantics + the one deliberate MCP-transport spawn exception. --- docs/harness-subsystems.md | 25 ++++++++++++++++--------- packages/core/src/cli.ts | 28 +++++++++++++++++++--------- packages/core/src/lib/fs/process.ts | 7 ++++++- packages/core/tests/cli.test.ts | 28 +++++++++++++++++++++++++++- packages/core/tests/process.test.ts | 19 +++++++++++++++++++ 5 files changed, 87 insertions(+), 20 deletions(-) diff --git a/docs/harness-subsystems.md b/docs/harness-subsystems.md index 386f2a4..7c18aa6 100644 --- a/docs/harness-subsystems.md +++ b/docs/harness-subsystems.md @@ -224,21 +224,28 @@ that leaks keypress listeners. `tests/wizard.test.ts` (reducer/render/lifecycle), `tests/write-config.test.ts` (merge), `tests/scan.test.ts` (read-only + caps). -## lib/fs — `src/lib/fs/process.ts`, fs helpers, `src/lib/scope.ts` +## lib/fs — `src/lib/fs/process.ts`, fs helpers, `src/lib/scope/scope.ts` The ONE shared command runner; path normalization; scope checks. -**Invariants** ONE place runs shell commands (gate + `run` both route here) so -cancellation + kill-timeout are uniform; a timeout/abort kills the whole process -group (no leaked `&` child); argv (no-shell) form for any content-built command (e.g. -`add_dependency` → `bun add …`) — the model's own `run` tool is the one deliberate -shell form, gated by `isReadOnlyCommand` in plan mode and the destructive-shell policy -otherwise; a missing binary → exit 127, not a throw. +**Invariants** ONE place runs shell commands (gate + `run` + the `--notify` hook all +route through `runShellCommand`/`runArgvCommand`) so cancellation + kill-timeout are +uniform; a timeout/abort kills the whole process group (no leaked `&` child); the +final output drain is always bounded (`FLUSH_GRACE_MS`); argv (no-shell) form for any +content-built command (e.g. `add_dependency` → `bun add …`) — the model's own `run` +tool is the one deliberate shell form, gated by `isReadOnlyCommand` in plan mode and +the destructive-shell policy otherwise; a missing binary → exit 127, not a throw; a +custom `env` REPLACES the inherited env (pass `{ ...process.env, … }` to add a var). +The long-lived MCP stdio transport (`src/mcp/stdio-transport.ts`) is the one +deliberate exception — a persistent server, not a one-shot command. **Risk areas** kill that leaves grandchildren (the fixed P2a); shell-injection via the -shell form; an uncapped read. +shell form; an uncapped read; a NEW spawn site that bypasses the runner and so skips +the kill-timeout (the fixed `runNotify` hang). -**Checklist** `tests/process.test.ts` group-kill; content-built commands use `runArgvCommand`. +**Checklist** `tests/process.test.ts` group-kill + bounded drain + missing-binary 127 ++ custom env; `tests/cli.test.ts` `runNotify` is timeout-bounded; content-built +commands use `runArgvCommand`. --- diff --git a/packages/core/src/cli.ts b/packages/core/src/cli.ts index 9df040c..165694b 100644 --- a/packages/core/src/cli.ts +++ b/packages/core/src/cli.ts @@ -13,7 +13,7 @@ import { shouldOpenAtPicker, type IPickerView, } from "./render/file-menu"; -import { listWorkspaceFiles, readFiles } from "./lib/fs"; +import { listWorkspaceFiles, readFiles, runShellCommand } from "./lib/fs"; import { renderCheck } from "./browser"; import { composeMessage } from "./loop/prompt"; import { @@ -2099,22 +2099,31 @@ function greenfieldDeps( }; } +/** A `--notify` hook is bounded: an unattended/cron run must not hang forever on a + * notifier that wedges (a `curl` to a dead host with no `--max-time`, a stray + * `read` on stdin). 30s is generous for a real ping yet always lets the run end. */ +const NOTIFY_TIMEOUT_MS = 30_000; + /** Run the `--notify` shell command (if any) with the run outcome in - * $TSFORGE_STATUS — a ping for unattended/cron runs. Best-effort: a failing or - * missing notifier never changes the run's exit code. */ -async function runNotify(cmd: string, status: string): Promise { + * $TSFORGE_STATUS — a ping for unattended/cron runs. Best-effort: a failing, + * missing, OR HANGING notifier never changes the run's exit code, because it + * routes through the shared runner (uniform kill-timeout) and is bounded. */ +export async function runNotify( + cwd: string, + cmd: string, + status: string, + timeoutMs: number = NOTIFY_TIMEOUT_MS +): Promise { if (cmd.length === 0) { return; } try { - const proc = Bun.spawn(["sh", "-c", cmd], { + await runShellCommand(cwd, cmd, { + timeoutMs, env: { ...process.env, TSFORGE_STATUS: status }, - stdout: "inherit", - stderr: "inherit", + onChunk: (text) => process.stdout.write(text), }); - - await proc.exited; } catch { // A broken notifier must not break the run. } @@ -2186,6 +2195,7 @@ async function greenfieldMode(args: ICliArgs): Promise { ); await runNotify( + args.dir, args.notify, `greenfield ${result.status} ${done}/${result.features.length}` ); diff --git a/packages/core/src/lib/fs/process.ts b/packages/core/src/lib/fs/process.ts index 2503f14..662660c 100644 --- a/packages/core/src/lib/fs/process.ts +++ b/packages/core/src/lib/fs/process.ts @@ -22,6 +22,10 @@ export interface IShellRunOptions { timeoutMs?: number; /** Forward each decoded output chunk live; omit to just capture. */ onChunk?: (text: string) => void; + /** Full environment for the child (REPLACES the inherited env, like Bun.spawn). + * Omit to inherit `process.env`. Pass `{ ...process.env, KEY: val }` to add a + * var while keeping the rest — e.g. the `--notify` hook injecting a status. */ + env?: Record; } /** Result of `runShellCommand` — captured streams + how it ended. */ @@ -74,7 +78,7 @@ export async function runArgvCommand( argv: string[], opts: IShellRunOptions = {} ): Promise { - const { signal, timeoutMs = 0, onChunk } = opts; + const { signal, timeoutMs = 0, onChunk, env } = opts; let proc: Bun.Subprocess<"ignore", "pipe", "pipe">; @@ -90,6 +94,7 @@ export async function runArgvCommand( stdout: "pipe", stderr: "pipe", detached: true, + ...(env === undefined ? {} : { env }), }); } catch (err) { const message = err instanceof Error ? err.message : String(err); diff --git a/packages/core/tests/cli.test.ts b/packages/core/tests/cli.test.ts index d8e7a43..40b9c96 100644 --- a/packages/core/tests/cli.test.ts +++ b/packages/core/tests/cli.test.ts @@ -1,7 +1,33 @@ import { test, expect } from "bun:test"; -import { parseArgs, isOneShot, applyRecipe } from "../src/cli"; +import { mkdtemp, rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { parseArgs, isOneShot, applyRecipe, runNotify } from "../src/cli"; import type { ITaskRecipe } from "../src/config/recipes"; +// Regression: runNotify used to spawn `sh -c cmd` with a bare `await proc.exited` +// — no timeout. A hanging notifier (curl to a dead host, a stray `read`) wedged +// the run forever at the finish line of an unattended/cron build. It now routes +// through the shared runner with NOTIFY_TIMEOUT_MS, so it ALWAYS returns. +test("runNotify is bounded — a hanging notifier cannot wedge the run", async () => { + const dir = await mkdtemp(join(tmpdir(), "tsforge-notify-")); + + try { + const start = Bun.nanoseconds(); + + // `sleep 30` stands in for a foreground-hanging notifier. With the override + // timeout (300ms) the runner kills it; the old unbounded `await proc.exited` + // would block the full 30s. Asserting a sub-5s return is impossible without + // the kill-timeout the shared runner provides. + await runNotify(dir, "sleep 30", "done 3/3", 300); + const elapsedMs = (Bun.nanoseconds() - start) / 1e6; + + expect(elapsedMs).toBeLessThan(5000); + } finally { + await rm(dir, { recursive: true, force: true }); + } +}); + test("parses task + files + accept + dir", () => { const a = parseArgs([ "add", diff --git a/packages/core/tests/process.test.ts b/packages/core/tests/process.test.ts index b1823fd..1448929 100644 --- a/packages/core/tests/process.test.ts +++ b/packages/core/tests/process.test.ts @@ -79,6 +79,25 @@ test("a missing binary returns exit 127 without throwing", async () => { } }); +// The shared runner must be able to inject env (the `--notify` hook passes +// $TSFORGE_STATUS this way). Without an `env` option, a notifier routed through +// the runner couldn't learn the run outcome. +test("runShellCommand passes a custom env to the child", async () => { + const dir = await mkdtemp(join(tmpdir(), "tsforge-proc-env-")); + + try { + const run = await runShellCommand(dir, 'printf "%s" "$TSFORGE_STATUS"', { + timeoutMs: 5000, + env: { ...process.env, TSFORGE_STATUS: "done 3/3" }, + }); + + expect(run.exitCode).toBe(0); + expect(run.stdout).toBe("done 3/3"); + } finally { + await rm(dir, { recursive: true, force: true }); + } +}); + test("a quick command still returns its output and does not report a timeout", async () => { const dir = await mkdtemp(join(tmpdir(), "tsforge-proc-ok-"));