Fix the defects the supervisor plan listed as prerequisites - #4632
Conversation
The supervisor implementation plan carried a "repository prerequisites" table: seven defects in the machinery that feature reuses, each a bug today. This fixes all seven and drops the table. **Step failure is terminal, not completion.** A failed step set `completed = true`, buried the cause in the result payload, and emitted no protocol-level `StepResult.error` — so dependents ran on failures and a later completion could overwrite a failed state. A step now sets `failed`/`error` and leaves `completed` false; `TaskExecutor` blocks dependents and marks them failed with the blocking step named, and a deadlocked or budget-exhausted plan fails its leftovers instead of leaving them pending. Several executor tests passed only because of this bug — their mock provider had no `generateLoop`, so every step failed and was recorded as done. **One execution policy for every agent mode** (`agent-policy.ts`). Script and graph branches bypassed plan approval, `maxTokens` never reached them, `maxSteps` was ignored once a plan had more than one task, `AgentNode` plan mode hardcoded 10 steps / 5 iterations over its own declared budgets, and only script mode capped fan-out. `Agent` now resolves one `AgentPolicy` and hands it to all four modes; task and fan-out dispatch share the bounded merge in `utils/merge-generators.ts`; the approval gate is a property of the run, so script and graph artifacts pass through it too; `AgentNode` gains a `max_steps` prop and maps `max_turns` to the per-step budget. **Signal propagation and tool fetch safety.** `ProcessingContext.copy()` dropped the cancellation signal and `run_subtask`/`run_search` spawned children without one, so a cancelled run kept driving provider calls. HTTP/browser tools ran private timeout controllers that ignored `context.signal`, and `BrowserTool` fetched a model-controlled URL with raw `fetch` where the HTTP tools use SSRF-protected `safeFetch`. **`step.tools` enforcement in task mode.** Plans carry per-step tool allow-lists; `TaskExecutor` handed every step the full collection while script mode enforced its list. Same plan, different privileges by mode. **One debug/validation service.** The execution-summary reducer and per-surface triage move to `@nodetool-ai/execution/debug`, consumed by the CLI harness, the new `POST /api/workflows/:id/debug` endpoint, and the `debug_workflow` tool (which reported a status string where the CLI reported node errors). `--no-server` without `--browser` now fails instead of reporting "ran clean on no surface", and `validate_workflow` without a registry returns an error rather than the graph unvalidated. **Failed plans no longer compile into apparent success.** A dependency deadlock was reported as a prose chunk and the compiler ran regardless. It is now an error log plus terminal task events; a plan whose every task failed throws, and a partially failed one names the missing tasks to the compiler. Checkpoint hashes cover step instructions, dependencies, tool lists, schemas and the model, so an edited plan cannot resume stale results. **`AgentWorkflowRunner` no longer mutates the shared context.** It ran on a scoped child (sharing agent memory, carrying the signal) instead of permanently replacing the caller's injected tools, and streams through the message-listener API instead of monkeypatching `context.emit`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FvWgdcQ3UKJM3QR8vr5hpN
There was a problem hiding this comment.
Pull request overview
This PR addresses seven prerequisite defects identified in the workflow-supervisor implementation plan by tightening agent execution semantics (step failure handling, consistent policy/budgets across modes, bounded fan-out), improving cancellation propagation and SSRF safety in tools, and unifying workflow debug/validation reporting across CLI/server/agent surfaces.
Changes:
- Make step failures terminal (not “completed”), block dependents on failed steps, and fail deadlocked/budget-exhausted leftovers instead of leaving them pending.
- Introduce a shared
AgentPolicyapplied consistently across task/plan/script/graph modes, plus a bounded async-generator merge to cap fan-out concurrency. - Unify run triage via shared execution-summary + verdict logic (
@nodetool-ai/execution/debug) and add a server/api/workflows/:id/debugendpoint used by the agent-facingdebug_workflowtool.
Reviewed changes
Copilot reviewed 48 out of 48 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/websocket/src/http-api.ts | Adds /api/workflows/:id/debug via handleWorkflowRun(..., debug=true) returning summary + verdict. |
| packages/runtime/tests/context.test.ts | Adds tests for ProcessingContext.copy() signal propagation, memory sharing, listener inheritance, and tool scoping. |
| packages/runtime/src/context.ts | Extends ProcessingContext.copy() with options and ensures run cancellation signal is carried to children. |
| packages/llm-nodes/src/nodes/agents.ts | Adds max_steps prop and maps plan-mode budgets to the shared agent policy defaults. |
| packages/execution/src/index.ts | Re-exports shared debug collector/verdict/types from @nodetool-ai/execution. |
| packages/execution/src/debug/collector.ts | New: folds processing messages into a structured ExecutionSummary with preview-safe values. |
| packages/execution/src/debug/verdict.ts | New: builds per-surface RunVerdict and ordered issue list from an ExecutionSummary. |
| packages/execution/src/debug/types.ts | New: shared types for execution summaries and verdicts. |
| packages/execution/src/debug/index.ts | New: subpath export (@nodetool-ai/execution/debug) for debug reducer + verdict + types. |
| packages/execution/package.json | Adds ./debug export map entry for the new debug subpath. |
| packages/cli/tests/debug-report.test.ts | Adds coverage for “no surface ran” verdict behavior. |
| packages/cli/src/debug/verdict.ts | Switches per-surface issue collection to shared execution package; requires at least one surface to run. |
| packages/cli/src/debug/types.ts | Re-exports execution-summary types from @nodetool-ai/execution/debug. |
| packages/cli/src/debug/harness.ts | Hard-fails --no-server without --browser to avoid “clean on no surface”. |
| packages/cli/src/debug/collector.ts | Re-exports the shared collectExecutionSummary/previewValue from execution package. |
| packages/agents/tests/task-executor.test.ts | Updates expectations for unschedulable steps to be terminal failures with protocol-level errors. |
| packages/agents/tests/task-executor-failure.test.ts | New: tests step-failure blocking semantics and per-step tool allow-list enforcement in task mode. |
| packages/agents/tests/step-executor.test.ts | Updates step failure assertions: failed ≠ completed; requires StepResult.error. |
| packages/agents/tests/plan-cache-checkpoint.integration.test.ts | Switches checkpoint matching to hashPlanCheckpointKey (broader plan-shape hashing). |
| packages/agents/tests/plan-approval.test.ts | Adjusts mock loop provider to complete execution turns so failures don’t masquerade as success. |
| packages/agents/tests/parallel-task-executor.test.ts | Updates deadlock behavior assertions: task failure via terminal events/logs (not prose chunks). |
| packages/agents/tests/merge-generators.test.ts | New: tests bounded async-generator merge concurrency behavior and error propagation. |
| packages/agents/tests/mcp-tools.test.ts | Updates validation tool behavior: registry-less validation reports an error instead of returning an unvalidated graph. |
| packages/agents/tests/e2e/agent-e2e.test.ts | Updates E2E expectation: failed step is terminal and not completed. |
| packages/agents/tests/agent.test.ts | Updates execution-turn mocking to ensure planned steps can finalize without planner tools. |
| packages/agents/tests/agent-workflow-runner-coverage.test.ts | Verifies AgentWorkflowRunner uses a scoped child context (tools isolated, memory shared). |
| packages/agents/tests/agent-policy.test.ts | New: tests shared policy resolution defaults and selective overrides. |
| packages/agents/tests/_helpers/mock-context.ts | Expands mock context to support listener semantics and context copying used by runner changes. |
| packages/agents/src/utils/merge-generators.ts | New: bounded async-generator merge with concurrency cap and early-consumer cleanup. |
| packages/agents/src/types.ts | Extends Step with failed/error semantics (terminal failure separate from completion). |
| packages/agents/src/tools/run-subtask-tool.ts | Propagates context.signal into child executor to honor cancellation. |
| packages/agents/src/tools/run-search-tool.ts | Propagates context.signal into child executor to honor cancellation. |
| packages/agents/src/tools/mcp-tools.ts | Updates debug_workflow to call /debug with fallback to /run; changes validate behavior without registry. |
| packages/agents/src/tools/http-tools.ts | Adds requestSignal() to combine per-request timeout with run cancellation; uses safeFetch. |
| packages/agents/src/tools/browser-tools.ts | Uses SSRF-protected safeFetch for model-controlled URLs and honors cancellation via requestSignal(). |
| packages/agents/src/task-executor.ts | Implements failure-as-terminal semantics, per-step tool allow-lists, and bounded parallel execution. |
| packages/agents/src/step-executor.ts | Ensures failed steps set failed/error and emit protocol-level StepResult.error (do not mark completed). |
| packages/agents/src/script-runner.ts | Threads maxTokens into StepExecutor for script mode. |
| packages/agents/src/parallel-task-executor.ts | Uses checkpoint key hashing, bounded concurrency, deadlock/budget failure terminalization, and failed-task reporting. |
| packages/agents/src/index.ts | Exposes new exports: hashPlanCheckpointKey and shared AgentPolicy API. |
| packages/agents/src/compiler-agent.ts | Passes failed task IDs into compiler prompt to avoid synthesizing missing results. |
| packages/agents/src/checkpoint-store.ts | Adds hashPlanCheckpointKey covering plan shape + dependencies + tools + model/systemPrompt. |
| packages/agents/src/agent.ts | Resolves one AgentPolicy, applies approval gate uniformly, fails “all tasks failed” runs, and compiles partial results explicitly. |
| packages/agents/src/agent-workflow-runner.ts | Runs graphs on a scoped child context (tools isolated, memory shared), streams via listeners, and stamps max_tokens. |
| packages/agents/src/agent-policy.ts | New: shared agent execution policy resolver and defaults. |
| packages/agents/CLAUDE.md | Updates docs for unified policy and step-failure semantics. |
| docs/workflow-supervisor-implementation-plan.md | Removes prerequisites table now implemented by this PR and updates references to shared debug service. |
| CLAUDE.md | Updates docs to reference /debug endpoint and shared debug reducer/verdict location. |
| /** Whether an api helper returned its `{ error }` envelope rather than a body. */ | ||
| function isApiError(value: unknown): boolean { | ||
| return ( | ||
| typeof value === "object" && | ||
| value !== null && | ||
| typeof (value as Record<string, unknown>).error === "string" | ||
| ); | ||
| } |
There was a problem hiding this comment.
Correct, and it defeated the point of the endpoint — a failed run's report is the one worth keeping. Fixed in 5faa522: the fallback now triggers on HTTP 404/405 (via a new apiPostWithStatus helper) rather than on the response body, so a /debug response carrying error alongside summary/verdict is returned as-is. Two tests cover both branches.
Generated by Claude Code
`debug_workflow` fell back to `/run` whenever the `/debug` response carried an `error` string. A failed run's report carries exactly that, next to the summary and verdict — so the fallback discarded the report in the one case it exists for. Detect the missing endpoint by HTTP status (404/405) instead of by body shape, via an `apiPostWithStatus` helper. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FvWgdcQ3UKJM3QR8vr5hpN
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 48 out of 48 changed files in this pull request and generated no new comments.
Suppressed comments (2)
packages/agents/src/task-executor.ts:246
- When failing blocked steps, the error message doesn’t include which dependency is missing (only failed deps are named). For steps blocked by a nonexistent dependency, this makes the failure hard to diagnose from
step.error/step_result.error.
const message =
blocking.length > 0
? `Step blocked: dependency ${blocking.join(", ")} failed`
: `Step blocked: ${reason}`;
yield* this.failStep(step, message);
packages/websocket/src/http-api.ts:905
- The new
/api/workflows/:id/debugpath (anddebugresponse shape) isn’t covered by websocket HTTP API tests. Existing tests hithandleWorkflowRunguard branches, but nothing asserts that the debug endpoint returnssummary+verdict(and doesn’t regress back to the plain/runshape).
The workflow-supervisor implementation plan carried a "repository prerequisites" table: seven defects in the machinery that feature reuses, each described as a bug today. This fixes all seven and removes the table from
docs/workflow-supervisor-implementation-plan.md.P0 — Step failure is terminal, not completion
A failed step set
completed = true, buried the cause in the result payload, and emitted no protocol-levelStepResult.error— so dependents ran on failures and a later completion could overwrite a failed state.Stepgainsfailed/error; a failed step leavescompletedfalse and emitsstep_result.error.TaskExecutorblocks dependents of a failed step and marks them failed, naming the blocking step. A dependency deadlock or an exhausted step budget fails the leftovers instead of leaving them pending forever.ParallelTaskExecutor.detectTaskFailurereads the step's own failure state first.Several executor tests passed only because of this bug: their mock provider had no
generateLoop, so every step failed and was recorded as complete. The mocks now drive the real loop.P1 — One execution policy for every agent mode
Script and graph branches bypassed plan approval,
maxTokensnever reached them,maxStepswas ignored once a plan had more than one task,AgentNodeplan mode hardcoded 10 steps / 5 iterations over its own declared budgets, and only script mode capped fan-out.packages/agents/src/agent-policy.ts:AgentPolicy+resolveAgentPolicy, resolved once byAgentand handed to all four modes.utils/merge-generators.ts), replacing two copies of an unbounded one.AgentNodegains amax_stepsprop and mapsmax_turnsto the per-step turn budget.P1 — Signal propagation and tool fetch safety
ProcessingContext.copy()carries the cancellation signal (and gained opt-inshareMemory/inheritMessageListeners).run_subtask/run_searchpasscontext.signalto their child executors.BrowserTooluses SSRF-protectedsafeFetchfor the model-controlled URL, matching the HTTP tools.ScreenshotToolkeeps a plain fetch (operator-configuredBROWSER_URL) but honors cancellation.P1 —
step.toolsenforcement in task modePlans carry per-step tool allow-lists;
TaskExecutorhanded every step the full collection while script mode enforced its list. Same plan, different privileges by mode. An empty list now grants nothing rather than falling back to everything.P1 — One debug/validation service
@nodetool-ai/execution/debug(dependency-free subpath export), consumed by the CLI harness, a newPOST /api/workflows/:id/debug, and thedebug_workflowtool — which previously returned a status string where the CLI returned node errors. It falls back to/runagainst older servers and says so.--no-serverwithout--browsernow fails instead of reporting "ran clean on no surface"; the verdict requires at least one surface to have run.validate_workflowwithout a registry returns an error instead of the graph plus a note, which read as a pass.P2 — Failed plans no longer compile into apparent success
A dependency deadlock was reported only as a prose chunk and the compiler ran regardless. It is now an error log plus terminal
task_failedevents; a plan whose every task failed throws, and a partially failed one names the missing tasks to the compiler so it does not fill the gaps. Checkpoint hashes (hashPlanCheckpointKey) now cover step instructions, dependencies, tool lists, schemas, mode templates, and the model, so an edited plan cannot resume stale results.P2 —
AgentWorkflowRunnershared-context mutationIt now runs on a scoped child context (sharing agent memory, carrying the signal) instead of permanently replacing the caller's injected tools, and streams via
addMessageListenerinstead of monkeypatchingcontext.emit. Two concurrent runs on one context no longer clobber each other.Testing
npm run build:packages,npm run lint(exit 0), andnpm run typecheckpass. Tests: agents 1822, cli 445, execution 11, runtime 2638, websocket 1992, llm-nodes 491, web + electron 13148 — all green. New coverage for the policy resolver, the bounded merge, step-failure blocking, per-step tool allow-lists, context copy semantics, the zero-surface verdict, and registry-less validation.Mobile typecheck/tests fail in this sandbox because
mobile/dependencies are not installed — unrelated to this change.🤖 Generated with Claude Code
https://claude.ai/code/session_01FvWgdcQ3UKJM3QR8vr5hpN
Generated by Claude Code