fix: read replay scripts on the caller and ship them with the request - #1810
Conversation
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
|
|
Reviewed exact head b7c66b8. The caller-owned replay source-bundle design is sound: replay/test payload preparation, daemon no-filesystem execution, Maestro include closure, ADR 0006 additive fields, and ADR 0012 digest/repair compatibility are substantively covered. One readiness blocker: CodeQL reports a new high-severity incomplete-escaping alert at |
|
CodeQL alert addressed at head 6ae91bc ( |
Fixed the CLI startup regression the size report caughtThe size bot was right, and the cause was mine: the command registry evaluates every command family's module on startup —
Guarded, not just fixed. Numbers — matched builds on one machine, 15 runs each, via the same
Chunks: the engine left the startup graph entirely and became its own on-demand chunk.
Wall-clock-free confirmation, walking the static import graph of the built output from (The engine is located by its own
|
CI confirms it: startup regression goneThe size job's own base-vs-PR run on a quiet runner, after
Was 67.4 → 95.7 ms (+28.3). Package total is now +10.0 kB raw / +3.1 kB gzip (was +131 kB of startup chunk): |
ac2b9ee to
7018c3d
Compare
|
Code delta review at ac2b9ee is clean: the CodeQL fix is resolved, Maestro loads only for flow input, async writer propagation reaches every production caller, the planted import-closure regression is meaningful, size evidence restores CLI startup, and exact-head CI is green. Readiness remains blocked because the PR body was overwritten with the unrelated #1801 remote-diagnostics feature: wrong linked issue, behavior, 38-file scope, routes, validation, and wire claims. Restore an accurate #1802 replay-source description, including the remote |
PR description restoredApologies for the confusion — the description on this PR was briefly overwritten with the text of a different PR (#1801, remote diagnostics). That was a stray The body is now the #1802 replay-script-source-bundle description again, with the CLI cold-start section re-added after the restore (it had been written after the point the restore was taken from). Verified before and after editing: Also, the branch has been rebased onto current Re-run on the rebased head: typecheck clean, wire-compat + replay-compat green ( |
|
Clean re-review at 7018c3d: the rebased series is range-diff equivalent to the previously reviewed code, and the PR body is restored to the actual #1802 replay-source change with correct 91-file scope, validation, cold-start fix, and remote-test evidence limitation. Exact-head CI is fully green and merge state is CLEAN. Ready-for-human. |
The command registry evaluates every command family on CLI startup, so the replay script-source builder's static @agent-device/maestro import put the YAML parser on the --help path. It now loads on demand behind the format check, and the startup import-closure guard covers the engine the way it already covers node:http.
…d client views The new replay script-source flags appear in both CliFlags and CommandExecutionOptions, which fallow flagged as a clone; ReplayRequestFields declares them once. The test-suite handler's missing-sources rejection now travels the typed-error path its sibling rejections already use, so the fix adds no branch to handleSessionReplayCommands.
7018c3d to
1dcd0fc
Compare
Split the pure refactor out — this PR is smallerFair challenge, and you were right about which part. Moving the
Almost all of it is the move: What stayed, because the fix genuinely requires it:
So the extraction is genuinely optional for this PR, which is the test it had to pass to be split out.
Also rebased onto current |
|
Re-reviewed exact head |
Summary
replay <path>andtest <path-or-glob>now name files on the caller's filesystem, everywhere.The client resolves the path, reads the script (and, for Maestro, every
runFlowinclude it canreach), and sends the text with the request. The daemon executes only what arrived and opens no
caller path at all.
Before: the path was resolved on the caller but opened on the daemon. On one host that works, so a
flow passes locally and then fails on its first remote run with
ENOENTon a path the caller canread. Against a remote daemon
replaysimply did not work.After:
The one typed concept is a replay script source bundle (
CONTEXT.md, Recording & replay): anentry display path plus a resolved-path-to-text map. It rides in the request as
replayScriptSource(one run) /replayScriptSources(one pertestsource), the same wayreplayShellEnvalready carries caller-local input. Local and remote use the identical path — thereis no remote-only branch, which is what stops "passes locally, ENOENT in CI" from coming back.
What moved:
runReplayScriptFile→runReplayScriptSource; it reads the bundle, neverreq.positionals[0].inspectAdReplaytakes script text;replay-device-selectionreads the bundle (it previouslyswallowed its own
ENOENTand silently skipped pre-binding on every remote replay).createMaestroProgramLoadertakes areadSourceport — fs on the caller, bundle lookup on thedaemon — so
runFlowincludes resolve identically on both.packages/maestrono longer importsnode:fs.testpath/glob expansion moved to the caller (src/replay/source-discovery.ts); the daemoncapability now only inspects the sources it was sent, in the order they arrived. Sharding,
ordering, and platform filtering are unchanged.
and what to do, instead of an
ENOENTfor a file that exists on the machine that ran the command.Older clients against a newer daemon get that message; the field is additive, so a newer client
against an older daemon is the normal command-error case ADR 0006 describes.
--save-scriptagainst a remote daemon is now refused with a typedINVALID_ARGSnaming thesplit. Reading was fixable by sending content; writing is not symmetric — the healed
.adispublished by the daemon's
SessionScriptWriterat session teardown, on the daemon's disk, next to asource path that does not exist there. Returning the healed script in the response would mean new
teardown→response plumbing well outside this change, so option (b) from the design: refuse loudly
now, follow up separately. Local
--save-scriptbehavior is untouched.Closes #1802
Validation
Regression tests, proven red first. With only the client writer reverted to
origin/main,src/__tests__/cli-replay-script-sources.test.tsfails on the wire payload:With the daemon side reverted (
session-replay-runtime{,-plan}.ts,ad-replay/inspect.ts),src/daemon/handlers/__tests__/session-replay-script-source.test.tsfails on both halves — the runthat must not touch the disk, and the refusal that must exist:
Also covered: Maestro include collection (transitive, through
repeat/retry/inlinerunFlow.commandsand lifecycle hooks, deduplicated, cycle-safe), a missing include naming its ownpath, the bundle byte cap, the caller-side not-found error, and the remote
--save-scriptrefusal.inspectAdReplay's plan digest is pinned to a literal so an ADR 0012--from/--plan-digestresume issued before this change stays valid after it.
Live, iPhone 17 simulator (
1604B975-…), repo CLI, isolated state dirs.Local, relative path from the flow's own directory —
Replayed 3 steps in 3.1s. A wrong selectorstill names the caller's path and line:
Divergence at step 3 (/…/flows/broken.ad:4).Remote mode via
agent-device proxy --port 4319, driven from a different cwd and a differentstate dir with
AGENT_DEVICE_DAEMON_BASE_URL/AGENT_DEVICE_DAEMON_AUTH_TOKEN—replay ./settings.ad→Replayed 3 steps in 3.2s. The proof that the daemon opens nothing: the script wasdeleted one second after the client started, while the run was still in flight —
replay ./settings.ad --save-scriptover the same proxy →--save-script is not supported against a remote daemon…;replay ./no-such-flow.ad→replay script not found on this machine: ./no-such-flow.ad, with no daemon round-trip.Honest limitation of the live run: a single host cannot reproduce the reporter's original
ENOENT,because the daemon shares the caller's filesystem and pre-fix code resolves through
meta.cwd. Thecross-filesystem property is what the deleted-script run above and the daemon-side regression test
demonstrate.
All sessions closed, both daemons stopped, the proxy killed, and the simulator shut down.
pnpm check:affected --rungreen (check:daemon-wire-compat: 151 declarations, 0 changed — the newfields sit inside the
CommandFlags/InternalRequestOptionswaiver ADR 0006's additive rulecovers). One unrelated contention flake on the way (
scripts/fuzz/harness.test.ts5s timeout);passes in isolation and on the final green run.
Tradeoffs and follow-ups
runFlowinclude whose path is templated on a value only the daemon binds cannot becollected before the run. Collection is best effort past the entry flow (
when:-guarded includesmay never execute), and a run that does need such a file fails naming exactly which file was
missing from the sources sent. Includes templated on
env:/AD_VAR_*/-evalues are resolved,through the Maestro package's own interpolation.
.adscripts and flows are kilobytes,and the upload route stays reserved for binaries.
--save-scriptremote support (returning the healed script in the teardown response) is thefollow-up left open.
Scope
89 files. Scope stayed in the replay/test command family and its packages, with three deliberate
edges: the remote request-prep gate (
--save-scriptrefusal), the@agent-device/maestrosourceport, and
@agent-device/replay-test's discovery port, which no longer takes inputs or a cwd.A pure refactor has been split out in response to review: moving the
test-suite orchestrationout of
handleSessionReplayCommandswas not needed by this fix, and now lives in its own stacked PR(#1826,
session-replay.ts413 -> 34 lines,session-test-suite-command.ts+410). Removing it tookthis PR from 91 files / +2646 -1208 to 89 files / +2268 -852 — 356 fewer added and 356 fewer
removed lines, all of it moved code. What is left is the fix and what the fix itself requires.
Four commits: the fix; a CodeQL follow-up replacing a hand-escaped regex in a new test with a
substring assertion; the CLI cold-start fix below; and one small refactor the fix does require —
the new replay flags appear in both
CliFlagsandCommandExecutionOptions, which fallow flags asa clone, so
ReplayRequestFieldsdeclares them once, and the suite handler's missing-sourcesrejection travels the typed-error path its sibling rejections already use so the fix adds no branch
to
handleSessionReplayCommands. Docs:CONTEXT.mdglossary,agent-device help scripting, andthe command reference.
session-replay.tsstays over the 300-line tripwire it was already over before this PR (382 -> 413lines); #1826 is the follow-up that fixes that, and is deliberately not required for this one.
CLI cold start
The size job caught a real regression in the first version of this change: the command registry
evaluates every command family's module on startup,
--helpincluded, so a static@agent-device/maestroimport in the new script-source builder put the YAML parser on the coldpath —
CLI --help67.4 ms -> 95.7 ms (+28.3) and a 131 kB startup chunk, for a format mostinvocations never touch.
The engine now loads on demand inside
loadReplayScriptSourceBundle, reached only once an entryresolves to a flow; the native
.adbranch reads its entry file and nothing else. That made the onebundle constructor async, so
DaemonWritergained a siblingAsyncDaemonWriter— replay/test andbatch declare it, every other family's writer stays synchronous and untouched, and the registry
awaits whichever kind a command declares. One construction path is preserved: CLI, Node client and
MCP still converge on
loadReplayScriptSourceBundle.CI's own base-vs-PR run after the fix:
CLI --help65.3 -> 66.2 ms (+0.8), withCLI --versionmoving +0.7 ms on the same run — and
--versioncannot be affected by this change, so the remainderis that run's drift. Package total is +10.0 kB raw / +3.1 kB gzip.
src/__tests__/cli-startup-import-closure.test.ts— the AST walker that already keepsnode:httpoff the cold path (#1681) — now holds the same line for the engine, and was proven red by restoring
the static import. Walking the built output confirms it independently of wall clock: the Maestro
chunk is absent from
dist/src/cli.js's 46-chunk static closure and reachable only throughscript-source-bundle.js -> src4.js's dynamic import.