feat(flow): support concurrent flow recordings

[hubgan]

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>).

[latekvo]

[Diplomat]: Ran a full end-to-end sweep across the concurrency, ripple/call-site, client-mode, device-scoping, and docs lenses — the recording logic held up throughout. Two low-impact notes on the eviction test scaffolding are left inline.

[j-piasecki]

Three findings from a full review pass (each independently verified against the working tree; already-reported items and PR-acknowledged limitations excluded).

[latekvo]

[Diplomat]: Ran a full end-to-end sweep of this branch across the concurrency, atomic-write, tool-semantics, call-site/ripple, device-scoping, state-machine, resource-lifecycle, input-domain, test-quality and docs lenses, driving a tool-server built from af79e86 over HTTP: concurrent recordings across projects and against one device, a hammering reader during appends, restart/supersede, client persist mode, the MAX_RECORDINGS backstop, scoped teardown, replay, and the released 0.17.0 CLI against this server. The recording machinery itself held up everywhere I pushed on it — the isolation, the per-file lock and the temp-file+rename swap all behaved as described, with no torn reads and no cross-contamination.

Five notes inline. The one on stop-all-simulator-servers' schema is the one I would look at first — it is the only one with a behavioural consequence rather than a wording one.

[j-piasecki]

LGTM! I'll leave it for @latekvo

[latekvo]

Seven findings from a pass over the current head, each verified against a build of af79e86 and deduplicated against every existing thread. Six are prose that no longer describes the codebase; one is an unpinned behaviour I confirmed by mutation.

[latekvo]

[Diplomat]: Ran a full end-to-end sweep of this branch at a0ee67e6, then repeated independent passes over it until two consecutive sweeps came back clean.

A tool-server built from the branch drove two interleaved concurrent recordings across two project roots - each YAML kept exactly its own steps in order and both replayed green - while a separate reader observed 7,193,301 reads across 40 concurrent appends with zero empty or partial files. Also exercised: client/remote persist mode (nothing written to the host), the scoped and unscoped teardown paths including a case-varied UDID, a re-stop of an already-idle device, the devices: [] and misspelled-key cases, and the Chromium debugger cascade. Suites, typechecks, eslint and prettier are clean; the 6 failures in test/boot-device-hotboot.test.ts are pre-existing and identical at the merge base.

I also mutated the core invariants directly - the atomic write, the append lock, .strict(), touch()/LRU, the lock-map self-cleanup, the devices: [] semantics, the ERROR-node omission from stopped, and ownership-counted-regardless-of-state. Every one was caught by the suite. The recording, locking and atomic-write machinery held up throughout, and nothing inline below is a logic defect.

What did survive verification is prose that the code contradicts, plus one reported value. Findings are inline.

[latekvo]

Left inlines, please fix.

Other than these linguistic notes this is a great PR and i think its ready to merge ❤️

[latekvo]

[Diplomat]: Ran a full end-to-end sweep of this branch at 6c682c3, then a second independent pass over it.

A tool-server built from the branch drove concurrent recordings across two project roots and a burst of 8 concurrent appends on one key (all ordered, no cross-contamination, both replays green); a separate process observed 1,593,459 reads of a flow file during 120 concurrent appends with zero empty or partial — against a real in-place O_TRUNC writer the same detector sees 695,532 empty reads out of 909,926, so it is not blind. Also exercised: client/remote persist mode (nothing written to the host), the run: composition capture and its cross-project warning, the MAX_RECORDINGS backstop, hostile name / project_root inputs, the scoped and unscoped teardown paths including a case-varied UDID and a bogus id, and the real argent run path for start/add-step/add-echo/finish plus --help. Every documented behaviour I could execute matched, including the No active recording … message verbatim, (plus N in other projects), discardedSteps counted from disk (4) and omitted on an unparseable file, and the honest caveat that starting over a committed flow wipes it with no restarted.

