feat(screenshot): burst capture with --count and --interval-ms - #4291

I reviewed 6764cfb and found two problems to fix before merge. CI is green: 11 checks, none failing. There are no conflicts.

On a remote daemon, a burst delivers one usable frame and N-1 paths the client cannot read. The remote input policy strips the screenshot positional (remote-host-input-policy.ts:160), so outPath is undefined and the burst writes every frame under a daemon-host temp root. Request finalization tracks only the top-level path as a downloadable artifact (request-finalization.ts:126-160), and the remote client maps only that field to a local file (daemon-artifacts.ts:303-314). So frames[0].path is a daemon path too, and the temp root and frames 2..N stay on the remote host forever. The rule: every file a response reports must go through the response artifact pipeline. That means one PendingArtifact per frame in data.artifacts, a client mapping that rewrites each frames[].path to a local path, or a typed refusal of --count on remote requests until that exists. I read this from source, not from a running remote daemon.

The burst also changes live capture dispatch, but the PR body reports only unit tests, typecheck and lint. Please run agent-device screenshot ./burst/page.png --count 5 --interval-ms 200 --json on one iOS simulator or Android emulator with an animation on screen. Paste the JSON showing 5 frames with increasing capturedAtMs, and ls -la ./burst showing page-0001..0005.png with differing sizes or hashes. Please also run one remote or provider session, or state that remote is refused.

Could the design be smaller? Keeping the flag pair is a smaller surface than a new record mode. But if every frame went through the existing response-artifact list (artifacts: frames.map(...)), finalization would track them and the remote client would download them. Reserving the frames through the adapter's reserveOutput would give failure cleanup and remove the hand-rolled mkdtemp. The time model could be a runtime deadline instead of only an admission formula. Screenshot could also have its own frames/frameIntervalMs keys, which would drop explicitOnlyFlags at two surfaces and the overloaded usage strings on the shared flags. The remote artifact mapping would first need to accept an indexed or list field, not only path.

Not blocking, and fine to take or leave: admission assumes 500 ms per frame and allows up to 60 s, but nothing in the loop checks elapsed time (contracts/screenshot.ts:116), so a backend at about 1 s per frame with --count 100 --interval-ms 100 hits the request timeout and loses every completed frame (a deadline that stops before a frame that would overrun, returning the frames so far with a typed warning or failing with a typed reason and cleanup, would enforce the model). Also, capturedAtMs is stamped after captureScreenshotArtifact returns (screenshot-runtime.ts:311), so it includes scaling and publishing; stamp it when the platform write completes, or document it as the completion time. I did not measure per-frame latency on any backend.

The open inline threads still apply: the abort check after the inter-frame sleep (I did not trace whether a backend checks the signal itself), config count test, burst temp root cleanup, aggregated notes test, distinct path test, abort during wait test, plus the two lower-priority ones on frame filtering and Number.isFinite.

Before merge, route every burst frame through the response-artifact pipeline (or refuse --count on remote with a typed reason), clean the burst temp root on failure, re-check the signal after the inter-frame sleep, and attach the live 5-frame run.

This now conflicts with main at 6764cfb. Please rebase onto main. The two problems and the live run from the earlier review still apply.

Tracked in #4318, group C (new public surface). We're reviewing the six feature PRs together for coherence and for performance impact on paths that don't use them, and we'll post a direction there. Please hold changes here until then.

Thanks @strzhao. We reviewed this with the rest of group C in #4318. Bounded frame sampling is useful for remote devices, but every frame has to be a managed, downloadable artifact.

• Remote frames aren't delivered. Request finalization registers only the top-level path, so nested frame paths such as frames[0].path stay on the remote host (request-finalization.ts:123, daemon-artifacts.ts:303). A local-only version isn't complete for this feature.

What we need:

• Reuse single-frame capture and artifact reservation. Register and materialize every frame through the existing artifact pipeline, which needs indexed or list artifact mapping first.
• Define the interval as the delay between captures, and timestamps as capture completion. Use screenshot-specific options, with no exceptions to the shared repetition defaults.
• On cancellation or deadline, report completed frames consistently and clean up frames that weren't returned. Never start a capture after cancellation.
• Budget: one-frame behavior unchanged. At most 100 frames, and a total deadline of no more than 60 s or the remaining request budget.
• Done when every returned path is verified locally through direct-remote and proxy/provider routes, including rotation and numbered destinations.

