fix(workflows): harden TUI input, stage chat interrupts, and executor options

[lavaman131]

Summary

Hardens workflow TUI input handling and run-control semantics across picker shortcuts, stage chat interrupt/resume, and executor execution modes. Also expands workflow runner to support direct task/chain/parallel runs and adds package-level workflow resource discovery.

Changes

TUI / Picker

• Replaces ctrl+s run shortcut with terminal-safe ctrl+enter in the inputs picker and updates all shortcut hint labels
• Adds grapheme-aware text editing (cursor movement, deletion, scrolling) to the inputs picker for correct handling of multi-byte and wide characters
• Improves Escape/Ctrl+C cancellation and responsive shortcut hints in picker and form views

Stage Chat Interrupt & Resume

• Escape key now mirrors the main coding-agent chat interrupt path for live stages: requests a controlled pause while keeping the composer active
• Paused stages resume by typing a follow-up and pressing Enter (handle.resume(text))
• Settled stages with an active live handle remain a full chat session (Enter calls handle.prompt(text)); Escape interrupts active post-stage responses without mutating workflow dependencies
• Separates resumable interrupt semantics from destructive kill behavior for workflow run control

Executor & Workflow Runner

• Extends ctx.chain, ctx.task, and ctx.parallel to forward output options (output, outputMode, reads, worktree, maxOutput, artifacts) and file-based artifact handling
• Adds chainDir as a shared artifact directory for relative reads and outputs in chain runs
• Adds concurrency and failFast controls to parallel execution modes
• Expands WorkflowDefinition with direct task/tasks/chain modes, so callers can invoke named workflows or ad-hoc task definitions through a single unified interface
• Forwards concurrency from WorkflowDefinition into executor config

Package & Discovery

• Adds workflows as a first-class resource type in the package manager (auto-discovery, manifest parsing, file-pattern matching for .ts/.js/.mjs/.cjs)
• Reads manifests from both the Atomic app-name key and the legacy pi key for backwards compatibility
• Exposes getWorkflowResources() on the extension API; passes resolved workflow resources through the extension loader chain

Extension Typings (coding-agent)

• Adds getFooterDataProvider() to ExtensionUIContext so embedded extension UIs can reuse the core footer
• Tightens model type from Model<any> to Model<Api> on ExtensionContext
• Tightens ToolRenderContext and ToolDefinition generics from any to unknown; applies bivariance hack to render function signatures to preserve assignability
• Removes unused progress field from WorkflowTaskSessionFields and WorkflowDirectOptions

Tests & Docs

• Adds 12+ new test files covering executor output options, stage chat view, workflow runner, workflow schema, run-detail render, session-confirm list, status list render, store terminal guard, store widget installer, workflow attach pane, workflow list render, and runtime tunables
• Updates existing tests for picker input, wiring adapters, overlay graph, slash dispatch, stage runner, and extension loader
• Refreshes coding-agent docs and UI shortcut labels (README, extensions.md, packages.md, sessions.md, tui.md, etc.)

Notes

• No package-versioned breaking change is declared.
• Workflow interrupt now represents resumable pause semantics; destructive removal is handled by kill.
• packages/workflows ships raw TypeScript — no build step introduced.

[claude]

Review — fix(workflows): harden picker keys and run control

Reviewed the substantive code changes (skipped the Ctrl→CTRL doc churn). Overall this is a solid hardening pass — clean interrupt/kill split, useful type cleanup, and the giant packages/subagents/src/tui/render.ts diff is a mechanical helper-extraction/reformat, not a logic shuffle (low review risk). A few things worth a second look before merge.

Bugs / correctness

1. resolveToolStageTarget silently picks first prefix match — packages/workflows/src/extension/index.ts:945

const stage = run?.stages.find(
  (s) => s.id === target || s.id.startsWith(target) || s.name === target,
);

resolveRunIdPrefix / ambiguousRunMessage (lines 957–961) correctly surface ambiguous run-id prefixes, but the stage variant doesn't. Two stages with IDs like stage-abc and stage-abcd make stage-ab resolve to whichever is first in run.stages. For stage-scoped resume that's a footgun. Worth mirroring the ambiguity-detection pattern used for runs.

2. session_shutdown reason guard fails open — packages/workflows/src/extension/index.ts:~689

The new handler only calls killAllRuns when reason === "quit", gated by typeof event === "object" && event !== null && "reason" in event. If pi ever changes the event shape (or emits a partial payload), in-flight runs leak silently. Safer to default to kill on shutdown and require an explicit non-quit reason to skip.

3. Duplicated manifest parsing — packages/coding-agent/src/core/package-manager.ts:523 ↔ packages/coding-agent/src/core/extensions/loader.ts:483

