fix(workflow): run documented scripts and harden cancellation

[Hmbown]

What

The #4131 live dogfood pass found that every checked-in imperative Workflow fixture was unrunnable: the VM accepted plain scripts and declarative export default workflow({...}), but not the documented export default async function (args) {...} shape.

This PR now closes the full live-dogfood chain:

• safely desugars line-leading export default without matching examples inside strings, templates, or comments;
• accepts fixtures that provide both a short description and the actual prompt (prompt wins);
• makes WF-A3 deterministic with a real one-token child failure and nullable parallel() slot;
• treats run cancellation as fatal control flow instead of a partial-success null;
• finalizes running child rows and streams authoritative run_cancelled state after racing completion events;
• makes repeated cancellation an idempotent no-op;
• updates the WF-A1..A4 fixtures and records the live results in docs/DOGFOOD_AUTOMATIC_WORKFLOWS.md;
• clears the Rust 1.97 all-target lint findings in the touched Workflow/TUI paths.

Live evidence

• WF-A1 workflow_dd5de6d0: 4/4 children, two phases, read-only audit plus synthesis.
• WF-A2 workflow_97ae14dc: isolated implementer then parent verifier, 2/2, isolation confirmed.
• WF-A3 workflow_2f590eec: one visible budget failure, one null slot, synthesis from two survivors.
• WF-A4 workflow_45629ac6: one completed + two cancelled children, lifecycle cancelled, no unreachable phase, no result.

Verification

• cargo test -p codewhale-workflow-js --locked — 42 VM integration tests + 8 unit tests passed.
• cargo test -p codewhale-tui --locked dogfood_ — 5 dogfood tests passed.
• cargo test -p codewhale-tui --locked workflow_cancel_interrupts_vm_and_blocks_further_spawns — passed twice after replacing a fixed sleep with a bounded first-child wait.
• cargo clippy -p codewhale-tui -p codewhale-workflow-js --all-targets --locked -- -D warnings — passed on Rust stable 1.97 after merging current main / fix(tui): clear clippy 1.97 lints in test targets #4322.
• cargo fmt --all -- --check and git diff --check — passed.

Blocks completion of #4131 and supplies release evidence for #4110, #4175, #4177, and #4179. Part of the v0.8.68 milestone.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[claude]

Claude finished @Hmbown's task in 3m 23s —— View job

---

Review: fix(workflow-js): run export-default imperative workflow scripts

• Gather context (diff, changed files, CLAUDE.md/AGENTS.md)
• Review crates/workflow-js/src/vm.rs changes
• Review crates/workflow-js/tests/vm_tests.rs changes
• Check provider/model prefix safety implications
• Post review feedback

Summary

