mirror of
https://github.com/software-mansion/argent.git
synced 2026-09-14 19:27:14 +08:00
a2ed83e041
## The defect
`sendCommand` returns `void`. It writes the JSON command onto the
WebSocket — or queues it behind `once("open")` — and returns without
ever reading the reply:
```ts
export function sendCommand(api, cmd): void {
const ws = getOrCreateWs(api);
const payload = JSON.stringify({ id: String(++cmdId), ...cmd });
if (ws.readyState === WebSocket.OPEN) ws.send(payload);
else ws.once("open", () => ws.send(payload)); // queued, never awaited
}
```
simulator-server *does* acknowledge every command; argent just never
looked. Probing the shipped binary's `/ws` endpoint:
| sent | reply |
|---|---|
| `{"id":"1","cmd":"touch","type":"Down",…}` |
`{"id":"1","status":"ok"}` |
| `{"id":"7","cmd":"button","button":"notARealButton"}` |
`{"status":"error","message":"parse error: unknown variant
`notARealButton`…"}` |
Because the return type was `void` there was no ack to check and no
error to surface, so `gesture-tap` ended in an unconditional `return {
tapped: true, timestampMs }`.
**Repro:** shut a simulator down underneath an open session. The server
sends no reply at all and `gesture-tap` still reports `{ tapped: true
}`. Every transport failure — that one, a dead socket, a wedged device —
looks identical to success from the agent's side.
This is argent's own bug, and it is what turns an Apple regression into
a silent one: awaiting the reply would have made #932 an actionable
failure on day one instead of a silent no-op.
## The fix
`sendCommand` now returns a promise that resolves on `{"status":"ok"}`
and rejects when:
| condition | `error_code` |
|---|---|
| server answers `status: "error"` | `SIMULATOR_COMMAND_REJECTED` |
| socket fails or closes with the command in flight |
`SIMULATOR_COMMAND_TRANSPORT_FAILED` |
| reachable server never answers (5s) | `SIMULATOR_COMMAND_ACK_TIMEOUT`
|
Every WebSocket call site awaits it: `gesture-tap`, `gesture-swipe`,
`gesture-custom`, `gesture-pinch`, `gesture-rotate`, `button`, `rotate`.
Two details of the wire protocol shaped the implementation:
- **A success reply echoes the command id; an error reply carries no id
at all.** So a rejection can only be matched positionally, against the
oldest outstanding command. Since every send is awaited, that is
normally the only one in flight.
- **The abort paths in `gesture-swipe` / `gesture-rotate` lift the
finger before unwinding, and that lift is now fallible.** It is
therefore best-effort: a refused lift rides along as the `AbortError`'s
`cause` rather than replacing it — matching the existing pattern in
`gesture-drag`, because `format-error.ts` and `cdp-session.ts` key on
`err.name === "AbortError"`.
## Verified on a real simulator
Before, after shutting the device down mid-session: `{"tapped": true}`.
After:
```
result: {"tapped":true,"timestampMs":…} ← healthy tap, unchanged
--- simulator shut down underneath the open session ---
tap FAILED as it should:
code: SIMULATOR_COMMAND_TRANSPORT_FAILED
msg : simulator-server did not accept the 'touch' command: connect ECONNREFUSED
127.0.0.1:62818. The command was NOT delivered to the device. …
```
The failure signal is fully populated for telemetry (`error_kind:
"network"`, `network_failure: "connection_reset"`, `failure_command:
"simulator_server"`).
## Cost
An ack is a localhost round-trip behind an already-open socket: **0.06ms
p50 / 0.17ms max**, measured over 200 sends against a booted iOS sim.
Awaiting one per event adds a flat **~30ms to a swipe of any duration**
— against the 36–142ms of `setTimeout` skew the 16ms frame loop already
carries:
| `durationMs` | wall before | wall after | added |
|---|---|---|---|
| 200 | 236ms | 266ms | +30ms |
| 500 | 559ms | 591ms | +31ms |
| 1000 | 1142ms | 1178ms | +35ms |
Flagging it explicitly since the swipe cadence is deliberately tuned. If
reviewers would rather keep the cadence byte-identical, the alternative
is to pipeline within a gesture and await all acks at the end; that
costs more code and defers failure to the end of the gesture, so I kept
the simpler per-send await.
## Deliberately out of scope
- **The MoQ (`ios-remote`) transport** is unchanged — it has no
equivalent ack.
- **The stdin `pressKey` channel**, used by `keyboard` and paste's ⌘V,
is untouched: simulator-server never replies on stdin, so it has the
same blind spot with no local remedy. The WebSocket protocol *does*
expose an acked `key` command
(`{"cmd":"key","direction":"Down","code":4}` → `{"status":"ok"}`), so
moving those onto it would close the gap — a separate, riskier change to
the text-entry path that deserves its own PR.
## Tests
`packages/tool-server/test/simulator-command-ack.test.ts` — 8 tests
against a fake `/ws` server that mirrors the real wire format, **all 8
fail on `main`**: ack resolves, `status: error` rejects, socket death
rejects in flight, silent server times out, no phantom success,
`gesture-tap` throws instead of claiming a rejected tap, and the aborted
swipe still reports `AbortError`.
Full suite: 369 files / 4840 tests pass, `npm run lint` clean, `tsc
--build` clean.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_011bEvNDV7o3CpKdFmzjVBYF
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **New Features**
* Simulator commands now confirm successful delivery before completing.
* Added clear errors for rejected commands, connection failures, and
acknowledgement timeouts.
* **Bug Fixes**
* Button, rotation, tap, swipe, pinch, and custom gestures now report
failures instead of silently succeeding.
* In-progress gestures handle connection interruptions and cleanup
failures more reliably.
* **Tests**
* Added coverage for command acknowledgements, rejections, timeouts,
transport failures, and gesture error handling.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
---------
Co-authored-by: latekvo <f.kaminski2000@gmail.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
176 lines
6.2 KiB
TypeScript
176 lines
6.2 KiB
TypeScript
import { describe, expect, it, vi, beforeEach } from "vitest";
|
|
|
|
// Keep the real module (blueprints import from it too) but neutralise the
|
|
// fire-and-forget WebSocket send so no real socket is opened during the test.
|
|
vi.mock("../src/utils/simulator-client", async (importOriginal) => ({
|
|
...(await importOriginal<typeof import("../src/utils/simulator-client")>()),
|
|
sendCommand: vi.fn(),
|
|
}));
|
|
|
|
import { zodObjectToJsonSchema } from "@argent/registry";
|
|
import { gestureRotateTool } from "../src/tools/gesture-rotate";
|
|
import { sendCommand } from "../src/utils/simulator-client";
|
|
|
|
const udid = "AAAAAAAA-AAAA-AAAA-AAAA-AAAAAAAAAAAA";
|
|
const services = { simulatorServer: {} } as never;
|
|
|
|
// The physical screen the normalized coordinates land on in these assertions.
|
|
const SCREEN_W = 1080;
|
|
const SCREEN_H = 2400;
|
|
|
|
interface TouchCmd {
|
|
cmd: string;
|
|
type: "Down" | "Move" | "Up";
|
|
x: number;
|
|
y: number;
|
|
second_x: number | null;
|
|
second_y: number | null;
|
|
}
|
|
|
|
function touches(): TouchCmd[] {
|
|
return vi.mocked(sendCommand).mock.calls.map(([, cmd]) => cmd as unknown as TouchCmd);
|
|
}
|
|
|
|
beforeEach(() => {
|
|
// Reset (not just clear) so the abort test's mockImplementation can't leak.
|
|
vi.mocked(sendCommand).mockReset();
|
|
});
|
|
|
|
describe("gesture-rotate radiusX/radiusY", () => {
|
|
it("orbits a physical circle with constant finger separation when both are set", async () => {
|
|
// One physical radius, two normalizations: 0.4·1080 = 0.18·2400 = 432 px.
|
|
await gestureRotateTool.execute(services, {
|
|
udid,
|
|
centerX: 0.5,
|
|
centerY: 0.5,
|
|
radiusX: 0.4,
|
|
radiusY: 0.18,
|
|
startAngle: 0,
|
|
endAngle: 90,
|
|
durationMs: 48, // 3 steps → 4 frames, keeps the test fast
|
|
});
|
|
|
|
const frames = touches();
|
|
expect(frames).toHaveLength(4);
|
|
expect(frames[0]!.type).toBe("Down");
|
|
expect(frames[3]!.type).toBe("Up");
|
|
|
|
for (const f of frames) {
|
|
const px1 = f.x * SCREEN_W;
|
|
const py1 = f.y * SCREEN_H;
|
|
const px2 = f.second_x! * SCREEN_W;
|
|
const py2 = f.second_y! * SCREEN_H;
|
|
// Each finger stays 432 physical px from the center; separation 864.
|
|
expect(Math.hypot(px1 - 0.5 * SCREEN_W, py1 - 0.5 * SCREEN_H)).toBeCloseTo(432, 9);
|
|
expect(Math.hypot(px1 - px2, py1 - py2)).toBeCloseTo(864, 9);
|
|
}
|
|
});
|
|
|
|
it("keeps the legacy radius-only behavior byte-identical", async () => {
|
|
await gestureRotateTool.execute(services, {
|
|
udid,
|
|
centerX: 0.4,
|
|
centerY: 0.6,
|
|
radius: 0.15,
|
|
startAngle: 0,
|
|
endAngle: 90,
|
|
durationMs: 48,
|
|
});
|
|
|
|
const frames = touches();
|
|
expect(frames).toHaveLength(4);
|
|
frames.forEach((f, i) => {
|
|
// The pre-radiusX formula, replicated operation-for-operation so the
|
|
// expected values are bit-identical, not merely close.
|
|
const t = i / 3;
|
|
const angleDeg = 0 + (90 - 0) * t;
|
|
const angleRad = (angleDeg * Math.PI) / 180;
|
|
expect(f.x).toBe(0.4 + 0.15 * Math.cos(angleRad));
|
|
expect(f.y).toBe(0.6 + 0.15 * Math.sin(angleRad));
|
|
expect(f.second_x!).toBe(0.4 - 0.15 * Math.cos(angleRad));
|
|
expect(f.second_y!).toBe(0.6 - 0.15 * Math.sin(angleRad));
|
|
});
|
|
});
|
|
});
|
|
|
|
describe("gesture-rotate inputSchema", () => {
|
|
it("keeps the radius trio optional at the top level", () => {
|
|
const schema = zodObjectToJsonSchema(gestureRotateTool.zodSchema!);
|
|
expect(schema.type).toBe("object");
|
|
const required = schema.required as string[];
|
|
expect(required).toEqual(
|
|
expect.arrayContaining(["udid", "centerX", "centerY", "startAngle", "endAngle"])
|
|
);
|
|
expect(required).not.toContain("radius");
|
|
expect(required).not.toContain("radiusX");
|
|
expect(required).not.toContain("radiusY");
|
|
expect(schema).not.toHaveProperty("$schema");
|
|
// The cross-field rule cannot live in the schema a client may receive (see
|
|
// tool-input-schema-contract.test.ts), so the description has to carry it.
|
|
expect(gestureRotateTool.description).toMatch(/one half of the pair alone/i);
|
|
});
|
|
|
|
it("the zod refinements accept exactly the documented radius shapes", () => {
|
|
const base = { udid, centerX: 0.5, centerY: 0.5, startAngle: 0, endAngle: 90 };
|
|
const shapes: Array<[radii: Record<string, number>, valid: boolean]> = [
|
|
[{ radius: 0.15 }, true],
|
|
[{ radiusX: 0.4, radiusY: 0.18 }, true],
|
|
[{ radius: 0.15, radiusX: 0.4, radiusY: 0.18 }, true],
|
|
[{}, false],
|
|
[{ radius: 0.15, radiusX: 0.4 }, false],
|
|
[{ radius: 0.15, radiusY: 0.18 }, false],
|
|
[{ radiusX: 0.4 }, false],
|
|
[{ radiusY: 0.18 }, false],
|
|
];
|
|
for (const [radii, valid] of shapes) {
|
|
const params = { ...base, ...radii };
|
|
expect(
|
|
gestureRotateTool.zodSchema!.safeParse(params).success,
|
|
`zod schema on ${JSON.stringify(radii)}`
|
|
).toBe(valid);
|
|
}
|
|
});
|
|
});
|
|
|
|
// The ctx-less path (execute called with no third argument, as in direct unit
|
|
// invocations) is covered by every test in the describe above.
|
|
describe("gesture-rotate abort", () => {
|
|
it("lifts the fingers where they are and rejects when aborted mid-gesture", async () => {
|
|
const controller = new AbortController();
|
|
// Abort synchronously from inside the send mock after the 3rd dispatched
|
|
// event — deterministic, no real-time races.
|
|
let sent = 0;
|
|
vi.mocked(sendCommand).mockImplementation(() => {
|
|
if (++sent === 3) controller.abort();
|
|
return Promise.resolve();
|
|
});
|
|
|
|
await expect(
|
|
gestureRotateTool.execute(
|
|
services,
|
|
{
|
|
udid,
|
|
centerX: 0.5,
|
|
centerY: 0.5,
|
|
radius: 0.15,
|
|
startAngle: 0,
|
|
endAngle: 90,
|
|
durationMs: 480, // 30 steps → 31 frames if run to completion
|
|
},
|
|
{ signal: controller.signal } as never
|
|
)
|
|
).rejects.toThrow(/gesture-rotate aborted/);
|
|
|
|
const frames = touches();
|
|
// Down + 2 Moves before the abort lands, then only the terminal Up.
|
|
expect(frames.map((f) => f.type)).toEqual(["Down", "Move", "Move", "Up"]);
|
|
// The lift happens at the last dispatched finger positions, not the arc end.
|
|
const last = frames[3]!;
|
|
const prev = frames[2]!;
|
|
expect(last.x).toBe(prev.x);
|
|
expect(last.y).toBe(prev.y);
|
|
expect(last.second_x).toBe(prev.second_x);
|
|
expect(last.second_y).toBe(prev.second_y);
|
|
});
|
|
});
|