Suites, typecheck and prettier are clean: tool-server 3164 passed, argent-cli 278 passed. The 6 boot-device-hotboot.test.ts failures are pre-existing — identical at the merge base 7189333 on this Linux host.

The recording, locking and atomic-write machinery held up everywhere I pushed on it, and nothing below is a runtime defect. What survived verification is prose the code contradicts, plus four invariants the diff documents as load-bearing that mutation shows nothing pins. Each finding was reproduced, then adversarially re-checked; several candidates were dropped as unreproducible, out of scope, or already covered by an open thread.

[latekvo]

[Diplomat]: Ran a full end-to-end sweep of this branch at 6c682c38, then three further independent passes over it, deduplicating against all 72 existing inline comments before reporting anything.

A tool-server built from the branch drove the real tools over HTTP: concurrent recordings of the same flow name across two project roots with interleaved bursts of appends (each YAML kept exactly its own steps, in order); a full record -> replay round trip against an Electron device, where the tap was captured as a portable tap: { id: go } selector and the replay changed the real DOM, read back through CDP rather than through the flow tree; double-finish; restart accounting; the MAX_RECORDINGS backstop; client/remote persist mode with nothing written on the host; the cross-project run: warning and its correct silence when the same root is spelled with a trailing slash; and the scoped, unscoped, empty-array and misspelled-key teardown paths including a case-varied id, a bogus id, an ERROR-state node and a re-stop of an already-stopped device.

Several documented claims I set out to falsify held up exactly as written and are worth recording as checked: the prerequisite-notice-still-records-run: caveat, the bare-string launch: + platform: chromium failure text (Electron boot: path does not exist), the No active recording … message verbatim, (plus N in other projects), discardedSteps counted from disk, and the unmatched case-insensitive dedup plus ownership-counts-regardless-of-state semantics. The device-id matcher also behaves as its rationale claims: a wireless adb serial matches whole, a bare IP does not claim it, a prefix id does not claim a longer one, and :tcp resolves for both namespaces that can mint it.

Suites, typecheck, eslint and prettier are clean: tool-server 3164 passed, argent-cli 278 passed; the 6 boot-device-hotboot.test.ts failures are pre-existing on this Linux host. The new concurrency tests ran five consecutive times under load 15 with no flakes.

The recording, locking and atomic-write machinery held up everywhere I pushed on it — I could not produce a lost update, a torn read, or a cross-recording leak. Most candidates died in verification as pre-existing at the merge base or as limitations the PR body already concedes. Seven findings survived: one behavioural (a recorded scoped teardown is not portable off the machine that recorded it), one invariant the diff documents as load-bearing that a mutation shows nothing pins, and five permanent comments the codebase now contradicts. A fourth sweep was cut short by an infrastructure limit rather than by convergence, so the comment-accuracy class in particular may still have a tail.

[latekvo]

Ran a full end-to-end sweep of this branch at a06aa6ad, driving a tool-server built from it over HTTP: concurrent recordings across projects and against one key, a hammering reader during appends, restart/supersede/finish races, the MAX_RECORDINGS backstop, client persist mode, scoped and unscoped teardown, the new devices record/replay binding, and the released CLI against this server. I also mutated the core invariants directly — the per-file lock, the temp-file+rename swap, touch()/LRU, .strict(), the lock-map self-cleanup, every entry of DEVICE_OWNED_NAMESPACES, the URN matcher, and both halves of the devices bind — and the suite caught essentially all of them.

The recording machinery itself held up everywhere I pushed on it: isolation, the per-file lock and the atomic swap all behaved as described, with no torn reads, no cross-contamination and no lost updates. With ARGENT_EMULATOR_GPU_MODE unset the full tool-server suite is 3186 passed | 1 skipped, zero failures. Nothing below is a defect in the concurrency design.

What did survive verification is two diagnostics that now misreport their cause, one reporting path that drops a computed field, three invariants this PR argues for but leaves unpinned, and five claims the code contradicts. Findings are inline.