Both files independently define PiManifest and a near-identical getManifestFromPackageJson / manifestFromPackageJson. The two implementations agree today, but the next bugfix to one will almost certainly forget the other. Extract to a single shared helper (probably in package-manager.ts) and import from loader.ts.

Code quality

4. formatHintLabel is a no-op — packages/coding-agent/src/modes/interactive/components/keybinding-hints.ts:80

function formatHintLabel(description: string): string {
  return description;
}

Either it's an unfinished stub for description normalization (paired with the new MODIFIER_LABELS/SPECIAL_KEY_LABELS for keys), or it's dead indirection. If intentional, drop it or add a // TODO: so future readers know it's a seam. The @deprecated capitalize field on KeyTextFormatOptions is also accepted-but-ignored — consider deleting both at the same time.

5. Type.Unsafe<T>({}) disables runtime validation at the tool boundary — packages/workflows/src/extension/workflow-schema.ts:7-11, 43-48

SdkSessionOptionSchema and SdkSessionOptionArrayElementSchema return empty Type.Unsafe<...>({}). The schema gives you a precise TS type via Static<>, but at runtime the tool dispatcher will accept any shape for model, authStorage, modelRegistry, noTools, etc. This is probably a deliberate trade-off to inherit types from @bastani/atomic without duplicating the schema — if so, a brief comment at the helper site explaining that runtime validation is delegated to the SDK would prevent confusion. Otherwise, add minimal predicates for the highest-risk fields (model, noTools).

6. reads is forwarded without normalization — packages/workflows/src/extension/workflow-schema.ts:67, runtime extraction in runtime.ts:177

reads: Type.Optional(Type.Union([Type.Array(Type.String()), Type.Literal(false)])) accepts arbitrary string paths and the runtime threads them through without resolve() / workspace-boundary checks. Worth a guard — at minimum normalize and refuse .. traversals — since these arrive from workflow definitions or tool-call args.

7. workflowRunCreatesStage regex check — packages/workflows/src/extension/discovery.ts:~163

Using Function.prototype.toString() + regex to detect whether a workflow run calls stage|task|chain|parallel is fragile (minified/transpiled output, conditional calls, dynamically-resolved methods all evade it). Consider an explicit declarative marker on defineWorkflow rather than introspecting the source.

Test coverage

The new extensions-discovery.test.ts cases cover (a) discovery via the new atomic manifest key and (b) the legacy pi shim still working. Missing the precedence case: what happens when a package.json defines both atomic and pi? The implementation prefers atomic, but there's no regression guard, and that's the exact place a future refactor of getManifestFromPackageJson will silently flip behavior.

Positives

• interrupt vs kill semantics are cleanly separated, with symmetric interruptAllRuns / destroyAllRuns and stage scoping. The wiring of onKillRun through persistenceRef.current is initialized synchronously before the overlay is built, so no race on first dispatch.
• The noAskQuestionToolSet = ["read, bash, edit, write, todo"] → ["read", "bash", "edit", "write", "todo"] fix in builtin/deep-research-codebase.ts and builtin/ralph.ts is a real pre-existing bug (a 5-tool list being passed as a single comma-joined tool name). Good catch.
• Wide-reaching any → Api/unknown cleanup across ToolDefinition, Model, agent-session*, tool-execution.ts. Matches the CLAUDE.md no-any rule.
• subagents/src/tui/render.ts (+1844/-1062) is helper extraction + tab/space reformatting, not a behavioral rewrite — the export surface and state machine are preserved.
• WorkingStatusComponent is correctly consumed by packages/workflows/src/tui/stage-chat-view.ts:910 — re-exporting it from the SDK is intentional, not orphaned.

Performance & security

