Conversation
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
latekvo
reviewed
Sep 3, 2026
hubgan
force-pushed
the
feat/flow-script-drop-logs
branch
from
September 7, 2026 11:11
230a0ae to
0d14194
Compare
hubgan
force-pushed
the
feat/flow-bash-scripts
branch
from
September 7, 2026 11:37
680f4df to
ab3febd
Compare
latekvo
reviewed
Sep 8, 2026
latekvo
left a comment
latekvo
left a comment
Member
There was a problem hiding this comment.
[Diplomat, Opus 5]: Reviewed at 103a25f94, diffed against the base 0d14194db so only this PR's own changes were in scope.
Everything below was produced by running the code, not by reading it. Most cases went through runFlowScriptStep — the seam both flow-run.ts and flow-add-script.ts use — against the built dist, with an isolated HOME, TMPDIR and project root per case; a few went through argent flow run itself, and the config cases through packages/argent-cli's own config command. Host: macOS arm64, /opt/homebrew/bin/bash 5.3.15 and Apple /bin/bash 3.2.57, and there is no /usr/bin/bash here.
What the review covered, in the order it ran: every claim in the diff's prose and error text against the behaviour that produces it; each changed rule beside its nearest twin elsewhere in the repo; the non-happy paths (errors, timeouts, cancellation, signals, partial success); hostile and boundary inputs at each new entry point; whether each new branch can fire and whether its value reaches the consumer that motivated it; what outlives a step (processes, exchange directories, descriptors, os.tmpdir() growth); the absence side — prose the change silently falsified, half-pairs, and mutation testing of the new constants and guards; then the win32-only half, the queue and sweep lifecycle, the validateWrite change's effect on existing config keys, the runner's descriptor and signal protocol, parse/registry/replay plumbing, secrets and redaction, regression risk to the existing .mjs step, the numbers against each other, the exchange files as a filesystem contract, the verdict classification as a matrix, packaging and a from-scratch rebuild, the CLI and MCP surfaces, path and encoding shapes, and every user-facing string this PR adds.
Four areas came back with nothing, each after a real attempt to break them. The runner's descriptor and signal protocol: descriptors enumerated from inside bash (3/4/5 really are three separate /dev/null opens, nothing above 5 leaks), forged result and failure lines written to stdout, stderr and fds 3/4/5 including exec 5>/dev/fd/5, NODE_CHANNEL_FD probed from a grandchild, /dev/tty, planted ARGENT_SECRET_* values dumped from bash's own environment, every signal shape at 16 offsets, a 200 MiB stdout flood, and a ps survivor diff — none of it got through. The existing .mjs step: a base-vs-head behavioural diff over pass, throw, timeout, cancel, huge document, over-long failure message, self-exit, background job, signal and missing script showed no unintended change. Path shapes: 19 project roots and TMPDIRs containing spaces, quotes, backslashes, newlines, $(...), backticks, globs, leading dashes, combining marks, RTL overrides, emoji and 255-byte components all passed with the document read back verbatim and the exchange removed. Packaging: a from-scratch npm run build on a dist-free tree produced a dist byte-identical to the worktree's, with all three .mjs runner assets present, and the PR adds no asset that copy-build-assets.mjs would miss.
The findings are inline. Each names the trigger that reaches it and the output it produced.
latekvo
reviewed
Sep 8, 2026
latekvo
left a comment
latekvo
left a comment
Member
There was a problem hiding this comment.
[Diplomat, Opus 5]: Reviewed at f42bb79 against the merge-base 0d14194, on macOS arm64 / Node v24.14.1, with /bin/bash 3.2.57 and /opt/homebrew/bin/bash 5.3.15 both available.
What I ran: the .sh and .mjs paths driven through runFlowScriptStep and FlowScriptExecutor.execute from the worktree's own dist; a base-vs-head behavioural diff of the .mjs path over 20 cases; every config key written with 24 hostile values on both sides; orphan reaping under a SIGKILLed driver; the stale-exchange sweep; 14 concurrent bash steps; a reason-budget sweep; and test/flows/script/ (517), configuration-core (150), registry (122), argent-cli (615), knip and prettier --check, all green. The one full-suite failure I saw (http-tools-meta.test.ts) passes 23/23 in isolation and is untouched by this branch.
Comments below are inline, plus replies on the existing threads where a finding is the same issue already opened there.
latekvo
reviewed
Sep 9, 2026
latekvo
left a comment
latekvo
left a comment
Member
There was a problem hiding this comment.
[Diplomat, Opus 5]: Reviewed at d6cf45deb, against the base 0d14194db, with the four commits since my last pass (f42bb797c..d6cf45deb) in the sharpest focus — d6cf45deb narrowing scripts.bash to the global scope is a behaviour change, not a docs one.
Everything below was produced by running the code. The .sh step went end to end through the real entry point first: argent flow run from this branch's own dist, against a booted iOS simulator, with an isolated HOME and project — ✓ 1 script ../../scripts/seed.sh, ✓ 2 launch. The scope change itself was then driven the same way, four rows: a committed project pin is ignored and the step falls through to PATH (5.3.15), a global pin is honoured (3.2.57), a wrong global pin errors and names the global file, and config set --scope project is refused — matching scripts.maxTimeoutMs line for line across get/set/unset/list. Local CI: test/flows/script 517/517, the full tool-server suite 6277 across six shards (one flake in shard 1, 940/940 on an isolated re-run), configuration-core 152, registry 122, argent-cli 615, typecheck:tests, knip, prettier --check and npx docusaurus build all clean.
The moves, in the order they ran: claims vs code over every added line touching the scope story; each changed rule beside its nearest twin (scripts.bash against scripts.maxTimeoutMs/heapLimitMb, the .sh path against the .mjs path cell by cell); non-happy paths and hostile inputs at each new entry point; reachability both ways through the runner's new branches; what outlives a step; and the absence pass — half-pairs, prose the change silently falsified, and mutation testing of the new constants and guards.
I also re-ran the original reproduction for all eight defects answered "Fixed in …" last round and never re-checked: SIGQUIT in the held signal set, the group-signal settle against short timeouts, carriageReturnProblem on the non-zero-exit branch, the lossy output decode, the reason truncation marker, the byte-budget units, the probe's environment, and the discarded candidate reasons. All eight land. The four areas that came back clean this pass were the protocol module and the queue (adversarial frames, waiter accounting, slot release on every path, the depth cap), flow composition and the recording/replay round trip (thirteen .sh/.mjs pairs in identical flow positions, byte-identical in status, tally, skip propagation and escaping), isolation between steps, and the exchange-file filesystem contract.
One problem outside this diff: a fork that fails at the spawn syscall throws out of execute() and exits the tool server. The line is byte-identical at the base, so it is not this PR's — filed as #1079 with the repro.
The findings are inline. Each names the trigger that reaches it and the output it produced.
hubgan
force-pushed
the
feat/flow-script-drop-logs
branch
from
September 9, 2026 14:29
0d14194 to
fdd1774
Compare
hubgan
force-pushed
the
feat/flow-bash-scripts
branch
from
September 9, 2026 14:43
73ce8d6 to
03d6d79
Compare
A `script` step's work is as often a shell one-liner as a Node program: `curl` an internal API, `psql -f` a seed file, `adb shell` a settings change. This adds `.sh` as a second language on the existing step - one new extension, no new key, no new tool, no new step kind and no new report field. The contract is one line each: input is the environment, the output document is the file named by `$ARGENT_OUTPUT`, the verdict is the exit status, and a failure reason is the file named by `$ARGENT_REASON`. Bash runs inside the runner rather than instead of it. The executor forks the same `flow-script-runner.mjs`, as the entry module rather than as an `--import` preload, and the runner spawns bash as its own child. That keeps one process group per step, so the parent's group stop and both watchdogs' group kills reach a bash descendant unchanged - which matters because a bash polling loop is exactly the orphan the watchdogs exist for, and a shell has no thread to watch a descriptor from.
`readReasonFile` clamped `$ARGENT_REASON` at a fixed `MAX_REASON_CHARS`, on the premise that the 1024 characters left over were always enough for the exit line in front of it - and so that the whole-message `clampText` never fired on that path. The interpreter path inside that line is the one term nothing bounds. A bash some 900 characters down a directory tree spends the reserve, the message passes the ceiling, and `clampText` cuts the reason's own marker in half. That marker is load-bearing. `redactTruncated` in the parent reads it to find where the reason was cut, and drops the half of a secret straddling that cut - a prefix no whole-value scrub can match. With the marker gone the omission marker matches first, the head it hands `partialSecretTail` ends in marker fragments rather than reason text, and the half survives into the report. The reason now takes the smaller of `MAX_REASON_CHARS` and what the exit line leaves, and its marker counts against that budget the way `clampText`'s does. A budget too narrow for the marker keeps no reason text at all, so there is no half of a secret to leave behind either. Reproduced and re-checked through `argent flow run` with `scripts.bash` at a 934-character path: before, the report ended `…CUTHERE99y… [$ARGENT_REASON ho… [61 more characters omitted]`; after, `… [$ARGENT_REASON holds 14335 bytes; this report keeps the first 7137 characters]`.
`asPresentText` returned `undefined` only for `undefined`, so a JSON `null`
fell through to `JSON.stringify` and came back as the four-character string
"null". Every other parser in the schema - `asBoolean`, `asString`,
`asNumber`, `asPositiveInteger`, `asStringArray` - reads `null` as an absent
key, so `scripts.bash` was the one key where `null` meant "set to a value
nothing can use" rather than "unset".
A config generator that writes `null` for a key it has no value for bricked
every `.sh` step in that scope, with a message reporting a relative-path
problem about a value that is not a path at all.
Reproduced through `argent flow run` with a project `.argent/config.json` of
`{"scripts":{"bash":null}}`: before, the step errored with `The configured
bash (scripts.bash = null, …) is not an absolute path`; after, the key is
absent and the step runs under the bash on PATH.
Both host-specific strings for the key named a path the host they are shown on may not have. `expected` said "`/usr/bin/bash` on macOS and Linux", and macOS ships no `/usr/bin/bash` at all - it ships bash 3.2 at `/bin/bash`, which is what this feature's own docs say. `example` branched only on win32, so every POSIX host was offered `/opt/homebrew/bin/bash`, which exists only on an arm64 Mac with Homebrew. `asAbsolutePath` checks shape and never existence, so either spelling was accepted and written, and the failure only surfaced later, at every `.sh` step, as "The configured bash (scripts.bash = …) does not exist" - with no fallback to PATH, because the value is set. A wrong value written to a project `.argent/config.json` also travels to teammates. Both now name `/bin/bash`, the one path every macOS and Linux host has, and a test pins that the example exists on the host it is printed on and that `expected` names the same path. Verified through the CLI: `argent config set scripts.bash bin/bash` now prints `Example: argent config set scripts.bash /bin/bash`, and that command is accepted.
Deleting the `process.platform === "win32"` branch of `asAbsolutePath` left every suite green. It is the write side of a rule the tool server reads back through the same `WINDOWS_ROOTED_PATH_RE`, and the comment on that constant records the two having already disagreed once in exactly one direction: `argent config set` stored a POSIX path that every `.sh` step then refused with "names no drive". With nothing pinning this half, that disagreement could return without CI noticing, on the platform this repository cannot run its unit tests on by default. The new block stubs the platform to win32 and drives the real `setConfigValue`: a drive-rooted path, a drive-rooted path with forward slashes and a UNC share are written; a POSIX path, a path rooted on no drive and a relative path are refused. Removing the branch now fails two of them.
Routing a write through `validateWrite` falsified the docstring beside the error it raises, which still said "Thrown when a value fails the schema's `parse` validator". For `scripts.bash` the two disagree by design - `parse` is `asPresentText`, which keeps everything present so the resolver can name a wrong value, and `validateWrite` is `asAbsolutePath` - so the one key that introduced the second validator was the key the docstring was wrong about: a relative path passes `parse` and still raises this.
The probe's output cap was applied per chunk and was all-or-nothing, but the string it protects sits at the END of the output: `BASH_PROBE_COMMAND` prints the version line last, after whatever the candidate greeted with. Once one chunk pushed the buffer past 4 KiB every later chunk was dropped whole - including the one carrying `argent-bash-version:` - and the marker was then tested against text it had been cut out of. So a wrapper of the shape the leading newline in the probe command exists for (`#!/bin/sh`, a banner, `exec /bin/bash "$@"`) was refused with a statement that is false about it: it printed its version. The cut was exactly at 4096 characters of greeting, and the same wrapper one character shorter ran the step. On the search path the same candidate was skipped silently instead. The window is now on the tail, where the answer is, and is bounded the same way. Reproduced through `argent flow run` with `scripts.bash` at such a wrapper: before, `is not a bash: running it printed no $BASH_VERSION`; after, the step passes. Pinned by a case whose wrapper greets with 64 KiB.
Dropping the `\S` from `BASH_PROBE_MARKER` left the whole `test/flows/script/` suite green, and the version after the marker is the whole of what separates bash from the shells that would run the file with different word-splitting and array semantics - `notBashProblem`'s own docstring says so, naming zsh, ksh and dash as answering with an empty version. What the guard prevents is not a loud failure: a non-bash accepted there runs the file, writes nothing to `$ARGENT_OUTPUT`, and exits 0, so the parent reads back the document it seeded and reports the step green. The new case configures a shell that answers the probe the way those three do - the marker and nothing after it - and expects the step to be refused. Dropping the `\S` now fails it.
The signal branch of `notBashProblem` fired on ANY signal death of the probed candidate but offered one explanation - "(the check waits 5 seconds, then stops the candidate)". That is false whenever the candidate is not the one Argent stopped: a wrapper that segfaults, one the kernel kills for its memory, one that kills itself, all answer in milliseconds and got the same sentence. `askForBashVersion` already recorded which signal it sent, so it now reports whether the death was its own doing and the message follows: a candidate the check stopped reads "did not answer … within 5 seconds, and was stopped with SIGKILL"; one that died on its own reads "died from SIGSEGV", with no claim about a wait. Reproduced through `argent flow run` with `scripts.bash` at `#!/bin/sh` + `kill -SEGV $$`: before, "was stopped by SIGSEGV (the check waits 5 seconds…)" at 240 ms; after, "died from SIGSEGV". A candidate that really does hang still reports the wait, verified against `trap "" TERM` + `sleep 300` at 6.0 s.
Both calls in the search loop computed a sentence and both were used as predicates and `continue`d past. When every candidate was rejected the caller got `notFoundMessage()`, which is written for the case where nothing exists: it lists where the executor looked and says "Install bash". So on a host that HAS a bash which fails the probe, the step's reason was the opposite of what happened - `which bash` answers on that host, and the reason that would name the real problem existed inside the resolver and was dropped. The reasons are kept now, and the message names them: "No bash this host offers could run the script: the executor looked at …, and refused what it found - /usr/local/bin/bash is not a bash: running it printed no $BASH_VERSION". A candidate a host simply lacks is left out, and so is the WSL launcher, which the message already names - only a file that is really there earns a line. Pinned by a case that puts a stub on PATH and takes the fixed locations away: the message names the stub and no longer says "Install bash". The "nothing anywhere" message is unchanged and still pinned by the two cases beside it. This one is not reachable through `argent flow run` on a host that has a real bash at a fixed location, which is why the pin is the resolver itself.
The refusal explained a leading backslash, and the values that reach it most
often begin with a forward slash: `path.win32.isAbsolute("/usr/bin/bash")` is
true, so a POSIX path passes the check above and lands here. The comment on
`WINDOWS_ROOTED_PATH_RE` names exactly that case as the reason the write gate
exists - `argent config set` stored a POSIX path that every `.sh` step then
refused - so it is the expected input to this branch, not an edge case.
The sentence now says "begins with a slash or a backslash", which is the same
mechanism for both: on Windows either spelling is rooted on whatever drive the
process is on. Pinned by both spellings.
"or unset it to use the one on this host's PATH" was true only where exactly one scope held a value. The key takes both with `merge: "prioritize-local"`, so unsetting the file the message had just named falls through to the OTHER scope's value, not to PATH. With a project `.argent/config.json` of `/bin/ls` over a global one of `/bin/bash`, following the printed advice silently swapped the interpreter to the global value on a host whose PATH bash is a different one - and a step that then fails does not mention `scripts.bash` at all, so nothing points back at the value now in force, which is the situation this key exists to make visible. The refusal now reads "unset it there to fall back to /bin/bash, which <global file> sets - unset it in both files to use the one on this host's PATH" when a value is behind it, and is unchanged when none is. Reproduced through `argent flow run` with exactly those two files.
The probe's `spawn` passed no `env`, so the candidate inherited the tool server's whole environment. The bash that actually runs the step does not: `buildChildEnv` copies only `ALLOWED_ENV_NAMES`. The divergence ran both ways. `BASH_ENV` is the one variable that changes what a non-interactive `bash -c` does, and it is deliberately outside that allowlist. So a host that exported it had every `.sh` step refused before the fork, reporting that no bash exists - on a host with three - and the remedy the refusal names goes through the same probe, which answered "is not a bash" about a bash 5.3 that answers correctly when asked without that variable. In the other direction the candidate is an arbitrary executable named `bash`, and inheriting handed it `ARGENT_AUTH_TOKEN`, `ARGENT_PORT` and every `ARGENT_SECRET_*` value - the three things the comment above `ALLOWED_ENV_NAMES` says the allowlist exists to keep out of a script's reach. The resolver now takes the environment to probe in, and the executor passes the step's own. The test helper that gates the bash suites passes the same one through `buildChildEnv`, which is now exported for it. Reproduced through `argent flow run` with a shim first on the resolver's path that records what it was given. Before: BASH_ENV set errored every step with "No bash was found on this host", and the probe run saw 3 argent token/port/secret names against the step run's 0. After: the step passes with BASH_ENV set, and both runs see 0.
The helper swapped `HOME`/`USERPROFILE` for a directory of its own before
asking the resolver, and restored them before returning. None of the six files
it guards does the same, so every `.sh` step below reached
`resolveBashInterpreter` under the developer's real home and did read
`~/.argent/config.json` - the outcome the swap was written to prevent. The
gate therefore answered "this host has a bash" about a bash the steps never
run under.
Measured on macOS with `/opt/homebrew/bin/bash` 5.3 on PATH, changing only
`~/.argent/config.json` between runs of `test/flows/script/`:
no config 500 passed
{"scripts":{"bash":"/nonexistent/bash"}} 64 failed | 436 passed, 0 skipped
The second row is the case the helper exists for, and it failed instead of
skipping: the failures read as source regressions (`expected 'error' to be
'fail'`) rather than as a machine problem. The swap itself suppressed the
skip - called the way the steps call it, the resolver answers "The configured
bash … does not exist".
The gate now asks the resolver exactly the way a step does: same environment,
same home. The same run is 429 passed | 71 skipped, and an ordinary valid pin
is exercised by the steps and reported by the gate as the same bash.
`GROUP_SIGNALS` held SIGTERM, SIGINT and SIGHUP, and the comment called it "the signals a script can aim at its own process group". SIGQUIT is the omission where bash and Node disagree in the direction that matters: GNU bash 5.x ignores SIGQUIT in a non-interactive shell, and Node's default kills on it. So a script whose body or EXIT trap ran `kill -QUIT 0` killed the runner and left bash running - the failure `holdGroupSignals` exists to prevent. With no terminal message the parent fell to the signal row of `classifyOutcome` and reported "The script process was killed by SIGQUIT before it returned output. It did not stop itself." Both halves were false: bash was not killed, it ignored the signal, and the script's own line is what killed the runner. Its sharpest spelling was `trap 'kill -QUIT 0' EXIT` on a script that had already written a complete `$ARGENT_OUTPUT` and exited 0 - reported as a fault of the machine it ran on, its document discarded. The verdict now follows the bash: 5.x survives and the step passes on the document it wrote; Apple's 3.2 dies on SIGQUIT and the step reports the same `kill 0` guidance the SIGTERM case does. Reproduced through `argent flow run`. Before: `✗ … killed by SIGQUIT … It did not stop itself`. After, under bash 5.3: `✓ 1 script`, the background job reaped. Under `scripts.bash = /bin/bash` (3.2): `killed by SIGQUIT … A `kill 0` in the body of the script ends bash the same way`.
`toString("utf8")` is lossy: an invalid byte sequence becomes U+FFFD. Nothing
downstream re-validated - `commitOutput` in the executor checks the size,
`JSON.parse`, object-ness and an own `__proto__`, none of which notices a
substituted character - so bytes the script wrote were silently rewritten and
the step was a pass.
A step running `printf '{"token":"\xc3\x28abc"}' > "$ARGENT_OUTPUT"` came back
`{"ok":true,"output":{"token":"�(abc"}}`. The corrupted value flowed into
flow state for later steps to read and compare against, with no failure, no
note, and nothing that lets the author see the document differs from what the
script emitted. A `.mjs` step cannot reach that state, because there the
document is a live JavaScript value - so this was a difference between the two
languages the step result did not surface.
The read now decodes with a fatal `TextDecoder` and reports the bytes rather
than rewriting them. Reproduced through `argent flow run`: before, `✓ 1
script`; after, `✗ … the document the script wrote to $ARGENT_OUTPUT is not
valid UTF-8`.
Review found a regression in edbdf4d: when stderr went through a consumer (`exec 2> >(…)`) and a quiet background job held the streams open, the reason took the stderr line as it stood when bash exited - often nothing, since the consumer had not written yet - and dropped the script's error that 4edac01 reported. The line now follows how the settle ended: the last line overall when the streams closed on their own, the line stderr ended on before Argent's stop when what held them went quiet, and the line as bash exited only when a process was still writing at the settle's limit. A job's answer to the stop still never becomes the reason.
…t wrote Review found that 979c9c4 quoted the last non-blank line a refused bash candidate wrote to stderr. asdf, the version manager the fix was for, writes its reason first ("No version is set for command bash") and then lists the versions it has, one per line, so the refusal quoted "bash 5.2.37" - a line that reads as if the candidate were a bash. The refusal now quotes the first non-blank line. The docs say which line is quoted, and that only a candidate that exits without a version gets one.
Review found that 06b7891's longer settle ignored the request's abort: the timer and the abort listener are removed when the script's process exits, so a run cancelled while a job the script left running was still writing waited up to 3 s more (before: at most 0.5 s). The settle now takes the request's signal and ends at once; if output was still arriving, the log is marked cut as it is at the limit.
Review found that 851daee weakened "has finished its sweep by the time the step resolves": its spy delayed every fs.promises.rm call, and since the step now removes its own exchange directory through fs.promises.rm too, that removal outlasted the sweep and the case passed with the executor's `await pendingSweep` deleted. The spy now delays only the abandoned directory's removal, so the case fails again without the wait.
…le later Review found that 979c9c4, by piping a candidate's stderr to quote it, made a candidate that leaves a job holding stderr cost the probe's 250 ms settle on every .sh step: `close` now waited for stderr as well. The answer is on stdout, so the probe now answers once the candidate has exited and its stdout has ended, as it did before stderr was piped.
Review found that 06b7891's note and docs said Argent stopped a process still writing when the settle gave up. On Windows the tree stop reaches nothing once the runner has exited, and on macOS and Linux it cannot reach a job in a process group of its own - the page's own background-job rules say both. What holds everywhere is that Argent stops reading, which is what the note and the docs paragraph now say. The note no longer names the 3 s limit, which a cancelled run does not reach.
Review found the case that pins the log-cut note tight under load: its job wrote its first line 200 ms after bash exited, and a stall of that size between the runner's answer and the read of stderr would put the job's line in the reason. The job now waits 300 ms, still inside the 500 ms quiet window the case needs. The case's name and comment no longer say Argent stops the job, which it cannot do everywhere.
Review found that 06b7891 added a wait outside `timeout` - after the script exits, a step waits up to 3 s for a command that still writes to the log - while the page still said two waits sit outside it. The page now says three and describes the third. The claim that the step reports each wait is dropped: this one is reported only when it cuts the log.
Review found a regression in 851daee: removeTree built full paths, so a tree the script left deeper than the system's longest path - 1 024 bytes on macOS - failed with ENAMETOOLONG and stayed in $TMPDIR for good, since every later sweep failed at the same place. The rmSync it replaced walked such a tree by descriptor and removed it, after freezing the tool server. removeTree now moves a directory whose path has grown past 512 bytes up under the directory being removed before it walks it, which keeps every path it hands the system short. The main thread still does not stall.
Second review found that 4a5f9b5, by answering the probe once the candidate had exited and its stdout had ended, cut off the stderr line a refusal quotes when that line arrives after the candidate is gone: a wrapper that sends stderr through `tee`, or hands it to a job. The early answer is only needed for a candidate that answered with a version, so a refused one waits for `close` or the settle again, as it did before. The probe-timing case's grandchild now lives 1 s instead of 10.
Second review found that a1913b0 read the settle's "cut" as a sign that stderr was still being written, when a job chattering on stdout alone also ran the settle to its limit. With a stderr consumer in front of the script's error and such a job, the reason took the line as bash exited - empty, the consumer not having written yet - and dropped the error that 4edac01 reported. The line as bash exited is now used only when stderr itself was still being written as the settle ended.
Second review found four problems in the sentences this series added to the script-step reference. "Three waits sit outside it" undercounted, as "two" had: the stop's grace and the removal of the private directory also sit outside `timeout`. The page no longer counts them. "Argent stops reading the log" was not what happens at the 3 s mark - Argent stops waiting, and still reads what a stopped job writes - so it now says that. A 29-word sentence is split under the 25-word limit and names stderr, which is what now decides the line. The post-exit wait uses the same tense as the paragraph it repeats.
Second review found that 76b619e ended the post-exit wait at once for every cancelled run, including one cancelled before the script answered. That run was already stopped, and a job in a group of its own outlives the stop: before, the wait still saw it writing and marked the log cut; after, the log stopped short with no flag and no note. The settle now takes the abort only when it had not happened by the script's exit, so only a cancel during the wait itself ends it. The cancel case also pins that a job still writing then marks the log cut.
Second review found that removeTree (851daee) opened the directory it was handed with opendir, which follows a symbolic link. The stale sweep hands it every entry of $TMPDIR whose name carries the executor's prefix and a past stamp, so a link planted under such a name had the directory it pointed to emptied - where the recursive rm it replaced removed the link alone. A script that replaced its own exchange directory with a link got the same. removeTree now lstats the top and removes anything that is not a directory as itself; below the top a directory entry already says whether it is a link.
Second review found that removeTree (096afdd) moved a directory up only once its path passed 512 bytes, on the premise that a name adds at most 255. APFS takes 255 characters, which in UTF-8 is up to 765 bytes, so a file or directory with a long multibyte name under a 490-byte path got ENAMETOOLONG and was left in $TMPDIR for good. The directory is now moved once its path passes 257 bytes, so any name below it fits in the 1 023 bytes macOS takes. The docstring and the deep-tree case no longer say the rmSync it replaced walked such a tree: on Node 20, which CI runs, it fails the same way. The cleanup cases now clear their roots with `rm -rf`, since a throw from fs.rmSync in a finally would hide the failure that left the tree there.
Second review found that removeTree's move-up (096afdd) left an empty read-only directory behind for good. Moving a directory to another parent needs write permission on the directory itself, which a plain rmdir never asks for, so the rename was refused and the whole removal failed where the recursive rm it replaced succeeded. A refused move now leaves the directory where it is and walks it there: its own path still fits, and an empty directory needs nothing below it named.
Third review found that the only case guarding 8e3d9b1's threshold runs on macOS alone, while CI runs unit tests on Linux, where no file system takes a 765-byte name: putting REMOVE_TREE_HOIST_AT_BYTES back to 512 left every CI case green. The fs mock now records each rename, and a case pins that a directory whose path has passed 257 bytes is moved before it is walked - it fails at 512 on any host. The cleanup helper moves below the imports and drops a chmod no case needs, and the long-name case's comment no longer says one name alone passes 1 024 bytes.
Third review found that the comments added by 24ba6d7 put the line between a cancel that ends the post-exit wait and one that does not at the script's answer. The code reads the abort after the script's process has exited, a few milliseconds later; the comments now say so.
Third review found that fd41a28 let a job's stray stderr line become a bash step's reason: a mock server that logs requests on stdout keeps the settle open, and its one stderr line a second after bash exited ("listening on :8080") was taken over the script's error, which 4edac01 reported. After bash exits, stderr carries the script's own lines late - a consumer working through its backlog - and a job's lines; the first come as one run from the exit, the second whenever the job writes. The reason now takes the line stderr stood on when it first went quiet for 500 ms after the exit. Streams that close while stderr is still running on count in full; stderr that never goes quiet is a job still writing, and then the line is the one bash exited on. The docs say the same.
Third review found that removeTree (851daee) failed on an empty directory without read permission - `mkdir -m 000`, or a umask of 0777 - because opendir needs read permission where rmdir asks only the parent. The step noted EACCES, and every later sweep failed the same way, where the recursive rm it replaced removed the directory. An EACCES from opendir, before anything was listed, now falls through to that final rm; a directory with entries still fails into the note as before.
Fourth review found that f22f4b4's docs sentence said the reason takes the line bash exited on whenever stderr is never quiet for 500 ms. When the streams close on their own while stderr is still running, every line counts, which is what the code does and what the sentence before it implies. The pre-exit line applies only when the wait for the log reaches its limit with stderr still running; the page now says that.
Fourth review found that a block of the tool server's event loop across the 500 ms mark - a synchronous call in an I/O callback - made the reason freeze on the line before the script's error. After such a block the due timer runs before the poll that reads what arrived meanwhile, so both the quiet watch and the settle judged stderr quiet on stale data: with a stderr consumer and a surviving job, the reason ended with "step 1: seeding" instead of "FATAL: the real error". Each now lets one turn go by after a timer, so pending pipe data is read before the judgement, and the watch stops once the settle ends so a late check cannot take a line the stop provoked.
Fourth review found that a cancel landing within 500 ms of a stderr consumer's late line dropped that line from the reason: the settle ended on the abort before stderr had been quiet for 500 ms, and the reason fell back to the line as bash exited - empty, the consumer not having written yet. A cancel says nothing about whether a job is still writing, so a settle it ended now takes the stderr line as it stands.
Fifth review found that a cancel landing while a stderr consumer was still delivering the script's last lines - after "step 1" but before the error - made the reason "step 1: seeding": 76b619e let a cancel end the post-exit wait at once, and ede7863 then took the line as it stood. 4edac01 gave the error, because its 500 ms settle ran whatever the caller did. A cancel now ends the wait only once that first settle has passed, so the consumer gets as long as it always had and a cancel waits no longer than it did before the wait could stretch. The abort leaves the race once it has fired, so the loop does not spin while it waits.
# Conflicts: # packages/tool-server/src/tools/flows/flow-run.ts # packages/tool-server/src/tools/flows/flow-utils.ts
# Conflicts: # packages/configuration-core/src/config-schema.ts # packages/docs/docs/reference/configuration.mdx # packages/tool-server/src/tools/flows/flow-run.ts # packages/tool-server/src/tools/flows/flow-utils.ts
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
2 participants
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.
Lets a flow
scriptstep run a.shfile under Bash, next to the existing.mjssupport.Why
The setup or cleanup work before a flow is often a shell command:
curlan internal API,psql -fa seed file,adb shella setting. Today each of those needs an.mjsfile whose whole body is onechild_processcall. Now the step can point at the shell script itself:There is no new directive, key or tool. The file extension picks the interpreter, and
flow-add-scriptrecords a.shstep the same way it records an.mjsone.What a
.shstep does.mjsstep. The file needs no execute permission, and the#!line is ignored.set -euo pipefailthat line is the error of the command that stopped the script, so a plain script gets a useful reason with no Argent-specific code:scriptLogin the report,logfromflow-add-script, and below the step inargent flow run), with the same size limits and secret redaction as an.mjsstep.timeout, cancellation and the script queue work as they do for.mjs.Choosing Bash
First hit wins:
scripts.bash, a new config key that holds an absolute path. It is global-only: an absolute path belongs to one machine, and a committed project value would break.shsteps for teammates on another OS. An unusable value fails the step and names the key, andargent config setaccepts only an absolute path.bashon the tool-server'sPATH./bin/bashand/usr/bin/bash; on Windows, the usual Git for Windows and Scoop install locations.Each candidate must answer as Bash before it is used. On Windows, candidates under
%SystemRoot%are skipped:System32\bash.exeis the WSL launcher, which runs the file inside a Linux distribution where the project path does not exist.How it works
The executor forks the same
flow-script-runner.mjsit uses for.mjs, and in bash mode the runner spawns Bash as its child instead of importing a module. Both languages therefore share one path: Bash and everything it starts sit in the runner's process group, which the existing group stop and the deadline and lifeline watchdogs already reach, and the verdict, log capture and redaction are unchanged. Descriptors 3-5 point at the null device in Bash, so a script cannot write to the runner's protocol channel and forge a verdict.The flow's output document reaches Bash as a file named by
$ARGENT_OUTPUT, inside a private temp directory created for the step and removed when it ends. Bash steps also sweep directories that a crashed tool-server left behind.On Windows, which has no process groups, both watchdogs now also run
taskkill /t, so a tool-server that dies there no longer leaves a script's subprocesses running. This applies to.mjssteps too.Docs and tests
reference/flow-yaml.mdx(a Bash section, and the script log for both languages),reference/configuration.mdx(scripts.bash),features/flows.mdx,reference/tools.mdx, and theargent-create-flowskill references.Most deleted lines in the diff are comments removed from the files it touches, mainly
flow-run.tsandflow-utils.ts. They do not change behavior.