fix(core+cli): R7 review fixes — 6 substantive + tmux re-verified (#4721)

Wenshao's R7 review (with real build + tmux verification on the
merged state) approved the PR but surfaced 6 valid findings. All
fixed, all RED-first tested, all verified end-to-end against
qwen3-coder-plus via DashScope.

## 1. Dialog-cancel drops accumulated logs (Critical)

`registry.setRecentLogs(...)` previously guarded `status === 'running'`
only. Dialog-initiated cancel marks `status='cancelled'` synchronously
BEFORE the tool's catch arm tries to write logs — so cancelled runs
always showed an empty Logs section in the dialog. Allow the write
after a `'cancelled'` transition too; keep `'completed'` and `'failed'`
as final-state rejects. 2 regression tests: cancel-then-logs-still-
writes, and complete/fail-still-reject.

## 2. WorkflowRunRegistry missing session-reset wiring (Critical)

Sibling drift miss from P4b: `BackgroundTaskRegistry` /
`BackgroundShellRegistry` / `MonitorRegistry` all exposed `reset()` +
`abortAll()`, and `backgroundWorkUtils` (`hasBlockingBackgroundWork`,
`resetBackgroundStateForSessionSwitch`) wired all three. `Workflow
RunRegistry` had neither. Result: `/clear` and session-resume ran
while a workflow was mid-run (orphaned dispatch loop), and terminal
rows leaked from session to session in the pill / dialog /
`/workflows` list.

Added `hasRunningEntries()`, `reset()` (drops entries, no controller
touch), `abortAll()` (cancels every running entry + aborts its
controller). Wired into both `backgroundWorkUtils` helpers + updated
their tests for the 4-sibling shape.

## 3. Phase dedup inconsistency — sandbox vs registry (Critical)

`phase('X'); phase('X')` previously yielded `outcome.phases = ['X','X']`
(sandbox `safePhase` unconditional push) but `entry.phases = ['X']`
(registry `onPhaseStarted` collapsed). The same run showed different
phase counts in the terminal `returnDisplay` JSON vs the live UI.
The `agent({phase})` wrapper already deduped (`__b.lastPhase()`); my
docstring on `safePhase` claimed it deduped too, but it didn't.