Nothing notable on perf (the per-render formatKeyText/formatKeyPart work is cheap). Security-wise, the only items are the unvalidated reads paths (#6) and the disabled boundary validation in Type.Unsafe schemas (#5). No arbitrary-code-execution paths introduced — package discovery still requires explicit manifest entries.

---

Nothing here is a blocker IMO; #1, #2, and #3 are the items I'd most want addressed before merging.

[claude]

Code Review — PR #961

Solid, large-scoped PR. The interrupt (resumable pause) vs kill (destroy) split, the Model<any> → Model<Api> tightening, and the new package-level workflow discovery are good directional moves. Test coverage is strong (12+ new test files exercise the new surfaces). A few items worth a look before merge.

Correctness / potential bugs

workflowRunCreatesStage static-regex guard is too strict — packages/workflows/src/extension/discovery.ts:148-156

const source = Function.prototype.toString.call(run);
return /\.\s*(?:stage|task|chain|parallel)\s*\(/.test(source);

This will reject perfectly valid workflows that don't use a literal .stage(/.task(/.chain(/.parallel( call, e.g.:

• const { stage } = ctx; await stage(...)
• ctx['stage'](...) (bracket access — possible after refactors)
• A helper: async function runStages(ctx) { … } invoked from run
• Bundled/minified extensions where the property may have been mangled

Consider downgrading this to a discovery-time warning diagnostic, or moving the empty-graph check to be evaluated after the run executes (where you actually have ground truth). The current rejection message says "graph is empty (cachedLayout.length === 0)" — that condition can be detected post-run without a regex on user source.

interrupt vs kill is a semantic breaking change — packages/workflows/src/extension/index.ts:616+

/workflow interrupt previously did what kill now does (destroy and drop from history). Anyone scripting against the old semantics will silently get the new pause-resume behavior. The PR description claims "No package-versioned breaking change is declared." For a published package, this is worth a BREAKING CHANGE: line in packages/coding-agent/CHANGELOG.md even without a major bump, and consider whether interrupt should print a deprecation hint pointing to kill for one or two releases.

session_shutdown reason gating — packages/workflows/src/extension/index.ts:2485+

if (reason === "quit") {
  killAllRuns({ ... });
}

This silently does nothing for any other reason value (or when event is missing the property entirely). If the host SDK emits reason: undefined on a real quit path (e.g. older pi versions, or an unhandled shutdown path), workflows will leak. Worth either: (a) defaulting to "behave as quit" when reason is missing, or (b) at minimum logging a debug message so silent leaks are visible.

UX nits in user-facing strings

In packages/coding-agent/src/modes/interactive/interactive-mode.ts and the workflow command labels, several hints were converted from English sentences to telegraphic key-action pairs in a way that reads awkwardly:

• "A bash command is already running. Press Esc to cancel it first." → "A bash command is already running. esc cancel first." — ungrammatical.
• \${keyText("app.interrupt")} to interrupt`→`${keyText("app.interrupt")} Interrupt` — lowercase key followed by capitalized verb (esc Cancel, ctrl+c Interrupt`) looks like a typo or unfinished sentence.
• Pi widget hint: Attached to … Press "h" or ctrl+d to hide, "q" to interrupt, esc to close. → … h/ctrl+d hide · q kill · esc close. — fine as a help bar, but inconsistent with the chat surfaces that still use full sentences.

Pick one style. The middot-separated key-action list (h hide · q kill · esc close) is great in a one-line footer; the full-sentence form is better inside prose error messages. Mixing them feels half-finished.

Dead / inconsistent code

packages/coding-agent/src/modes/interactive/components/keybinding-hints.ts:80-82 introduces a formatHintLabel helper that is just description => description. Either implement the normalization the name suggests (e.g. lowercase the description so Cancel/Interrupt become consistent with the key labels) or inline the value and delete the helper.

Same file: KeyTextFormatOptions.capitalize is now @deprecated and ignored, but keyDisplayText still passes { capitalize: true }. Either delete the field entirely (no consumers rely on it now that the formatter ignores it) or remove the call site.

Type tightening — downstream impact

Model<any> → Model<Api> and ToolRenderContext<any, any> → ToolRenderContext<unknown, unknown> in packages/coding-agent/src/core/extensions/types.ts is the right move, but it is a soft breaking change for extension authors who consume @bastani/atomic types. Confirm CHANGELOG mentions it so authors aren't surprised when their extension builds fail on type errors after upgrading.

The bivariance hack on renderCall/renderResult is a reasonable escape hatch; consider a one-line comment on the hack explaining why it's needed (preserving assignability when a definition has narrower TParams than the registry's TSchema).

Package-manager / config

packages/coding-agent/src/core/package-manager.ts and loader.ts both read manifests via pkg[APP_NAME] then fall back to pkg.pi. APP_NAME defaults to "pi" per config.ts:441, so when atomic isn't configured (or the resolution fails), the same key is read twice. Not a correctness bug, but consider short-circuiting when APP_NAME === "pi" so the fallback isn't run redundantly, or move the dedup into getManifestFromPackageJson/manifestFromPackageJson. Also note these two helpers are duplicated across package-manager.ts and extensions/loader.ts — worth a shared util.

FILE_PATTERNS.workflows: /\.(ts|js|mjs|cjs)$/ accepts ESM/CJS variants while extensions: /\.(ts|js)$/ does not. Intentional per the discovery story, but worth a one-line comment so a future reader doesn't "unify" them.

Resource loader filter

