mirror of
https://github.com/software-mansion/argent.git
synced 2026-09-14 19:27:14 +08:00
feat/flow-bash-scripts
6 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Hubert Gancarczyk
|
aa029bd6ec | chore: remove comments | ||
|
Hubert Gancarczyk
|
58822a6abd |
feat(flows): report a bash step's output, and take its reason from stderr
A `.sh` step now reports what it prints the way a `.mjs` step does on `main`: bash inherits the runner's stdout and stderr, which are the pipes `ScriptLogCapture` reads, so the log, its limits, the drain and the secret scrub apply with no bash-specific code. The failure reason comes from stderr rather than from a file. bash has no `throw`; what it has is an exit code and stderr, and every command a script runs already writes its error there. So a non-zero exit reports the runner's exit line followed by the last non-blank line the script wrote to stderr - under `set -euo pipefail`, the error of the command that stopped it, with no Argent-specific code in the script. The runner never reads stderr: bash writes straight into the parent's pipe. The executor's capture keeps the line (`LastLineTracker`), from the raw text ahead of the scrub and the limits, so a script that floods stderr and then says why still says it. Only the head of a line is kept - 1,000 characters and the usual omission marker - so a cut always lands at the end of the message, where `redactTruncated` repairs one, and the line joins the failure text before it is clamped and redacted. A line of nothing but whitespace is blank at any length. stdout is never the reason, and only the `exit` kind gets the line. `$ARGENT_REASON` goes, and with it the runner's bounded read of the reason file, its kept-count marker and `REASON_KEPT_RE`, its UTF-8 refusal, the CRLF hint for a reason that strayed, and the second exchange file. `ARGENT_REASON` is an ordinary environment name again; the CRLF hint for `$ARGENT_OUTPUT` stays. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X2EFhLrnjAkfgPkN1fva1k |
||
|
Hubert Gancarczyk
|
5d30cdc031 |
fix(flows): carry the restored log capture through a bash step's run
With #970 no longer under this branch, `main`'s `ScriptLogCapture` is back, but the split this branch made - `prepare` handing a `ChildRun` to `runChild` - still carried #970's `StderrSignalWatch` in the run. The capture was built in one function and read in the other, so the file did not compile. The run now carries the capture itself. The three step-run helpers #970 added and this branch's bash tests call (`markPath`, `markingScript`, `readMark`) come back with it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X2EFhLrnjAkfgPkN1fva1k |
||
|
Hubert Gancarczyk
|
6fff86eaf3 |
feat(flows): run a .sh script step under bash
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. |
||
|
Hubert Gancarczyk
|
0f90d182b2 |
feat(flows): record a script step by running it (#940)
#864 (the executor) and #865 (the `script:` step) are merged. This branch now sits directly on `main` as a single commit, so the diff and the commit list are this change alone. A `script:` step could only enter a flow by being typed into the YAML after the recording ended. The authoring skill said so in as many words, and listed it among the legal unrecorded insertions. That made it the one directive exempt from every rule the recorder exists to enforce, and it cost two things: - **The script never ran while the flow was authored.** It first executed during the closing replay, where a failure reads as a flow that was never right rather than a script that needs one more attempt. Worse, the device steps recorded after it were walked against backend state the author had established by hand — so the recording proved nothing about whether the script establishes that same state. - **The author never saw what the script returned.** A later release reads `{{output:user.id}}`; the only way to know that path exists is to have seen the document. This adds one MCP tool, `flow-add-script`, that runs a local `.mjs` and records the step it ran. **No new flow syntax** — the `script:` step is unchanged, and this is a second way for one to enter a flow file. ```text flow-add-script { name: "checkout", project_root: "/Users/dev/shop", path: "../../scripts/seed-order.mjs", timeout: 60000 } ``` ## What it adds - **One shared execution path.** Path resolution, the on-disk casing guard, the executor call and the verdict were private to `flow-run.ts`. They are now `runFlowScriptStep` in `flow-script-step.ts`, with the file-ref helpers in `flow-file-refs.ts`, and `runScriptStep` is that call plus its `ExecState` plumbing. This is the load-bearing change: "the recording runs the script exactly as replay will" is one function, not two code paths that agree today. - **A failure records nothing.** The opposite of `flow-add-step`, deliberately. A failed script did not establish the state the rest of the walkthrough would then be recorded against, so an appended red step costs every step after it, not just itself. The `message` also says whether a child actually *ran* — a path that resolved to no file forked nothing, while a script that threw halfway did not roll back — because the author's next move is a retry or a cleanup and they cannot pick without knowing. - **The returned document, as JSON text.** Text rather than an object because this is the first tool result carrying bytes the server did not author: every result is deep-walked for `__argentClientFile` and `__argentArtifact` directives, which match on shape alone and write or fetch files on the agent's machine. As a string it inherits none of that. Bounded at 64 KiB with `outputTruncated`, for the reason `log` is bounded. - **`{{output:` refused by `parseFlow`.** Surfacing the document invites an author to write a reference that has no meaning yet — it would be printed literally by a step that passes, green and wrong. The scan runs over exactly the field list the output release declares supported, so it is a seam that release converts from reject to resolve rather than scaffolding it deletes. Patterns are excluded: a `{{` in a regular expression is a quantifier the author meant. - **Two boundaries.** A `client`-mode recording is refused — its `.mjs` never left the agent's machine, the same anchor reason `run:`, `snapshot` and uploaded flows are refused. And the tool is `longRunning`, so the MCP adapter's 30s fetch abort cannot retry a call whose whole purpose is a side effect; the comment records what that flag does *not* close, and where the rest belongs. - **`flow-add-step` says when the call already ran.** The `{{output:` refusal is the one refusal this release adds on a path where the tool call has already fired, and its message asks for an edit-and-retry — which repeats the device action. Verified: a `keyboard` step recorded twice left `{{output:code}}{{output:code}}` in the field. That one refusal now carries what already happened. ## Documentation Rule 5 of `argent-create-flow` no longer lists `script:` as a legal unrecorded insertion — leaving it there would tell agents to keep doing the thing this PR removes. `live-authoring.md` gains the recorder contract, the return fields that are not self-evident, and the ordering rule: a setup script is recorded *before* the `restart-app` it prepares state for, because that is where it runs at replay. A leading script does not make the flow a fragment. `qa-flows` and `reliability-and-recovery` follow the count and the wording. ## Verification `npm run build`, `npx eslint . --max-warnings 0`, `npx prettier --check` over tracked files, and `npm run typecheck:tests -w @argent/tool-server` are green. `npm test -w @argent/tool-server`: 358 files, 4657 passed, 1 skipped. Exercised end to end against a live tool server built from this branch, not only in tests — recorded a flow, then replayed the saved YAML through `argent flow run` with no hand editing: ``` flow-add-script → status "pass", durationMs 124 log: "seeding order for the flow\nwarn: using the fake backend\n" outputJson: {"order":{"id":"ord_4417","total":39.5}, "user":{"id":"usr_9","email":"buyer@example.com"}} recorded: 1. script: ../../scripts/seed-order.mjs (timeout 30000ms) ``` ```yaml steps: - script: path: ../../scripts/seed-order.mjs timeout: 30000 - echo: order seeded; expect Home ``` ``` Flow "checkout" ✓ 1 script ../../scripts/seed-order.mjs │ seeding order for the flow │ warn: using the fake backend › order seeded; expect Home PASS — 1 passed, 0 failed, 0 errored, 0 skipped ``` The three refusals, live on the same recording: - A script that threw after creating records: `status "fail"`, `stepCount` unchanged at 1, and the message naming the side effects that were not rolled back. - A path resolving to no file: refused before any fork, no `durationMs`, and "Nothing ran, so there is nothing to clean up". - `flow-add-echo` holding `{{output:user.id}}`: refused with the author-facing message naming the step, the field and the later release. No device is involved at any point — a recording may open with a seeding script before it boots anything. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added the `flow-add-script` tool to run local `.mjs` scripts and record successful executions in flows. * Added script status, output, duration, logs, and failure details to recording results. * Added clearer handling for script cancellation, timeouts, missing files, and path casing. * **Documentation** * Updated flow guidance to clarify end-to-end classification and script setup or cleanup usage. * Documented script path requirements, failure conditions, and recording workflows. * **Bug Fixes** * Flows now reject unsupported output-reference templates with more precise diagnostics. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Hubert Gancarczyk
|
9c4bc9ef5b |
feat(flows): run an external script as a flow step (#865)
Stacked on #864, which adds the executor. This branch adds the `script:` step itself: the YAML directive, the runner integration, the CLI and MCP rendering, and the authoring reference. ```yaml - script: { path: ../../scripts/seed-order.mjs } - script: { path: ../../scripts/seed-order.mjs, timeout: 60000 } ``` A `script:` step runs an external `.mjs` file from the project, in a fresh Node process. What the script does is its own business — seeding an order, creating a test account, cleaning up what a run left behind are the reasons flows have wanted one. It drives no device, so a flow of nothing but scripts runs with nothing booted. ## What it adds - **The directive.** The value is always a map; `path` is required and obeys the same name rules a `run:` target does, spelled for `.mjs`. It resolves against the directory of the flow file containing the step, so a fragment reaches the same script whichever flow composed it. A mis-cased path is refused rather than run, because macOS and Windows would open it and Linux CI would not. - **The report.** The script's stdout and stderr come back on the step and print under the step line, on a pass as well as a failure — it is the only record of what the step did to the backend. Both renderers (CLI, MCP) carry it, live and buffered. - **The fail/error split.** A `fail` is the script's own answer; an `error` is the host's. That is what lets CI tell a regression from the machine it ran on. - **Two boundaries.** An uploaded flow carrying a `script:` step is refused before anything runs — its `.mjs` never left the client. A flow whose script step sits beside a `run:` step still resolves a device. ## Review follow-ups The last three commits answer a review of the three above them. - A launch behind a leading `script:` was not read as the flow's leading launch, so a Chromium flow that seeded a backend first hoisted no boot, bound to whatever browser happened to be up, and passed a launch it never performed. The same lead-in walked a fragment past the `executionPrerequisite` refusal its `echo:`-led twin is given. Both readers now share one exhaustively-switched predicate. - Coverage: the whole failure-kind table, the mis-cased arm CI never ran, the two unreached `scriptFileProblem` arms, symlink resolution for a script path, and the CLI live-progress closure that no test had executed. - Docs: the reference claimed the log was secret-redacted (it is not), never mentioned the environment allowlist, the working directory, the time-limit ceiling or the log caps, stated a fail/error rule that was not the code's, and was missing from its own table of contents. The skills stated a closed list of legal unrecorded insertions that excluded the only way this step can ever be written. ## Verification `npm run build`, `npx eslint . --max-warnings 0`, `npx prettier --check .`, `npm run knip`, `npm run typecheck:scripts` and `npm run test:scripts` are green, as are the three `tsconfig.test.json` typechecks. Suites: tool-server 357 files / 4552 passed, `@argent/cli` 469, `@argent/mcp` 83, `@argent/registry` 90. Reproduced end to end against a real tool server and a real Electron app, not only in tests: a flow of `script:` then `launch:` now boots its own Chromium instance and reports it, where before it attached to a stray browser and reported a green launch for an app path that did not exist. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added support for local `.mjs` scripts as flow steps, including configurable timeouts, validation, cancellation, and device-free execution. - Script output is displayed in CLI runs and included in MCP flow results, with indentation and truncation notices. - Script failures now provide clearer diagnostics and verdicts. - Flow authoring tools recognize and guide creation of script steps. - **Documentation** - Added guidance for script syntax, execution limits, output handling, classification, and remote-run restrictions. - Updated flow creation and QA guidance to include script steps. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Hubert Gancarczyk <claude-hubert.gancarczyk@swmansion.com> |