diff --git a/docs/orchestration-lifecycle.md b/docs/orchestration-lifecycle.md index 5491a3ac..1de62d7c 100644 --- a/docs/orchestration-lifecycle.md +++ b/docs/orchestration-lifecycle.md @@ -82,6 +82,13 @@ calls can overshoot the limit by one concurrency wave; the gate stops new calls and retries, not tokens already being generated. Workflow journals and progress files remain the source for replay and inspection. +`phase(title)` takes one string and only labels a milestone; work runs through +`agent()`, `parallel()`, and `pipeline()`. A script that passes `phase()` a second +argument, such as a callback, is rejected before the run starts, with its line +number, and the VM rejects the same misuse when the call cannot be seen +statically. A run that completes without calling any agent returns a warning at +the top of its result instead of silently reporting success. + The foreground workflow tool streams progress snapshots while it runs. The tool row shows running and queued counts, completed/total agents, failures, cache hits, cancellations, token spend, and the current phase. Each accepted agent diff --git a/packages/coding-agent/src/features/workflow/runtime.ts b/packages/coding-agent/src/features/workflow/runtime.ts index 0537c3c5..d419d473 100644 --- a/packages/coding-agent/src/features/workflow/runtime.ts +++ b/packages/coding-agent/src/features/workflow/runtime.ts @@ -1213,9 +1213,19 @@ function errorMessage(error: unknown): string { return error instanceof Error ? error.message : String(error); } +/** Prepended to a completed run that never called agent(); a log-only script is valid but rarely intended. */ +export const NO_AGENTS_WARNING = + "Warning: this workflow completed without calling any agents. phase() only labels milestones; agent(), parallel(), and pipeline() do the work."; + export function workflowToolResult(value: WorkflowRunResult): AgentToolResult { + const text = JSON.stringify(value, null, 2); return { - content: [{ type: "text", text: JSON.stringify(value, null, 2) }], + content: [ + { + type: "text", + text: value.status === "completed" && value.agentCalls === 0 ? `${NO_AGENTS_WARNING}\n${text}` : text, + }, + ], details: value, isError: value.status !== "completed", }; diff --git a/packages/coding-agent/src/features/workflow/script-lint.ts b/packages/coding-agent/src/features/workflow/script-lint.ts new file mode 100644 index 00000000..87b53c74 --- /dev/null +++ b/packages/coding-agent/src/features/workflow/script-lint.ts @@ -0,0 +1,149 @@ +/** + * Pre-run checks on workflow script source. They reject misuse that the VM + * would otherwise accept silently, before a journal or child process exists. + */ + +/** Message shared by the static check and the VM's runtime guard. */ +export const PHASE_USAGE_MESSAGE = + "phase(title) takes one string and only labels a milestone; it runs no callback. Call agent(), parallel(), or pipeline() as separate statements after it."; + +/** Throw when any `phase(...)` call passes more than one argument. */ +export function assertWorkflowScriptUsage(script: string): void { + const line = findMultiArgumentPhaseCall(script); + if (line !== undefined) throw new Error(`Workflow script line ${line}: ${PHASE_USAGE_MESSAGE}`); +} + +/** + * Line (1-based) of the first `phase(...)` call with a top-level comma, or + * undefined. A lexical scan, not a parser: it skips comments, strings, and + * templates, ignores `obj.phase(`, `function phase(`, and method definitions, + * and accepts a trailing comma after a single argument. + */ +export function findMultiArgumentPhaseCall(source: string): number | undefined { + let index = 0; + let previousWord = ""; + let previousChar = ""; + while (index < source.length) { + const skipped = skipNonCode(source, index); + if (skipped !== index) { + index = skipped; + continue; + } + const char = source[index] ?? ""; + if (/[A-Za-z_$]/u.test(char)) { + let end = index + 1; + while (end < source.length && /[\w$]/u.test(source[end] ?? "")) end += 1; + const word = source.slice(index, end); + if (word === "phase" && previousChar !== "." && previousWord !== "function") { + const open = skipWhitespace(source, end); + if (source[open] === "(") { + const call = scanArguments(source, open); + const after = skipWhitespace(source, call.close + 1); + if (call.multiple && source[after] !== "{") return lineAt(source, index); + } + } + previousWord = word; + previousChar = ""; + index = end; + continue; + } + if (!/\s/u.test(char)) { + previousChar = char; + previousWord = ""; + } + index += 1; + } + return undefined; +} + +function scanArguments(source: string, open: number): { close: number; multiple: boolean } { + let depth = 0; + let commaAt = -1; + let index = open; + while (index < source.length) { + const skipped = skipNonCode(source, index); + if (skipped !== index) { + index = skipped; + continue; + } + const char = source[index]; + if (char === "(" || char === "[" || char === "{") depth += 1; + else if (char === ")" || char === "]" || char === "}") { + depth -= 1; + if (depth === 0) { + // `phase("a",)` is one argument; anything after the comma is a second. + const multiple = commaAt >= 0 && source.slice(commaAt + 1, index).trim() !== ""; + return { close: index, multiple }; + } + } else if (char === "," && depth === 1 && commaAt < 0) commaAt = index; + index += 1; + } + return { close: source.length, multiple: false }; +} + +/** Index after a comment, string, or template starting at `index`; `index` itself otherwise. */ +function skipNonCode(source: string, index: number): number { + const char = source[index]; + const next = source[index + 1]; + if (char === "/" && next === "/") { + const end = source.indexOf("\n", index); + return end < 0 ? source.length : end; + } + if (char === "/" && next === "*") { + const end = source.indexOf("*/", index + 2); + return end < 0 ? source.length : end + 2; + } + if (char === '"' || char === "'") { + let cursor = index + 1; + while (cursor < source.length && source[cursor] !== char && source[cursor] !== "\n") { + cursor += source[cursor] === "\\" ? 2 : 1; + } + return cursor + 1; + } + if (char === "`") return skipTemplate(source, index + 1); + return index; +} + +function skipTemplate(source: string, start: number): number { + let cursor = start; + while (cursor < source.length) { + const char = source[cursor]; + if (char === "\\") cursor += 2; + else if (char === "`") return cursor + 1; + else if (char === "$" && source[cursor + 1] === "{") cursor = skipBraces(source, cursor + 1); + else cursor += 1; + } + return source.length; +} + +/** Index after the `}` matching the `{` at `open`, skipping nested non-code. */ +function skipBraces(source: string, open: number): number { + let depth = 0; + let cursor = open; + while (cursor < source.length) { + const skipped = skipNonCode(source, cursor); + if (skipped !== cursor) { + cursor = skipped; + continue; + } + if (source[cursor] === "{") depth += 1; + else if (source[cursor] === "}") { + depth -= 1; + if (depth === 0) return cursor + 1; + } + cursor += 1; + } + return source.length; +} + +function skipWhitespace(source: string, index: number): number { + let cursor = index; + while (cursor < source.length && /\s/u.test(source[cursor] ?? "")) cursor += 1; + return cursor; +} + +function lineAt(source: string, index: number): number { + let line = 1; + for (let cursor = 0; cursor < index; cursor++) if (source[cursor] === "\n") line += 1; + return line; +} diff --git a/packages/coding-agent/src/features/workflow/step-workflow.ts b/packages/coding-agent/src/features/workflow/step-workflow.ts index 1acdd884..d75ee266 100644 --- a/packages/coding-agent/src/features/workflow/step-workflow.ts +++ b/packages/coding-agent/src/features/workflow/step-workflow.ts @@ -21,6 +21,7 @@ import { workflowProgressResult, } from "./rendering.ts"; import { WorkflowRuntime, workflowToolResult } from "./runtime.ts"; +import { assertWorkflowScriptUsage } from "./script-lint.ts"; import { isWorkflowPathInside } from "./tool-profile.ts"; import type { WorkflowAgentRunner, WorkflowProgress, WorkflowRunResult } from "./types.ts"; import type { UltraloopTurnState } from "./ultraloop-opt-in.ts"; @@ -113,6 +114,7 @@ export async function resolveWorkflowScript( } if (Buffer.byteLength(script, "utf8") > WORKFLOW_MAX_SCRIPT_BYTES) throw new Error(`Workflow script exceeds ${WORKFLOW_MAX_SCRIPT_BYTES} bytes`); + assertWorkflowScriptUsage(script); return { name: name.slice(0, 200), script, ...(sourcePath ? { sourcePath } : {}) }; } @@ -145,7 +147,7 @@ export function createStepWorkflowExtension(options: StepWorkflowExtensionOption renderShell: "self", renderCall: renderWorkflowCall, renderResult: renderWorkflowResult, - description: `Run an isolated JavaScript workflow that coordinates many agents through phase(), parallel(), pipeline(), agent(), and the HoH iterate() loop. Scripts run in a sandboxed VM (no process, require, network, wall clock, or randomness); every agent call is journaled under .stepcode/workflows/runs so runs can resume. Provide exactly one of script (inline), scriptPath, or name (saved workflow); /workflows lists saved workflows and recent runs. + description: `Run an isolated JavaScript workflow that coordinates many agents through agent(), parallel(), pipeline(), and the HoH iterate() loop; phase(title) takes one string and only labels a milestone (it runs nothing). Scripts run in a sandboxed VM (no process, require, network, wall clock, or randomness); every agent call is journaled under .stepcode/workflows/runs so runs can resume. Provide exactly one of script (inline), scriptPath, or name (saved workflow); /workflows lists saved workflows and recent runs. Ultracode and Ultraloop name this same multi-agent capability. Interactive controls are /ultracode on, off, status, and help; /ultraloop is an alias. For a single task use ultracode: or ultraloop:. When asked about these modes, explain the controls without starting a workflow. These controls do not expand the active tool profile. diff --git a/packages/coding-agent/src/features/workflow/vm.ts b/packages/coding-agent/src/features/workflow/vm.ts index c5deedce..bfe564c6 100644 --- a/packages/coding-agent/src/features/workflow/vm.ts +++ b/packages/coding-agent/src/features/workflow/vm.ts @@ -44,6 +44,7 @@ import type { QuickJSRuntime, QuickJSWASMModule, } from "quickjs-emscripten-core"; +import { PHASE_USAGE_MESSAGE } from "./script-lint.ts"; import type { WorkflowJsonSchema, WorkflowMeta, WorkflowTaskFailure } from "./types.ts"; const MAX_SCRIPT_BYTES = 128 * 1024; @@ -485,7 +486,10 @@ globalThis.pipeline = async (items, ...stages) => { return value; })); }; -globalThis.phase = (title) => { __workflow_phase_ref(String(title === undefined ? "" : title)); }; +globalThis.phase = (...args) => { + if (args.length > 1) throw new TypeError(${JSON.stringify(PHASE_USAGE_MESSAGE)}); + __workflow_phase_ref(String(args[0] === undefined ? "" : args[0])); +}; globalThis.log = (message) => { __workflow_log_ref(String(message === undefined ? "" : message)); }; globalThis.budget = Object.freeze({ total: __workflow_total, diff --git a/packages/coding-agent/src/step/system-prompt.ts b/packages/coding-agent/src/step/system-prompt.ts index 7d66fc2f..a633b6e1 100644 --- a/packages/coding-agent/src/step/system-prompt.ts +++ b/packages/coding-agent/src/step/system-prompt.ts @@ -273,7 +273,7 @@ export function buildStepSystemPromptAppendix( "- When ultraloop is on for the session that opt-in is standing: author and run a workflow for every substantive task by default, treat token cost as unconstrained, and aim for the most exhaustive correct answer. Multi-phase work often means several workflows in sequence — one per phase — so you stay in the loop between them. Lean toward adversarially verifying findings unless the work is already verified. Solo only on conversational turns or trivial mechanical edits. When session mode is off, revert to the per-turn opt-in rule.", "- Without any signal, prefer targeted tools — search_files, read_file, or a single subagent — and never reach for workflow on single-file edits, one-shot lookups, or exploration that a few targeted queries can answer.", "- With opt-in, workflow fits broad multi-subsystem audits, mechanical migrations across many sites, multi-way parallel review with adversarial verification, research sweeps, and HoH convergence on a spec.", - "- Scripts compose agent() calls using phase() for visible milestones, parallel() and pipeline() for fan-out, iterate() for HoH loops, nested workflow() for sub-runs, log() for progress, and the budget object; pass inputs via args as real JSON values, never JSON-encoded strings.", + "- Scripts compose agent() calls using phase(title) to label visible milestones (one string; it runs nothing), parallel() and pipeline() for fan-out, iterate() for HoH loops, nested workflow() for sub-runs, log() for progress, and the budget object; pass inputs via args as real JSON values, never JSON-encoded strings.", "- parallel() is a barrier that awaits every call before returning; pipeline() threads each item through the stages independently with no barrier between stages, so a stage never sees sibling items' earlier-stage output. Default to pipeline() and use a stage-wide parallel() only when a stage needs cross-item context such as dedup, early exit, or cross-referencing.", "- iterate({spec, maxIterations, stopWhenSpecCoverage}) runs the HoH loop: a read-only Planner, a single-writer Developer, and an independent read-only QA, with role boundaries enforced by toolProfile and readOnly/writable mounts; it stops on spec coverage, stagnation, max iterations, an empty objective, or exhausted budget.", "- Pass a JSON schema to agent() whenever a later stage consumes the result; output is forced to structured JSON and mismatches are retried up to 3 times before the call fails.", diff --git a/packages/coding-agent/test/workflow-extension.test.ts b/packages/coding-agent/test/workflow-extension.test.ts index 2cf36205..e191d4f6 100644 --- a/packages/coding-agent/test/workflow-extension.test.ts +++ b/packages/coding-agent/test/workflow-extension.test.ts @@ -248,6 +248,17 @@ test("workflow script resolution rejects ambiguous, oversized, and escaping sour await expect(resolveWorkflowScript(cwd, { name: "escape" }, homeRoot)).rejects.toThrow("was not found"); }); +test("workflow script resolution rejects phase() with a callback for inline and path sources", async () => { + const cwd = await workspace(); + const homeRoot = await workspace("step-workflow-home-"); + const misuse = `log("start");\nphase("Review", async () => { await agent("review"); });\n`; + await writeFile(path.join(cwd, "misuse.js"), misuse); + await expect(resolveWorkflowScript(cwd, { script: misuse }, homeRoot)).rejects.toThrow( + "Workflow script line 2: phase(title) takes one string", + ); + await expect(resolveWorkflowScript(cwd, { scriptPath: "misuse.js" }, homeRoot)).rejects.toThrow("line 2"); +}); + test("the turn-state budget applies unless an explicit budget parameter overrides it", async () => { vi.stubEnv("STEP_DISABLE_WORKFLOW", ""); const cwd = await workspace(); diff --git a/packages/coding-agent/test/workflow-script-lint.test.ts b/packages/coding-agent/test/workflow-script-lint.test.ts new file mode 100644 index 00000000..47f8c16f --- /dev/null +++ b/packages/coding-agent/test/workflow-script-lint.test.ts @@ -0,0 +1,60 @@ +import { expect, test } from "vitest"; +import { NO_AGENTS_WARNING, workflowToolResult } from "../src/features/workflow/runtime.ts"; +import { assertWorkflowScriptUsage, findMultiArgumentPhaseCall } from "../src/features/workflow/script-lint.ts"; +import type { WorkflowRunResult } from "../src/features/workflow/types.ts"; + +test("flags phase() calls that pass a callback or a second argument", () => { + expect(findMultiArgumentPhaseCall(`phase("Review", async () => { await agent("x"); });`)).toBe(1); + expect(findMultiArgumentPhaseCall(`log("a");\n\nawait phase ( "Build" , () => 1 );`)).toBe(3); + expect(findMultiArgumentPhaseCall(`phase(\`Round \${i}\`, fn)`)).toBe(1); + expect(() => assertWorkflowScriptUsage(`phase("a", () => {})`)).toThrow(/line 1: phase\(title\) takes one string/); +}); + +test("accepts single-argument phase() and lookalikes", () => { + const valid = [ + `phase("Review");\nawait agent("review", { label: "r" });`, + `phase(names.join(", "));`, + `phase(fmt("a", "b"));`, + `phase([a, b].join("/"));`, + `phase(\`Round \${f(a, b)}\`);`, + `phase("trailing",);`, + `// phase("a", b)\n/* phase("a", b) */\nconst s = 'phase("a", b)';`, + `const t = \`phase("a", \${b})\`;`, + `obj.phase("a", b); tracker.phase(1, 2);`, + `function phase(a, b) {}`, + `const o = { phase(a, b) { return a; } };`, + `const myphase = (a, b) => a; myphase("a", 1); phases("a", 1);`, + ]; + for (const script of valid) expect(findMultiArgumentPhaseCall(script), script).toBeUndefined(); +}); + +function runResult(overrides: Partial): WorkflowRunResult { + return { + schemaVersion: 1, + runId: "r", + name: "inline", + status: "completed", + value: null, + meta: null, + startedAt: 0, + finishedAt: 0, + spentTokens: 0, + cacheHits: 0, + agentCalls: 0, + phases: [], + ...overrides, + } as WorkflowRunResult; +} + +test("a completed run with no agent calls leads with a warning", () => { + const text = (result: WorkflowRunResult) => { + const block = workflowToolResult(result).content[0]; + return block?.type === "text" ? block.text : ""; + }; + expect(text(runResult({}))).toMatch( + new RegExp(`^${NO_AGENTS_WARNING.replace(/[.*+?^${}()|[\]\\]/gu, "\\$&")}\n\\{`), + ); + expect(workflowToolResult(runResult({})).isError).toBe(false); + expect(text(runResult({ agentCalls: 1 }))).not.toContain(NO_AGENTS_WARNING); + expect(text(runResult({ status: "failed" }))).not.toContain(NO_AGENTS_WARNING); +}); diff --git a/packages/coding-agent/test/workflow-vm.test.ts b/packages/coding-agent/test/workflow-vm.test.ts index 6c6b5e72..bd66dfb5 100644 --- a/packages/coding-agent/test/workflow-vm.test.ts +++ b/packages/coding-agent/test/workflow-vm.test.ts @@ -94,6 +94,16 @@ test("exposes JSON args, metadata, and every host primitive", async () => { expect(calls.agents[0]?.options).toEqual({ label: "worker" }); }); +test("phase() rejects a second argument instead of silently dropping it", async () => { + const { host, calls } = createHost(); + // Built at runtime so the static pre-run check cannot see it; the VM guard must. + await expect( + runInQuickJs(`const p = phase; p("Review", async () => { await agent("never"); }); return 1;`, undefined, host), + ).rejects.toThrow("phase(title) takes one string"); + expect(calls.phases).toEqual([]); + expect(calls.agents).toEqual([]); +}); + test("reports an unlimited budget as null", async () => { const { host } = createHost({ budgetTotal: () => null }); const result = await runInQuickJs(`return { total: budget.total };`, undefined, host);