Repository navigation
fix(tv): auto-describe after tv-remote, and on Android TV and Vega #1262
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 13 commits
16dd5eb
798c773
03a628f
1200c3a
32c5905
7e9523c
e114998
00672ef
1118b17
cfe2854
733c631
38723ce
015b62c
64ffac0
0141592
6f7d446
fb3eb17
4fd1357
5a9c0ac
2b7e9cd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ export const AUTO_SCREENSHOT_TOOLS = new Set([ | |
| "paste", | ||
| "rotate", | ||
| "fold", | ||
| "tv-remote", | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A secret typed into a plain text field on a TV now shows up in the next
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Diplomat, Opus 5.5]: Not changing this. The capture after a secret is the same on every platform: on a phone the next |
||
| "launch-app", | ||
| "restart-app", | ||
| "open-url", | ||
|
|
@@ -44,6 +45,7 @@ export const AUTO_SCREENSHOT_DELAY_MS_BY_TOOL: Record<string, number> = { | |
| "gesture-rotate": 1500, | ||
| "run-sequence": 15000, | ||
| "button": 1500, | ||
| "tv-remote": 1500, | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Diplomat, Opus 5.5]: On an Apple TV simulator, every MCP
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| "rotate": 1000, | ||
| // The fold tool already waits for the panel hand-over; the cap covers the | ||
| // repaint after it. | ||
|
|
@@ -80,6 +82,7 @@ export const AUTO_DESCRIBE_TOOLS = new Set([ | |
| "paste", | ||
| "rotate", | ||
| "fold", | ||
| "tv-remote", | ||
|
latekvo marked this conversation as resolved.
|
||
| "launch-app", | ||
| "restart-app", | ||
| "open-url", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -173,6 +173,14 @@ describe("autoDescribeEnabled", () => { | |
| }); | ||
| }); | ||
|
|
||
| describe("auto-capture after tv-remote", () => { | ||
| it("captures the screen and the focus tree after a TV remote press", () => { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This checks that
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Diplomat, Opus 5.5]: Not adding an |
||
| expect(shouldAutoScreenshot("mcp__argent__tv-remote")).toBe(true); | ||
| expect(shouldAutoDescribe("mcp__argent__tv-remote")).toBe(true); | ||
| expect(AUTO_SCREENSHOT_DELAY_MS_BY_TOOL["tv-remote"]).toBe(1500); | ||
| }); | ||
| }); | ||
|
|
||
| describe("shouldAutoDescribe", () => { | ||
| it("is true for every listed tool, with or without a client prefix", () => { | ||
| for (const tool of AUTO_DESCRIBE_TOOLS) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,12 +40,14 @@ export function tvControlRef(device: DeviceInfo): { | |
| }; | ||
| } | ||
|
|
||
| // Per tool-server process, so neither a reap nor a dispose touches another | ||
| // server's daemon, and a daemon left by an earlier process can't unlink ours. | ||
| function axSocketPath(udid: string): string { | ||
| return `/tmp/argent-tv-ax-${udid.slice(0, 8)}.sock`; | ||
| return `/tmp/argent-tv-ax-${udid.slice(0, 8)}-${process.pid}.sock`; | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Diplomat, Opus 5.5]: The factory ( |
||
| } | ||
|
|
||
| function hidSocketPath(udid: string): string { | ||
| return `/tmp/argent-tv-hid-${udid.slice(0, 8)}.sock`; | ||
| return `/tmp/argent-tv-hid-${udid.slice(0, 8)}-${process.pid}.sock`; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -129,6 +131,16 @@ function spawnAxDaemon(udid: string, socketPath: string): ChildProcess { | |
| return proc; | ||
| } | ||
|
|
||
| // Kills every in-sim reader bound to `socketPath`. SIGKILLing `simctl spawn` | ||
| // does not reach its reader, and a reader left running unlinks the path when | ||
| // its idle `--timeout` fires (or on SIGTERM), even after a newer reader has | ||
| // bound it. SIGKILL, so the reader never runs that unlink. | ||
| function reapAxReaders(socketPath: string): Promise<void> { | ||
| const pattern = `tvos-ax-service --socket ${socketPath.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")}`; | ||
| // pkill exits 1 when nothing matched, the common case. | ||
| return new Promise((resolve) => execFile("pkill", ["-KILL", "-f", pattern], () => resolve())); | ||
| } | ||
|
|
||
| // Run the HID injector on the host. It loads SimulatorKit and holds one | ||
| // SimDeviceLegacyClient open against the UDID for its lifetime. | ||
| function spawnHidDaemon(udid: string, socketPath: string): ChildProcess { | ||
|
|
@@ -302,11 +314,11 @@ export const tvControlBlueprint: ServiceBlueprint<TvControlApi, DeviceInfo> = { | |
| axProc.removeListener("error", onAxError); | ||
| hidProc.removeListener("exit", onHidExit); | ||
| hidProc.removeListener("error", onHidError); | ||
| // SIGKILL the ax daemon: under `simctl spawn` SIGTERM doesn't reliably | ||
| // reach the in-sim process, which would orphan it. The hid daemon is a | ||
| // direct host process, so SIGTERM reaps it. | ||
| // The ax reader is reaped by `reapAxReaders`. The hid daemon is a direct | ||
| // host process, so SIGTERM reaps it. | ||
| if (!axProc.killed) axProc.kill("SIGKILL"); | ||
| if (!hidProc.killed) hidProc.kill("SIGTERM"); | ||
| await reapAxReaders(axSock); | ||
| // Drop any socket a daemon already bound, so a stale file can't make the | ||
| // next factory's accept-probe read as "ready" against a dead socket. | ||
| for (const p of [axSock, hidSock]) { | ||
|
|
@@ -334,6 +346,7 @@ export const tvControlBlueprint: ServiceBlueprint<TvControlApi, DeviceInfo> = { | |
| } catch { | ||
| /* no stale socket to remove */ | ||
| } | ||
| await reapAxReaders(axSock); | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Diplomat, Opus 5.5]: This reap now SIGKILLs the old in-simulator reader even while it is answering another caller's request. Before, killing |
||
| axProc = spawnAxDaemon(udid, axSock); | ||
| axProc.on("exit", onAxExit); | ||
| axProc.on("error", onAxError); | ||
|
|
@@ -465,11 +478,11 @@ export const tvControlBlueprint: ServiceBlueprint<TvControlApi, DeviceInfo> = { | |
| /* respawn failed — nothing extra to kill beyond the current axProc */ | ||
| } | ||
| } | ||
| // SIGKILL for the same reason as spawnFreshAx: under `simctl spawn` | ||
| // SIGTERM doesn't reliably reach the in-sim process, which would leave | ||
| // it orphaned. The hid daemon is a direct host process, so SIGTERM does. | ||
| // The ax reader is reaped by `reapAxReaders`. The hid daemon is a | ||
| // direct host process, so SIGTERM reaps it. | ||
| if (!axProc.killed) axProc.kill("SIGKILL"); | ||
| if (!hidProc.killed) hidProc.kill("SIGTERM"); | ||
| await reapAxReaders(axSock); | ||
| for (const p of [axSock, hidSock]) { | ||
| try { | ||
| fs.unlinkSync(p); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Every Apple TV screenshot is written to
$TMPDIR/argent-tv-screenshot-<udid8>-<hrtime>.png(tools/screenshot/index.ts:139-142), and nothing in the repo deletes it. Withtv-remotecaptured, every remote press on Apple TV now leaves one more file; they were 10-600 KB each at the default scale in my run. Before this PR, only explicit screenshots,launch-appandkeyboardwrote them.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Diplomat, Opus 5.5]: Confirmed, and it is older than this PR: every backend that writes its own PNG into
os.tmpdir()has it. On this machine$TMPDIRholds 380vega-screenshot-*and 57argent-tv-screenshot-*files, and ios-remote writesargent-remote-screenshots/<uuid>.pngthe same way. Nothing unlinks a registered artifact. The registry store is an in-memory map with no eviction. Withtv-remotecaptured, the per-press rate goes up, so I filed it as #1277 rather than patching only the Apple TV path here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Diplomat, Opus 5.5]: Still present as of 5a9c0ac. Each
tv-remotepress on Apple TV or Vega writes one PNG underos.tmpdir()that is never removed. Tracked in #1277, which is still open.