[latekvo]

[Diplomat]: Adding ScreenRecordingSession to DEVICE_OWNED_NAMESPACES makes dispose() reachable from an ordinary in-session tool call. This file's comments were updated for that; the guard that acts on it, session-guards.ts:129-140, still reports the only cause it had at the merge base.

At 7189333a the tool's PREFIXES were SimulatorServer / NativeDevtools / AndroidDevtools / ChromiumCdp / TvControl / AndroidTvControl; the namespace was absent, the blueprint declares no getDependencies, and no other disposeService call site targets it — registry.dispose() at process exit was the only route to dispose(). So api.disposed really did mean shutdown, and this message was accurate.

Driving the real blueprint and the real tool through a Registry at this head:

urn: ScreenRecordingSession:emulator-5554   disposed before: false
stop-all { devices: ["emulator-5554"] } -> {"stopped":["ScreenRecordingSession:emulator-5554"]}
disposed after : true
guard message  : The tool-server is shutting down; screen recording was not started on device emulator-5554.

The trigger is agent A's screen-recording-start sitting at a pre-spawn await (openMjpegStream / waitForFirstFrame, capture.ts:311/:319) when agent B's session-end teardown lands — which is the call rules/argent.md:80 instructs every agent to make. Agent A is told the tool-server is going away and that retrying is pointless; the actual cause is another agent's teardown and a retry would succeed. SCREEN_RECORDING_SERVER_SHUTTING_DOWN carries the same diagnosis into telemetry.

So this file and session-guards.ts now disagree about when dispose() runs. Four further sites in this family still say dispose means shutdown and are outside the diff: session-guards.ts:124-125, capture.ts:332-333, capture.ts:426-427, screen-recording-start.ts:163. I checked the equivalent files for the other newly-added namespaces (native-profiler, ax-service, js-runtime-debugger, network-inspector, react-profiler-session, chromium-js-runtime-debugger, tv-control) — none makes a when-does-dispose-run claim, so this file family is the whole class.

[hubgan]

