fix(cli): bound the --notify hook through the shared runner#47
Conversation
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.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the --notify hook to route through the shared runner (runShellCommand), ensuring that notifications are bounded by a 30-second timeout and do not hang indefinitely. It also adds support for passing custom environment variables to spawned processes and includes corresponding tests. The review feedback suggests two improvements: sanitizing the custom env object to filter out undefined values before passing it to Bun.spawn to avoid coercion issues, and defensively handling cases where the cmd parameter in runNotify might be undefined or null.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
1 participant
What
Harness-review pass over the
lib/fssubsystem (shared command runner, scope, fs helpers). One verified P2 fixed, plus manifest staleness.Fix —
--notifycould hang the run forever (P2)runNotify(cli.ts) spawnedBun.spawn(["sh","-c",cmd])thenawait proc.exited— no timeout, no abort — bypassing the shared runner whose entire purpose is uniform kill-timeout (manifest invariant #1 forlib/fs).Reproduced:
runNotify("sleep 3")blocks the full ~3s. A notifier that genuinely hangs (acurlto a dead host without--max-time, a strayreadon stdin) wedges the run forever — and it fires at the end of a multi-hour greenfield build, the worst possible place. This contradicts its own docstring ("best-effort: a failing or missing notifier never changes the run's exit code" — an infinite hang means there is no exit code).Changes
envoption torunShellCommand/runArgvCommand(REPLACES inherited env, likeBun.spawn) — the runner had none, and the notifier needs$TSFORGE_STATUS.runNotifythroughrunShellCommandwith a 30sNOTIFY_TIMEOUT_MS, forwarding output viaonChunk. Timeout is an optional param for testability.lib/fspaths (scope.ts→scope/scope.ts); document the--notifyroute,envsemantics, and the one deliberate non-runner spawn (the long-lived MCP stdio transport).Tests
process.test.ts: shared runner passes a custom env to the child.cli.test.ts:runNotifyis timeout-bounded — flip-confirmed (reverting to the unbounded spawn blocks to the test ceiling).Verified clean this pass
group-kill / no leaked
&child; bounded output drain; missing-binary→127; no other bypassing spawn sites;add_dependencyargv form; scope traversal rejection.