From 4dd9c19d27f5dd52fd185d5c295a453faf5d8c84 Mon Sep 17 00:00:00 2001 From: Filip131311 Date: Sat, 1 Aug 2026 18:17:48 +0200 Subject: [PATCH] fix(flows): let a nested run's verdict reach the run that contains it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A flow step that runs another orchestrator reported `pass` whatever the nested run actually did. The generic `tool` step treats any non-throwing result as a pass, and both `flow-execute` and `run-sequence` report failure in their result rather than by throwing. So the same flow reported `ok: false, failed: 1` run directly and `ok: true, passed: 1` run nested — with the failing sub-report sitting inside the very result object being called a pass. Two shapes were lost for `flow-execute`: a composed flow that ran and failed, and one that ran nothing at all because its execution prerequisite was never acknowledged. They map to the two statuses the runner already has — a flow that ran and failed its assertions is a failure, a step that was never runnable as written is an error, the class the runner already uses for an unreadable fragment or a cyclic reference. Both hard-stop, because in this runner every fail and every error hard-stops. A cancelled nested run is a skip, matching the rule the runner applies to its own steps. `run-sequence` had the same hole and no verdict field at all: every one of its failure paths pushes an error entry, breaks the loop and returns normally, so a sequence that stopped on its first of eight steps looked like an ordinary result. Fixed here rather than left as a known identical bug on the same line. These are two named, tool-scoped branches, not a general "a result with ok: false fails the step" rule. There is no such contract here to generalise — the only other soft-verdict tool spells it `success`, run-sequence spells it neither way, and this step dispatches tools whose results are typed `unknown`, several carrying app-derived payloads. A blanket rule would bind all of them, and everything added later, to a key name. There is a test pinning that. The whole sub-report still rides on the step's `result`, so nothing that was visible before is lost; the reason string carries the sub-flow's own first failure, because the CLI renders only the reason. Fixes #606 --- .../src/tools/flows/flow-nested-outcome.ts | 155 ++++++++++++ .../tool-server/src/tools/flows/flow-run.ts | 16 ++ .../test/flows/flow-nested-outcome.test.ts | 236 ++++++++++++++++++ 3 files changed, 407 insertions(+) create mode 100644 packages/tool-server/src/tools/flows/flow-nested-outcome.ts create mode 100644 packages/tool-server/test/flows/flow-nested-outcome.test.ts diff --git a/packages/tool-server/src/tools/flows/flow-nested-outcome.ts b/packages/tool-server/src/tools/flows/flow-nested-outcome.ts new file mode 100644 index 000000000..67791dabe --- /dev/null +++ b/packages/tool-server/src/tools/flows/flow-nested-outcome.ts @@ -0,0 +1,155 @@ +import type { StepStatus } from "./flow-run"; + +/** + * Reading the verdict of a nested orchestrator step. + * + * Two registered tools run other tools and report what happened in their + * result rather than by throwing: `flow-execute` and `run-sequence`. The flow + * runner dispatches both through the generic `tool` step, which — apart from one + * `await-ui-element` special case — treats any non-throwing result as a pass. So + * a composed flow that failed every step, or a sequence that stopped on its + * first tool, was reported as a green pass by the run that contained it (#606). + * + * These are deliberately two named, tool-scoped branches rather than a general + * "a result with `ok: false` fails the step" rule. There is no such contract in + * this codebase to generalise: the only other soft-verdict tool spells it + * `success` (`await-ui-element`), `run-sequence` spells it neither way, and the + * generic `tool` step dispatches tools whose results are typed `unknown` or + * `Record` — several of them carrying app-derived payloads. A + * blanket rule would silently bind all of those, and every tool added later, to + * "a key called `ok` decides my flow's verdict". `isUnmetUiWaitResult` set the + * precedent for naming the tool instead. + * + * Everything here narrows defensively: results cross the registry boundary as + * `unknown`, and a shape this does not recognise must fall through to the + * runner's existing behaviour rather than guess at a verdict. + */ + +export const FLOW_EXECUTE_TOOL_ID = "flow-execute"; +export const RUN_SEQUENCE_TOOL_ID = "run-sequence"; + +export interface NestedOutcome { + status: StepStatus; + reason: string; +} + +function isRecord(value: unknown): value is Record { + return typeof value === "object" && value !== null; +} + +/** The first step in a nested flow report that did not pass, rendered for a human. */ +function firstFailingStep(steps: unknown): string | undefined { + if (!Array.isArray(steps)) return undefined; + for (const entry of steps) { + if (!isRecord(entry)) continue; + if (entry.status !== "fail" && entry.status !== "error") continue; + // Prefer the tool id; fall back to the step kind. Both are checked for being + // strings rather than coerced — this report crossed the registry boundary as + // `unknown`, and an object here would render as "[object Object]". + const what = + typeof entry.tool === "string" + ? entry.tool + : typeof entry.kind === "string" + ? entry.kind + : "step"; + const why = typeof entry.reason === "string" ? entry.reason : "no reason given"; + return `${what}: ${why}`; + } + return undefined; +} + +function count(value: unknown): number { + return typeof value === "number" ? value : 0; +} + +/** A nested `flow-execute` result: a run report, or a prerequisite notice. */ +function flowExecuteOutcome(result: Record): NestedOutcome | undefined { + const flow = typeof result.flow === "string" ? result.flow : "the composed flow"; + + // A notice means the sub-flow ran NOTHING. Reported as an error rather than a + // failure: nothing was asserted, so "the app misbehaved" would be untrue — + // the step was never runnable as written, which is the class the runner + // already labels error (an unreadable fragment, a cyclic reference). + if (!("steps" in result) && typeof result.notice === "string") { + const prerequisite = + typeof result.executionPrerequisite === "string" && result.executionPrerequisite + ? `: ${result.executionPrerequisite}` + : ""; + return { + status: "error", + reason: + `flow "${flow}" did not run — its execution prerequisite was not acknowledged${prerequisite}. ` + + `Add prerequisiteAcknowledged: true to the step's args, or compose with run: instead.`, + }; + } + + // A cancelled run is a skip, never a failure — the same rule the runner + // applies to its own steps when the signal fires mid-flight. + if (result.aborted === true) { + return { status: "skip", reason: `flow "${flow}" was aborted` }; + } + + if (result.ok === false) { + const detail = firstFailingStep(result.steps); + return { + status: "fail", + reason: + `flow "${flow}" failed: ${count(result.passed)} passed, ${count(result.failed)} failed, ` + + `${count(result.errored)} errored${detail ? ` (${detail})` : ""}`, + }; + } + + return undefined; +} + +/** + * A nested `run-sequence` result. + * + * `run-sequence` has no verdict field at all. Every one of its failure paths — + * a disallowed tool, an unsupported operation, an unmet `await-ui-element`, a + * tool that threw — pushes an `error` entry, breaks the loop, and returns + * normally. So a sequence that stopped on its first of eight steps returned a + * perfectly ordinary result, and the flow step reported a pass. + */ +function runSequenceOutcome(result: Record): NestedOutcome | undefined { + const steps = result.steps; + if (!Array.isArray(steps)) return undefined; + + const failed = steps.find((s) => isRecord(s) && typeof s.error === "string"); + if (failed && isRecord(failed)) { + const tool = typeof failed.tool === "string" ? failed.tool : "step"; + return { + status: "fail", + reason: + `run-sequence stopped at ${tool} after ${count(result.completed)} of ` + + `${count(result.total)} steps: ${String(failed.error)}`, + }; + } + + // No error entry but the sequence stopped short: its only other exit is the + // abort check. Cancellation is a skip, matching the flow-execute branch. + const total = count(result.total); + if (total > 0 && steps.length < total) { + return { + status: "skip", + reason: `run-sequence was aborted after ${count(result.completed)} of ${total} steps`, + }; + } + + return undefined; +} + +/** + * Classify a nested orchestrator's result, or `undefined` when this is not one + * of them, when it succeeded, or when the shape is not recognised — in every + * one of which cases the runner's existing handling is correct. + */ +export function nestedOrchestratorOutcome( + tool: string, + result: unknown +): NestedOutcome | undefined { + if (!isRecord(result)) return undefined; + if (tool === FLOW_EXECUTE_TOOL_ID) return flowExecuteOutcome(result); + if (tool === RUN_SEQUENCE_TOOL_ID) return runSequenceOutcome(result); + return undefined; +} diff --git a/packages/tool-server/src/tools/flows/flow-run.ts b/packages/tool-server/src/tools/flows/flow-run.ts index 4ce009fcb..1758d4fe1 100644 --- a/packages/tool-server/src/tools/flows/flow-run.ts +++ b/packages/tool-server/src/tools/flows/flow-run.ts @@ -39,6 +39,7 @@ import { stepRequiresDevice, type FlowPlatform, } from "./flow-device"; +import { nestedOrchestratorOutcome } from "./flow-nested-outcome"; import { runDirective, invokeOnDevice, @@ -1190,6 +1191,21 @@ async function execLeafStep( reason: `await-ui-element condition not met${note ? `: ${note}` : ""}`, }; } + // `flow-execute` and `run-sequence` run other tools and report what + // happened in their result instead of throwing, so without this a + // composition that failed everything counted as a passing step (#606). + const nested = nestedOrchestratorOutcome(step.name, result); + if (nested) { + return { + ...base, + status: nested.status, + tool: step.name, + reason: nested.reason, + result, + outputHint, + args, + }; + } return { ...base, status: "pass", tool: step.name, result, outputHint, args }; } catch (err) { return { ...base, status: "error", tool: step.name, reason: errMsg(err) }; diff --git a/packages/tool-server/test/flows/flow-nested-outcome.test.ts b/packages/tool-server/test/flows/flow-nested-outcome.test.ts new file mode 100644 index 000000000..0f35041b5 --- /dev/null +++ b/packages/tool-server/test/flows/flow-nested-outcome.test.ts @@ -0,0 +1,236 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import * as fs from "node:fs/promises"; +import * as os from "node:os"; +import * as path from "node:path"; +import type { Registry } from "@argent/registry"; +import { createRunFlowTool, type FlowRunResult } from "../../src/tools/flows/flow-run"; +import { nestedOrchestratorOutcome } from "../../src/tools/flows/flow-nested-outcome"; + +/** + * Issue #606: a step that runs a nested orchestrator reported `pass` whatever + * the nested run actually did. The generic `tool` step treats any non-throwing + * result as a pass, and both `flow-execute` and `run-sequence` report failure in + * their result rather than by throwing. + * + * Measured before the fix: the same flow reported `ok=false, failed=1` when run + * directly and `ok=true, passed=1` when nested — with the failing sub-report + * sitting inside the result object being called a pass. + */ + +const PROJECT_ROOT = path.join(os.tmpdir(), `flow-nested-tests-${process.pid}`); + +function makeRegistry(invoke: (id: string, args: unknown) => Promise) { + return { + invokeTool: vi.fn(invoke), + getTool: vi.fn(() => undefined), + } as unknown as Registry; +} + +async function writeFlow(yaml: string): Promise { + const flowsDir = path.join(PROJECT_ROOT, ".argent", "flows"); + const file = path.join(flowsDir, "outer.yaml"); + await fs.mkdir(flowsDir, { recursive: true }); + await fs.writeFile(file, yaml, "utf8"); + return file; +} + +afterEach(async () => { + await fs.rm(PROJECT_ROOT, { recursive: true, force: true }); +}); + +function asRun(r: FlowRunResult | { notice: string }): FlowRunResult { + if (!("steps" in r)) throw new Error(`expected a FlowRunResult, got a notice: ${r.notice}`); + return r; +} + +/** A nested orchestrator step, followed by a step that must not run if it fails. */ +const OUTER = (tool: string) => `executionPrerequisite: "" +steps: + - tool: ${tool} + args: + name: sub + - tool: gesture-tap + args: + udid: X + x: 0.5 + y: 0.5 +`; + +async function run(tool: string, nestedResult: unknown) { + const flowFile = await writeFlow(OUTER(tool)); + const registry = makeRegistry(async (id) => (id === tool ? nestedResult : { ok: true })); + const result = asRun( + await createRunFlowTool(registry).execute( + {}, + { name: "outer", project_root: PROJECT_ROOT, flow_file: flowFile, device: "DEV" } + ) + ); + return { result, registry }; +} + +/** A sub-flow that ran and failed. */ +const FAILED_SUBFLOW = { + flow: "sub", + device: "DEV", + executionPrerequisite: "", + ok: false, + passed: 0, + failed: 1, + skipped: 0, + errored: 0, + steps: [ + { + index: 0, + kind: "await", + status: "fail", + tool: "await-ui-element", + reason: "no element matched the selector before timeout", + }, + ], +}; + +describe("a nested flow-execute reports its own verdict", () => { + it("fails the step when the composed flow failed", async () => { + const { result, registry } = await run("flow-execute", FAILED_SUBFLOW); + + expect(result.steps[0].status).toBe("fail"); + expect(result.steps[0].reason).toMatch(/flow "sub" failed/); + expect(result.steps[0].reason).toMatch(/1 failed/); + // The sub-flow's own reason is surfaced, so the CLI — which renders only + // `reason` — says what actually went wrong rather than just "it failed". + expect(result.steps[0].reason).toMatch(/no element matched/); + // The whole sub-report still rides along for clients that render results. + expect(result.steps[0].result).toEqual(FAILED_SUBFLOW); + + // …and the run hard-stops, exactly as an inline `run:` composition would. + expect(result.steps[1].status).toBe("skip"); + expect(registry.invokeTool).not.toHaveBeenCalledWith("gesture-tap", expect.anything()); + expect(result.ok).toBe(false); + expect(result.failed).toBe(1); + }); + + it("errors the step when the composed flow ran nothing at all", async () => { + // An unmet executionPrerequisite returns a notice and zero steps. Nothing + // was asserted, so this is not a failure of the app — it is a step that was + // never runnable as written. + const { result } = await run("flow-execute", { + flow: "sub", + notice: "This flow has an execution prerequisite that must be fulfilled before it can run.", + executionPrerequisite: "Settings is open on the root page", + }); + + expect(result.steps[0].status).toBe("error"); + expect(result.steps[0].reason).toMatch(/did not run/); + expect(result.steps[0].reason).toMatch(/Settings is open on the root page/); + // The remedy has to be in the reason: it is all the CLI shows. + expect(result.steps[0].reason).toMatch(/prerequisiteAcknowledged/); + expect(result.errored).toBe(1); + expect(result.ok).toBe(false); + }); + + it("treats a cancelled nested run as a skip, not a failure", async () => { + const { result } = await run("flow-execute", { ...FAILED_SUBFLOW, aborted: true }); + + expect(result.steps[0].status).toBe("skip"); + expect(result.steps[0].reason).toMatch(/aborted/); + expect(result.failed).toBe(0); + }); + + it("still passes a composed flow that succeeded", async () => { + const passing = { ...FAILED_SUBFLOW, ok: true, passed: 1, failed: 0, steps: [] }; + const { result, registry } = await run("flow-execute", passing); + + expect(result.steps[0].status).toBe("pass"); + expect(result.steps[0].result).toEqual(passing); + expect(result.steps[1].status).toBe("pass"); + expect(registry.invokeTool).toHaveBeenCalledWith("gesture-tap", expect.anything()); + expect(result.ok).toBe(true); + }); +}); + +describe("a nested run-sequence reports its own verdict", () => { + // run-sequence has no verdict field at all: every failure path pushes an + // `error` entry, breaks the loop and returns normally, so a sequence that + // stopped on its first step looked like an ordinary result. + it("fails the step when a step in the sequence failed", async () => { + const { result } = await run("run-sequence", { + completed: 1, + total: 3, + steps: [ + { tool: "gesture-tap", result: { tapped: true } }, + { tool: "keyboard", error: "keyboard failed: device not found" }, + ], + }); + + expect(result.steps[0].status).toBe("fail"); + expect(result.steps[0].reason).toMatch(/run-sequence stopped at keyboard/); + expect(result.steps[0].reason).toMatch(/1 of 3/); + expect(result.steps[0].reason).toMatch(/device not found/); + expect(result.steps[1].status).toBe("skip"); + expect(result.ok).toBe(false); + }); + + it("skips when the sequence was cut short by cancellation", async () => { + // No error entry, but fewer step results than steps requested — the only + // other way run-sequence leaves its loop. + const { result } = await run("run-sequence", { + completed: 1, + total: 4, + steps: [{ tool: "gesture-tap", result: { tapped: true } }], + }); + + expect(result.steps[0].status).toBe("skip"); + expect(result.steps[0].reason).toMatch(/aborted/); + expect(result.failed).toBe(0); + }); + + it("still passes a sequence that ran every step", async () => { + const { result } = await run("run-sequence", { + completed: 2, + total: 2, + steps: [ + { tool: "gesture-tap", result: { tapped: true } }, + { tool: "keyboard", result: { typed: true } }, + ], + }); + + expect(result.steps[0].status).toBe("pass"); + expect(result.ok).toBe(true); + }); +}); + +describe("the check is deliberately scoped to the two orchestrator tools", () => { + // There is no `ok` contract in this codebase to generalise: await-ui-element + // spells it `success`, run-sequence spells it neither way, and the generic + // `tool` step dispatches tools whose results are typed `unknown` — some + // carrying app-derived payloads. A blanket "ok: false fails the step" rule + // would bind all of those, and everything added later, to a key name. + it("leaves an ordinary tool's `ok` field alone", async () => { + const { result } = await run("gesture-tap", { ok: false }); + expect(result.steps[0].status).toBe("pass"); + }); + + it("ignores a result shape it does not recognise", async () => { + for (const shape of [null, "text", 42, {}, { steps: [] }, { ok: "no" }]) { + expect(nestedOrchestratorOutcome("flow-execute", shape)).toBeUndefined(); + } + }); + + it("never throws on a malformed nested report", () => { + expect(() => + nestedOrchestratorOutcome("flow-execute", { ok: false, steps: [null, 7, { status: "fail" }] }) + ).not.toThrow(); + expect(() => nestedOrchestratorOutcome("run-sequence", { steps: "nope" })).not.toThrow(); + }); + + it("says which step failed even when the sub-report is partly malformed", () => { + const out = nestedOrchestratorOutcome("flow-execute", { + ok: false, + steps: [null, { status: "fail", kind: "assert" }], + }); + expect(out?.status).toBe("fail"); + // No tool and no reason on that entry — it still names the kind rather than + // rendering "undefined". + expect(out?.reason).toMatch(/assert: no reason given/); + }); +});