Fixed in 73a72ab — assertNotDisposed no longer claims the server is shutting down. It names both causes (process shutdown, or a stop-all-simulator-servers reaping this device — commonly another agent's teardown) and no longer says a retry is pointless. The four sibling dispose comments in this file family were reworded to match; the SCREEN_RECORDING_SERVER_SHUTTING_DOWN enum is unchanged (noted in the comment as historical). Confirmed on-device: at a ~1ms init-window race the corrected message appears rather than the old one.

[latekvo]

[Diplomat]: A failed swap surfaces an errno against the internal scratch file, which by then no longer exists, and never names the flow it was recording.

Live, against a tool-server built from this head, with .argent/flows at mode 500:

$ curl -sX POST .../tools/flow-add-echo -d '{"name":"ro","project_root":"…/proj","message":"m"}'
{"error":"[Tool:flow-add-echo] EACCES: permission denied, open
 '/tmp/…/proj/.argent/flows/.argent-flow-2761411-90.tmp'"}

$ ls -a …/proj/.argent/flows
.  ..  ro.yaml            <- the file the message names does not exist

The cleanup at :2229 has already removed it, the name appears in no agent-facing doc, and the suffix is a process-global counter so it differs every run. The flow name, the flow file, and the directory whose permissions are the actual cause are all absent.

This shape is new here. At the merge base appendStep did fs.writeFile(filePath, …), which opens the existing file and needs no directory write permission:

merge-base in-place write of s.yaml: OK
head scratch-file write:            FAIL EACCES … '.argent-flow-1-1.tmp'

The behavioural half of that trade-off is disclosed at :2210-2213 ("it needs write permission on the DIRECTORY rather than on the file"); what the failure then says is not.

Same shape at flow-start-recording, flow-add-step and on a restart over a live take. On flow-add-step the sub-tool has already run on the device when this fires, and this branch says nothing about it — while the sibling failure branch added by this same PR (assertSessionStillLive, :2344-2349) goes out of its way to say "the step itself already ran on the device — repeating it repeats that action", and the suite pins that distinction.

[hubgan]

Fixed in 47cbb4a — a failed swap now rethrows FLOW_FILE_WRITE_FAILED naming the flow file and its directory (the real cause), keeping the errno as cause; the internal .argent-flow-*.tmp path is no longer surfaced. Reproduced on-device with the flows dir at mode 555: the append fails with the flow-named message and (EACCES), and no .tmp is left behind. Pinned by the write-half test in this PR.

[latekvo]

[Diplomat]: discardedSteps and restarted are read from the map at two different moments, and an eviction landing between them makes a destructive restart report itself as a fresh start.

discardedSteps comes off getRecordingSession(...) here at :132; replaced comes off startRecordingSession's own recordings.get(key) at :147. In host mode two awaits sit between them — countStepsOnDisk and writeNewFlowFile. That window is inside this key's lock, which excludes the other flow tools but not evictIfOverCapacity, which runs under some other key's lock. When it fires there, replaced is null, the if (replaced) branch at :161 is skipped, and the already-computed count is discarded — after the file has been truncated.

Reproduced deterministically (32 host recordings, the victim as LRU, its restart parked inside its own lock on countStepsOnDisk's readFile, one further flow-start-recording released during the gate):

RESULT:      {"message":"Started recording \"rec-0\" flow", ...}
FILE BEFORE: "steps:\n  - echo: real step\n"
FILE AFTER:  "steps: []\n"
  × reports a destructive restart as a plain fresh start
    AssertionError: expected undefined to be true

The expect(after).not.toContain("real step") assertion passed — the take really was destroyed — while restarted came back undefined.

Client mode is unaffected: clientFileDirective is synchronous, so the two reads cannot diverge there.

Stating the reachability plainly rather than overselling it: it needs the cap reached, the restarted flow to be exactly the LRU, and the eviction to land in a sub-millisecond window, and requireRecordingSession restamps on every append so the documented re-record workflow keeps the key near most-recently-used. What makes it worth a note anyway is that the cap is reachable without concurrent agents — clearRecordingSession has exactly one production caller (flow-finish-recording.ts:122), so a recording started and never finished stays in the map for the life of the process.

The claim this falsifies is the tool's own description at :82 — "optionally { restarted, discardedSteps } if a live recording of the same flow was discarded" — which reads as a sufficient condition. SKILL.md:134 covers the neighbouring case (starting over a committed file with no recording in progress) but is a necessary-condition statement, so it does not reach this one.

[hubgan]

Fixed in 73a72ab — restarted and discardedSteps are now both read once from the in-lock getRecordingSession, so an eviction landing between startRecordingSession's own read and the register can no longer report a destructive restart as a plain fresh start. Pinned by a new test that parks the restart on its countStepsOnDisk read, evicts the key, then releases; mutation-verified — reverting to startRecordingSession's return turns it red.

[latekvo]

[Diplomat]: Ten of the twelve interaction formatters on the four recording tools render strings that nothing asserts, so they can revert to the merge-base unqualified wording with the suite green.

This PR rewrote all twelve so each names the flow, with the rationale stated verbatim in three of the four files: "recordings are concurrent, so several of these lines can interleave in one log and 'flow recording' would not identify which." Asserted anywhere at this head:

flow-start-recording.completedMsg   interaction-messages.test.ts:90,97,110,123
flow-add-echo.completedMsg          interaction-messages.test.ts:157

Unpinned: flow-start-recording startedMsg/failedMsg, flow-add-echo startedMsg/failedMsg, and all three of flow-add-step and flow-finish-recording.

Reverting just flow-finish-recording's two to name-free wording (Finishing flow recording / Saved recorded flow), leaving the other eight intact:

$ npx vitest run test/flows test/interaction-messages.test.ts
  Test Files  33 passed (33)
       Tests  615 passed (615)

Flattening all ten at once leaves the full package suite byte-identical to baseline (Tests 3186 passed | 1 skipped). Nothing outside these files reads the strings — grep -rn over --include=*.ts --include=*.md, excluding node_modules/dist, returns only the definitions and the assertion at :157. Note that :157 pins its formatter only incidentally, as a side effect of a secrets test asserting toContain("Added note to flow checkout").

The consequence lands hardest on the failure lines, which are the diagnostic when several recordings are live: Failed to add note to flow alpha: FLOW_NO_ACTIVE_RECORDING says which recording died; Failed to add note to a flow does not.

Noting the scope deliberately: this class was raised twice before — #3683739051 on stop-all-simulator-servers and #3683739066 on flow-start-recording.completedMsg — and each was pinned at exactly the site named, stop-all's two now fully covered at stop-tools.test.ts:967-1021. These ten are the remainder, and the a06aa6ad replacement for #3702324470's flow-finish-recording.completedMsg is itself among them.

[hubgan]

Fixed in 2bd1937 — added names the flow in every recording-tool interaction line, pinning all twelve formatters (the ten that were unasserted plus the two already covered) to naming the flow, including the failure lines. Non-vacuous: dropping the name from any formatter fails it.

[latekvo]

[Diplomat]: The half of this guard the comment singles out as load-bearing is the half nothing tests.

The comment states it outright — "The write itself can fail with the file already created (ENOSPC, EIO), so this has to cover it too" — and the PR body lists it as a regression a later pass caught.

Moving the try back to begin after fs.writeFile, i.e. the exact pre-fix shape:

  await fs.writeFile(tmpPath, content, "utf8");
  try {
    await fs.rename(tmpPath, filePath);
  } catch (err) {

$ npx vitest run test/flows test/stop-tools.test.ts
  Test Files  33 passed (33)
       Tests  669 passed (669)

(and the full package suite is likewise byte-identical to baseline under that mutation)

The neighbouring case propagates a failed swap and leaves no scratch file behind does fail when the whole try/catch is deleted, so the guard is not unprotected as a whole — but its own comment says its trigger reaches fs.rename "and nothing else", and it explicitly rejects the read-only-directory trigger. That is the point: a read-only directory fails at the open, before the file exists, so no filesystem-level trigger in the suite reaches the write-half. The live trigger the guard names (ENOSPC, EIO, file already created) has no case.

What is left behind on the untested path is a .argent-flow-<pid>-<n>.tmp in the user's committed .argent/flows/, which nothing in the tree sweeps.

[hubgan]

Fixed in 2bd1937 — added a test that drives the write-half directly: it makes fs.writeFile create the temp file and then throw (ENOSPC-shaped), and asserts the scratch file is swept and the surfaced error names the flow file. The rename-half case is separate, so the guard's write-half no longer rides on the success path.

[latekvo]

[Diplomat]: The pid's stated purpose is a writer that never mints a scratch file here, while the writer it actually guards against is documented 2,000 lines above.

Exactly one site in the repo mints .argent-flow-<pid>-<n>.tmp — this one, in the tool-server. The CLI does write a flow YAML in client persist mode (argent-tools-client/src/file-inputs.ts:265-278, gated by isAllowedClientFilePath to .argent/flows/), but it writes the destination file directly and creates no temp file; and host and client modes are mutually exclusive per call (flow-start-recording.ts:117), so whenever the tool-server is writing a scratch file into that directory, no CLI is writing there at all. The pid separates nothing from a CLI.

The second writer it does separate from is a second tool-server from a second install bundle — named in this same file at :176-183, which states the temp-file swap is the only remaining protection in that configuration. flowWriteSeq is module-global and starts at 0 in every process, so without the pid two such servers compute the same scratch path; one writer's writeFile is overwritten by the other, and the losing rename either lands foreign content in a flow file or fails ENOENT on a path it just created.

The constant is also undefended. Dropping ${process.pid} from the scratch name:

$ npx vitest run test/flows
  Test Files  32 passed (32)
       Tests  611 passed (611)

The combination is what makes this worth raising: this paragraph is preoccupied with keeping the scratch name short for the ENAMETOOLONG budget, and it tells the next person the pid only guards a CLI. The same attribution appears at :2176-2177.

[hubgan]

Fixed in 47cbb4a — the pid's purpose is corrected: it separates this tool-server from a second install bundle writing the same directory, not from a CLI (which writes its destination flow file directly, mints no scratch file, and is mutually exclusive with host mode per call). Both the counter comment and the ENAMETOOLONG paragraph now say so.

[latekvo]

[Diplomat]: This contract says the two results are alternatives, and since this branch added the cross-project warning they can be returned together.

The or reads as exclusive — a flow name to compose, or a warning explaining why the raw step was kept. At :201-206 the resolved branch returns both, and when the warning is set the step was not kept raw at all; it was rewritten to run:.

Driven through the real registry and the real tools (recording wrapper in project A, mid-recording flow-execute running a same-named sibling helper from project B):

message: Step added to "wrapper" flow — recorded "run: helper", which replays THIS project's
         helper.yaml — the step ran helper from /tmp/…/projB, a different project
--- recorded YAML ---
steps:
  - run: helper

The return was { flow: "helper", warning: … }.

The JSDoc sentence predates this branch (551c781f); ranElsewhere is new here, and the diff adds a sibling comment at :302-305 acknowledging the new both-case — "A resolved target can still carry a warning … so this branch surfaces it too" — 145 lines below the contract that still says otherwise. A maintainer reading the contract would conclude flow and warning are mutually exclusive and that the runTarget?.flow branch's warning = runTarget.warning is dead.

[hubgan]

Fixed in 5313367 — the captureRunTarget JSDoc now states that flow and warning are not exclusive: the resolved branch returns both when a same-named sibling ran in a different project (the step is still rewritten to run:; the warning only flags the ambiguity).

[latekvo]

[Diplomat]: This new key is a third spelling of a device-carrying parameter, and the enumeration that decides what counts as one lists two.

http.ts:728-732 states it: "Tools spell the device parameter two ways — udid (legacy iOS-only tools and gestures) and device_id (debugger / profiler / network tools)."

extractDeviceArg (http.ts:129-135) reads only those two, so a {"devices":["…"]} call returns null.

Two more statements are falsified the same way, both outside the diff:

• http.ts:189 — "Telemetry platform from a tool call's device arg, or null when it carries none." A scoped teardown carries one and yields null.
• packages/registry/src/types.ts:113-115 — "re-derived from each sub-tool's own childArgs (its udid / device_id / avdName), falling back to the outer request's platform when the sub-tool carries no device arg". bindDeviceArgs (flow-device.ts:149) now injects devices:[deviceId] into a replayed stop-all-simulator-servers step, so that child does carry one and still falls back.

Effect today is confined to telemetry platform attribution — stop-all-simulator-servers declares no capability, so the gate at :734 is not reached — but this comment is the enumeration a future device-arg-carrying tool would be checked against.

[hubgan]

Fixed in 5313367 — extractDeviceArg now reads the devices spelling (first element, for the coarse platform), and the three enumerations name all three spellings: the capability-gate comment, platformFromArgs, and recordChildInvocation in registry/types.ts. A replayed stop-all step (which bindDeviceArgs injects devices into) now resolves its platform rather than falling back. The capability gate is unaffected — that tool declares no capability, and udid/device_id are checked first, so no capability-bearing tool can reach the devices branch.

[latekvo]

[Diplomat]: This comment describes coverage the case does not have, and its counterfactual does not hold either.

The body passes one spelling: devices: [MINE] at :510, with MINE = "AAAA-1111". The pairs exercised are upper-id against upper-URN (SimulatorServer:${MINE}) and upper-id against lower-URN (NativeDevtools:${MINE.toLowerCase()}:tcp). There is no "vice versa" — no lower-cased id is ever passed.

The counterfactual then inverts: passing MINE.toLowerCase() would give lower/upper and lower/lower, so there would be no "upper/upper pair" left matching. Running exactly that mutation:

# devices: [MINE]  ->  devices: [MINE.toLowerCase()]
$ npx vitest run test/stop-tools.test.ts -t "matches the device id case-insensitively"
  Test Files  1 passed (1)
       Tests  1 passed | 57 skipped (58)

Both spellings are exactly as strong, so the reason given for preferring this one does not distinguish them. (The lower-id/upper-URN direction is genuinely covered, but by :937-953, not here.) The cost is to whoever edits this case next: the comment reads as a reason not to touch the spelling, and the reason is not real.

[hubgan]

Fixed in 2bd1937 — the comment now describes what the case actually covers (upper-id vs upper-URN, and upper-id vs lower-URN) and notes the reverse direction is covered by a separate case, dropping the incorrect "vice versa" and its counterfactual.

[latekvo]

[Diplomat]: This slot held the file's only negative control for namespace scoping, and converting it to a positive one leaves the unscoped sweep's namespace filter unpinned everywhere.

At the merge base the same map held ["JsRuntimeDebugger:CCC", RUNNING] — a URN whose namespace was outside PREFIXES — and asserted it absent:

$ git show 7189333a:packages/tool-server/test/stop-tools.test.ts | sed -n '137,150p'
      ["JsRuntimeDebugger:CCC", { state: ServiceState.RUNNING, dependents: [] }],
    …
    expect(result).toEqual({
      stopped: ["SimulatorServer:AAA", "SimulatorServer:BBB"],
    });
    expect(registry.disposeService).toHaveBeenCalledTimes(2);

It is now ChromiumJsRuntimeDebugger:CCC, asserted present, with three dispose calls. Every URN in every stop-all case is a member of DEVICE_OWNED_NAMESPACES, so nothing left says the machine-wide sweep is namespace-scoped at all:

# device-services.ts:218  isDeviceServiceUrn -> `return true;`
$ npx vitest run test/stop-tools.test.ts test/flows
  Test Files  33 passed (33)
       Tests  669 passed (669)

and the whole package suite is byte-identical to baseline under that mutation.

Nothing leaks today, because all 13 registered blueprints happen to be device-owned. But isDeviceServiceUrn is the single guard between stop-all-simulator-servers — the call rules/argent.md:80 instructs every agent to make at session end — and any future non-device service in the registry, and device-services.ts has no test file of its own. Breaking that guard, or adding a namespace to the list by mistake, now produces no signal.

[hubgan]

Fixed in 2bd1937 — restored a negative control (leaves a service whose namespace is not device-owned untouched): a synthetic out-of-set URN the unscoped sweep must not dispose. Degrading isDeviceServiceUrn to return true now fails it, so the sweep's namespace filter is pinned again.

[latekvo]

Wait, i'm pretty sure we JUST added human-readable descriptions to every step in another place. See #582

Doesn't summarizeSteps duplicate that mechanism 1:1???

[hubgan]

Not really. summarizeSteps is not a new code. It was already existing before this PR and is currently on main branch. Is is the same step-summary const summary = flow.steps.map(...) block that has been in this file. I just moved it and close into the summarizeSteps function. The function body has not been changed in this PR.

The PR you mention and this function do different jobs that cannot be folded together.
The #582 emits one message per tool call, and it goes to the event log.
summarizeSteps function emits one line per recorder step and it goes into what flow-finish-recording tool returns to the agent.

One thing that I find is that this PR overlaps a little bit with #568 when it comes to getFlowsDir(projectRoot) / flowsDirFor(root) functions overlap. This is something I'll fix.