Delete the narration, restatement and prose-rationale comments the branch
introduced, keeping only what the code cannot carry: the prototype and
never-echo-a-value guards, Zod's own reporting quirks, the `error` field's
contract with a pre-`issues` CLI, the ordering and snapshot invariants, and
the deliberate choices (root-anchored cross-field rules, the unhinted
directive list, the Messages API's refusal of a top-level combinator).
No executable code changes.
This PR added `flow_name` as an alias for `name` on flow-execute and
flow-read-prerequisite, on the grounds that it is the spelling callers reach for
when the tool is called `flow-execute`. But the reason the wrong spelling cost
whole turns was never the spelling — it was that Zod's raw issue array named the
parameter the tool WANTED and never the one the caller SENT. That is fixed, in
this same PR: the rejection now reads
Pass exactly one flow source: name or flow_path. flow-execute needs the flow's
name in `name` — it resolves <project_root>/.argent/flows/<name>.yaml.
You sent: `flow_name`, `project_root`.
which names the parameter, says where it resolves, and echoes the caller's own
keys. One turn, no alias. A second accepted spelling on top of that buys nothing
and costs a duplicated enumeration in both tools' schemas, their file-input
specs, flow-add-step's nested-call validation, and every error string that had to
spell out both.
So: one parameter, one spelling. `unwrapWhenSet` goes back to a single key
(registry's file-inputs.ts is now identical to main), `flow_file` back to one
spec, and `resolveFlowName` — which existed only to fold the two together — is
gone; `resolveFlowSource` reads `params.name` as it did before.
One behaviour returns to main's: an empty `name` answers 500 rather than 400,
since it now reaches `assertSafeFlowName` (FLOW_NAME_INVALID, error_kind
"validation") instead of a leaf InvalidToolInputError. Mapping every validation
FailureError to 400 is a boundary-wide decision, not an alias one.
Verified end to end against a tool-server built from this branch: a `flow_name`
call answers 400 with the message above over HTTP and through the MCP stdio
adapter; `name`, `flow_path`, a client-derived `flow_file`, and a remote upload
by name all still run; `name` + an unresolvable `flow_path` still answers the
schema's 400 rather than a 422 about the file; and the recorder still captures a
nested flow-execute as `run: <name>.yaml`.
The branch added ~640 comment lines across these files, most of them restating
the test title or the assertion below. What is left records a fixture
invariant, a timing constraint or a trap that made an earlier version of the
test lie.
This branch added 2768 comment lines against 4814 lines of code — more
than a third of the diff, and several blocks ran to essays. The worst
was flow-add-step.ts at 723, where a 70-line docblock describing
`runnerSideReadClause` sat orphaned two doc-comments above
`retargetRemedy`, documenting neither.
Rewritten to the ASD-STE100 rules the request names: short sentences,
one topic each, active voice, and nothing that restates what the code
already says. 727 comment lines removed, no code touched.
Four things went, and they are the four that were not carrying facts:
- Restatement. `platformOf`'s four lines explained why a `typeof` guard
needs no try/catch; `indeterminateReasonCaveat`, `SPELLING_CLAUSE` and
`UNREADABLE_WAIT_WARNING` each paraphrased the very string printed
beneath them.
- Changelog narration in the tests. "This test USED to serve an
`alpha: 0` view", "left the whole suite green", "Nothing pinned it".
What each was justifying stays, as a constraint on the fixture rather
than a history of it — whether the AX tree reports a transparent view
is a device question the suite cannot settle, so the fixture may not
rest on it.
- Repetition across blocks. The MCP retry-storm story appeared in full
twice in one file, at `PROBE_MAX_TREE_READ_MS` and at `longRunning`;
it now sits at the flag it explains.
- Enumerations that had outgrown their point — the Chromium divergence
arm spent 25 lines on two cases the code decides in one condition
check.
What stayed is the rationale nothing else records: the two-read probe
budget and why a one-read ceiling loses the warning, the three-tier
`timeoutCause` classification, the two anchor checks and the edit that
defeats both, and every per-platform tree story. Those are shorter, not
gone.
The tool `description` strings and the skill markdown are untouched —
they are agent-facing output, and the docs tests assert on them.
Verified: `tsc --build` clean; `typecheck:tests` clean (skills and ui
have no such script, as before); prettier and eslint clean on all 29
files; tool-server 4461 passed / 1 skipped, registry 118, argent-cli
468, argent-tools-client 194.
`InvalidToolInputError` defaults to `TOOL_INPUT_INVALID`, so a tool that
rejects its arguments from inside `execute` passes the gate above with
args that parse perfectly well. `resolveFlowName` is exactly that throw,
reached by a flow `- tool: flow-execute` step whose `name` is empty — and
with the guard gone the step answers "Cannot read properties of undefined
(reading 'issues')" in place of the message the tool wrote.
Say what the branch is for, and cover it, so the next reader is not told
it cannot fire while the suite agrees with them.
The interaction-messages suite builds its expectations from `definitionsById`
now, so the individual tool factories it used to instantiate are unreferenced.
The issue array quoted as flow-execute's old behaviour was never produced by
flow-execute. `name` is `.optional()` on that tool at the PR base and on
origin/main alike, so a `flow_name` call tripped the exactly-one-source
superRefine, not an `invalid_type` on `name`:
flow-execute [{"code":"custom","path":["flow_path"],
"message":"Pass exactly one flow source: …"}]
flow-start-recording [{"expected":"string","code":"invalid_type",
"path":["name"], …}]
The quoted JSON is the second one — `flow-start-recording`, whose `name`
genuinely is required, and which is one of the flow tools the alias was NOT
added to. The alias still earns its place: flow-execute's real answer named
`flow_path`, the one source field the caller had no reason to send, and named
`flow_name` nowhere.
Corrected in all three places the attribution was repeated: the schema
comment, `describeParamIssues`' docblock, and the test file's header.
The registry renders "You sent:" from the args it was handed, and on the
flow runner's `tool:` path those are `bindDeviceArgs`' output — every device
key stripped off the recorded step and the resolved one re-injected. So the
list named `udid`, a key the flow author cannot have written: the recorder
strips it on the way in precisely so flows stay portable, and a hand-written
one is discarded and replaced too.
steps: [- tool: gesture-tap / args: {xx: 0.5, y: 0.5}]
before `x` is required (number) and was not provided. You sent: `xx`, `y`, `udid`.
after `x` is required (number) and was not provided. You sent: `xx`, `y`.
The same sentence run-sequence already produced, through the same
`describeNestedParamError` helper, so the two dispatchers no longer disagree
about the same step args. It is the class rather than the one key — a
`debugger-evaluate` step also listed `device_id`.
The `custom` arm returned the author's message bare, dropping `issue.path` —
the one issue kind that opted out of the qualification every other arm does.
That is right for a cross-field rule, which is about the payload as a whole,
but not for a `.refine()` BOUND to a field. `selector.text`'s
visible-character rule reaches it at `["selector","text"]`, and
`await-ui-element` also declares `expectedText`, so the prose read as a
complaint about an unidentified parameter — "You sent:" carries only
top-level keys, so the offending sub-key was named nowhere. The raw JSON this
replaced carried the path.
Qualifying every non-empty path alone would have mis-attributed the two flow
tools' source-count rules, which anchored on `flow_path` for a message that
is mostly about `name`. They are whole-payload rules, so they move to
`path: []` — which the CLI already renders as the sentence alone.
POST /tools/await-ui-element {"udid":"X","condition":"visible",
"selector":{"text":"<zero-width>"}}
before text must contain at least one visible character … You sent: `udid`, `condition`, `selector`.
after `selector.text`: text must contain at least one visible character …
argent run flow-execute --project_root /tmp/f1e2e
before Error: --flow_path Pass exactly one flow source: name or flow_path. …
after Error: Pass exactly one flow source: name or flow_path. …
`--json` still carries the issue list, and the exit code is still 2.
- The two tests that said they covered resolveFlowName throwing an
InvalidToolInputError never reached it: the schema's exactly-one-source rule
fires first and `execute` is never entered, and the rule was deliberately
given the same wording, so both passed over whatever resolveFlowName did.
Swapping the class for a plain Error left them green. An EMPTY name is the one
input that reaches the throw — covered now, with a spy proving execute ran and
the class asserted on the cause the HTTP boundary maps.
- flow-read-prerequisite's alias was only ever exercised through a direct
execute(), which bypasses zod. Covered through the registry, and the oneOf
both tools advertise is pinned.
- Added the uncovered cases: an UPLOADED flow named through the alias, a
name-only call now that a non-interpolating ${flow_name} spec sits ahead of
its spec, and activeFlowState's unreadable-file branch.
flow-execute's no-source rejection now spells out that `flow_name` is accepted;
its pre-flight twin kept the bare "Pass exactly one flow source". The skill has
agents call the prerequisite FIRST, so the tool they hit first was the one
staying quiet about the spelling that would have worked. A dual-source call
stays terse — naming two sources is not a spelling problem.
createRunFlowTool's interaction messages read `params.name || params.flow_name`,
which the alias path needs, but a call with NEITHER spelling — the name-less
call resolveFlowName rejects with a 400 — still resolved to `undefined`. The
interaction message fires inside invokeTool BEFORE execute(), so that doomed
call logged "Running flow undefined" to the event log, telemetry and MCP
progress before the 400. Add a final "(no flow name)" fallback via a shared
flowLabel().
Also pin three behaviors this stack left asserted only by comment:
- flow-add-step refuses a nested recorder tool BEFORE JSON.parse(args), so a
malformed `args` payload cannot pre-empt the guidance with a SyntaxError.
- the production flow_file file-input specs order `${flow_name}` before
`${name}`, so `name` wins the client's last-write-wins merge (matching
resolveFlowName); a reorder would upload a different flow than the run
reports on a remote host.
- describeParamIssues never leaks a value on the invalid_value (enum) and
unrecognized_keys branches, not only invalid_type.
Review follow-ups on this PR's own additions, found while verifying it:
- resolveFlowName threw a plain Error, so a flow-execute / flow-read-prerequisite
call with neither `name` nor `flow_name` returned HTTP 500 and logged
ARGENT_UNCLASSIFIED_FAILURE: a client-input error dressed as an internal fault.
Making `name` optional (for the alias) moved this check out of zod, so it now
throws InvalidToolInputError, which the HTTP boundary maps to 400 and telemetry
classifies as `validation`, matching its sibling validations (assertSafeFlowName,
resolveFlowFilePath) and the pre-alias zod rejection it replaced.
- describeParamIssues rendered an OMITTED required enum/literal as
"Invalid option: expected one of ..." (Zod emits invalid_value, not
invalid_type, for a missing enum), reading as if a bad value had been sent.
It now decides "missing" from the input, the value at the issue's path being
undefined, instead of a locale-fragile match on Zod's English message: an
omitted enum reads "is required", while a present-but-wrong value (a bad
option, a null, a wrong type) still reads with its per-issue wording.
- The tap/launch/run directive hints promised the recorder "rewrites it into the
`tap:` step for you", but every rewrite also gates on `delayMs === undefined`
(a replay delay has no directive form). The rewrite hints now state that
opt-out, so an author who sets delayMs is not promised a step the recorder
then keeps as a raw `tool:` step.
- `name` going optional dropped the CLI `(required)` marker for the flow name;
its schema description now states it is required (via `name` or the `flow_name`
alias), which `tools describe` / `run --help` print.
Tests: the HTTP 400 status and prose body for a name-less flow-execute and for a
misspelled required key; the name-less call's `validation` classification;
omitted-enum-is-missing vs present-but-wrong-enum wording, and null-is-not-missing;
the delayMs opt-out in the tap rewrite hint.
Two review fixes to the param-error changes:
- flow-add-step looked up its directive and recorder-tool tables with a bare
index on caller-controlled `command`, so a value equal to an inherited member
("__proto__", "constructor", "toString", ...) read truthy off the prototype
chain and refused the call with a garbage message ("[object Object] Nothing
was executed...") instead of the plain "not found". Look them up with
Object.hasOwn so those names fall through to the ordinary not-found path.
- flow-execute gained the `flow_name` alias but the sibling flow-read-prerequisite
(which reads the same flow) still required `name`, so an agent that learned the
alias hit a bare "name required". Extend the alias there too, reusing
resolveFlowName (now exported and tool-name-parameterized) and the same
two-spec file-input merge.
Also correct activeFlowState's doc (host mode does refresh the in-memory
snapshot) and add tests: a prototype-key command falls through to not-found,
name wins over flow_name at the tool level, and flow-read-prerequisite honors
the alias and names itself when neither spelling is present.
Follow-ups on this PR's own additions, found in review:
- The `flow_name` alias uploaded nothing over a REMOTE tool-server. The
file-input template keyed on `${name}`, so an alias-only call interpolated
no path, sent no file, and the server then read a client-side path that is
not on its host — a confusing ENOENT, the exact failure the alias exists to
avoid. flow-execute now declares flow_file twice (`${flow_name}` then
`${name}`); the client resolves whichever was sent, and `name` stays LAST so
it wins the last-write-wins merge, matching `resolveFlowName`'s precedence.
- The interaction messages still read `params.name`, which the alias leaves
unset, so a `flow_name`-only run logged "Running flow undefined". They now
resolve `name || flow_name`.
- `resolveFlowName`'s error called `flow_file` "discarded before the tool
runs"; it is a recognized path param, not a discarded spelling. Reworded.
- The `launch` and `run` directive hints promised an unconditional rewrite,
but a `restart-app` with an extra arg (or a `run` target that is not a
resolvable sibling) is kept as a raw `tool:` step. The hints now state that
condition, so the author is not sent looking for a directive that was never
written.
- A recorder tool passed as `command` with a malformed `args` payload hit
`JSON.parse` before the refusal, so a JSON error pre-empted the guidance.
The refusal now runs before the parse (it needs only `command`).
- `describeParamIssues` signals a truncated "You sent:" list with an ellipsis
instead of silently dropping keys (the list is the only clue to a misspelling
when the schema strips unknowns), and no longer renders a bare ".".
- Tests: rename the mislabeled flow-execute enum-value test to what it checks;
add direct `describeParamIssues` coverage for the paths flow-execute's flat
schema cannot reach (nested-missing, nested unrecognized key, values never
leaked, the 24-key cap) and a two-spec file-input test for the alias upload.
An invalid call returned Zod's raw issue array —
`[{"expected":"string","code":"invalid_type","path":["name"]}]`. That names the
parameter the tool WANTED and never the one the caller actually sent, so the
reader cannot see that they wrote `flow_name` where `name` was expected. It cost
whole turns to work out.
`describeParamIssues` renders a schema failure as one sentence per bad
parameter, and closes with the caller's own top-level keys ("You sent: …") —
which is what makes the mistake self-evident. Key names only, never values: the
string reaches logs, telemetry and the agent transcript, and a params object can
carry a secret. Used by both entry points, the registry's `invokeTool` and the
HTTP boundary.
`flow-execute` additionally accepts `flow_name` as an alias for `name`, since
that is the spelling callers reach for. The name is resolved in code rather than
by the schema, because a Zod `required` failure would again name only the field
it wanted.
And a flow DIRECTIVE passed as `command` — `echo`, `wait`, `tap`, `long-press` —
now names the tool that records it instead of "Tool not found", saying whether
the recorder rewrites that tool into the directive or stores it raw for the
polish pass to convert.
Passing a RECORDER tool as `command` is refused outright. Nesting one appends
twice: the inner tool writes its own directive, and flow-add-step additionally
records a raw `tool:` step that fails on every replay, when no recording is
open. It reported success either way, so nothing signalled the corruption.
Hubert Gancarczyk