Could you open an issue with this contract and rebase onto main? We'll pick up the PR once the artifact mapping lands.

Rebased onto main (through 0.21.37). Three semantic merges: the screenshot help now carries the burst text grafted onto the newer prose (screen-sharing window detail and the iOS status-bar wait stay); the CLI-options tests keep #4298's device-selector block beside the burst explicit-only block; and the MCP tool-input merge keeps the structured { input, binding, devicePin } result — the burst explicit-only tests assert through input now. The daemon burst temp root's node imports, which main's reworks had removed from the file, are restored.

The rebase to e5ea581 does not change the two problems from the earlier review (#4291 (comment)), so both still need work. CI is green on this head (14 checks, 0 not passing), and there are no conflicts.

On a remote daemon, the input policy strips screenshot positionals (https://github.com/callstack/agent-device/blob/e5ea581/packages/daemon/src/server/remote-host-input-policy.ts#L172), so outPath is undefined and every frame goes into a daemon-host temp root (https://github.com/callstack/agent-device/blob/e5ea581/packages/daemon/src/execution/screenshot-runtime.ts#L294). Only the top-level path becomes a downloadable artifact, and the remote client maps only path (https://github.com/callstack/agent-device/blob/e5ea581/packages/remote/src/daemon-artifacts.ts#L303). On remote and cloud devices, a burst returns one usable frame plus N-1 paths the client cannot read, and it leaks files on the remote host. The rule: every file a response reports must go through the response artifact pipeline. Please emit each frame as a screenshot artifact and have the remote client rewrite frames[].path to client-local paths. Alternatively, refuse --count on remote requests with a typed reason until that exists.

The burst adds a device-facing capture loop (https://github.com/callstack/agent-device/blob/e5ea581/packages/daemon/src/execution/screenshot-runtime.ts#L304). The PR reports only unit tests, typecheck and lint. Nothing shows that frames differ on a real device, that per-frame capture fits the 500 ms admission budget, or that all frames land on disk. Please run agent-device screenshot ./burst/page.png --count 5 --interval-ms 200 --json on an iOS simulator or Android emulator with an animation on screen. Paste the JSON showing 5 frames with increasing capturedAtMs, and ls -la ./burst showing page-0001..0005.png with differing sizes or hashes. Please also run one remote or provider session, or show the typed refusal.

These open threads still apply: the cancel check before the sleep (#4291 (comment)), and the missing cleanup on throw or abort (#4291 (comment)). The other six open review threads also still apply.

I reviewed the range-diff and the 4 head commits, not the full upstream delta. I did not run tests or a device, so burst behavior and remote artifact mapping come from reading the code. I did not check whether captureScreenshotArtifact honours abort before platform capture. That decides how bad the extra frame after cancel is. The earlier notes on the missing runtime deadline and on capturedAtMs being the completion time are unchanged, and the question about per-frame artifacts through the artifact adapter is still open.

Before merge, the burst frames must reach the client through the artifact pipeline (or --count must be refused on remote with a typed reason), and the live burst run above must be attached.

Both problems are addressed at the pushed head, plus every open thread.

Remote bursts refuse with a typed reason. The remote capture lane refuses a screenshot request carrying count (input or flags) with REMOTE_SCREENSHOT_BURST_DENIED before any device work — no N-1 unreadable paths, no host leaks; single captures pass unchanged. The alternative (per-frame artifacts) stays open for whoever needs it; #4476/#4509-style, the refusal documents the boundary.

Live burst run attached. docs/evidence/ios-burst-capture-2026-11-13.md: five frames over an animation on the iPhone 17 Pro simulator, capturedAtMs strictly increasing, five distinct hashes, all five files beside the --out path. A remote/provider session was not run; the typed refusal above is the remote story.

Threads. The abort-during-wait now re-checks the signal before the next capture (P1); a failed or canceled burst removes its implicit temp root (failure-only finally); the cancel test aborts from inside the first capture and asserts exactly one frame; the warning-bound test uses distinguishable cross-frame contributions against the bound; the temp-root test asserts distinct implicit paths; the client filters an inside-the-bound malformed frame (and requires a finite capturedAtMs); the resolver gains the intervalMs-removal and crop-on-beside-config-count cases.

Thanks for the update. At 77cc16f the earlier review findings (#4291 (comment)) are fixed in code, and I found nothing that must change before merge. CI is green: 14 checks, none failing. There are no conflicts. Nothing blocks merge now, apart from the open inline threads below that still apply.

Not blocking, and you can take or leave these: (1) the cancel test in screenshot-runtime-burst.test.ts aborts inside the first capture, so deleting the post-sleep check in screenshot-runtime.ts would keep it green; abort from a timer during the interval (frame 1 resolves, intervalMs 1000, abort after ~20 ms) and assert the abort reason or code and exactly 1 capture call. (2) The new remote refusal REMOTE_SCREENSHOT_BURST_DENIED is not in the screenshot help; one sentence saying a remote daemon refuses --count, so run the burst against a local daemon, would cover it. (3) In remote-host-input-policy.ts, the policy checks and strips input.count and input.intervalMs, but the daemon reads the burst only from flags (screenshot-runtime.ts:76-77); check only flags.count and drive the tests with flags and positionals as the remote client sends them. (4) docs/evidence/ios-burst-capture-2026-11-13.md names no SHA, and its capturedAtMs deltas imply about 520-710 ms per frame, above perFrameBudgetMs (500); record the SHA and either raise the budget or reword its doc comment. (5) The cleanup test around screenshot-runtime-burst.test.ts:146 can read mock.calls[0]![0]!.outPath directly instead of using try/catch plus toBeDefined.

Of the open inline threads, these still apply: the remote policy test request shape (#4291 (comment)), which matches note 3 above. Two lower-priority threads on the evidence doc still apply (missing md5 hashes and missing --json envelope), as does the warnings test whose expected array would be identical if frame 2's warnings were never aggregated. The thread on input.intervalMs does not apply, because flags.intervalMs survives and the daemon refuses --interval-ms without --count (#4291 (comment)). The threads on the post-sleep abort check, the config strip tests, temp dir cleanup, distinct frame paths, the first-capture abort test, and the client capturedAtMs finite check are fixed at this head and can be resolved (#4291 (comment), #4291 (comment), #4291 (comment), #4291 (comment), #4291 (comment), #4291 (comment), #4291 (comment)).

I did not run tests or a device. The live burst result comes from the evidence doc, which names no SHA. I checked the remote refusal through the unit test and by reading both restrictRemoteHostInputs call sites, not with a live remote daemon. I did not trace whether MCP or the Node client could put count in input instead of flags; that affects only note 3, not the refusal.

All five take-or-leave notes are taken at 3f5ffa8: the cancel test now aborts from a timer during the 1000 ms interval wait and asserts the abort reason with exactly one capture (the post-sleep check is what makes this pass); the screenshot help names REMOTE_SCREENSHOT_BURST_DENIED and points bursts at a local daemon; the remote policy judges flags.count — the key the daemon arms the burst from — and its tests drive flags and positionals as the remote client sends them; the evidence names head 7f3741906 and frames the observed 520-710 ms per-frame interval against the 500 ms budget's doc comment (the budget bounds the admission wait, not the capture round trip); and the cleanup test reads the captured frame directly. The evidence also carries the five md5 hashes and the abridged --json envelope, and the warnings-bound test consumes distinguishable cross-frame contributions.

Thanks for the update. Reviewed 3f5ffa8. The code changes since the earlier review are mostly fixed, one test still does not test what it claims, and CI is green on all 14 checks. No conflicts.

The test "a configured intervalMs is removed from a screenshot request like the count" in resolve-cli-options.test.ts writes {count:3, intervalMs:250} with projectConfigDirWithCount() at line 229. It then calls the same factory again for cwd at line 234, and each call makes a new directory. So resolveCliOptions reads a config with only count, and the intervalMs assertion passes even if intervalMs leaks into a screenshot request. Such a leak would make an ordinary screenshot fail the daemon's interval-requires-count check. Please create one cwd, write both count and intervalMs to its agent-device.json, and pass that same cwd to resolveCliOptions.

On the open review threads, these still apply and need a fix: the remote policy test input (r4565067930), the evidence interval range (r4571606745), and, as a small note, the evidence commit description. These do not apply and can be resolved: r4565067935 because validateScreenshotBurst already refuses an interval without a count, r4565171241 and r4565171262 because both are fixed at head.

I read the code and did not run the tests or the live burst again. The live evidence is from 7f37419, and the files that affect behavior are unchanged since then. Once the test fixture, the remote policy test input and the evidence interval range are fixed, nothing else blocks merge. The evidence commit description can go in the same push.