mirror of
https://github.com/software-mansion/argent.git
synced 2026-09-14 19:27:14 +08:00
feat/android-atomic-clear
4 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Hubert Gancarczyk
|
afd722924f |
feat(flow): support concurrent flow recordings (#574)
## Problem
Only one flow could be recorded at a time, anywhere on the machine.
Recording state was three module globals in `flow-utils.ts` — harmless
in a per-client process, but the tool-server is a **host-wide singleton
per install bundle**. Every MCP client, every subagent sharing a
parent's connection, every `argent` CLI call and every project on the
machine attaches to the same process, the same `Registry`, and those
same three globals. HTTP requests are served concurrently with no
serialization.
Three failures followed, all reproduced against the pre-change code:
1. **A second recording clobbered the first.** `startRecordingSession`
just overwrote, so the first agent's next `flow-add-step` silently
appended into the *other* agent's flow file — in a different project.
2. **Any agent could finish any agent's recording.**
`flow-finish-recording`'s schema was literally `z.object({})`.
3. **Replay stomped recording.** `resolveFlowFilePath` called
`setActiveProjectRoot`, so a `flow-execute` in project B rebound the
global root mid-recording of project A. Already worked around, locally
only, in `flow-add-step`.
## Approach
Recording state is now a `Map` keyed by the resolved flow file path,
`<project_root>/.argent/flows/<name>.yaml` — the identity of the
artifact being built, so "same key" means "same output file" and two
sessions on one key are a genuine collision rather than an accident of
scoping. `flow-add-step`, `flow-add-echo` and `flow-finish-recording`
each take `name` + `project_root`, so every call is self-contained;
required rather than optional-with-fallback, since a fallback would
succeed while an agent is alone and fail only once a second recording
exists.
`resolveFlowFilePath` is pure, which fixes defect 3 and retires
`FLOW_PROJECT_ROOT_REQUIRED` (the state it guarded can no longer exist).
Rejected alternatives — keying by device udid (flows are deliberately
device-portable, and `flow-add-echo`/`flow-finish-recording` have no
device at all), an opaque `recordingId` (an agent that loses the token
cannot recover), and per-caller identity from the transport
(`ToolContext` carries no caller identity, and subagents share their
parent's MCP connection, so it would fail the exact scenario being
fixed).
### Locking, and why readers needed more than a lock
Every mutation of one flow file runs under a lock keyed by that path —
appends, `flow-start-recording`'s truncate-and-register, and
`flow-finish-recording`'s read-summarize-clear. Keyed by file rather
than owned by the session, because a restart *replaces* the session: a
session-owned lock cannot exclude the operation that supersedes it. Per
file, not global, so two recordings never queue behind each other. An
append whose session was finished, restarted or evicted meanwhile fails
with `FLOW_NO_ACTIVE_RECORDING` rather than writing into a different
take.
The lock serializes writers only, and no reader of a flow YAML can join
it — `flow-execute`'s own load, its `run:` fragment load,
`flow-read-prerequisite`, `flow-add-step`'s sibling-fragment check, and
the `argent` CLI reading from another process entirely, where an
in-process lock cannot reach. Both writers used a plain `fs.writeFile`,
which opens `O_TRUNC`, so a reader could land between the truncate and
the write. That case is silent: `parseFlow("")` returns `{ steps: [] }`
with no error and `summarize` derives `ok` from "no failures", so a
`flow-execute` racing an append reports a **top-level PASS having
replayed zero steps**. Both writes now go through a sibling temp file
and a rename, so a reader sees either the whole old file or the whole
new one — and unlike the lock, that also holds cross-process.
### `stop-all-simulator-servers` gets a `devices` scope
Every agent is told to call this at session end, and unscoped it walked
the whole registry — so agent A wrapping up tore down agent B's devtools
mid-recording, degrading B's flow to brittle coordinate taps. It now
takes an optional `devices` scope and reports `unmatched` ids, so a
mistyped id doesn't read as a clean machine.
Which namespaces count as "owned by a device" turned out to be the
load-bearing part, because nothing cascades to most of them and
`unmatched` turns any omission into an actively wrong diagnosis — a
correct device id reported as a typo. The set now covers `AXService`
(the in-sim ax daemon, spawned `--timeout 3600`; an iOS session that
only ran boot/launch/describe owns this and nothing else),
`ScreenRecordingSession` (an ffmpeg child plus the touch-visualizer
overlay it enabled on the device), `NativeProfilerSession` (an xctrace
child, or an on-device perfetto process and its trace file) and
`JsRuntimeDebugger` (a bound loopback server, the CDP socket to Metro, a
log handle). The debugger URNs interpose the Metro port —
`<ns>:<port>:<deviceId>` — so the matcher understands both shapes,
consuming only the first colon so a wireless adb serial after the port
still compares whole.
`stop-simulator-server` shared none of this and had drifted: it looked
URNs up with an exact, case-sensitive `services.get()`, so a lower-cased
UDID silently no-op'd there while the scoped stop-all reaped it. Both
tools now share one matcher. Their namespace *sets* stay deliberately
different, documented where they are defined — `stop-simulator-server`
is also the documented recovery for a wedged transport, and widening it
to devtools/AX would make a routine retry drop the native-devtools
connection another agent's recording depends on, which is the hazard the
`devices` scope exists to prevent.
## Verification
**End-to-end, three devices, one shared tool-server** (iPhone 16 Pro,
iPhone 17 Pro, Pixel 3a): three interleaved recordings, plus an
unrelated `flow-execute` in a fourth project while all three were live.
Both iOS devices held their own live native-devtools concurrently (two
per-UDID sockets, `connected: true`). Each YAML contained exactly its
own steps in order, taps captured as selectors rather than coordinates,
and neither picked up another's or the third root's path. All three
replayed green — also through the **released 0.17.0 CLI** against the
modified server, which checks wire compatibility. A device-scoped
teardown of sim 1 left sim 2 and the emulator running.
Re-run against the final build:
- Two interleaved recordings across two project roots on one device,
with a **separate process** polling one of the flow files as fast as it
could for the duration. Zero empty or truncated observations; each YAML
held exactly its own steps; both replayed green (6/6 and 5/5). Against
the pre-change in-place write the same reader observes zero-step reads
interleaved among the real ones — the silent-PASS input.
- A scoped teardown of the iPhone reaped `AXService` and
`NativeDevtools` for that UDID only, reported no `unmatched`, and left
the second sim and the emulator running. Lower-casing the UDID matched;
a genuinely bogus id was still reported.
- With a screen recording live on the Pixel 3a, a scoped teardown reaped
`ScreenRecordingSession` and the **ffmpeg child actually exited** (1 →
0). Before the fix it survived to the 180 s cap while the tool called
the correct serial a typo.
The pre-change repros were re-run afterwards to confirm observed
behavior changed, not just the tests.
`npx vitest run` in `packages/tool-server` (295 files, 3154 tests) and
`packages/argent-cli` (280), `tsc --build`, `tsc --noEmit` on both test
projects, eslint and prettier — all green.
## Review notes
Several rounds of review found real defects in this change; each was
reproduced before fixing and re-verified after. Worth knowing when
reading the diff, since the tests exist to pin them:
- `flow-start-recording` truncated the `.yaml` outside any lock, so a
step from the take being discarded landed in the freshly reset file and
was reported as success (26-29 of 200 runs).
- `flow-finish-recording` never took the lock; its `await fs.readFile`
was a yield during which a concurrent append committed, leaving the
reported summary disagreeing with disk (11-16 of 200).
- A failed finish destroyed the recording. Hand-editing the `.yaml`
mid-recording is a documented workflow, so `parseFlow` can legitimately
throw — and the error told the agent to call `flow-start-recording`,
which truncates the very take it would recover.
- `unmatched` reported a device this session had just stopped, because
`disposeService` returns a node to IDLE and keeps it.
- A bare IP claimed every wireless-adb device at that address, since an
adb serial is itself `ip:port`.
- The eviction backstop stamped with `Date.now()`; ties at millisecond
resolution let it drop the session that was just used.
- `requireRecordingSession` ended with "Call flow-start-recording first"
— reached for a key that was finished, superseded or evicted as well as
one never started, and on those branches the file on disk is fully
populated while that call truncates it and reports no `restarted`.
- The atomic write regressed twice on its own terms: its scratch name
was derived from the flow file's basename, which has no length cap, so a
long flow name that appended fine before failed `ENAMETOOLONG`; and the
cleanup guard began after the temp file was created, leaking a scratch
file into a committed directory on any write that failed after opening.
- Two tests passed against implementations they claimed to reject: the
`stop-simulator-server` narrowness guard used a udid that classifies as
*android*, where the iOS-only services it guards can never appear, so
widening the iOS branch kept it green; and the client-mode finish
asserted only the *shape* of `savedTo`, never its content, though that
directive is the only thing that lands the file in client mode.
## Notes for reviewers
- Client/remote persist mode is the least-exercised path, and now has
its own concurrency tests (overlapping appends, and a restart
superseding an in-flight one) — it previously had none. The
`project_root` key is byte-stable across the file boundary (`kind:
"probe"` passes it through unchanged), which is what keeps a remote
recording addressable from `flow-add-step`, which declares no file
input.
- Two spellings of one flow file still mint two sessions — and two
independent locks — over one file, which bypasses every guarantee here.
Two ways in: the root spelled two ways (a symlink, or a case variant on
APFS), and the likelier one, the *name* cased two ways on a
case-insensitive volume. Neither is normalized away because the correct
normalization is the filesystem's: case-folding the key would wrongly
merge two distinct flows on ext4, and resolving symlinks is impossible
in client mode, where the root does not exist on this host. Documented
at `getFlowPath`.
- No recording tool can detect that another agent took its key.
`flow-add-step`, `flow-add-echo` and `flow-finish-recording` all
re-resolve the key on every call, so after a takeover they act on
whichever take is now live and report success: an append lands in the
other agent's file, and a finish finishes and clears it. The only case
that fails loudly is the narrow one the liveness check covers - an
append already IN FLIGHT when the restart lands. There is no caller
identity to key ownership on (see the rejected alternatives above), so
this is spelled out for the agent under "Pick a name unique to your
task" in `argent-create-flow` rather than mechanized.
- Skew hazard in one direction only: a *new* client against an *older*
tool-server. Zod strips the undeclared `name`/`project_root`, and the
old server falls back to its module globals — so concurrent recordings
silently collapse onto one session. The other direction (released 0.17.0
CLI against this server) was verified end-to-end.
- `packages/argent-private/docs/reference.md` still describes
`flow-add-step` as appending to "the current recording", and names two
tool ids that do not exist (`flow-insert-echo`/`flow-run` are file
names; the ids are `flow-add-echo`/`flow-execute`). It is a submodule,
so the fix cannot ride on this branch.
- A restart reported `discardedSteps` from the superseded session's
in-memory flow while it truncates the *file*. Hand-editing the `.yaml`
mid-recording is documented, so the two diverge: four steps wiped, one
reported. It now counts the file, and reports no number at all when that
file cannot be read or parsed - 0 is the answer a genuinely empty take
gives. Client mode still counts from memory (this host cannot see the
client's file), and the description now says which mode it is promising.
- `ChromiumJsRuntimeDebugger` cascades from `ChromiumCdp` and was
therefore already being torn down - silently, while `NetworkInspector`
and `ReactProfilerSession` cascade the same way and were named.
`stopped` is documented as the services that were live and got shut
down, so it is listed now too.
- The client-mode case named for a superseded in-flight append restarted
*between* calls, so the next append re-resolved the key and succeeded -
the guard was never reached, and neutering it left the whole file green.
It now parks an append in its live sub-tool call across the restart.
- `stop-tools.test.ts`' mock handed the tool the live service map where
the real `getSnapshot` copies, so a cascade rewrote the state the sweep
was still reading and the answer depended on map insertion order. The
mock copies and recurses like `_teardown` now, and the cascade case runs
under both orders.
- Prose the code contradicted, each verified against source before
rewriting: Vega owns `JsRuntimeDebugger`/`NetworkInspector` once the
debugger has run (and can match anything at all through an ERROR node);
"nothing here cascades" was denied twice in its own file; an iOS
boot/launch/describe session owns `NativeDevtools` too; `:tcp` is
mintable but unreached in production; the tool-server is a singleton
*per install bundle*, not per machine; `argent flow list` does enumerate
`.argent/flows`; the stop-tools comments narrated a "before" that only
existed on this branch.
Found while verifying, out of scope, worth their own issues: a `tool:
flow-execute` self-reference recurses past both cycle guards (`run:` is
guarded, a nested `flow-execute` reseeds the stack); react-profiler
artifacts collide host-wide on a second-resolution filename;
`stop-metro` is port-scoped and kills another agent's bundler;
`debugger-reload-metro`'s HTTP fallback broadcasts to every attached
client. A tool's `capability` is enforced only by the HTTP layer, not by
`registry.invokeTool`, so a call that reaches the registry another way -
`flow-add-step` takes `command` as a bare string - can resolve a service
the tool declares it does not support, leaving an ERROR node behind
(e.g. `SimulatorServer:<vega serial>`).
---------
Co-authored-by: Hubert Gancarczyk <claude-hubert.gancarczyk@swmansion.com>
|
||
|
filip131311
|
3fff176a32 |
fix(cli): let a boolean flag take the value you typed (#699)
Fixes #586. ## The bug `--includeChildren false` left the flag **true** and silently dropped the word, so the tool ran with the opposite of what was asked. The reporter spent a while chasing a filter bug that did not exist — the output was simply the unfiltered kind. Reproduced on 0.18.1, device-free, with a discriminator that shows the flag's value directly: ``` $ argent run screenshot-diff --udid U --baselinePath A.png --currentPath B.png --captureCurrent=false { "summary": "Screenshot diff summary … pixel_mismatch: 1.79% …" <-- false, diff runs $ argent run screenshot-diff --udid U --baselinePath A.png --currentPath B.png --captureCurrent false [Tool:screenshot-diff] Provide either currentPath or captureCurrent, not both. <-- stayed TRUE ``` That message is the whole problem in one line: the user typed `false`, and the tool refuses on the grounds that they asked to capture. ## Why the old behaviour existed, and why it does not need to The parser deliberately declined to look ahead: > `// Bare boolean flag: true. Don't consume next argv to avoid surprising behavior when a positional follows.` That protects a case which does not exist. `parseFlags` has exactly one caller, and **`positional` is never read anywhere in the repo** — every bare token is collected and dropped, for booleans and for every other type. There is no positional to steal. The module header had documented the space form as legal all along: ``` // --name value (string / number / integer / boolean-with-value) ``` This matters because several tool booleans **default to true**, so `false` is the only way to say what you mean: `screenshot.includeImageInContext`, `network-request.includeBody`, `debugger-component-tree.onScreenOnly`, `debugger-inspect-element.resolveSourceMaps`. ## What changed A boolean flag consumes a following token when it is exactly the word `true`/`false`, case-insensitively. `1`/`0` and everything else are left alone — a bare number after a switch is more likely an unrelated argument than a value, and guessing at an ambiguous token is what caused this bug. `--flag -- false` still keeps the word positional. Three consequences are handled here rather than left to bite later: - **`argent run` now reports arguments it ignored.** This is what stops `--flag 0` and `--flag yes` being the same silent inversion, in a syntax users are now being taught works. stderr, so `--json` stays parseable. - **`--no-flag true` / `--no-flag false` now name the positive form** instead of quietly meaning false. One is a contradiction, the other a double negative. - **`--flag=True` used to throw** while `--flag True` would now work — the inline coercion was case-sensitive and did not trim. Both paths now share one helper, so a single word cannot get two answers one function apart. The **flow** and **lens** parsers take switches that accept no value at all, and would have silently ignored the syntax their users just learned elsewhere. They now refuse it — in the flow parser that also stops `false` being swallowed as the flow name, since its first bare token is the flow to run. `--help` gains **one legend line** for tools with a boolean field, rather than widening the flag column on every field of every tool: ``` Booleans: --flag or --flag true sets true; --flag false, --flag=false, or --no-flag sets false. ``` It deliberately does not begin with `--`. `scripts/e2e-full/lib/discover-tools.sh` reads any such line in the `Flags:` section as a flag row and takes the first `--token` as its name, so a legend starting with a flag would inject a phantom flag into every tool model it builds. **There is a test pinning that shape**, verified live: running the harness's awk over the new help output yields exactly the real flags and no legend row. ## Verified live ``` --captureCurrent false -> diff runs (was: "Provide either currentPath or captureCurrent, not both.") --captureCurrent=false -> unchanged --captureCurrent 0 -> "Note: ignoring unused argument(s): 0. Pass values as --flag <value> or --flag=<value>." screenshot --help -> legend present; harness awk parses only the real flags ``` 25 new tests; 8 confirmed to fail before and pass after. The rest pin what deliberately did *not* change: bare flag still true, a following flag or non-boolean word left alone, `1`/`0` still positional in the space form, `--` escape, string and unknown fields untouched, `--no-flag` semantics, boolean arrays, last-write-wins. The parser had **zero** boolean coverage before — which is why this survived. Full CLI suite 298 passing; lint, prettier and `typecheck:tests` clean. ## Scope and neighbours Only `flag-parser.ts` (the schema-driven parser for `argent run`) changes semantics. The other CLI parsers take fixed, default-off switches where `false` has no coherent meaning; three of them already reject a stray token outright, and the two that did not now do. `flow.ts`'s deliberate rejection of `--flag=value` is untouched. **#648** also edits `flag-parser.ts` (rewrites `renderFlagName`, inserts helpers at the same anchor). Behaviourally compatible — it keeps boolean → bare `--name` — so this is a textual conflict only, and either order works. ## Found while here, not fixed `scripts/e2e-full/lib/discover-tools.sh:60-64` detects flag kinds with `\bboolean\b` in awk, where `\b` is **backspace**, not a word boundary — so it never matches and every non-enum flag is typed `unknown`. That makes the `bad-type` validation case downstream unreachable. Pre-existing and unrelated to this fix; filing separately. --- > **Stacked on #648** (`filip/run-cli-validation-messages`). Both rewrite the same region of `packages/argent-cli/src/flag-parser.ts` — #648 factors out `flagNameFor`/`isJsonField` and rewrites `renderFlagName`, this one adds `booleanWord` beside them and a boolean legend to `formatSchemaUsage`. Merging them independently conflicts; this branch is rebased on #648, so review only the top commit. GitHub retargets it to `main` when #648 merges. --------- Co-authored-by: Filip131311 <f.kaminski2000@gmail.com> |
||
|
Ignacy Łątka
|
c5bab2a83d |
fix(cli): let a tool's own --args field win over the whole-payload flag (#458)
## Bug
`argent run flow-add-step` declares its own `args` field — a JSON string
holding the recorded step's tool arguments. But the CLI flag parser
hardcoded `--args` as a whole-payload escape hatch, so `--args` shadowed
the tool's own `args` field. The documented per-flag form
```
argent run flow-add-step --command gesture-tap --args '{"udid":"...","x":0.5,"y":0.5}'
```
silently dropped the step arguments: the `--args` JSON was consumed as
the whole payload and the tool's `args` field stayed `undefined`,
producing a misleading downstream validation error. Only the nested
full-payload form worked.
## Fix
Make `--args` schema-aware in `parseFlags()`:
- When the tool's schema declares a property named `args`
(`schema.properties.args`), `--args <value>` is routed through the
normal per-field handling and coerced by its declared type (a `string`
for flow-add-step, so the raw JSON string passes straight into the
`args` field). Both `--args value` and `--args=value` forms are
supported.
- When the schema does NOT declare `args`, the existing whole-payload
behavior is preserved exactly, including the `--args -` stdin sentinel.
`printToolHelp()` no longer prints the global whole-payload `--args
<json>` / `--args -` lines for tools that declare their own `args`
field, so the help stops contradicting the per-field `--args` shown in
the schema flags.
The change is general: any tool with an `args` field benefits, with no
special-casing by tool name.
## Verification
- `npm run build` (root tsc) — clean.
- `npm test -w @argent/cli` — 172 passed, including 6 new `parseFlags`
cases covering schema-with-`args` (space-separated + inline forms, plus
sibling fields) and schema-without-`args` (whole-payload preserved,
`--args -` sentinel, no-schema fallback).
- Prettier + ESLint (`--max-warnings 0`) clean on the touched files.
Full CLI end-to-end (running `flow-add-step` against a live tool-server
with an active flow recording) was not exercised here; the added unit
tests prove the parser behavior.
Fixes #452
---
### CLI end-to-end verification
Ran the documented per-flag form against a live tool-server with an
active flow recording:
- **Before (global argent 0.13.0):** `argent run flow-add-step --command
gesture-tap --args '{"udid":"...","x":0.5,"y":0.35}'` → `Invalid params
for tool "gesture-tap": udid expected string, received undefined ...`
(args dropped).
- **After (this branch):** same command → `{ "message": "Step added to
\"colltest\" flow", "toolResult": { "tapped": true } }`, and the
recorded YAML contains the step **with** its args (`udid`/`x`/`y`). The
per-flag form now works as documented.
|
||
|
Ignacy Łątka
|
a7722df8a2 |
fix(cli): reject empty numeric flags and array/-json interleave crash (#432)
Fixes two related defects in `packages/argent-cli/src/flag-parser.ts`,
the argv-to-JSON flag parser used by `argent run`. The package
previously had no dedicated test file for it; this PR adds one covering
both fixes.
## Defect A: empty/whitespace numeric values coerced to 0
`coerceScalar` validates number/integer fields with `Number.isNaN` /
`Number.isInteger`, but `Number("")` and `Number(" ")` both evaluate to
`0`, so those guards pass. An empty or whitespace-only value was
therefore silently accepted as `0` instead of being rejected.
Reachable via `--x=` (empty inline), `--x ""`, and `--x " "`.
- Observed: `parseFlags(["--x="], { type: "object", properties: { x: {
type: "number" } } })` returned `{ x: 0 }`.
- Expected: rejected, the same way a non-numeric string like `"abc"`
already is.
Fix: reject an empty or whitespace-only value up front for
number/integer types, throwing the same `FlagParseException` the
function already throws for `"abc"` (`--<flag> expected a
number/integer, got "<raw>"`). Valid numbers are unaffected.
## Defect B: array + `-json` interleave throws a raw TypeError
`seenArrayFields` is only populated by the array branch, but the
`--field-json` escape hatch writes `args[fieldName]` directly and can
set it to a non-array. If a field was already seen as an array, a later
array-form occurrence ran `(args[flag] as unknown[]).push(...)` on a
non-array value and threw a raw `TypeError`. `run.ts` only catches
`FlagParseException` and re-throws anything else, so the user got an
uncaught stack trace instead of a clean error.
Repro:
```
parseFlags(
["--tags", "a", "--tags-json", '"b"', "--tags", "c"],
{ type: "object", properties: { tags: { type: "array", items: { type: "string" } } } }
)
```
- Observed: throws `TypeError: args[flag].push is not a function`
(uncaught, surfaces as a stack trace through `run.ts`).
- Expected: a clean validation error.
Fix: in the array branch, when `args[flag]` is not actually an array
(because `--field-json` replaced it), throw a `FlagParseException`
explaining the `--field` and `--field-json` forms cannot be mixed for
the same field, never a raw `TypeError`.
## Tests
Adds `packages/argent-cli/test/flag-parser.test.ts`. The new tests fail
before the fix (4 failures) and pass after; the full `argent-cli` suite
stays green (90 passed). Behavior for valid inputs is unchanged.
|