The fix addresses a real, well-diagnosed bug: sources wrapped in an async IIFE choke on the module-only export keyword, so the documented imperative export default async function (args) {...} shape (used by all four #4131 dogfood fixtures) never parsed. Desugaring the default export into a captured globalThis.__workflow_default and invoking it with args in one place (run_in_ctx) is the right layer — it benefits the TUI tool path, lane, and CLI uniformly, and reuses the existing args global set up by install_host.

Findings

1. Correctness — naive line-prefix scan can misfire inside multi-line template literals/comments (crates/workflow-js/src/vm.rs:384-407, inline comment posted)

desugar_export_default finds the target line with line.trim_start().starts_with("export default"), with no awareness of whether that line is actually inside a string, template literal, or comment. A script that embeds example/documentation code in a multi-line template literal, e.g.:

const example = `
export default async function (args) { ... }
`;

will have that inner line falsely matched and rewritten, corrupting the template literal's content. This is plausible in this crate's domain (workflow scripts that generate, document, or reference other .workflow.js sources).

The new plain_scripts_are_untouched_by_export_desugaring test only covers a single-line string ("const note = \"export default docs\";") — that line doesn't start with export default, so it never actually exercises the naive-scan hazard; it isn't a regression guard for the real risk.

The codebase already has a more careful precedent for exactly this class of problem: extract_workflow_object in crates/workflow/src/js_authoring.rs char-scans while tracking in_string/backtick state specifically to avoid treating string/template contents as syntax. Applying the same discipline here (or restricting the match to "first non-whitespace token of the entire trimmed source," matching the documented single-default-export shape) would close this gap. A regression test with a multi-line template literal starting a line with export default would confirm the fix.

2. Minor/non-blocking:

• desugar_export_default treats any value with typeof === "function" as invokable, so export default class Foo {} would hit await __wf_default(args) and throw ("Class constructor cannot be invoked without 'new'") instead of returning the class value like other non-function exports. Edge case, low priority given the documented/only supported shapes are plain and async functions.
• let mut lines: Vec<&str> only needs .iter_mut() for the reconstruction loop but never mutates through it — .iter() would be simpler and avoids the mut binding; cosmetic only.

Other notes

• No provider/model/route prefix logic is touched by this diff — category 2 from the review checklist (v0.8.65 EPIC v0.8.65 EPIC: Separate provider facts, model facts, offerings, and route resolution #2608 invariant) doesn't apply here.
• Test coverage for the happy paths (async function + args round-trip, sync function, non-function export, no-touch guard) is solid; the gap is specifically the string/template/comment false-positive case noted above.
• I could not run cargo test -p codewhale-workflow-js --locked in this review sandbox (Bash execution beyond simple git diff requires interactive approval that isn't available here), so I verified behavior by code reading rather than by re-running the suite. The PR description's reported 39 passed / 4 new tests is consistent with what's in the diff.

[claude]

Correctness: naive line-prefix scan can misfire inside multi-line template literals

source.lines().position(|line| line.trim_start().starts_with("export default")) only checks each line's leading text — it has no notion of "am I inside a string/template literal or comment". A script like:

const example = `
export default async function (args) { ... }
`;
return example;

will have its second line matched and rewritten (globalThis.__workflow_default = async function (args) { ... }), corrupting the template literal's textual content, even though there is no real top-level export default in this script. Given workflow-js scripts can legitimately embed example/documentation snippets (e.g. a meta-workflow that generates or explains other .workflow.js files) this isn't a purely theoretical case.

The new plain_scripts_are_untouched_by_export_desugaring test (vm_tests.rs) only covers a single-line string ("const note = \"export default docs\";"), whose line doesn't start with export default in the first place, so it doesn't exercise this hazard at all.

The codebase already has a more careful precedent for this exact problem: extract_workflow_object in crates/workflow/src/js_authoring.rs char-scans while tracking in_string/backtick state to avoid treating string/template contents as syntax. Worth borrowing that discipline here — e.g. only desugar when export default is the first non-whitespace token of the entire trimmed source (matching the documented single-default-export shape), or track quote/backtick/comment state while scanning for the line.

Fix this →

[claude]

Claude finished @Hmbown's task in 2m 7s —— View job

---

Review: fix(workflow-js): run export-default imperative workflow scripts

• Gather context (diff, changed files, prior review comment)
• Review new commit 5df4a5f (addresses prior string/template-literal false-positive finding)
• Re-check desugar_export_default correctness
• Check new tests
• Post final review feedback

Summary

Commit 5df4a5f directly fixes the correctness gap I flagged in my prior review (and that copilot/claude inline comments also raised): the naive line-prefix scan is now replaced with a proper char-scanner, line_leading_export_default (crates/workflow-js/src/vm.rs:405-490), that tracks quote/backtick, line-comment (//), and block-comment (/* */) state while walking the source, and only treats export default as real syntax when it's the first non-whitespace token on a line outside any of those contexts. This is the same discipline as extract_workflow_object in crates/workflow/src/js_authoring.rs:130-171 (which the prior review pointed to as precedent), reimplemented byte-wise instead of char-wise but equivalent in behavior.

I traced the state machine by hand against the tricky cases:

• Multi-line template literals (`) correctly stay "in quote" across newlines (byte 444: if byte == b'\n' { line_has_only_whitespace = true; escaped = false; } without clearing quote), so an export default line embedded inside a template body is never matched.
• Block comments spanning multiple lines are tracked independently and closed only on */.
• The match check (line 472) is only reachable when quote, line_comment, and block_comment are all clear, so string/comment content can never trigger a false positive.

Test coverage

export_default_examples_inside_multiline_text_are_not_desugared (crates/workflow-js/tests/vm_tests.rs, new in this commit) is exactly the regression test the prior review asked for — it embeds a multi-line template literal and a block comment, each containing a line starting with export default, and asserts the script's own return template.includes(...) executes untouched (i.e., no desugaring fired). This closes the gap where the previous plain_scripts_are_untouched_by_export_desugaring test only exercised a single-line string that never actually reached the hazard.

Combined with the four tests from the original commit (async function + args round-trip, sync function, non-function export, single-line string guard), coverage now includes the specific failure mode that was found.

Remaining non-blocking notes (unchanged from prior review, not addressed here, low priority)

• export default class Foo {} would still hit typeof __wf_default === "function" → await __wf_default(args) and throw ("Class constructor cannot be invoked without 'new'") rather than returning the class value. Edge case outside the documented/supported authoring shapes (plain/async function or plain value).
• The previously-noted cosmetic let mut lines: Vec<&str> (unnecessary mut) is now moot — the function was rewritten and no longer uses that pattern.

Verification

I was not able to run cargo test -p codewhale-workflow-js --locked in this review sandbox (Bash commands beyond simple git/read-only operations require interactive approval that isn't available here), so this review is based on careful code reading of the full state machine rather than executing the suite. The commit message reports 40 passed (36 existing + 4 new... consistent with the original 39 plus this commit's added regression test).

Verdict

This resolves the correctness finding from my previous pass. No new blocking issues found in 5df4a5f.

[claude]

Claude finished @Hmbown's task in 3m 44s —— View job

---

Review: fix(workflow-js): run export-default imperative workflow scripts

• Gather context (diff since last review, new commits)
• Review new commits: 86b158c, aa3f40c, 7f102f2
• Check whether prior findings (string/template false-positive, class export) were addressed
• Post final review feedback

Summary

This push adds three commits beyond what was previously reviewed (5df4a5f): 86b158c ("close live dogfood gaps" — task() prompt/description duplicate-field fix, cancellation-vs-parallel() handling, cancel-receipt idempotency), aa3f40c (mechanical let-chain rewrites for stable 1.97 clippy), and 7f102f2 (strengthens the cancel test). The prior finding (naive line-prefix export default scan) was already fixed in 5df4a5f and remains fixed here — not revisited.

Findings

1. Correctness (plausible, non-blocking) — isFatalTaskError may not catch every path a whole-run cancellation can take through task() (crates/workflow-js/src/vm.rs:667-699, crates/workflow-js/src/vm.rs:803)

task_host_inner's cancellation race:

let completion = tokio::select! {
    _ = cancel.cancelled() => return Err("task(): run cancelled".to_string()),
    completion = completion_rx => completion...,
};

has no biased;, so when both branches are simultaneously ready, tokio picks pseudo-randomly. SubAgentWorkflowDriver::force_cancel_all (crates/tui/src/tools/workflow.rs:1836-1849, new in this commit) calls record_task_completion(id, &TaskCompletion::Cancelled) for every outstanding child immediately after vm_cancel.cancel() in WorkflowRunController::cancel (crates/tui/src/tools/workflow.rs:84-92) — with no .await between them, so both the cancel signal and the manufactured Cancelled completion are typically ready by the next poll. If completion_rx wins that race, task_host_inner returns Err("task(): subagent cancelled") (vm.rs:698) instead of "task(): run cancelled". isFatalTaskError (vm.rs:801-804) only string-matches "run cancelled", not "subagent cancelled", so that outcome is still treated as a non-fatal, droppable slot by parallel()/pipeline() — the same class of bug WF-A4 was fixed for.

In practice this is masked for the default (wait: false) path because WorkflowRunController::cancel also calls handle.abort() on the spawned run task, which drops the whole VM future — so JS execution can't continue into phase("unreachable...") regardless of which branch of the internal race won. The gap is real only for wait: true callers (no run_handle to abort) or any future non-TUI WorkflowDriver (lane/CLI) that reuses this cancellation shape without an equivalent abort backstop. Given the new regression test (parallel_does_not_continue_after_external_run_cancellation) only exercises FakeReply::Never (completion never fires, so the race can't occur), it doesn't cover this path. Worth either matching on "subagent cancelled" too in isFatalTaskError, or having force_cancel_all check cancel.is_cancelled() before manufacturing per-task Cancelled completions, and adding a test where a task's completion and the run cancellation are both simultaneously ready.

2. Minor — cancel_workflow's "already cancelled" guard has a narrow TOCTOU window (crates/tui/src/tools/workflow.rs:665-710)

The controller is removed from controllers_guard, then already_cancelled is read from runs_guard in a separate lock acquisition. Two concurrent cancel calls for the same run_id arriving before either commits status = Cancelled can both pass the guard and each push a run_cancelled event, producing duplicate terminal events (the new cancelled_again test only exercises sequential, not concurrent, calls). Low severity — cancelling the same run twice concurrently is an unlikely client pattern — but note it since the whole point of this commit is making repeat-cancel idempotent.

3. Verified correct:

• TaskOptions.prompt/description split (vm.rs:711-736) correctly fixes the "duplicate field via alias" serde failure the dogfood log describes, and prompt taking precedence matches the new task_prompt_takes_precedence_over_short_description test.
• The aa3f40c let-chain rewrites in mouse_ui.rs, provider_picker.rs, tool_routing.rs, ui.rs are behavior-preserving (verified each nested if/if let condenses to the same AND-chain semantics); map_or(true, ...) → is_none_or(...) is a straight clippy rename, no behavior change.
• cancel_workflow's authoritative re-emit of run_cancelled after a racing run_completed is a sound fix for the panel-stuck-failed symptom described in the dogfood log.

Verification

Could not run cargo test -p codewhale-workflow-js --locked or cargo test -p codewhale-tui --locked in this review sandbox (Bash beyond read-only git operations requires interactive approval unavailable here), so this pass is based on code reading, not test execution. Reasoning about the tokio::select! race in Finding 1 is a plausible-but-unverified hazard, not a confirmed reproduction — flagging for the author's judgment given it touches cancellation-under-load, an area already core to this commit's intent.

[claude]

Claude finished @Hmbown's task in 3m 1s —— View job

---

Review: fix(workflow): run documented scripts and harden cancellation

• Gather context (diff since last review, new commit)
• Review new commit fddb0a8 ("scope row finalization to cancel")
• Check whether it interacts with previously-flagged findings
• Post review feedback

Summary

This push adds one commit beyond what was previously reviewed (7f102f2): fddb0a8, which splits SubAgentWorkflowDriver::force_cancel_all into two functions — finalize_running_tasks_cancelled (marks still-Running child rows Cancelled and emits their terminal events) and a slimmed-down force_cancel_all (clears spawn permits, aborts child agents, releases waiters). WorkflowRunController::cancel (crates/tui/src/tools/workflow.rs:84-93) now calls both, in that order, so the explicit user-cancel path is unchanged in effect. This correctly fixes a real bug the prior scan of force_cancel_all had: since record_task_completion (workflow.rs:2134-2158) unconditionally overwrites record.status/record.output even when was_running is false, the old code — which ran record_task_completion(id, Cancelled) for every tracked child id at every VM teardown, including normal success (vm.rs:206-209) — would silently relabel already-Completed children as Cancelled at the end of a normal run. That's gone now.

Findings

1. Correctness (plausible, non-blocking) — narrowing finalization to explicit cancel() likely reintroduces the "stuck Running" row for non-cancel teardown paths (crates/tui/src/tools/workflow.rs:1838-1864,2289-2291, crates/workflow-js/src/vm.rs:206-209,299-306)

WorkflowDriver::cancel_all (the trait method invoked at every VM run teardown — success, script error, or guard-drop per the doc comment at vm.rs:147-155) now only calls force_cancel_all, which no longer calls record_task_completion. force_cancel_all still forcibly aborts any still-in-flight child via cancel_child_agents (workflow.rs:1847), so a child that's Running when the script returns/throws without ever being awaited (e.g. a task() call whose promise the script never awaits before an early return, or one leg of a Promise.all/parallel() that's still pending when a sibling leg rejects and the uncaught error propagates) will be aborted mid-flight but its task_records entry stays status: Running forever — no terminal TaskCompleted event is ever emitted for it, since the only remaining call to record_task_completion for stray children is gated behind the explicit-cancel path (finalize_running_tasks_cancelled, only invoked from WorkflowRunController::cancel). This is the same panel-stuck-Running symptom the WF-A4 fix in 86b158c addressed, just now reopened for the non-cancel teardown paths instead of the cancel path.

This is plausible-but-unverified — I didn't reproduce it, and it depends on a script leaving a spawned task unawaited at teardown, which the documented parallel()/pipeline() helpers likely avoid in the common case (they appear to await/settle every spawned child). It's most reachable via an uncaught script error while one or more task() calls are still outstanding. Worth either a quick regression test (spawn a child, throw before awaiting it, assert the child's row reaches a terminal status) or restoring finalization in cancel_all/force_cancel_all for stray children while leaving finalize_running_tasks_cancelled's Cancelled-labeling specific to the explicit-cancel path.

Previously flagged, still open (not touched by this commit)

• isFatalTaskError may not catch the "task(): subagent cancelled" string (vm.rs:698, vm.rs:801-804) — flagged in the prior review pass; finalize_running_tasks_cancelled/force_cancel_all still only race against cancel.cancelled() vs completion_rx without biased;, so this remains open.
• cancel_workflow's "already cancelled" guard has a narrow concurrent-call TOCTOU window (crates/tui/src/tools/workflow.rs:665-710) — also unaddressed, low severity.

Verification

Could not run cargo test -p codewhale-tui --locked or cargo clippy in this review sandbox (Bash beyond read-only git/read operations requires interactive approval unavailable here), so this pass is based on code reading and tracing the call graph (WorkflowRunController::cancel → finalize_running_tasks_cancelled/force_cancel_all; WorkflowDriver::cancel_all → force_cancel_all; both call sites of cancel_all in vm.rs), not test execution.