software-mansion__argent

skills/software-mansion__argent

Fork 0

mirror of https://github.com/software-mansion/argent.git synced 2026-09-14 19:27:14 +08:00

Commit Graph

Author	SHA1	Message	Date
filip131311	3fff176a32	fix(cli): let a boolean flag take the value you typed (#699 ) Fixes #586. ## The bug `--includeChildren false` left the flag true and silently dropped the word, so the tool ran with the opposite of what was asked. The reporter spent a while chasing a filter bug that did not exist — the output was simply the unfiltered kind. Reproduced on 0.18.1, device-free, with a discriminator that shows the flag's value directly: ``` $ argent run screenshot-diff --udid U --baselinePath A.png --currentPath B.png --captureCurrent=false { "summary": "Screenshot diff summary … pixel_mismatch: 1.79% …" <-- false, diff runs $ argent run screenshot-diff --udid U --baselinePath A.png --currentPath B.png --captureCurrent false [Tool:screenshot-diff] Provide either currentPath or captureCurrent, not both. <-- stayed TRUE ``` That message is the whole problem in one line: the user typed `false`, and the tool refuses on the grounds that they asked to capture. ## Why the old behaviour existed, and why it does not need to The parser deliberately declined to look ahead: > `// Bare boolean flag: true. Don't consume next argv to avoid surprising behavior when a positional follows.` That protects a case which does not exist. `parseFlags` has exactly one caller, and `positional` is never read anywhere in the repo — every bare token is collected and dropped, for booleans and for every other type. There is no positional to steal. The module header had documented the space form as legal all along: ``` // --name value (string / number / integer / boolean-with-value) ``` This matters because several tool booleans default to true, so `false` is the only way to say what you mean: `screenshot.includeImageInContext`, `network-request.includeBody`, `debugger-component-tree.onScreenOnly`, `debugger-inspect-element.resolveSourceMaps`. ## What changed A boolean flag consumes a following token when it is exactly the word `true`/`false`, case-insensitively. `1`/`0` and everything else are left alone — a bare number after a switch is more likely an unrelated argument than a value, and guessing at an ambiguous token is what caused this bug. `--flag -- false` still keeps the word positional. Three consequences are handled here rather than left to bite later: - `argent run` now reports arguments it ignored. This is what stops `--flag 0` and `--flag yes` being the same silent inversion, in a syntax users are now being taught works. stderr, so `--json` stays parseable. - `--no-flag true` / `--no-flag false` now name the positive form instead of quietly meaning false. One is a contradiction, the other a double negative. - `--flag=True` used to throw while `--flag True` would now work — the inline coercion was case-sensitive and did not trim. Both paths now share one helper, so a single word cannot get two answers one function apart. The flow and lens parsers take switches that accept no value at all, and would have silently ignored the syntax their users just learned elsewhere. They now refuse it — in the flow parser that also stops `false` being swallowed as the flow name, since its first bare token is the flow to run. `--help` gains one legend line for tools with a boolean field, rather than widening the flag column on every field of every tool: ``` Booleans: --flag or --flag true sets true; --flag false, --flag=false, or --no-flag sets false. ``` It deliberately does not begin with `--`. `scripts/e2e-full/lib/discover-tools.sh` reads any such line in the `Flags:` section as a flag row and takes the first `--token` as its name, so a legend starting with a flag would inject a phantom flag into every tool model it builds. There is a test pinning that shape, verified live: running the harness's awk over the new help output yields exactly the real flags and no legend row. ## Verified live ``` --captureCurrent false -> diff runs (was: "Provide either currentPath or captureCurrent, not both.") --captureCurrent=false -> unchanged --captureCurrent 0 -> "Note: ignoring unused argument(s): 0. Pass values as --flag <value> or --flag=<value>." screenshot --help -> legend present; harness awk parses only the real flags ``` 25 new tests; 8 confirmed to fail before and pass after. The rest pin what deliberately did not change: bare flag still true, a following flag or non-boolean word left alone, `1`/`0` still positional in the space form, `--` escape, string and unknown fields untouched, `--no-flag` semantics, boolean arrays, last-write-wins. The parser had zero boolean coverage before — which is why this survived. Full CLI suite 298 passing; lint, prettier and `typecheck:tests` clean. ## Scope and neighbours Only `flag-parser.ts` (the schema-driven parser for `argent run`) changes semantics. The other CLI parsers take fixed, default-off switches where `false` has no coherent meaning; three of them already reject a stray token outright, and the two that did not now do. `flow.ts`'s deliberate rejection of `--flag=value` is untouched. #648 also edits `flag-parser.ts` (rewrites `renderFlagName`, inserts helpers at the same anchor). Behaviourally compatible — it keeps boolean → bare `--name` — so this is a textual conflict only, and either order works. ## Found while here, not fixed `scripts/e2e-full/lib/discover-tools.sh:60-64` detects flag kinds with `\bboolean\b` in awk, where `\b` is backspace, not a word boundary — so it never matches and every non-enum flag is typed `unknown`. That makes the `bad-type` validation case downstream unreachable. Pre-existing and unrelated to this fix; filing separately. --- > Stacked on #648 (`filip/run-cli-validation-messages`). Both rewrite the same region of `packages/argent-cli/src/flag-parser.ts` — #648 factors out `flagNameFor`/`isJsonField` and rewrites `renderFlagName`, this one adds `booleanWord` beside them and a boolean legend to `formatSchemaUsage`. Merging them independently conflicts; this branch is rebased on #648, so review only the top commit. GitHub retargets it to `main` when #648 merges. --------- Co-authored-by: Filip131311 <f.kaminski2000@gmail.com>	2026-08-03 12:18:11 +02:00
Ignacy Łątka	c5bab2a83d	fix(cli): let a tool's own --args field win over the whole-payload flag (#458 ) ## Bug `argent run flow-add-step` declares its own `args` field — a JSON string holding the recorded step's tool arguments. But the CLI flag parser hardcoded `--args` as a whole-payload escape hatch, so `--args` shadowed the tool's own `args` field. The documented per-flag form ``` argent run flow-add-step --command gesture-tap --args '{"udid":"...","x":0.5,"y":0.5}' ``` silently dropped the step arguments: the `--args` JSON was consumed as the whole payload and the tool's `args` field stayed `undefined`, producing a misleading downstream validation error. Only the nested full-payload form worked. ## Fix Make `--args` schema-aware in `parseFlags()`: - When the tool's schema declares a property named `args` (`schema.properties.args`), `--args <value>` is routed through the normal per-field handling and coerced by its declared type (a `string` for flow-add-step, so the raw JSON string passes straight into the `args` field). Both `--args value` and `--args=value` forms are supported. - When the schema does NOT declare `args`, the existing whole-payload behavior is preserved exactly, including the `--args -` stdin sentinel. `printToolHelp()` no longer prints the global whole-payload `--args <json>` / `--args -` lines for tools that declare their own `args` field, so the help stops contradicting the per-field `--args` shown in the schema flags. The change is general: any tool with an `args` field benefits, with no special-casing by tool name. ## Verification - `npm run build` (root tsc) — clean. - `npm test -w @argent/cli` — 172 passed, including 6 new `parseFlags` cases covering schema-with-`args` (space-separated + inline forms, plus sibling fields) and schema-without-`args` (whole-payload preserved, `--args -` sentinel, no-schema fallback). - Prettier + ESLint (`--max-warnings 0`) clean on the touched files. Full CLI end-to-end (running `flow-add-step` against a live tool-server with an active flow recording) was not exercised here; the added unit tests prove the parser behavior. Fixes #452 --- ### CLI end-to-end verification Ran the documented per-flag form against a live tool-server with an active flow recording: - Before (global argent 0.13.0): `argent run flow-add-step --command gesture-tap --args '{"udid":"...","x":0.5,"y":0.35}'` → `Invalid params for tool "gesture-tap": udid expected string, received undefined ...` (args dropped). - After (this branch): same command → `{ "message": "Step added to \"colltest\" flow", "toolResult": { "tapped": true } }`, and the recorded YAML contains the step with its args (`udid`/`x`/`y`). The per-flag form now works as documented.	2026-07-07 13:37:09 +02:00
Ignacy Łątka	a7722df8a2	fix(cli): reject empty numeric flags and array/-json interleave crash (#432 ) Fixes two related defects in `packages/argent-cli/src/flag-parser.ts`, the argv-to-JSON flag parser used by `argent run`. The package previously had no dedicated test file for it; this PR adds one covering both fixes. ## Defect A: empty/whitespace numeric values coerced to 0 `coerceScalar` validates number/integer fields with `Number.isNaN` / `Number.isInteger`, but `Number("")` and `Number(" ")` both evaluate to `0`, so those guards pass. An empty or whitespace-only value was therefore silently accepted as `0` instead of being rejected. Reachable via `--x=` (empty inline), `--x ""`, and `--x " "`. - Observed: `parseFlags(["--x="], { type: "object", properties: { x: { type: "number" } } })` returned `{ x: 0 }`. - Expected: rejected, the same way a non-numeric string like `"abc"` already is. Fix: reject an empty or whitespace-only value up front for number/integer types, throwing the same `FlagParseException` the function already throws for `"abc"` (`--<flag> expected a number/integer, got "<raw>"`). Valid numbers are unaffected. ## Defect B: array + `-json` interleave throws a raw TypeError `seenArrayFields` is only populated by the array branch, but the `--field-json` escape hatch writes `args[fieldName]` directly and can set it to a non-array. If a field was already seen as an array, a later array-form occurrence ran `(args[flag] as unknown[]).push(...)` on a non-array value and threw a raw `TypeError`. `run.ts` only catches `FlagParseException` and re-throws anything else, so the user got an uncaught stack trace instead of a clean error. Repro: ``` parseFlags( ["--tags", "a", "--tags-json", '"b"', "--tags", "c"], { type: "object", properties: { tags: { type: "array", items: { type: "string" } } } } ) ``` - Observed: throws `TypeError: args[flag].push is not a function` (uncaught, surfaces as a stack trace through `run.ts`). - Expected: a clean validation error. Fix: in the array branch, when `args[flag]` is not actually an array (because `--field-json` replaced it), throw a `FlagParseException` explaining the `--field` and `--field-json` forms cannot be mixed for the same field, never a raw `TypeError`. ## Tests Adds `packages/argent-cli/test/flag-parser.test.ts`. The new tests fail before the fix (4 failures) and pass after; the full `argent-cli` suite stays green (90 passed). Behavior for valid inputs is unchanged.	2026-07-03 16:19:22 +02:00

Author

SHA1

Message

Date

filip131311

3fff176a32

fix(cli): let a boolean flag take the value you typed (#699 )

Fixes #586.

## The bug

`--includeChildren false` left the flag **true** and silently dropped
the word, so the tool ran with the opposite of what was asked. The
reporter spent a while chasing a filter bug that did not exist — the
output was simply the unfiltered kind.

Reproduced on 0.18.1, device-free, with a discriminator that shows the
flag's value directly:

```
$ argent run screenshot-diff --udid U --baselinePath A.png --currentPath B.png --captureCurrent=false
{ "summary": "Screenshot diff summary … pixel_mismatch: 1.79% …"      <-- false, diff runs

$ argent run screenshot-diff --udid U --baselinePath A.png --currentPath B.png --captureCurrent false
[Tool:screenshot-diff] Provide either currentPath or captureCurrent, not both.   <-- stayed TRUE
```

That message is the whole problem in one line: the user typed `false`,
and the tool refuses on the grounds that they asked to capture.

## Why the old behaviour existed, and why it does not need to

The parser deliberately declined to look ahead:

> `// Bare boolean flag: true. Don't consume next argv to avoid
surprising behavior when a positional follows.`

That protects a case which does not exist. `parseFlags` has exactly one
caller, and **`positional` is never read anywhere in the repo** — every
bare token is collected and dropped, for booleans and for every other
type. There is no positional to steal. The module header had documented
the space form as legal all along:

```
//   --name value          (string / number / integer / boolean-with-value)
```

This matters because several tool booleans **default to true**, so
`false` is the only way to say what you mean:
`screenshot.includeImageInContext`, `network-request.includeBody`,
`debugger-component-tree.onScreenOnly`,
`debugger-inspect-element.resolveSourceMaps`.

## What changed

A boolean flag consumes a following token when it is exactly the word
`true`/`false`, case-insensitively. `1`/`0` and everything else are left
alone — a bare number after a switch is more likely an unrelated
argument than a value, and guessing at an ambiguous token is what caused
this bug. `--flag -- false` still keeps the word positional.

Three consequences are handled here rather than left to bite later:

- **`argent run` now reports arguments it ignored.** This is what stops
`--flag 0` and `--flag yes` being the same silent inversion, in a syntax
users are now being taught works. stderr, so `--json` stays parseable.
- **`--no-flag true` / `--no-flag false` now name the positive form**
instead of quietly meaning false. One is a contradiction, the other a
double negative.
- **`--flag=True` used to throw** while `--flag True` would now work —
the inline coercion was case-sensitive and did not trim. Both paths now
share one helper, so a single word cannot get two answers one function
apart.

The **flow** and **lens** parsers take switches that accept no value at
all, and would have silently ignored the syntax their users just learned
elsewhere. They now refuse it — in the flow parser that also stops
`false` being swallowed as the flow name, since its first bare token is
the flow to run.

`--help` gains **one legend line** for tools with a boolean field,
rather than widening the flag column on every field of every tool:

```
  Booleans: --flag or --flag true sets true; --flag false, --flag=false, or --no-flag sets false.
```

It deliberately does not begin with `--`.
`scripts/e2e-full/lib/discover-tools.sh` reads any such line in the
`Flags:` section as a flag row and takes the first `--token` as its
name, so a legend starting with a flag would inject a phantom flag into
every tool model it builds. **There is a test pinning that shape**,
verified live: running the harness's awk over the new help output yields
exactly the real flags and no legend row.

## Verified live

```
--captureCurrent false  -> diff runs (was: "Provide either currentPath or captureCurrent, not both.")
--captureCurrent=false  -> unchanged
--captureCurrent 0      -> "Note: ignoring unused argument(s): 0. Pass values as --flag <value> or --flag=<value>."
screenshot --help       -> legend present; harness awk parses only the real flags
```

25 new tests; 8 confirmed to fail before and pass after. The rest pin
what deliberately did *not* change: bare flag still true, a following
flag or non-boolean word left alone, `1`/`0` still positional in the
space form, `--` escape, string and unknown fields untouched,
`--no-flag` semantics, boolean arrays, last-write-wins. The parser had
**zero** boolean coverage before — which is why this survived. Full CLI
suite 298 passing; lint, prettier and `typecheck:tests` clean.

## Scope and neighbours

Only `flag-parser.ts` (the schema-driven parser for `argent run`)
changes semantics. The other CLI parsers take fixed, default-off
switches where `false` has no coherent meaning; three of them already
reject a stray token outright, and the two that did not now do.
`flow.ts`'s deliberate rejection of `--flag=value` is untouched.

**#648** also edits `flag-parser.ts` (rewrites `renderFlagName`, inserts
helpers at the same anchor). Behaviourally compatible — it keeps boolean
→ bare `--name` — so this is a textual conflict only, and either order
works.

## Found while here, not fixed

`scripts/e2e-full/lib/discover-tools.sh:60-64` detects flag kinds with
`\bboolean\b` in awk, where `\b` is **backspace**, not a word boundary —
so it never matches and every non-enum flag is typed `unknown`. That
makes the `bad-type` validation case downstream unreachable.
Pre-existing and unrelated to this fix; filing separately.

---

> **Stacked on #648** (`filip/run-cli-validation-messages`). Both
rewrite the same region of `packages/argent-cli/src/flag-parser.ts` —
#648 factors out `flagNameFor`/`isJsonField` and rewrites
`renderFlagName`, this one adds `booleanWord` beside them and a boolean
legend to `formatSchemaUsage`. Merging them independently conflicts;
this branch is rebased on #648, so review only the top commit. GitHub
retargets it to `main` when #648 merges.

---------

Co-authored-by: Filip131311 <f.kaminski2000@gmail.com>

2026-08-03 12:18:11 +02:00

Ignacy Łątka

c5bab2a83d

fix(cli): let a tool's own --args field win over the whole-payload flag (#458 )

## Bug

`argent run flow-add-step` declares its own `args` field — a JSON string
holding the recorded step's tool arguments. But the CLI flag parser
hardcoded `--args` as a whole-payload escape hatch, so `--args` shadowed
the tool's own `args` field. The documented per-flag form

```
argent run flow-add-step --command gesture-tap --args '{"udid":"...","x":0.5,"y":0.5}'
```

silently dropped the step arguments: the `--args` JSON was consumed as
the whole payload and the tool's `args` field stayed `undefined`,
producing a misleading downstream validation error. Only the nested
full-payload form worked.

## Fix

Make `--args` schema-aware in `parseFlags()`:

- When the tool's schema declares a property named `args`
(`schema.properties.args`), `--args <value>` is routed through the
normal per-field handling and coerced by its declared type (a `string`
for flow-add-step, so the raw JSON string passes straight into the
`args` field). Both `--args value` and `--args=value` forms are
supported.
- When the schema does NOT declare `args`, the existing whole-payload
behavior is preserved exactly, including the `--args -` stdin sentinel.

`printToolHelp()` no longer prints the global whole-payload `--args
<json>` / `--args -` lines for tools that declare their own `args`
field, so the help stops contradicting the per-field `--args` shown in
the schema flags.

The change is general: any tool with an `args` field benefits, with no
special-casing by tool name.

## Verification

- `npm run build` (root tsc) — clean.
- `npm test -w @argent/cli` — 172 passed, including 6 new `parseFlags`
cases covering schema-with-`args` (space-separated + inline forms, plus
sibling fields) and schema-without-`args` (whole-payload preserved,
`--args -` sentinel, no-schema fallback).
- Prettier + ESLint (`--max-warnings 0`) clean on the touched files.

Full CLI end-to-end (running `flow-add-step` against a live tool-server
with an active flow recording) was not exercised here; the added unit
tests prove the parser behavior.

Fixes #452
---

### CLI end-to-end verification

Ran the documented per-flag form against a live tool-server with an
active flow recording:

- **Before (global argent 0.13.0):** `argent run flow-add-step --command
gesture-tap --args '{"udid":"...","x":0.5,"y":0.35}'` → `Invalid params
for tool "gesture-tap": udid expected string, received undefined ...`
(args dropped).
- **After (this branch):** same command → `{ "message": "Step added to
\"colltest\" flow", "toolResult": { "tapped": true } }`, and the
recorded YAML contains the step **with** its args (`udid`/`x`/`y`). The
per-flag form now works as documented.

2026-07-07 13:37:09 +02:00

Ignacy Łątka

a7722df8a2

fix(cli): reject empty numeric flags and array/-json interleave crash (#432 )

Fixes two related defects in `packages/argent-cli/src/flag-parser.ts`,
the argv-to-JSON flag parser used by `argent run`. The package
previously had no dedicated test file for it; this PR adds one covering
both fixes.

## Defect A: empty/whitespace numeric values coerced to 0

`coerceScalar` validates number/integer fields with `Number.isNaN` /
`Number.isInteger`, but `Number("")` and `Number(" ")` both evaluate to
`0`, so those guards pass. An empty or whitespace-only value was
therefore silently accepted as `0` instead of being rejected.

Reachable via `--x=` (empty inline), `--x ""`, and `--x "   "`.

- Observed: `parseFlags(["--x="], { type: "object", properties: { x: {
type: "number" } } })` returned `{ x: 0 }`.
- Expected: rejected, the same way a non-numeric string like `"abc"`
already is.

Fix: reject an empty or whitespace-only value up front for
number/integer types, throwing the same `FlagParseException` the
function already throws for `"abc"` (`--<flag> expected a
number/integer, got "<raw>"`). Valid numbers are unaffected.

## Defect B: array + `-json` interleave throws a raw TypeError

`seenArrayFields` is only populated by the array branch, but the
`--field-json` escape hatch writes `args[fieldName]` directly and can
set it to a non-array. If a field was already seen as an array, a later
array-form occurrence ran `(args[flag] as unknown[]).push(...)` on a
non-array value and threw a raw `TypeError`. `run.ts` only catches
`FlagParseException` and re-throws anything else, so the user got an
uncaught stack trace instead of a clean error.

Repro:

```
parseFlags(
  ["--tags", "a", "--tags-json", '"b"', "--tags", "c"],
  { type: "object", properties: { tags: { type: "array", items: { type: "string" } } } }
)
```

- Observed: throws `TypeError: args[flag].push is not a function`
(uncaught, surfaces as a stack trace through `run.ts`).
- Expected: a clean validation error.

Fix: in the array branch, when `args[flag]` is not actually an array
(because `--field-json` replaced it), throw a `FlagParseException`
explaining the `--field` and `--field-json` forms cannot be mixed for
the same field, never a raw `TypeError`.

## Tests

Adds `packages/argent-cli/test/flag-parser.test.ts`. The new tests fail
before the fix (4 failures) and pass after; the full `argent-cli` suite
stays green (90 passed). Behavior for valid inputs is unchanged.

2026-07-03 16:19:22 +02:00

3 Commits