mirror of
https://github.com/software-mansion/argent.git
synced 2026-09-14 19:27:14 +08:00
d08c3a3cfc
Fixes #645. ## Before ``` $ argent run gesture-tap [ { "expected": "string", "code": "invalid_type", "path": ["udid"], "message": "Invalid input: expected string, received undefined" }, ... two more ] ``` ## After ``` $ argent run gesture-tap Error: missing required flags --udid, --x, --y argent run gesture-tap [flags] ...full per-flag help... ``` ## Scope The issue names the missing-flag case. Reproducing it showed the same raw dump has a second trigger: any constraint the tool itself rejects, which the CLI cannot know in advance. | invocation | before | after | |---|---|---| | `gesture-tap` (no flags) | raw issue array | `missing required flags --udid, --x, --y` | | `screenshot --scale 99` | raw issue array | `--scale Too big: expected number to be <=1` | | `gesture-scroll --udid X --x .5 --y .5` (zero deltas) | raw issue array | `Pass a non-zero deltaX and/or deltaY …` | | `run-sequence --udid X` | raw issue array | `missing required flag --steps-json` | | `gesture-tap --x abc` | already good | unchanged | | runtime failure | plain message, exit 1 | unchanged | Fixing only the first row would have left the CLI's error voice less predictable than before, so both go through the same renderer. ## How it works Two sources, one output. Missing flags are found locally from the tool's published schema — a **presence** check only, never a type or range judgement, which stays exclusively the server's. That is sound because the schema generator excludes defaulted fields from `required` (`debugger-connect` requires `device_id`, not `port`), so it cannot reject a call the server would have accepted. Everything else is read off the rejection the tool-server already returns. Both produce one report rendered by one function and reported through one call site, so the wording, the output channel and the exit code cannot drift between them. Recognition of a rejection is **structural** — the shape of the issue list plus the tool's own schema — never the wording of a message, which is not ours to depend on and changes between Zod releases. Four gates must all hold, and any failure leaves the existing handling untouched. Details worth a reviewer's attention: - **A rule spanning the whole payload reports no field.** `gesture-scroll` and `gesture-rotate` validate at the object level, and `screenshot-diff` is `.strict()`. Treating those as field-scoped would print `Error: --undefined …`; they are stated on their own instead. - **A rule firing on an *optional* field is not called a missing required flag.** `await-ui-element` refines on `expectedText`, which it does not require — reporting that as missing would contradict the help block printed directly underneath, where the field carries no `(required)` marker. - **The check runs after `--args` is merged**, so `argent run gesture-tap --args '{...}'` is not falsely reported as missing everything. Verified live: it reaches the device and taps. - Presence uses own-key semantics, so a required field named e.g. `constructor` is not treated as supplied by the prototype chain. ## Behaviour changes a reviewer should weigh - **A tool-rejected value now exits 2 instead of 1.** `run.ts` already uses 2 for bad input and 1 for a failed run; a Zod 400 means the tool never ran. No in-repo consumer tests for a specific code — `scripts/e2e-full` only checks zero/non-zero. Note this does **not** make 2 universal: an unknown tool name still exits 1, which is untouched here. - **`--json` gains a defined failure shape** (it had none — the blob printed regardless). One object on stderr, nothing on stdout, so `--json | jq` on a failed run reads an empty stream and a non-zero status: ```json { "error": "missing required flags --udid, --x, --y", "missing": ["--udid", "--x", "--y"], "issues": [] } ``` `issues` carries the tool's list verbatim, which is the raw data the issue asked to keep. - **One adjacent fix**, called out because it is not the reported bug: the global-option parse ran *outside* the error handler, so `argent run screenshot --out` printed a raw stack trace. Same bad-input-to-ugly-output defect, one line above the fix. ## Not done - The unknown-flag hole (`argent run gesture-tap --bogusflag 1` is silently accepted and the tool runs). The pass-through is deliberate — see the comment in `flag-parser.ts` — and belongs with #586, the same CLI-argument family. - `argent flow` builds its own payload and has its own required check; it cannot reach this path. - The server still returns the issue list verbatim, so MCP and other clients are unaffected. All prettifying is CLI-side. ## Verification Live against the built CLI for every row of the table above, plus: `debugger-connect` asks only for `--device_id` and never `--port`; `--args '{...}'` reaches the device and taps; a thrown runtime error still prints its bare message with no `Error:` prefix, no help block, exit 1. 43 new tests. `run-telemetry.test.ts` passes **unedited** — it already asserted the non-Zod passthrough, which makes it the regression guard for the untouched path. Full CLI suite 320 passed, registry 87 passed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- > **#699 is stacked on this branch** — it conflicts with this one in `flag-parser.ts`, so it was rebased on top rather than left to collide at merge time. Merge this first.
238 lines
8.1 KiB
TypeScript
238 lines
8.1 KiB
TypeScript
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
|
|
import { run } from "../src/run.js";
|
|
|
|
const toolsClientMock = vi.hoisted(() => ({
|
|
fetchTool: vi.fn(),
|
|
callTool: vi.fn(),
|
|
baseUrl: vi.fn(async () => ({ url: "http://127.0.0.1:3001", token: "t" })),
|
|
}));
|
|
|
|
const telemetryMock = vi.hoisted(() => ({
|
|
init: vi.fn(),
|
|
shutdown: vi.fn(async () => undefined),
|
|
track: vi.fn(),
|
|
}));
|
|
|
|
vi.mock("@argent/tools-client", async (importOriginal) => {
|
|
const actual = await importOriginal<Record<string, unknown>>();
|
|
return {
|
|
...actual,
|
|
createToolsClient: vi.fn(() => toolsClientMock),
|
|
materializeArtifacts: vi.fn(async (result: unknown) => ({ result, images: [] })),
|
|
};
|
|
});
|
|
|
|
vi.mock("@argent/telemetry", () => telemetryMock);
|
|
|
|
const gestureTap = {
|
|
name: "gesture-tap",
|
|
description: "Tap the screen",
|
|
inputSchema: {
|
|
type: "object",
|
|
properties: {
|
|
udid: { type: "string" },
|
|
x: { type: "number" },
|
|
y: { type: "number" },
|
|
clickCount: { type: "integer" },
|
|
},
|
|
required: ["udid", "x", "y"],
|
|
},
|
|
};
|
|
|
|
const runSequence = {
|
|
name: "run-sequence",
|
|
description: "Run steps",
|
|
inputSchema: {
|
|
type: "object",
|
|
properties: {
|
|
udid: { type: "string" },
|
|
steps: { type: "array", items: { type: "object" } },
|
|
},
|
|
required: ["udid", "steps"],
|
|
},
|
|
};
|
|
|
|
const invoke = (argv: string[]) => run(argv, { paths: {} as never });
|
|
|
|
function stderr(): string {
|
|
return (console.error as unknown as { mock: { calls: unknown[][] } }).mock.calls
|
|
.map((c) => String(c[0]))
|
|
.join("\n");
|
|
}
|
|
|
|
function stdout(): string {
|
|
return (console.log as unknown as { mock: { calls: unknown[][] } }).mock.calls
|
|
.map((c) => String(c[0]))
|
|
.join("\n");
|
|
}
|
|
|
|
describe("argent run input validation", () => {
|
|
let exitSpy: ReturnType<typeof vi.spyOn>;
|
|
let errorSpy: ReturnType<typeof vi.spyOn>;
|
|
let logSpy: ReturnType<typeof vi.spyOn>;
|
|
|
|
beforeEach(() => {
|
|
vi.clearAllMocks();
|
|
toolsClientMock.fetchTool.mockResolvedValue(gestureTap);
|
|
toolsClientMock.callTool.mockResolvedValue({ data: { tapped: true } });
|
|
exitSpy = vi.spyOn(process, "exit").mockImplementation(((code?: number) => {
|
|
throw new Error(`process.exit:${code}`);
|
|
}) as typeof process.exit);
|
|
errorSpy = vi.spyOn(console, "error").mockImplementation(() => undefined);
|
|
logSpy = vi.spyOn(console, "log").mockImplementation(() => undefined);
|
|
});
|
|
|
|
afterEach(() => {
|
|
exitSpy.mockRestore();
|
|
errorSpy.mockRestore();
|
|
logSpy.mockRestore();
|
|
});
|
|
|
|
it("names the missing flags and shows the tool's help without calling the tool", async () => {
|
|
await expect(invoke(["gesture-tap"])).rejects.toThrow("process.exit:2");
|
|
|
|
expect(toolsClientMock.callTool).not.toHaveBeenCalled();
|
|
expect(stderr()).toContain("Error: missing required flags --udid, --x, --y");
|
|
expect(stdout()).toContain("argent run gesture-tap [flags]");
|
|
expect(stdout()).toContain("--udid <value>");
|
|
expect(telemetryMock.track).toHaveBeenCalledWith(
|
|
"cli:run_fail",
|
|
expect.objectContaining({
|
|
tool: "gesture-tap",
|
|
error_code: "CLI_RUN_INPUT_VALIDATION_FAILED",
|
|
failure_stage: "cli_run_required_flags",
|
|
failure_area: "cli",
|
|
error_kind: "validation",
|
|
})
|
|
);
|
|
});
|
|
|
|
it("never prints a raw issue list", async () => {
|
|
await expect(invoke(["gesture-tap"])).rejects.toThrow("process.exit:2");
|
|
expect(stderr()).not.toContain("invalid_type");
|
|
expect(stderr()).not.toContain('"path"');
|
|
});
|
|
|
|
it("accepts required fields supplied through --args", async () => {
|
|
await invoke(["gesture-tap", "--args", '{"udid":"X","x":0.5,"y":0.5}']);
|
|
|
|
expect(toolsClientMock.callTool).toHaveBeenCalledWith("gesture-tap", {
|
|
udid: "X",
|
|
x: 0.5,
|
|
y: 0.5,
|
|
});
|
|
});
|
|
|
|
it("accepts required fields split across --args and flags", async () => {
|
|
await invoke(["gesture-tap", "--udid", "X", "--args", '{"x":0.5,"y":0.5}']);
|
|
expect(toolsClientMock.callTool).toHaveBeenCalledTimes(1);
|
|
});
|
|
|
|
it("reports only the fields still missing after --args is merged", async () => {
|
|
await expect(invoke(["gesture-tap", "--args", '{"x":0.5}'])).rejects.toThrow("process.exit:2");
|
|
expect(stderr()).toContain("missing required flags --udid, --y");
|
|
});
|
|
|
|
it("names a JSON-only field by its -json flag", async () => {
|
|
toolsClientMock.fetchTool.mockResolvedValue(runSequence);
|
|
|
|
await expect(invoke(["run-sequence", "--udid", "X"])).rejects.toThrow("process.exit:2");
|
|
expect(stderr()).toContain("Error: missing required flag --steps-json");
|
|
});
|
|
|
|
it("accepts a JSON-only field passed as --field-json", async () => {
|
|
toolsClientMock.fetchTool.mockResolvedValue(runSequence);
|
|
|
|
await invoke(["run-sequence", "--udid", "X", "--steps-json", '[{"tool":"button"}]']);
|
|
expect(toolsClientMock.callTool).toHaveBeenCalledTimes(1);
|
|
});
|
|
|
|
it("still prints help for --help without demanding flags", async () => {
|
|
await invoke(["gesture-tap", "--help"]);
|
|
|
|
expect(toolsClientMock.callTool).not.toHaveBeenCalled();
|
|
expect(stdout()).toContain("argent run gesture-tap [flags]");
|
|
expect(stderr()).not.toContain("missing required");
|
|
});
|
|
|
|
describe("a value the tool rejects", () => {
|
|
beforeEach(() => {
|
|
toolsClientMock.callTool.mockRejectedValue(
|
|
new Error(
|
|
JSON.stringify([{ code: "too_big", path: ["x"], message: "Too big: expected <=1" }])
|
|
)
|
|
);
|
|
});
|
|
|
|
it("is reported against its flag instead of as a raw issue list", async () => {
|
|
await expect(
|
|
invoke(["gesture-tap", "--udid", "X", "--x", "99", "--y", "0.5"])
|
|
).rejects.toThrow("process.exit:2");
|
|
|
|
expect(stderr()).toContain("Error: --x Too big: expected <=1");
|
|
expect(stderr()).not.toContain('"code"');
|
|
expect(stdout()).toContain("argent run gesture-tap [flags]");
|
|
expect(telemetryMock.track).toHaveBeenCalledWith(
|
|
"cli:run_fail",
|
|
expect.objectContaining({
|
|
error_code: "CLI_RUN_INPUT_VALIDATION_FAILED",
|
|
failure_stage: "cli_run_server_validation",
|
|
error_kind: "validation",
|
|
})
|
|
);
|
|
});
|
|
});
|
|
|
|
it("leaves an ordinary runtime failure exactly as it was", async () => {
|
|
toolsClientMock.callTool.mockRejectedValue(new Error("Simulator not booted"));
|
|
|
|
await expect(
|
|
invoke(["gesture-tap", "--udid", "X", "--x", "0.5", "--y", "0.5"])
|
|
).rejects.toThrow("process.exit:1");
|
|
|
|
expect(console.error).toHaveBeenCalledWith("Simulator not booted");
|
|
expect(stderr()).not.toContain("Error: ");
|
|
expect(stdout()).not.toContain("argent run gesture-tap [flags]");
|
|
expect(telemetryMock.track).toHaveBeenCalledWith(
|
|
"cli:run_fail",
|
|
expect.objectContaining({ error_code: "CLI_RUN_TOOL_CALL_FAILED", error_kind: "unknown" })
|
|
);
|
|
});
|
|
|
|
describe("--json", () => {
|
|
it("emits one machine-readable object on stderr and nothing on stdout", async () => {
|
|
await expect(invoke(["gesture-tap", "--json"])).rejects.toThrow("process.exit:2");
|
|
|
|
expect(stdout()).toBe("");
|
|
const envelope = JSON.parse(stderr());
|
|
expect(envelope).toEqual({
|
|
error: "missing required flags --udid, --x, --y",
|
|
missing: ["--udid", "--x", "--y"],
|
|
issues: [],
|
|
});
|
|
});
|
|
|
|
it("carries the tool's own issue list when the tool rejected a value", async () => {
|
|
const issues = [{ code: "too_big", path: ["x"], message: "Too big: expected <=1" }];
|
|
toolsClientMock.callTool.mockRejectedValue(new Error(JSON.stringify(issues)));
|
|
|
|
await expect(
|
|
invoke(["gesture-tap", "--json", "--udid", "X", "--x", "99", "--y", "0.5"])
|
|
).rejects.toThrow("process.exit:2");
|
|
|
|
const envelope = JSON.parse(stderr());
|
|
expect(envelope.issues).toEqual(issues);
|
|
expect(envelope.missing).toEqual([]);
|
|
expect(envelope.error).toBe("--x Too big: expected <=1");
|
|
expect(stdout()).toBe("");
|
|
});
|
|
});
|
|
|
|
it("reports a missing value for a global option instead of a raw stack", async () => {
|
|
await expect(invoke(["gesture-tap", "--out"])).rejects.toThrow("process.exit:2");
|
|
|
|
expect(stderr()).toContain("Error: --out requires a path");
|
|
expect(stderr()).toContain("argent run gesture-tap --help");
|
|
});
|
|
});
|