Repository navigation
Run commands with a cleared environment, a time limit and a bounded output tail - #33
Merged
Merged
Conversation
… tail Red: apps/server/src/factory/process.ts does not exist yet. Issue: #8
…utput tail Issue: #8
Red: the wrapper sh dies on SIGTERM and the group is SIGKILLed at once, so the grandchild's marker never reaches the tail. Issue: #8
…er too, kill on interrupt After SIGTERM, wait up to termGraceMs for the leader to exit and the group to be empty before SIGKILL; in shell mode the wrapper sh died first and the real command got no grace. Also signal the leader pid alongside the group (softfac kill_all), and spawn inside acquireUseRelease so an interrupt always kills the group and destroys the pipe. The leader-left-group and interrupt cases have no deterministic test. Issue: #8
…err merge Three mutation survivors from review. No red phase: the behaviour already works; these tests pin it. Issue: #8
rawphp
marked this pull request as ready for review
October 15, 2026 11:02
Author
|
Factory gate: passed (attempt 1, run by the factory in a fresh checkout of the pull request's head
|
…rrupt and drain tests - spawn's synchronous throws (a NUL byte in env or args) now fail with RunSpawnError instead of dying - RunSpawnError keeps the command only in cause; its attributes are shell and cwd - the env allowlist and defaults are no longer exported (knip) - RunResult.exitCode doc: a timed-out command can still exit 0, check timedOut first - the post-run kill comment says a process that left the group escapes - tests: an interrupted run kills the group, a stray outside the group does not hold run past the drain wait, a tail cut inside a UTF-8 character - design.md Shape diagram: gate commands go through node:child_process in factory/process.ts, not ProcessRunner
Author
|
Factory gate: passed again (attempt 2, run by the factory in a fresh checkout of the pull request's head
|
Callers depend on the FactoryProcess tag, so their tests can swap in a fake layer instead of starting real processes. The two process-group tests now use withTempDir and waitForDeath, so a failed assertion no longer leaks the temp dir, and their wall-clock bound is 5 s instead of 1 s; the grandchild death check still proves the kill.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #8
Contract
apps/server/src/factory/process.tsexportsrun({ command, args, shell, cwd, env, timeoutMs, tailBytes })returning an Effect of{ exitCode, timedOut, tail }overnode:child_process; the child sees onlyPATH,HOME,USER,LOGNAME,SHELL,LANG,LC_*,TMPDIR,TERMfrom the server plus the passedenv; it runs in its own process group; on timeout the group getsSIGTERM, thenSIGKILLonce the group is empty or 10 s have passed, withtimedOut: true; stdout and stderr share one pipe and only the lasttailBytes(default 16 KiB) are kept; design.md open question 2 is answered.vp test run apps/server/src/factory→ pass (22 tests, 4 files)Acceptance criteria
run(...)returns{ exitCode, timedOut, tail }, inapps/server/src/factory/process.tsbecause it usesnode:child_process—process.ts:117, every test inprocess.test.tsenv— tests "does not pass variables outside the allowlist", "passes PATH, LC_* and the env argument through" (includes a passedHOMEoverriding the inherited one)SIGTERM,SIGKILL10 s later,timedOut: true—detached: trueandkill(-pid)inprocess.ts; tests "a timeout kills the whole process group, grandchild included", "escalates to SIGKILL when SIGTERM is ignored", "gives a TERM-trapping grandchild its grace before SIGKILL" (grace istermGraceMs, default 10 000, exposed so the test runs in under a second)tailBytesonly (default 16 KiB) — one pipe via a/bin/shwrapper withexec 2>&1;Tailkeeps at most about twice the cap; tests "interleaves stdout and stderr in order", "merges stderr into the tail without a shell", "keeps only the last tailBytes of a 1 MB output"factory/docs/design.mditem 2 and the code-layout treeDecisions the issue left open
ProcessRunnerwas checked againstapps/server/src/processRunner.tsand Effect's Node spawner: it does detach the child into its own group and SIGTERMs the group on interrupt, but it always extends the parent environment (extendEnvis true wheneverenvis set), reads stdout and stderr on separate pipes, drops partial output on timeout and never escalates toSIGKILL. Soprocess.tsis outsidefactory/t3/and no seam was added.factory/scripts/check-seams.shexits 0.exitCodeisnumber | null(null when the process died by a signal, including every timeout), Node's own shape, rather than softfac's-1. The gate module that writes the ledger can map it.shell: truerunscommandas a/bin/sh -cscript and passesargsas its positional parameters ($0,$1, …). Argv mode goes through the same wrapper withexec "$0" "$@", so both modes share one pipe.timeoutMsis 30 minutes (softfac's gate default). POSIX only, as softfac is.SIGKILLed at once (softfac's "nothing the command started may outlive it"); the SIGTERM grace applies to the timeout path only, as the issue says.Gate
vp test run apps/server/src/factory→ exit 0 (.factory/gate-1.log)vp run --filter t3 typecheck→ exit 0 (.factory/gate-2.log; only pre-existingsuggestionlines in other files)Models
.dw/config.ymlhas nomodelsblock, so Tom's roster applied)Review
acquireUseRelease, so an interrupt always kills the group and destroys the pipe), testing-1, -2, -4 (tests added for env override, post-exit stray cleanup timing, argv-mode stderr merge; the two surviving mutants were re-applied by hand after the fix and both are killed)Tailis not observable throughrun(); testing it would mean exportingTail. Left untested. reliability-1 and the interrupt path have no deterministic test (a detached session leader cannot leave its group without a cooperating descendant; an interrupt race cannot be forced).Lessons
git checkoutthe file) on the fixed tip and records killed or survived on the receipt; no second review needed.Notes
origin/main(issue Factory skeleton: an empty FactoryLayer wired in through one seam #1 had just landed); fast-forwarded withgit merge --ff-only origin/mainbefore provisioning (field lesson 18).factory:*labels already existed).test:commits8b4567307(module absent, import fails) anded6472569(reported red withexpected '' to include 'caught') precede their fixes.dd6c4467chas no red phase by design: it pins behaviour that already worked.Commits
8b4567307test(factory): pin run() env allowlist, process-group kill and output tail— Make slice (red)5a1d95e79feat(factory): run commands with a cleared env, a time limit and an output tail— Make sliceb60ba307cfix(factory): process tests read time through Effect's Clock— Make slice (typecheck diagnostics)f49f63b9bdocs(factory): answer open question 2, process.ts uses node:child_process— Make sliceed6472569test(factory): a TERM-trapping grandchild gets its grace before SIGKILL— Check cycle 1 (red)cbfc11cedfix(factory): give the whole group its SIGTERM grace, signal the leader too, kill on interrupt— Check cycle 1dd6c4467ctest(factory): pin env override, post-exit stray cleanup and argv stderr merge— Check cycle 2Files
apps/server/src/factory/process.ts—run(), env allowlist,Tail, process-group lifecycleapps/server/src/factory/process.test.ts— the tests abovefactory/docs/design.md— open question 2 answered;process.tsmoved out oft3/in the layout tree