In packages/coding-agent/src/core/resource-loader.ts:413-422:

const workflowResources = [
  ...cliEnabledWorkflowResources,
  ...enabledWorkflowResources,
  ...builtinEnabledWorkflowResources,
].filter((resource) => resource.metadata.origin === \"package\");

The filter drops everything that isn't origin === \"package\", which is exactly the desired routing (user-global workflows are picked up by discoverWorkflows() itself). This is fine — but a one-line comment explaining the intent would save future-you from "why is this filter dropping user workflows?".

Tests

Coverage looks good across the new surfaces (executor output options, stage-chat resumes, workflow runner direct modes, workflow-list/run-detail/status-list renders, store-widget installer). One small gap: I don't see a direct test for the new workflowRunCreatesStage validator. Given the regex's fragility (above), a test asserting it accepts both ctx.stage(...) and destructured/aliased forms would be a useful regression fence.

---

Overall the structural moves are good and the test investment is real. The biggest risks I'd want addressed before merge are (1) the regex validator's false-negatives and (2) the silent interrupt semantics flip — both are easy fixes.

🤖 Generated with Claude Code

[claude]

Code Review — PR #961

I focused on the workflow extension, package-manager wiring, the TUI input/stage-chat paths, and the type tightening. Findings are grouped by severity; line numbers reference the diff hunks at the head of each file.

Bugs / correctness

1. session_shutdown reason guard is brittle. packages/workflows/src/extension/index.ts (session_shutdown handler near hunk @@ -2206,17 +2485,22) now teardowns workflows only when event.reason === "quit". The pi host can emit session_shutdown for app exit with other reason strings (or none at all), and the previous behavior was to always kill on shutdown. With the new guard, real app exits whose reason doesn't match "quit" will leak in-flight workflow subprocesses with no UI to surface them. Recommend either an allowlist of known shutdown reasons OR a fall-through that still kills when no other handler claims teardown. The comment promises "Only application exit owns workflow teardown" but the implementation also excludes most application exits.

2. deep-research-codebase.ts ships a silent behavior change. let noAskQuestionToolSet = [\"read, bash, edit, write, todo\"]; → [\"read\", \"bash\", \"edit\", \"write\", \"todo\"]. The old form was a single-element array whose only string had no matching tool — effectively no tools were allowlisted. The fix is correct, but this is a runtime behavior change (deep-research stages now see five real tools instead of zero) and it is not mentioned in the PR description or CHANGELOG. Please call this out so anyone with workflows that depended on the prior empty behavior knows.

3. Stage prefix resolution silently picks the first match. resolveToolStageTarget (index.ts ~line 800) walks stages and accepts the first that satisfies s.id === target || s.id.startsWith(target) || s.name === target. Unlike resolveRunIdPrefix, it never reports ambiguity, so two stages whose ids share a prefix will silently route to whichever comes first in run.stages. Recommend mirroring the run-prefix collector pattern and surfacing an ambiguous result.

4. task / tasks / chain are unconstrained peers in the schema. workflow-schema.ts declares all three as Type.Optional siblings on WorkflowParametersSchema with no oneOf constraint. A caller passing { task: \"...\", tasks: [...] } will silently take whichever branch the dispatcher checks first. Add a schema-level mutual-exclusion or have the dispatcher error on ambiguous input — silent precedence is a footgun.

5. workflowRunCreatesStage rejects legitimate workflows. discovery.ts:148-155 regex-checks Function.prototype.toString for \\.\\s*(?:stage|task|chain|parallel)\\s*\\(. This is fooled by perfectly valid destructuring (const { stage } = ctx; stage(...)), dynamic dispatch (ctx[\"stage\"](...)), or methods called through aliased references. Should be a discovery warning, not a hard rejection, given how easy it is to write a no-stage-looking-stage workflow that actually does call stage(...) via an alias.

6. kill confirmation now prompts for already-ended runs. index.ts ~line 1620: the previous guard was if (!yes && run && run.endedAt === undefined && ctx.ui); the new guard is if (!yes && run && ctx.ui). Since kill (destroy) removes the run from history/status, prompting on an already-ended run is arguably correct — but it's a UX change worth confirming was intentional.

7. PR description claims handle.interrupt() / handle.prompt(text) paths that don't exist in the diff. stage-chat-view.ts actually calls handle.pause() on Escape (gated by _canPause()) and handle.resume(message) on Enter; there is no handle.interrupt() call and no handle.prompt(text) call site introduced or modified. The behavior matches the broader "interrupt = resumable pause" semantics, but the PR body should be reworded so reviewers (and future spelunkers) aren't misled.

Smells / maintainability

8. /workflow interrupt and /workflow kill share ~150 lines of near-duplicate logic. index.ts hunks @@ -1326,6 +1504,87 and the following kill block differ only in verb (interruptRun vs destroyRun, message strings, confirmation copy). Extract a shared runRunControlAction(verb, runResolver, registry, ...) helper before this drifts.

9. Non-exhaustive ladders on PauseResult.reason. Both the tool handler (index.ts ~line 690) and the slash-command handler (~line 1640) use chained ternaries to map result.reason to a message. Adding a new reason to PauseResult will silently fall through. Use switch (result.reason) with assertNever(_) so additions break the build.

10. PiSdkSettingsManager exported as empty interface. wiring.ts:90: export interface PiSdkSettingsManager {}. Empty interfaces are structurally unknown-equivalent — any object satisfies them, defeating the purpose of typing the SDK seam. If the contract genuinely has no required methods today, consider either documenting why with a comment or making it a branded type so accidental wrong-shape assignments still error.

11. KillResult collapses into InterruptResult. render-result.ts:202-206: identical shape, distinguished only by the literal action. The as KillResult cast is harmless but redundant — switch (result.action) already narrows. Either tighten the discriminant (e.g. include a kill-specific field) or drop the cast.

12. workflows resource type uses different file patterns than other resources. package-manager.ts:48: workflows: /\\.(ts|js|mjs|cjs)$/ vs extensions: /\\.(ts|js)$/. The reason (workflows are loaded via jiti which accepts more formats) is reasonable but not documented in code. Add a one-line comment so future contributors don't "unify" the patterns.

13. workflows falls back to convention dirs even when manifest is present. package-manager.ts addPackageResources (~line 2025): when the manifest exists, all other resource types are treated as authoritative (no convention-dir fallback); workflows alone fall through to collectDefaultResources. This is intentional (workflows are new and packages may not declare them yet) but the inconsistency is silent. Worth a comment or, better, applying the same fallback to all resource types.

14. config.ts APP_NAME / CONFIG_DIR_NAME derivation is now subtle. The dynamic resolution from pkg.name → packageAppName → ${appName}Config → piConfig is correct, but the combined fallback appConfig?.configDir || (APP_NAME === \"pi\" ? \".pi\" : \\.${APP_NAME}\) is hard to reason about. A user who creates an atomicConfig block without a configDir will get .atomic instead of inheriting .pi. This is fine, but it deserves a CHANGELOG note since it affects credential/settings discovery.

Nits

15. UsageMeterComponent is now re-exported from components/index.ts but its declaration isn't shown in this PR — confirm it actually exists in footer.ts (otherwise this will be a build break for downstream importers).

16. KeyTextFormatOptions.capitalize is @deprecated but still part of the public type. If no caller passes it after this change, consider just removing the option in the same PR — leaving deprecated surface to clean up later tends to rot.

17. interruptRun is a one-line alias for pauseRun. Mirroring it as a separate export increases vocabulary without behavioral change. If the rename is meant to be permanent, migrate callsites and remove pauseRun; otherwise consider documenting why both verbs are exposed.

Type tightening — positive notes

The Model<any> → Model<Api> and ToolDefinition<…, any> → ToolDefinition<…, unknown> tightening throughout extensions/types.ts, runner.ts, tool-execution.ts, tool-definition-wrapper.ts, and tools/grep.ts is the right direction and aligns with CLAUDE.md ("avoid any and unknown ambiguous types"). The bivarianceHack for renderCall / renderResult is a known TS pattern to preserve method-style variance on function-property fields — fine as-is, but worth a one-line comment explaining why the indirection exists (someone will try to inline it).

Test coverage

37 test files touched, including new coverage for the new surfaces (workflow-runner, workflow-schema, stage-chat-view, run-detail, session-confirm, status-list, store-widget-installer, workflow-attach-pane, workflow-list-render, inputs-picker). Coverage looks proportional to the change. Two gaps worth considering:

• I didn't see explicit tests for the session_shutdown reason guard (finding add agent instructions #1). Worth a test that fires session_shutdown with reason: undefined and asserts subprocess teardown still happens for whatever the intended app-exit reasons are.
• The task + tasks + chain mutual-exclusion behavior (finding Flora131/feat/add skills #4) isn't covered. Add a test that passes two of them and asserts the dispatcher either errors or documents the precedence.

---

Overall this is a well-structured refactor and the type tightening is great. The blockers I'd want resolved before merging are #1 (shutdown leak), #2 (the silent tool-list fix should be CHANGELOG'd), and #4 (mutual-exclusion in WorkflowParametersSchema). The rest are quality-of-life improvements that can go in follow-ups.