Fix at the sandbox layer (single source of truth): `safePhase` skips
when `phases[last] === t`. Registry-side dedup is now redundant but
harmless (defense in depth, doesn't double-collapse). Updated test
in workflow-sandbox.test.ts pins
`phase('X'); phase('X'); phase('Y'); phase('X')` → `['X','Y','X']`.

## 4. /workflows tip pointed at non-functional path (UX/docs)

`/workflows` tip text said *"focus the Background tasks pill in the
footer (use ↓ from an empty composer) and press Enter for the
interactive dialog with phase tree + live updates."* But
`setPillFocused(true)` doesn't exist anywhere in the codebase
(confirmed by wenshao's grep, and reproducible on my own tmux runs
where `↓ Enter` never opened the dialog — I previously misattributed
to a tmux limitation). The dialog IS reachable through other paths
but the tip's specific instructions are wrong.

Soften the tip to point at the actually-working text-mode detail
view: `Tip: use /workflows <runId> for the per-run detail view
(name, description, phase tree, recent logs).` — same information,
working instructions.

## 5. `runId` validation comment was aspirational (docs)

Comment on `workflow-orchestrator.ts` `run()` claimed *"validates
the shape (`wf_<hex>`)"* but the code is `const runId = req.runId
?? generateRunId();` — no validation. Caller (`WorkflowTool`) does
use the same `wf_<8hex>` generator as `generateRunId()`, so the
behavior is safe in practice. Fixed the comment to describe what
the code actually does (trusts the caller, no validation).

## 6. Duplicate extractAndStripMeta test (test hygiene)

Two tests at workflow-sandbox.test.ts:227 and :242 used identical
source `{ name: args.x, description: 'd' }` — copilot R1 originally
flagged this, I declined as bot finding, wenshao re-confirmed. The
intent was to pin two distinct things: (a) generic unknown
identifier throws, (b) the bridge global `args` specifically is
not reachable. Updated the first test to use `totallyUnknown` (a
genuine unknown name) and kept the second as the explicit `args`
regression — now the two tests pin different things.

## Verification

- 231/231 core workflow tests pass (registry + sandbox + orchestrator
  + tool) — +6 new from R7 RED-first regression tests
- 81/81 CLI ripple tests pass (pill + dialog + hook + command +
  backgroundWorkUtils)
- tsc 0 errors on core + cli (after rebuilding core dist for the
  new registry methods)
- prettier + eslint clean on all touched files
- **tmux re-verified end-to-end** against qwen3-coder-plus via
  DashScope on a fresh `npm run bundle`:
  - Phase dedup: `phase("Phase A"); phase("Phase A")` →
    `outcome.phases = ["Phase A", "Phase B"]` (was `["Phase A",
    "Phase A", "Phase B"]` pre-fix) — confirmed both in the tool
    result JSON AND in `/workflows wf_xxx · 2 phases`
  - New `/workflows` tip text rendered as expected, no more
    advertising broken pill focus path
  - `/workflows <runId>` detail dump still works: name, status,
    runtime, phases tree, agent counts

Author: LaZzyMan

packages/cli/src/ui/commands/workflowsCommand.ts

@@ -151,7 +151,7 @@ export const workflowsCommand: SlashCommand = {
     if (context.executionMode === 'interactive') {
       lines.push(
         t(
-          'Tip: focus the Background tasks pill in the footer (use ↓ from an empty composer) and press Enter for the interactive dialog with phase tree + live updates.',
+          'Tip: use `/workflows <runId>` for the per-run detail view (name, description, phase tree, recent logs).',
         ),
         '',
       );

packages/cli/src/ui/utils/backgroundWorkUtils.test.ts

@@ -15,6 +15,7 @@ function createMockConfig(overrides?: {
   hasUnfinalizedTasks?: boolean;
   runningMonitors?: unknown[];
   hasRunningEntries?: boolean;
+  hasRunningWorkflows?: boolean;
 }): Config {
   return {
     getBackgroundTaskRegistry: () => ({
@@ -29,6 +30,10 @@ function createMockConfig(overrides?: {
       hasRunningEntries: () => overrides?.hasRunningEntries ?? false,
       reset: vi.fn(),
     }),
+    getWorkflowRunRegistry: () => ({
+      hasRunningEntries: () => overrides?.hasRunningWorkflows ?? false,
+      reset: vi.fn(),
+    }),
   } as unknown as Config;
 }
 
@@ -59,6 +64,17 @@ describe('hasBlockingBackgroundWork', () => {
     ).toBe(true);
   });
 
+  // R7 (wenshao): workflow registry is the 4th sibling. Without
+  // including it in the OR chain, /clear and session-resume happily
+  // ran while a workflow was mid-run, orphaning the dispatch loop.
+  it('returns true when a workflow is still running', () => {
+    expect(
+      hasBlockingBackgroundWork(
+        createMockConfig({ hasRunningWorkflows: true }),
+      ),
+    ).toBe(true);
+  });
+
   it('short-circuits: does not check monitors or shells when tasks are unfinalized', () => {
     const config = {
       getBackgroundTaskRegistry: () => ({
@@ -96,21 +112,24 @@ describe('hasBlockingBackgroundWork', () => {
 });
 
 describe('resetBackgroundStateForSessionSwitch', () => {
-  it('calls reset on all three registries', () => {
+  it('calls reset on all four registries', () => {
     const resetTasks = vi.fn();
     const resetMonitors = vi.fn();
     const resetShells = vi.fn();
+    const resetWorkflows = vi.fn();
 
     const config = {
       getBackgroundTaskRegistry: () => ({ reset: resetTasks }),
       getMonitorRegistry: () => ({ reset: resetMonitors }),
       getBackgroundShellRegistry: () => ({ reset: resetShells }),
+      getWorkflowRunRegistry: () => ({ reset: resetWorkflows }),
     } as unknown as Config;
 
     resetBackgroundStateForSessionSwitch(config);
 
     expect(resetTasks).toHaveBeenCalledOnce();
     expect(resetMonitors).toHaveBeenCalledOnce();
     expect(resetShells).toHaveBeenCalledOnce();
+    expect(resetWorkflows).toHaveBeenCalledOnce();
   });
 });

packages/cli/src/ui/utils/backgroundWorkUtils.ts

@@ -10,12 +10,21 @@ export function hasBlockingBackgroundWork(config: Config): boolean {
   return (
     config.getBackgroundTaskRegistry().hasUnfinalizedTasks() ||
     config.getMonitorRegistry().getRunning().length > 0 ||
-    config.getBackgroundShellRegistry().hasRunningEntries()
+    config.getBackgroundShellRegistry().hasRunningEntries() ||
+    // R7 (wenshao): the WorkflowRunRegistry is a 4th sibling that the
+    // earlier P4b commit forgot to wire here. Without this OR clause,
+    // /clear and session-resume happily ran while a workflow was
+    // mid-run, orphaning the dispatch loop.
+    config.getWorkflowRunRegistry().hasRunningEntries()
   );
 }
 
 export function resetBackgroundStateForSessionSwitch(config: Config): void {
   config.getBackgroundTaskRegistry().reset();
   config.getMonitorRegistry().reset();
   config.getBackgroundShellRegistry().reset();
+  // R7 (wenshao): symmetric with hasBlockingBackgroundWork — without
+  // this call, terminal workflow rows from the previous session
+  // leaked into the next session's pill / dialog / /workflows list.
+  config.getWorkflowRunRegistry().reset();
 }

packages/core/src/agents/runtime/workflow-orchestrator.ts

@@ -211,12 +211,14 @@ export interface WorkflowRunRequest {
    */
   emitter?: WorkflowOrchestratorEmitter;
   /**
-   * P4b: pre-generated run identifier (shape `wf_<hex>`). Callers that
-   * need the id at register-time (e.g. `WorkflowTool` registering the
-   * run with `WorkflowRunRegistry` before `run()` resolves) must
-   * pre-generate one and pass it here. Omitted by tests and by the
-   * historical contract; orchestrator falls back to its own generator
-   * so existing call sites work unchanged.
+   * P4b: pre-generated run identifier. Callers that need the id at
+   * register-time (e.g. `WorkflowTool` registering the run with
+   * `WorkflowRunRegistry` before `run()` resolves) must pre-generate
+   * one and pass it here. The orchestrator does NOT validate the
+   * shape — it trusts the caller (the production caller uses
+   * `wf_<8hex>` to match `generateRunId()`). Omitted by tests and by
+   * the historical contract; orchestrator falls back to its own
+   * generator so existing call sites work unchanged.
    */
   runId?: string;
 }
@@ -1080,9 +1082,11 @@ export class WorkflowOrchestrator {
     // P4b: callers (`WorkflowTool`) may pre-generate `runId` and pass it
     // in so they can register the run with `WorkflowRunRegistry` BEFORE
     // `run()` resolves — necessary because the registry needs the id at
-    // emit time, not at resolve time. The orchestrator validates the
-    // shape (`wf_<hex>`) but otherwise trusts the caller. When omitted,
-    // the orchestrator generates one as before.
+    // emit time, not at resolve time. The orchestrator trusts the caller
+    // and does not validate the shape — the only production caller
+    // (`WorkflowTool`) uses the same `wf_<8hex>` generator as
+    // `generateRunId()`. When omitted, the orchestrator generates one
+    // as before.
     const runId = req.runId ?? generateRunId();
 
     const maxAgents = resolveMaxAgentsPerRun();

packages/core/src/agents/runtime/workflow-sandbox.test.ts

@@ -223,9 +223,12 @@ describe('extractAndStripMeta', () => {
   // (Object.create(null) prototype), so the model cannot reach host
   // primitives during meta evaluation — even ones that the script-side
   // sandbox normally provides (args, agent, phase, log, parallel,
-  // pipeline). Referencing any of them throws ReferenceError.
-  it('rejects meta that references a name that does not exist in the eval context', () => {
-    const src = `export const meta = { name: args.x, description: 'd' }\nreturn 1`;
+  // pipeline). Referencing any of them throws ReferenceError. Two
+  // shapes pinned: a truly unknown identifier (R7 dedup — was a
+  // duplicate of the bridge-global case below) and explicit `args`
+  // bridge-global access.
+  it('rejects meta that references an unknown identifier', () => {
+    const src = `export const meta = { name: totallyUnknown, description: 'd' }\nreturn 1`;
     expect(() => extractAndStripMeta(src)).toThrow(
       /failed to evaluate meta object literal/,
     );
@@ -321,6 +324,24 @@ describe('extractAndStripMeta', () => {
     );
   });
 
+  // P4 Round 7 (wenshao): `phase('X'); phase('X')` previously yielded
+  // `outcome.phases = ['X','X']` (sandbox unconditional push) while the
+  // registry's onPhaseStarted deduped to `entry.phases = ['X']`. The
+  // two arrays diverged on the same run — terminal display vs live UI
+  // showed different phase lists. Fix at the sandbox layer so the
+  // sandbox is the single source of truth; the docstring on safePhase
+  // / phase() can then promise dedup without lying.
+  it('consecutive identical phase titles dedup at the sandbox layer', async () => {
+    const sandbox = createWorkflowSandbox({
+      args: undefined,
+      dispatch: async () => 'ignored',
+    });
+    await sandbox.run(
+      `phase('X'); phase('X'); phase('Y'); phase('X'); return 1`,
+    );
+    expect(sandbox.getPhases()).toEqual(['X', 'Y', 'X']);
+  });
+
   // P4 Round 4 (wenshao): the R3 thenable walker recursed without a
   // seen-guard. A meta literal that builds a cyclic object via spread
   // (no getters, no Promises, no exotic constructs — just self-reference)

packages/core/src/agents/runtime/workflow-sandbox.ts

@@ -624,6 +624,17 @@ export function createWorkflowSandbox(opts: SandboxOptions): WorkflowSandbox {
   const safePhase = (title: string): void => {
     if (phases.length < MAX_PHASE_ENTRIES) {
       const t = String(title);
+      // R7 (wenshao): collapse consecutive identical titles so the
+      // sandbox is the single source of truth for the phase list.
+      // Without this, `outcome.phases` (terminal `returnDisplay` JSON)
+      // carried duplicates while `entry.phases` on the registry
+      // (live UI / `/workflows` detail) was deduped by the registry's
+      // own `onPhaseStarted` collapse — the same run showed different
+      // phase counts in the terminal output vs the live UI. The
+      // `agent({phase})` wrapper already dedups (see the `__b.lastPhase()`
+      // check); this brings the bare `phase()` global into the same
+      // contract.
+      if (phases[phases.length - 1] === t) return;
       phases.push(t);
       // P4b: emit to host-side subscriber. Same defensive try/catch as
       // safeLog — subscriber errors must not bubble into the script.

packages/core/src/agents/workflow-run-registry.test.ts

@@ -190,4 +190,86 @@ describe('WorkflowRunRegistry', () => {
     // Must not throw.
     expect(() => r.complete('wf_throw', null, 1)).not.toThrow();
   });
+
+  // P4 Round 7 (wenshao): dialog-initiated cancel marks status='cancelled'
+  // synchronously, then the abort propagates to the tool's catch arm which
+  // calls setRecentLogs(runId, logs). The previous guard rejected this
+  // because status !== 'running', so cancelled workflows showed an empty
+  // Logs section in the dialog. The fix allows setRecentLogs after the
+  // 'cancelled' transition — Ctrl+C (signal.aborted at execute()'s top
+  // before the dialog touches the registry) is unchanged, and the
+  // unchanged guard still rejects logs arriving after 'completed' or
+  // 'failed' (those terminal states are final).
+  it('setRecentLogs after a cancel transition still writes (dialog-initiated)', () => {
+    const r = new WorkflowRunRegistry();
+    r.register(reg('wf_late_logs'));
+    r.cancel('wf_late_logs', 5_000);
+    r.setRecentLogs('wf_late_logs', ['line1', 'line2']);
+    const e = r.get('wf_late_logs')!;
+    expect(e.recentLogs).toEqual(['line1', 'line2']);
+    expect(e.status).toBe('cancelled');
+  });
+
+  it('setRecentLogs after complete/fail is rejected (terminal states are final)', () => {
+    const r = new WorkflowRunRegistry();
+    r.register(reg('wf_done'));
+    r.complete('wf_done', null, 1_000);
+    r.setRecentLogs('wf_done', ['too late']);
+    expect(r.get('wf_done')!.recentLogs).toEqual([]);
+
+    r.register(reg('wf_fail'));
+    r.fail('wf_fail', 'boom', 2_000);
+    r.setRecentLogs('wf_fail', ['too late']);
+    expect(r.get('wf_fail')!.recentLogs).toEqual([]);
+  });
+
+  // P4 Round 7 (wenshao): WorkflowRunRegistry must expose reset() and
+  // abortAll() to match its three sibling registries (agent, shell,
+  // monitor). Without these, /clear and session-resume leak prior-
+  // session workflow state into the next session — pill / dialog /
+  // /workflows listing all show stale rows, and in-flight workflows
+  // keep executing after the user cleared the session.
+  it('reset() drops every entry without aborting controllers', () => {
+    const r = new WorkflowRunRegistry();
+    const ac1 = new AbortController();
+    r.register(reg('wf_1', { abortController: ac1 }));
+    r.register(reg('wf_2'));
+    r.complete('wf_2', null, 1_000);
+    expect(r.list()).toHaveLength(2);
+    r.reset();
+    expect(r.list()).toEqual([]);
+    // Sibling shell registry's reset() does NOT touch processes — same
+    // contract here: reset just drops in-memory entries; abortAll() is
+    // the controller-aborting path.
+    expect(ac1.signal.aborted).toBe(false);
+  });
+
+  it('abortAll() aborts every running entry and marks them cancelled', () => {
+    const r = new WorkflowRunRegistry();
+    const ac1 = new AbortController();
+    const ac2 = new AbortController();
+    const acDone = new AbortController();
+    r.register(reg('wf_run1', { abortController: ac1 }));
+    r.register(reg('wf_run2', { abortController: ac2 }));
+    r.register(reg('wf_done', { abortController: acDone }));
+    r.complete('wf_done', null, 1_000);
+    r.abortAll();
+    expect(ac1.signal.aborted).toBe(true);
+    expect(ac2.signal.aborted).toBe(true);
+    // Already-terminal entry's controller is NOT re-aborted (no-op for
+    // settled entries).
+    expect(acDone.signal.aborted).toBe(false);
+    expect(r.get('wf_run1')!.status).toBe('cancelled');
+    expect(r.get('wf_run2')!.status).toBe('cancelled');
+    expect(r.get('wf_done')!.status).toBe('completed');
+  });
+
+  it('hasRunningEntries() reflects the running subset', () => {
+    const r = new WorkflowRunRegistry();
+    expect(r.hasRunningEntries()).toBe(false);
+    r.register(reg('wf_1'));
+    expect(r.hasRunningEntries()).toBe(true);
+    r.complete('wf_1', null, 1_000);
+    expect(r.hasRunningEntries()).toBe(false);
+  });
 });

packages/core/src/agents/workflow-run-registry.ts

@@ -202,10 +202,19 @@ export class WorkflowRunRegistry {
    * `getLogs()` array; we mirror it here for the UI so the dialog
    * doesn't have to thread a sandbox reference. Capped at 100 entries
    * (the tail) so a chatty workflow doesn't bloat the registry.
+   *
+   * R7 (wenshao): allowed after a `'cancelled'` transition too. The
+   * dialog-initiated cancel path calls `registry.cancel()` first
+   * (status flips to `'cancelled'` synchronously), then the abort
+   * propagates to the tool's catch arm which calls `setRecentLogs`.
+   * Without this, dialog-cancelled runs always showed an empty Logs
+   * section. `'completed'` / `'failed'` are still rejected — those
+   * terminal states ARE final (no late-arriving logs to absorb).
    */
   setRecentLogs(runId: string, logs: readonly string[]): void {
     const entry = this.entries.get(runId);
-    if (!entry || entry.status !== 'running') return;
+    if (!entry) return;
+    if (entry.status !== 'running' && entry.status !== 'cancelled') return;
     const tail = logs.length > 100 ? logs.slice(-100) : Array.from(logs);
     entry.recentLogs = tail;
     this.emitStatusChange(entry);
@@ -262,6 +271,75 @@ export class WorkflowRunRegistry {
     return Array.from(this.entries.values());
   }
 
+  /**
+   * R7 (wenshao): true if any entry is still `'running'`. Mirrors the
+   * three sibling registries' `hasUnfinalizedTasks()` /
+   * `hasRunningEntries()` / `getRunning().length > 0` so the unified
+   * `hasBlockingBackgroundWork()` helper (the gate `/clear` and session-
+   * resume both use to refuse a switch with live work) can count
+   * workflow runs the same way.
+   */
+  hasRunningEntries(): boolean {
+    for (const entry of this.entries.values()) {
+      if (entry.status === 'running') return true;
+    }
+    return false;
+  }
+
+  /**
+   * R7 (wenshao): drop every in-memory entry without touching
+   * controllers. Mirrors `BackgroundShellRegistry.reset()` and the
+   * other siblings' contract — callers (`/clear`, session-resume)
+   * MUST verify via `hasRunningEntries()` first that no still-running
+   * work exists before invoking. The companion path that aborts
+   * controllers is `abortAll()`.
+   */
+  reset(): void {
+    if (this.entries.size === 0) return;
+    // Snapshot a sample entry for the statusChange callback so a single
+    // subscriber notify is enough — the only consumer
+    // (`useBackgroundTaskView`) ignores the entry arg and re-pulls
+    // `list()` on every fire.
+    const sample = this.entries.values().next().value as
+      | WorkflowTask
+      | undefined;
+    this.entries.clear();
+    if (sample) this.emitStatusChange(sample);
+  }
+
+  /**
+   * R7 (wenshao): cancel every still-running entry. Called on session/
+   * Config shutdown so workflow runs don't outlive the CLI process and
+   * leak orphaned dispatches. Symmetric with `BackgroundShellRegistry.
+   * abortAll()` and `BackgroundTaskRegistry.abortAll()`.
+   *
+   * Settles each entry inline (status → 'cancelled', abort the
+   * controller) and fires the status-change callback exactly once
+   * after the loop — the per-entry `cancel()` path would have fired
+   * the callback for every running entry, wasteful on shutdown.
+   */
+  abortAll(): void {
+    const endTime = Date.now();
+    let lastCancelled: WorkflowTask | undefined;
+    for (const entry of Array.from(this.entries.values())) {
+      if (entry.status !== 'running') continue;
+      entry.status = 'cancelled';
+      entry.endTime = endTime;
+      entry.notified = true;
+      try {
+        entry.abortController.abort();
+      } catch (error) {
+        debugLogger.error(
+          'abortAll: failed to abort workflow controller:',
+          error,
+        );
+      }
+      lastCancelled = entry;
+    }
+    if (lastCancelled) this.emitStatusChange(lastCancelled);
+    this.evictTerminal();
+  }
+
   /**
    * Sweep terminal entries when they exceed `MAX_RETAINED_TERMINAL_WORKFLOWS`.
    * Running entries are always retained. Oldest terminal entries