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.
293 lines
10 KiB
TypeScript
293 lines
10 KiB
TypeScript
import { describe, it, expect } from "vitest";
|
|
import {
|
|
findMissingRequired,
|
|
describeServerValidationFailure,
|
|
formatValidationError,
|
|
missingFlagNames,
|
|
} from "../src/run-validation.js";
|
|
import type { JsonSchema } from "../src/flag-parser.js";
|
|
|
|
const gestureTapSchema: JsonSchema = {
|
|
type: "object",
|
|
properties: {
|
|
udid: { type: "string" },
|
|
x: { type: "number" },
|
|
y: { type: "number" },
|
|
clickCount: { type: "integer" },
|
|
},
|
|
required: ["udid", "x", "y"],
|
|
};
|
|
|
|
const debuggerConnectSchema: JsonSchema = {
|
|
type: "object",
|
|
// `port` carries a default, so the schema generator leaves it out of `required`.
|
|
properties: { port: { type: "number" }, device_id: { type: "string" } },
|
|
required: ["device_id"],
|
|
};
|
|
|
|
const runSequenceSchema: JsonSchema = {
|
|
type: "object",
|
|
properties: {
|
|
udid: { type: "string" },
|
|
steps: { type: "array", items: { type: "object" } },
|
|
tags: { type: "array", items: { type: "string" } },
|
|
filter: { type: "object" },
|
|
verbose: { type: "boolean" },
|
|
},
|
|
required: ["udid", "steps", "tags", "filter", "verbose"],
|
|
};
|
|
|
|
/** The shape the tool-server returns for a missing required string, verbatim from Zod. */
|
|
function missingIssue(field: string) {
|
|
return {
|
|
expected: "string",
|
|
code: "invalid_type",
|
|
path: [field],
|
|
message: `Invalid input: expected string, received undefined`,
|
|
};
|
|
}
|
|
|
|
describe("findMissingRequired", () => {
|
|
it("lists every unsupplied required field in schema order", () => {
|
|
expect(findMissingRequired({}, gestureTapSchema)).toEqual(["udid", "x", "y"]);
|
|
});
|
|
|
|
it("treats a supplied falsy value as supplied", () => {
|
|
// `0` and `false` are legitimate values; a truthiness check would demand them again.
|
|
expect(findMissingRequired({ udid: "X", x: 0, y: 0 }, gestureTapSchema)).toEqual([]);
|
|
expect(
|
|
findMissingRequired({ udid: "X", x: 0, y: 0, verbose: false }, gestureTapSchema)
|
|
).toEqual([]);
|
|
});
|
|
|
|
it("never asks for a field that carries a default", () => {
|
|
expect(findMissingRequired({ device_id: "X" }, debuggerConnectSchema)).toEqual([]);
|
|
});
|
|
|
|
it("does not confuse an inherited property name with a supplied value", () => {
|
|
const schema: JsonSchema = {
|
|
type: "object",
|
|
properties: { constructor: { type: "string" } },
|
|
required: ["constructor"],
|
|
};
|
|
expect(findMissingRequired({}, schema)).toEqual(["constructor"]);
|
|
});
|
|
|
|
it("reports nothing when the schema requires nothing", () => {
|
|
expect(findMissingRequired({}, { type: "object", properties: {} })).toEqual([]);
|
|
expect(findMissingRequired({}, undefined)).toEqual([]);
|
|
});
|
|
});
|
|
|
|
describe("formatValidationError", () => {
|
|
const report = (missing: string[]) => ({ missing, invalid: [], rawIssues: null });
|
|
|
|
it("names a single missing flag", () => {
|
|
expect(formatValidationError(report(["device_id"]), debuggerConnectSchema)).toBe(
|
|
"missing required flag --device_id"
|
|
);
|
|
});
|
|
|
|
it("names several missing flags in schema order", () => {
|
|
expect(formatValidationError(report(["y", "udid", "x"]), gestureTapSchema)).toBe(
|
|
"missing required flags --udid, --x, --y"
|
|
);
|
|
});
|
|
|
|
it("names a field that can only be passed as JSON by its -json flag", () => {
|
|
expect(formatValidationError(report(["steps"]), runSequenceSchema)).toBe(
|
|
"missing required flag --steps-json"
|
|
);
|
|
expect(formatValidationError(report(["filter"]), runSequenceSchema)).toBe(
|
|
"missing required flag --filter-json"
|
|
);
|
|
});
|
|
|
|
it("names an array of scalars and a boolean by their plain flag", () => {
|
|
expect(formatValidationError(report(["tags"]), runSequenceSchema)).toBe(
|
|
"missing required flag --tags"
|
|
);
|
|
expect(formatValidationError(report(["verbose"]), runSequenceSchema)).toBe(
|
|
"missing required flag --verbose"
|
|
);
|
|
});
|
|
|
|
it("attributes a rejected value to its flag", () => {
|
|
const text = formatValidationError(
|
|
{ missing: [], invalid: [{ path: ["x"], message: "out of range" }], rawIssues: [] },
|
|
gestureTapSchema
|
|
);
|
|
expect(text).toBe("--x out of range");
|
|
});
|
|
|
|
it("states a rule that spans the whole payload without inventing a flag", () => {
|
|
// An object-level rule reports an empty path — there is no single field to blame.
|
|
const text = formatValidationError(
|
|
{
|
|
missing: [],
|
|
invalid: [{ path: [], message: "pass a non-zero deltaX or deltaY" }],
|
|
rawIssues: [],
|
|
},
|
|
gestureTapSchema
|
|
);
|
|
expect(text).toBe("pass a non-zero deltaX or deltaY");
|
|
expect(text).not.toContain("undefined");
|
|
});
|
|
|
|
it("points inside a nested value", () => {
|
|
const text = formatValidationError(
|
|
{
|
|
missing: [],
|
|
invalid: [{ path: ["steps", 0, "tool"], message: "expected a string" }],
|
|
rawIssues: [],
|
|
},
|
|
runSequenceSchema
|
|
);
|
|
expect(text).toBe("--steps-json steps[0].tool expected a string");
|
|
});
|
|
|
|
it("reports missing and rejected fields together, missing first", () => {
|
|
const text = formatValidationError(
|
|
{ missing: ["udid"], invalid: [{ path: ["x"], message: "out of range" }], rawIssues: [] },
|
|
gestureTapSchema
|
|
);
|
|
expect(text.split("\n")[0]).toBe("missing required flag --udid");
|
|
expect(text).toContain("--x out of range");
|
|
});
|
|
});
|
|
|
|
describe("missingFlagNames", () => {
|
|
it("renders flag names a caller can echo back", () => {
|
|
expect(
|
|
missingFlagNames(
|
|
{ missing: ["steps", "udid"], invalid: [], rawIssues: null },
|
|
runSequenceSchema
|
|
)
|
|
).toEqual(["--udid", "--steps-json"]);
|
|
});
|
|
});
|
|
|
|
describe("describeServerValidationFailure", () => {
|
|
const forGestureTap = (err: unknown, payload: Record<string, unknown> = {}) =>
|
|
describeServerValidationFailure(err, payload, gestureTapSchema);
|
|
|
|
it("recognises missing required fields the server rejected", () => {
|
|
const err = new Error(
|
|
JSON.stringify([missingIssue("udid"), missingIssue("x"), missingIssue("y")])
|
|
);
|
|
const report = forGestureTap(err);
|
|
expect(report?.missing).toEqual(["udid", "x", "y"]);
|
|
expect(report?.invalid).toEqual([]);
|
|
});
|
|
|
|
it("recognises a value the tool rejected", () => {
|
|
const err = new Error(JSON.stringify([{ code: "too_big", path: ["x"], message: "too big" }]));
|
|
const report = forGestureTap(err, { udid: "X", x: 99, y: 0.5 });
|
|
expect(report?.missing).toEqual([]);
|
|
expect(report?.invalid).toEqual([{ path: ["x"], message: "too big" }]);
|
|
});
|
|
|
|
it("recognises a rule that spans the whole payload", () => {
|
|
// An object-level rule reports an empty path; it is still this tool rejecting this input.
|
|
const err = new Error(
|
|
JSON.stringify([{ code: "custom", path: [], message: "pass a non-zero delta" }])
|
|
);
|
|
const report = forGestureTap(err, { udid: "X", x: 0, y: 0 });
|
|
expect(report).not.toBeNull();
|
|
expect(report?.invalid).toEqual([{ path: [], message: "pass a non-zero delta" }]);
|
|
});
|
|
|
|
it("does not call an optional field a missing required flag", () => {
|
|
// A rule can fire on a field the schema does not require. Reporting it as missing would
|
|
// contradict the help block printed underneath, which marks no such field required.
|
|
const err = new Error(
|
|
JSON.stringify([
|
|
{ code: "custom", path: ["clickCount"], message: "condition requires clickCount" },
|
|
])
|
|
);
|
|
const report = forGestureTap(err, { udid: "X", x: 0.5, y: 0.5 });
|
|
expect(report?.missing).toEqual([]);
|
|
expect(report?.invalid).toEqual([
|
|
{ path: ["clickCount"], message: "condition requires clickCount" },
|
|
]);
|
|
});
|
|
|
|
it("treats an explicitly supplied null as rejected, not missing", () => {
|
|
const err = new Error(
|
|
JSON.stringify([
|
|
{ code: "invalid_type", path: ["udid"], message: "expected string, received null" },
|
|
])
|
|
);
|
|
const report = forGestureTap(err, { udid: null, x: 0.5, y: 0.5 });
|
|
expect(report?.missing).toEqual([]);
|
|
expect(report?.invalid).toHaveLength(1);
|
|
});
|
|
|
|
it("orders missing fields by the schema, whatever order the server reported them", () => {
|
|
const err = new Error(
|
|
JSON.stringify([missingIssue("y"), missingIssue("udid"), missingIssue("x")])
|
|
);
|
|
expect(forGestureTap(err)?.missing).toEqual(["udid", "x", "y"]);
|
|
});
|
|
|
|
it("keeps the raw issue list for programmatic callers", () => {
|
|
const issues = [missingIssue("udid")];
|
|
expect(forGestureTap(new Error(JSON.stringify(issues)))?.rawIssues).toEqual(issues);
|
|
});
|
|
|
|
describe("leaves anything that is not this tool's input validation alone", () => {
|
|
it.each([
|
|
["an ordinary runtime error", new Error("Simulator not booted")],
|
|
["a JSON object", new Error('{"error":"nope"}')],
|
|
["an empty array", new Error("[]")],
|
|
["an array of non-issues", new Error('[{"a":1}]')],
|
|
["an array of strings", new Error('["nope"]')],
|
|
["a non-Error rejection", { weird: true }],
|
|
])("%s", (_label, err) => {
|
|
expect(forGestureTap(err)).toBeNull();
|
|
});
|
|
|
|
it("a validation error about something other than this tool's input", () => {
|
|
// A tool that validates a device's response and lets that error escape names fields that
|
|
// are not flags of this tool; dressing it up as user error would be a lie.
|
|
const err = new Error(
|
|
JSON.stringify([
|
|
{ code: "invalid_type", path: ["batteryLevel"], message: "expected number" },
|
|
])
|
|
);
|
|
expect(forGestureTap(err)).toBeNull();
|
|
});
|
|
|
|
it("a mixed list where only some issues address this tool", () => {
|
|
const err = new Error(
|
|
JSON.stringify([missingIssue("udid"), { code: "custom", path: ["nope"], message: "x" }])
|
|
);
|
|
expect(forGestureTap(err)).toBeNull();
|
|
});
|
|
});
|
|
});
|
|
|
|
describe("the two ways an invocation is rejected read identically", () => {
|
|
it("renders the same message whether detected locally or reported by the server", () => {
|
|
const local = {
|
|
missing: findMissingRequired({}, gestureTapSchema),
|
|
invalid: [],
|
|
rawIssues: null,
|
|
};
|
|
// Deliberately shuffled: the renderer, not the server, decides the order.
|
|
const server = describeServerValidationFailure(
|
|
new Error(JSON.stringify([missingIssue("y"), missingIssue("udid"), missingIssue("x")])),
|
|
{},
|
|
gestureTapSchema
|
|
);
|
|
|
|
expect(server).not.toBeNull();
|
|
expect(formatValidationError(server!, gestureTapSchema)).toBe(
|
|
formatValidationError(local, gestureTapSchema)
|
|
);
|
|
expect(formatValidationError(local, gestureTapSchema)).toBe(
|
|
"missing required flags --udid, --x, --y"
|
|
);
|
|
});
|
|
});
|