fix(workflows): prompt for confirmation before quitting with active runs

[flora131]

Summary

Introduces a cancellable session_before_shutdown extension lifecycle event and wires the workflows extension to show a default-cancel confirmation dialog before killing active runs on interactive quit. Also fixes a bug where graph-quit resumable runs were incorrectly counted as active blockers, causing a spurious confirmation on a subsequent `/quit`.

Closes #1378

Changes

Core: cancellable pre-shutdown lifecycle hook

• Added SessionBeforeShutdownEvent / SessionBeforeShutdownResult types and the session_before_shutdown extension event, fired only on interactive quit (Ctrl+C / Ctrl+D / /quit / /exit), not on SIGHUP/SIGTERM signal exits.
• Exported emitSessionBeforeShutdownEvent helper from ExtensionRunner and wired it into InteractiveModeBase.shutdown() so extensions can cancel quit before UI/runtime teardown begins.
• Added shutdownConfirmationPending guard to suppress duplicate pre-shutdown prompts while one is already in progress, with try/finally cleanup so a thrown handler always releases the guard.

Workflows extension: quit confirmation UI

• Added openWorkflowQuitConfirm() overlay showing a "Quit with active workflows?" dialog with in-flight run count, stage progress, oldest run elapsed time, and run names. Cancel is focused by default.
• Added blocksAppShutdown predicate that correctly excludes graph-quit/resumable paused runs (paused + resumable + exitReason=quit + no endedAt) from the active-blocker count — fixing the spurious confirmation bug after graph q.
• Registered a session_before_shutdown handler: skips when no runs are in flight, fails open (undefined) in headless or degraded-UI contexts so automation cannot wedge on a prompt, and cancels quit with an info notification when the user declines.
• openKillConfirm (graph q) now routes through openWorkflowQuitConfirm's shared confirmation path: the graph pane stays open until the kill is confirmed, then closes — no speculative close on q.

Graph view fixes

• GraphView q no longer closes the pane speculatively: fires onQuit for live runs and lets the host close after confirmation; falls through (return false) when there is no live run so the outer overlay can handle q.
• Ctrl+C added as a cancel shortcut in handleKillConfirmInput alongside Escape.

Overlay hardening

• Added observeCustomMount() helper so custom UI hosts that reject or never invoke the factory settle the confirmation promise safely instead of hanging.
• openKillConfirm refactored to use the same settle() guard pattern, eliminating an earlier race where dispose could double-resolve.

Tests

• packages/coding-agent/test/extensions-runner/session-shutdown.suite.ts: cancellation and no-handler cases for emitSessionBeforeShutdownEvent.
• packages/coding-agent/test/suite/regressions/5080-signal-shutdown-extension-cleanup.test.ts: cancelled shutdown clears shutdownRequested, duplicate confirmation suppression, failed handler guard release.
• test/unit/extension.test.ts: quit confirm with active workflows cancels by default; headless / hasUI: false fail-open; confirmed quit path.
• test/unit/extension-shutdown.test.ts: session_before_shutdown does not prompt for graph-quit resumable runs; prompts and allows cancellation for genuinely active runs.
• test/unit/workflow-attach-pane-09.test.ts: q does not close before host confirms kill; q closes/falls through for completed runs.
• test/unit/session-confirm-list.test.ts: renderWorkflowQuitConfirm default-cancel rendering; fail-open when custom UI rejects or never mounts; Ctrl+C cancel variants.
• test/unit/session-overlays.test.ts: quit confirm overlay cancel/confirm flows, notification on cancel.
• test/integration/overlay-entrypoints.test.ts: q on a real custom mount mounts the confirmation overlay first, then kills on y.

Breaking changes

None. The new session_before_shutdown event is opt-in for extensions; existing extension code is unaffected.

Validation

• Targeted Bun lifecycle/workflow/TUI suites passed (273 pass, 0 fail).
• AGENT=1 bun run typecheck passed.
• Pre-push hooks ran bun run lint and bun run test:unit successfully.
• Full manual E2E session against real Atomic TUI — all scenarios passed (see exit-confirmation-manual-test-report.md).

[flora131]

Implementation Notes

Task: compleete issue #1378

Running Notes

• Repository preflight found a Bun monorepo with missing node_modules; ran bun install --frozen-lockfile successfully before implementation.
• Decision: implemented a new cancellable session_before_shutdown lifecycle event instead of coupling the host directly to workflows state. This keeps session_shutdown terminal/non-cancellable and preserves the extension boundary.
• Decision: InteractiveMode.shutdown() emits the new pre-shutdown hook only for non-signal shutdowns. Signal shutdown behavior remains unprompted.
• Decision: workflows handles quit confirmation only for reason === "quit" and only when in-flight workflow runs exist. Non-interactive/no-custom-UI contexts fail open, matching existing workflow session-switch behavior.
• Decision: the primary workflow quit confirmation reuses/generalizes the existing custom kill-confirm UI so the default focused action is Cancel. Generic ui.confirm() is not used as the primary destructive prompt because it defaults to Yes in the interactive host.
• Decision: graph q now delegates a kill request to the host adapter and closes only after confirmed kill; this required a small internal callback-contract adjustment through GraphView and WorkflowAttachPane.
• Test-harness adjustment: packages/coding-agent/test/suite/regressions/5080-signal-shutdown-extension-cleanup.test.ts used synthetic shutdown contexts without session.extensionRunner; updated the harness with a fake no-op extension runner rather than weakening production shutdown code.
• Validation: targeted regression test passed (5 pass), targeted lifecycle/workflow/TUI suites passed (203 pass after rerun), and bun run typecheck passed.
• Transient validation note: one combined targeted run initially failed in an existing before_agent_start system prompt ordering test, but the focused test, full extensions-runner file, and combined targeted suite all passed on rerun without code changes.
• E2E/TUI validation was not run. tmux was available, but there was no deterministic built/authenticated/mock model workflow scenario available to create an in-flight workflow and drive the quit confirmation end-to-end. Narrower lifecycle/component/input tests were used instead.
• Iteration 2 addressed reviewer findings from /tmp/atomic-ralph-run-V5mBYd/review-round-1.json via the updated research file: canceled pre-shutdown now clears shutdownRequested, preventing a canceled extension-requested quit from leaving stale idle-shutdown state.
• Iteration 2 decision: narrowed session_before_shutdown to quit-only in public types/docs/tests rather than documenting non-emitted reasons. SessionShutdownEvent remains unchanged for terminal cleanup reasons.
• Iteration 2 validation: targeted Bun tests passed (209 pass, 0 fail) and bun run typecheck passed. Validator also checked quit-only docs/types/test consistency.
• Iteration 2 E2E/TUI status: not run; tmux exists, but there is no checked-in deterministic TUI/tmux harness or exact fixture to start Atomic, create active workflows, press quit, and assert screen/state.
• Iteration 3 addressed round-2 reviewer findings from the updated research file: added an idempotency guard while session_before_shutdown confirmation is pending, so repeated quit requests do not spawn duplicate prompts. The pending guard is cleared in finally, including cancellation and thrown prompt paths.
• Iteration 3 changed the shared kill/quit confirmation input handler so Ctrl+C variants cancel, matching Escape/n destructive-prompt behavior.
• Iteration 3 refined graph q: live runs delegate kill confirmation and do not close speculatively; no-live-run cases now close when an onClose handler exists or return false to fall through instead of swallowing the key.
• Iteration 3 updated test/integration/overlay-entrypoints.test.ts so the graph q regression path drives the confirm overlay before expecting a killed run.
• Iteration 3 validation: targeted Bun tests passed (265 pass, 0 fail) and bun run typecheck passed.
• Iteration 3 E2E/TUI status: not run; tmux is available, but there is no checked-in deterministic harness/fixture to launch Atomic, create an active workflow, press q/quit, and assert terminal screen/state.
• Iteration 4 addressed review round 3 fail-open hardening: openWorkflowQuitConfirm() now settles undefined when ui.custom throws/rejects or returns/resolves without invoking the component factory, so interactive quit cannot hang on a broken/no-op custom UI host.
• Iteration 4 also hardened openKillConfirm() to settle false on rejecting/no-op custom UI, avoiding stranded graph q kill confirmations.
• Iteration 4 changed workflow session_before_shutdown to fail open when ctx.hasUI === false, even if a ctx.ui.custom function is present; this matches headless/degraded host behavior and avoids wedging automation.
• Iteration 4 tests: added direct overlay confirmation tests in test/unit/session-overlays.test.ts and supplemental fail-open coverage in test/unit/session-confirm-list.test.ts; updated test/unit/extension.test.ts hasUI=false coverage.
• Iteration 4 validation: targeted suite passed (268 pass, 0 fail), direct overlay tests passed (5 pass, 0 fail), and bun run typecheck passed.
• Iteration 4 E2E/TUI status: not run; tmux and expect are installed, but there is no checked-in deterministic TUI/tmux harness/fixture for the quit-confirm scenario, and a bespoke harness would be timing-sensitive.
• Iteration 5 research/review found no remaining mandatory code changes. A delegated codebase-analyzer review independently confirmed no concrete findings across shutdown pending/cancel semantics, quit-only docs/types, workflows hasUI fail-open behavior, overlay reject/no-op settling, graph q behavior, and test coverage.
• Iteration 5 made no code changes; only validation/review was performed.
• Iteration 5 validation: targeted Bun tests passed (273 pass, 0 fail), AGENT=1 bun run typecheck passed, and git diff --check passed.
• Iteration 5 E2E/TUI status: not run; no deterministic checked-in TUI/tmux harness exists for this full quit-confirm scenario.

[mintlify]

Preview deployment for your docs. Learn more about Mintlify Previews.

Project	Status	Preview	Updated (UTC)
bastani	🟢 Ready	View Preview	Jun 15, 2026, 10:00 AM

💡 Tip: Enable Workflows to automatically generate PRs for you.

[claude]

Review: fix(workflows): confirm quit with active runs (#1381)

Thorough, well-tested change. The new session_before_shutdown hook mirrors the existing session_before_switch pattern closely, the fail-open behavior for headless/degraded hosts is consistent, and re-entrancy is handled carefully. Docs and both changelogs are updated. Nice work. A few notes below — only one is a concrete bug, and it's test-only.

Bugs / correctness

1. Dead assertion in test/unit/session-confirm-list.test.ts (factoryCalls += 0;). Lines 157 and 189 increment by 0, so assert.equal(factoryCalls, 0) (lines 166, 198) passes tautologically and verifies nothing. The intent looks like it should match the parallel test in test/unit/session-overlays.test.ts, which correctly uses customCalls += 1 and asserts customCalls === 1 — i.e. "the host custom() was invoked but never called the inner factory, so we still fail safe." As written, the two "never mounts" cases here don't actually assert the host was reached. Suggest += 1 plus asserting the expected non-zero count.

Minor / observations

2. Quit overlay renders with the default theme, not the user's active theme. The handler calls openWorkflowQuitConfirm(ctx.ui, inFlightRuns, deriveGraphTheme({})) (packages/workflows/src/extension/index.ts:3985) and the factory ignores its _theme argument (session-overlays.ts:300). By contrast, the graph kill-confirm in overlay-adapter.ts uses the real theme via deriveGraphThemeFromPiTheme(theme). Users on a non-default theme will see a default-colored quit dialog. Consistent with other call sites in the extension that also pass deriveGraphTheme({}), so not a regression — just flagging the inconsistency in case the host theme is reachable here.

3. requestKill no longer kills on a host without custom. In overlay-adapter.ts, requestKill routes through openKillConfirm(ui ?? {}, ...), which resolves false when ui.custom is absent. Previously graph q called killRun directly. Since the graph overlay only exists where custom UI is available this is almost certainly fine, but it's a behavior change worth a moment's confirmation.

4. 5080-...test.ts "failed pre-shutdown confirmation" mocks a path the real runner can't produce. The test sets extensionRunner.emit to throw, but ExtensionRunner.emit catches handler errors and converts them to emitError (runner.ts:800-809) rather than rejecting — and the workflows handler additionally wraps openWorkflowQuitConfirm in its own try/catch. So emitSessionBeforeShutdownEvent won't actually reject in production. The test still validates the finally guard reset, which is worthwhile; just noting the premise is artificial.

Things I liked

• Re-entrancy is solid: the shutdownConfirmationPending guard plus the post-await if (this.isShuttingDown) return; correctly handle a SIGTERM arriving mid-confirmation, and resetting shutdownRequested = false on cancel prevents checkShutdownRequested() from re-triggering after streaming ends.
• Fail-open semantics and the dispose split are well thought out: once the overlay mounts, dispose → settle(false) (cancel, safe); only the never-mounted path settles undefined (fail open to quit). Escape/Ctrl+C map to cancel via handleKillConfirmInput.
• Making discoverExtensionsInDir sort entries gives deterministic extension/handler ordering — good supporting change.
• emitSessionBeforeShutdownEvent short-circuits via hasHandlers(...) before building context.

Overall LGTM once the factoryCalls += 0 assertions are fixed (or removed). The rest are optional.

🤖 Generated with Claude Code

[claude]

Review: fix(workflows): confirm quit with active runs (#1381)

Solid, well-scoped change. The cancellable session_before_shutdown hook is a clean addition, the signal vs. interactive split is correct, and the test coverage is genuinely broad (lifecycle ordering, fail-open paths, default-cancel rendering, overlay race hardening). The try/finally guard release and the observeCustomMount settle pattern are the right defensive choices. A few items below — one concrete test bug and some minor nits.

🐛 Bugs / correctness

1. Vacuous assertions in test/unit/session-confirm-list.test.ts (factoryCalls += 0)

Two spots increment a counter by zero, so the follow-up assert.equal(factoryCalls, 0) always passes regardless of behavior:

let factoryCalls = 0;
assert.equal(
  await openWorkflowQuitConfirm({
    custom: () => {
      factoryCalls += 0;   // <- never increments; assertion below is dead
      return undefined;
    },
  }, runs, theme),
  undefined,
);
assert.equal(factoryCalls, 0);

This appears in both the openWorkflowQuitConfirm and openKillConfirm "never mounts" cases. The intent (mirrored correctly in session-overlays.test.ts, which uses customCalls += 1 and asserts === 1) is to verify the custom callback was invoked exactly once without the factory firing. As written, the test would still pass even if custom were never called, so it doesn't guard what it claims. Suggest customCalls += 1 + assert.equal(customCalls, 1), matching the sibling test.

🔧 Minor / nits

2. Quit-confirm dialog ignores the user's theme. extension-lifecycle.ts passes deriveGraphTheme({}) (defaults) to openWorkflowQuitConfirm, whereas the kill-confirm path in overlay-adapter.ts correctly derives from the live theme via deriveGraphThemeFromPiTheme(theme). The custom factory also receives the live _theme arg but discards it. Net effect: the quit dialog renders with default border/warning colors rather than the user's theme. Cosmetic, but an inconsistency worth a follow-up if a theme is reachable from the shutdown ctx.

3. emitted field is never consumed in production. emitSessionBeforeShutdownEvent returns { cancelled, emitted }, but interactive-process-lifecycle.ts only reads cancelled; emitted is used solely by tests. Harmless, but consider dropping it or documenting why it is part of the contract.

4. Defensive event.reason narrowing is heavier than needed. In the session_before_shutdown handler the typeof event === "object" && "reason" in event guard is unusually verbose given SessionBeforeShutdownEvent is a well-typed { type; reason: "quit" }. Other handlers in this file destructure the event directly — fine to keep for safety, just noting the asymmetry.

5. requestKill in overlay-adapter.ts is fire-and-forget (void openKillConfirm(...).then(...)). A double q press could theoretically stack two confirmation overlays. Low risk since input routes to the topmost overlay, but a re-entrancy guard (ignore q while a confirm is already pending) would be tidier.

✅ Things done well

• Signal-driven shutdown bypassing the cancellable hook is correctly handled, and the re-check of isShuttingDown after the awaited prompt closes the obvious re-entrancy window.
• shutdownRequested = false on cancel correctly prevents checkShutdownRequested() from re-triggering the quit.
• bun:test for the workflows package and vitest for the coding-agent package are each used consistent with their package conventions (per CLAUDE.md).
• Docs (extensions.md lifecycle diagram + README.md) and both CHANGELOGs are updated under [Unreleased] with issue attribution — matches repo conventions.

Nothing blocking beyond fixing the dead assertions in (1). Nice work.

🤖 Automated review — verify suggestions before applying.

[claude]

Review: fix(workflows): confirm quit with active runs

Reviewed the full diff against CLAUDE.md conventions. This is a well-structured, thoroughly-tested change. The new session_before_shutdown lifecycle event mirrors the existing session_before_switch pattern closely, the fail-open behavior for headless/automation is handled carefully, and the try/finally guard around shutdownConfirmationPending is correct. Test coverage is excellent — cancellation, duplicate-suppression, thrown-handler guard release, and the custom-mount fail-open races are exercised in both packages. Docs + both changelogs are updated as required.

A few observations, none blocking:

Correctness / behavior

1. Quit dialog ignores the user's theme. The new handler renders with deriveGraphTheme({}) (hardcoded Mocha default):

   shouldQuit = await openWorkflowQuitConfirm(ctx.ui, inFlightRuns, deriveGraphTheme({}));

   Every other workflow overlay derives from the live pi theme — overlay-adapter.ts uses deriveGraphThemeFromPiTheme(theme), and lifecycle-notifications.ts/hil-answer-notifications.ts already have a themeFromRenderer() helper for exactly this. The quit confirmation will visually mismatch a user's custom theme. Consider sourcing the theme from ctx.ui.getTheme?.(...) the way the kill-confirm path does.

2. Ctrl+C can no longer force-quit past the prompt. handleKillConfirmInput now treats Ctrl+C as cancel, and the confirm overlay captures key input while mounted. So a user double-tapping Ctrl+C to quit and then hammering Ctrl+C to escape will instead cancel the quit on that next press. That's arguably the intended "are you sure" semantics, but it changes the muscle-memory escape hatch — worth confirming it's desired (SIGTERM/SIGHUP correctly still bypass the hook as the hard-kill path).

API hygiene

3. index-extensions.ts switched to export *. Replacing the explicit named re-export list with export * from "./core/extensions/index.ts" removes the need to hand-maintain the list (and is why the new types flow through), but it also means any symbol later added to core/extensions/index.ts is silently promoted to the public extension API surface. A deliberate tradeoff — flagging it so it's an intentional choice rather than an accident.

Minor / DRY

4. The event reason-narrowing block is now duplicated three times across extension-lifecycle.ts (session_before_switch, session_before_shutdown, session_shutdown):

   const reason = typeof event === "object" && event !== null && "reason" in event
     ? (event as { readonly reason?: string }).reason
     : undefined;

   A small readReason(event) helper would tidy this up.

5. renderWorkflowQuitConfirm guards oldestStartedAt with Number.isFinite (good defensive handling of an empty runs[]) even though the only production caller filters the empty case out first — just noting the render path is safe to call directly, which the unit tests rely on.

Confirmed good

• vitest in packages/coding-agent/test/** is correct (the fork's runner); bun:test in workspace test/** is correct. The split is respected.
• Touched files are under the 500-line gate (session-confirm.ts 384, session-overlays.ts 332).
• The graph-view-input.ts q fall-through (return false when no live run and no onClose) is intentional and documented; the no-speculative-close fix is the core of the change and looks right.
• Signal-driven shutdown (fromSignal: true) correctly bypasses the cancellable hook, and the post-await if (this.isShuttingDown) return; re-check guards the race where a signal forces teardown while the prompt is open.

Nice work — the fail-open paths and the observeCustomMount settle-guard for a host that rejects/never-invokes the factory are especially well thought out.

_{🤖 Automated review}

[claude]

Review: fix(workflows): confirm quit with active runs

Thorough, well-tested change. The cancellable session_before_shutdown lifecycle event is cleanly threaded through the runner discriminated-union types, the fail-open behavior for headless/degraded UI is the right call, and the try/finally guard release is exactly what you want. Test coverage is excellent (core helper, regression, overlay rendering, fail-open paths, default-cancel focus). A few things worth a look before merge — none are blockers.

Bugs / behavior worth confirming

1. Double-Ctrl+C force-quit is suppressed while the confirm prompt is open. handleCtrlC double-tap calls void this.shutdown() (no fromSignal), which hits the shutdownConfirmationPending guard in interactive-process-lifecycle.ts:25 and returns early. So while the confirmation overlay is mounted, the conventional double-Ctrl+C force-quit is dropped. It is not a hang (the overlay itself accepts Ctrl+C to cancel / y to confirm), but users who reflexively double-tap to force-quit will be surprised there is no escape hatch other than a signal. Intended? If so, a one-line comment near the guard would help the next reader.

2. ctx.shutdown() programmatic shutdowns now go through the cancellable path with a hardcoded reason: "quit". /exit, /quit, and any extension calling ctx.shutdown() flow through checkShutdownRequested to shutdown() to session_before_shutdown, so one extension programmatic shutdown can now be cancelled by another extension prompt. That appears intended (docs say /exit//quit prompt), but SessionBeforeShutdownEvent.reason is single-valued "quit", so handlers cannot distinguish a user Ctrl+D from a programmatic /exit or extension-initiated quit. Fine for now; flagging in case a future handler wants to treat them differently — the type would need widening.

3. requestQuit re-checks run liveness before the prompt but not after confirmation (overlay-adapter.ts). Between mounting the confirm overlay and the user pressing y, the run can finish, so quitRun(targetRunId) may fire on an already-ended run. Likely harmless if quitRun is idempotent, but worth a quick confirm.

Tests

4. The "failed pre-shutdown confirmation clears the pending guard" regression test mocks extensionRunner.emit to throw directly. In the real runner, ExtensionRunner.emit catches handler errors and routes them through onError rather than throwing, so emitSessionBeforeShutdownEvent would resolve normally instead of rejecting. The try/finally guard is still correct and valuable — but the test exercises a rejection path the real runner may not actually produce. A comment noting it tests the guard contract (not real runner behavior) would prevent confusion.

Minor / nits

5. index-extensions.ts switched a curated named re-export list to a wildcard export * from core/extensions/index.ts. Since index.ts is itself curated this is acceptable and a nice simplification, but it is an API-surface change unrelated to the stated fix: the public surface is now whatever index.ts exports (including emitSessionBeforeShutdownEvent, emitProjectTrustEvent, and so on), and the wildcard no longer guards against an accidental drop the way the explicit list did. Worth a sanity check that nothing internal-only leaks.

6. isCtrlC (session-confirm.ts) layers literal string checks (data === "ctrl+C" and the raw ETX byte) alongside matchesKey(data, Key.ctrl("c")). If matchesKey/Key.ctrl already normalize the CSI-u encodings the tests cover, the literal branches are redundant; if they do not, those encodings rely entirely on matchesKey. A brief comment on why the extra literals are needed would prevent a future dead-branch cleanup.

7. The workflow handler reason extraction is defensively verbose (the typeof event === "object" && "reason" in event dance in extension-lifecycle.ts) even though the typed SessionBeforeShutdownEvent guarantees reason. Could just be event.reason, consistent with how the other session-before handlers in this file use their typed events.

Style / conventions

.js import specifiers, no build step added to workflows, changelog entries under [Unreleased] in both packages, docs updated (extensions.md, keybindings.md, usage.md, README), and all touched files comfortably under the 500-line gate. The tryPiAccessor this-binding fix (fn.call(theme, color)) is a genuine latent bug fix with a dedicated regression test — nice catch.

Overall: solid, mergeable once the double-Ctrl+C behavior in (1) is confirmed intentional.

🤖 Automated review — flag anything that misreads intent.

[claude]

PR Review: fix(workflows): confirm quit with active runs

Reviewed the full diff against CLAUDE.md conventions. This is a well-built, well-tested change — the cancellable session_before_shutdown lifecycle event is cleanly modeled on the existing session_before_* family, the fail-open behavior for headless/degraded-UI contexts is the right default, and test coverage is genuinely thorough (lifecycle cancellation, duplicate-suppression guard, overlay fail-open on reject/never-mount, default-cancel rendering, receiver-bound theme handling). Docs and both changelogs are updated, and all touched files stay under the 500-line gate. Nice fixes along the way: fn.call(theme, color) in tryPiAccessor correctly preserves the receiver for Pi's receiver-dependent theme accessors, and the settle() guard removes the earlier dispose double-resolve race.

A few things worth a second look — mostly low-severity / questions, no blockers.

1. Programmatic ctx.shutdown() now goes through the cancellable prompt (low, confirm intent)

shutdown() only bypasses the hook on options.fromSignal. But several non-signal callers route through the same method:

• interactive-session-runtime.ts shutdownHandler (what ctx.shutdown() from an extension reaches via runner-context.ts)
• the session-selector quit callback in interactive-session-routing.ts
• /quit and /exit in interactive-input-handling.ts

So any extension that calls ctx.shutdown() expecting it to be authoritative can now be silently cancelled (or made to show the workflows overlay). The doc update ("Available in all contexts") suggests this is intended, but it is a behavior change for programmatic shutdowns, not just interactive Ctrl+C/Ctrl+D — worth confirming that is desired and ideally calling it out explicitly in extensions.md for the ctx.shutdown() path.

2. The "failed pre-shutdown confirmation" regression test exercises a path the real runner can't produce (low)

runGenericHandlers (runner-events.ts) catches handler exceptions via emitCaughtError and never rejects, so emitSessionBeforeShutdownEvent won't throw from a misbehaving handler in practice. The try/finally guard-release in shutdown() is good defensive code, but the test's premise (emit rejecting) doesn't reflect the real emit contract. It is still a valid unit test of shutdown()'s robustness — just flagging that it doesn't correspond to a reachable production state, in case a reader assumes it does.

3. Loose event casting in the workflows handler (nit)

const reason = typeof event === "object" && event !== null && "reason" in event
  ? (event as { readonly reason?: string }).reason
  : undefined;

event is typed SessionBeforeShutdownEvent (reason: "quit"), so this could be event.reason. CLAUDE.md discourages loose typing; the cross-package defensiveness is understandable, but the cast adds noise for little gain.

4. openKillConfirm / openWorkflowQuitConfirm near-duplication (nit)

observeCustomMount factors out the mount-observation, but the two factories are otherwise near-identical (settle guard, finish, render/handleInput/dispose wiring). Not a problem, but a shared helper parameterized by render fn + result type would cut the remaining duplication.

5. isCtrlC string literal (question)

isCtrlC matches Key.ctrl("c") and the raw ETX byte, but also the literal string "ctrl+C". The canonical key match plus the raw byte should cover the real encodings; the "ctrl+C" literal looks speculative. Is that an encoding the TUI actually emits, or belt-and-suspenders? Tests assert it, so it is covered either way — just want to confirm it is intentional rather than copied.

Minor

• Interactive quit kills all in-flight top-level runs on confirm, while graph q confirms a single run — both reuse the same overlay copy ("Quit & kill"). Reasonable, but the shared wording leans toward the single-run framing; fine as-is.

Overall: solid fix for #1378 with strong coverage. Items above are confirm-intent / polish, not blockers.

[claude]

Code Review — PR #1381 fix(workflows): confirm quit with active runs

Reviewed the full diff against main. This is a well-structured change: the new session_before_shutdown lifecycle hook is cleanly threaded through the core types/runner/runner-events, the fail-open behavior for headless/degraded-UI contexts is sensible, the try/finally guard release is correct, and the cancel-default focus + Ctrl+C handling match the stated intent. Typecheck/line-length conventions are respected (all touched files < 500 lines). Nice catch on the fn.call(theme, color) fix in graph-theme.ts — the previous unbound call would have lost this for prototype-method theme accessors.

A few things worth addressing before merge.

1. Test coverage gap vs. the PR description (please confirm intent)

The PR body lists changes to test/unit/overlay-graph.test.ts ("q does not close before host confirms kill; q closes/falls through for completed runs"), but the actual diff touches no overlay-graph* test shard — only extension, graph-theme, session-confirm-list, session-overlays, workflow-attach-pane-09, and overlay-entrypoints-hide-keys. Given the heavy merge-conflict history on the graph files, it looks like those test updates may have been dropped during conflict resolution.

The behavioral change in graph-view-input.ts is meaningful — q on a live run now returns true without calling onClose (no speculative close), and falls through to return false when there's no live target and no onClose. The existing tests in overlay-graph-navigation-01.test.ts don't assert this new invariant:

• The live-run + onQuit test (assert.deepEqual(quit, ["run-1"])) passes no onClose, so it can't verify that onClose is not called.
• "q calls onClose" passes regardless of the old-vs-new branch.

Recommend adding direct coverage for: (a) live run + onQuit set → onQuit fires and onClose does not; (b) no live run, no onClose registered → handleInput("q") returns false (falls through to the outer overlay).

2. index-extensions.ts switched to export * — broadens the public API surface

-export type { /* curated allowlist */ } ...
-export { createExtensionRuntime, discoverAndLoadExtensions, ExtensionRunner, ... }
+export * from "./core/extensions/index.ts";

The previous file was a deliberately curated re-export. export * now leaks every value/type exported by core/extensions/index.ts through the package entrypoint (src/index.ts does export * from "./index-extensions.js"), including internals that were previously hidden: emitSessionBeforeShutdownEvent, loadExtensions, loadExtensionFromFactory, isSearchToolResult, WorkflowResourceProvider, etc. Two concerns:

• This is an undeclared public-API expansion (not mentioned in the PR's "Breaking changes: None"). Anything exported here becomes a de-facto supported surface.
• src/index.ts has multiple export * sources; a name collision between two star-exports silently drops the name (ESM doesn't error unless it's imported), which can mask future regressions.

If the goal was just to expose emitSessionBeforeShutdownEvent, prefer adding it to the curated list rather than going to export *. If the broadening is intentional, please call it out in the changelog.

3. Asymmetric settle semantics in openWorkflowQuitConfirm — worth a comment

In session-overlays.ts:

• mount failure (observeCustomMount reject / factory-never-invoked) → settle(undefined) → caller fails open (quit proceeds).
• dispose after a successful mount → settle(false) → caller treats it as cancel (stays running).

Both are individually defensible, but the divergence is subtle (a disposed-without-input overlay cancels the quit, while a never-mounted one lets it through). A one-line comment documenting the deliberate asymmetry would save the next reader a head-scratch. Also note openWorkflowQuitConfirm's finish() resolves directly instead of going through settle() like the rest of the function — harmless given the settled guard, but slightly inconsistent.

4. Please verify the /quit (awaited) path doesn't starve overlay input

handleCtrlC/handleCtrlD use void this.shutdown() (fire-and-forget) — correct, the input loop stays free to deliver keystrokes to the confirmation overlay. But interactive-input-handling.ts:292 (/quit//exit) does await this.shutdown(). Since shutdown() now awaits an interactive overlay promise that only resolves on user input, this is worth a manual smoke test: confirm the confirmation dialog actually receives y/n/Enter when triggered via /quit (not just Ctrl+C/Ctrl+D). If the TUI dispatches stdin events independently of this awaited handler it is fine, but it is the one path where the await could matter.

Minor / nits

• extension-lifecycle.ts: the session_before_shutdown handler re-parses event.reason defensively with an inline structural type guard even though the event is statically typed { type; reason: "quit" }. Reasonable across the extension boundary, but the cast could be dropped now that the type exists.
• renderWorkflowQuitConfirm correctly guards Number.isFinite(oldestStartedAt) for runs with undefined/missing startedAt (queued-but-not-started) → renders "0s". Good.
• overlay-adapter.ts: OverlayUISurface.confirm? was added but isn't consumed in the new requestQuit path (which goes through openWorkflowQuitConfirm). If it is only needed for the existing openKillConfirm fallback, fine; otherwise it may be a leftover.

Summary

Solid, defensive implementation with good fail-open behavior and docs/changelog updates. The main item to resolve is the missing graph q test coverage (#1), then confirm the export * API broadening (#2) is intentional and the /quit await (#4) behaves under manual test. The rest are nits.

Automated review by Claude (Opus 4.8).

[claude]

Review: fix(workflows): confirm quit with active runs (#1381)

Thorough, well-tested PR. The new session_before_shutdown lifecycle event cleanly follows the existing session_before_* patterns, the cancel-default UX is implemented correctly (focusedButton: 0 -> Enter cancels), and the fail-open behavior for headless/degraded-UI paths is a thoughtful touch. Docs and changelog are updated correctly under [Unreleased]. Overall this looks solid and mergeable; comments below are mostly minor.

Strengths

• Correct cancel aggregation. runGenericHandlers short-circuits on the first handler returning cancel: true and swallows/logs thrown handlers, so any extension can veto and a crashing handler fails safe. The new helper's result?.cancel === true check matches.
• Genuine bugfix bundled in. graph-theme.ts switching fn(color) -> fn.call(theme, color) fixes a real this-binding bug when invoking pi runtime theme accessors. Good catch.
• Signal path correctly bypasses confirmation (fromSignal skips the cancellable hook), and the post-await if (this.isShuttingDown) return guards against a signal-driven teardown racing the open dialog.
• Re-entrancy guard (shutdownConfirmationPending with try/finally) plus the shutdownRequested = false reset on cancel correctly prevents the poll loop (checkShutdownRequested) from re-triggering after a declined quit.
• Strong, targeted test coverage across runner lifecycle, overlay fail-open, default-cancel rendering, and graph q behavior.

Questions / suggestions

1. exit-confirmation-manual-test-report.md at the repo root. This reads like a one-off manual-QA artifact. CLAUDE.md's convention is to keep the tree clean (e.g. deleting issues.md when done). Is this meant to be committed, or should it be removed before merge? If it has lasting value, consider moving it under a docs/testing path rather than the repo root.

2. observeCustomMount host-contract assumption (minor). The helper treats "mount promise resolved but factory not yet invoked" as host failure (settleHostFailure). This is correct only if pi's custom() invokes the factory synchronously during mount. If any host invoked the factory asynchronously after the mount promise settles, this could prematurely resolve the confirmation with undefined/false. Worth a one-line comment documenting the assumed host contract so the invariant is explicit.

3. Graph q fail-open kills without confirmation. In buildGraphOverlayAdapter.requestQuit, when openWorkflowQuitConfirm resolves undefined (no custom UI), the code proceeds to quitRun(...). That matches the legacy "q quits immediately" behavior, so likely intentional -- just flagging that in a degraded-UI graph context q performs the resumable quit without a prompt. Confirm that's the desired contract.

4. Verbose defensive reason extraction. In the session_before_shutdown handler, the typeof event === "object" && ... "reason" in event narrowing is defensive given the event is statically typed as { reason: "quit" } -- event.reason would suffice today. Acceptable as forward-compat, but it currently can never take the early-return branch.

5. Nit: emitSessionBeforeShutdownEvent returns { cancelled, emitted } but the lifecycle caller only consumes cancelled. Fine if emitted is there for tests/future use; otherwise it's dead surface area.

Verification

• File-length gate: all touched files are well under the 500-line limit (largest: overlay-adapter.ts at 429).
• Test framework conventions are respected -- vitest in packages/coding-agent (upstream pi fork) and bun:test in the packages/workflows suites, consistent with each package.
• Changelog entries are correctly placed under [Unreleased] (Added in coding-agent, Fixed in workflows) with issue attribution.

Nice work -- clean, defensive, and well covered.

[claude]

PR Review: fix(workflows): confirm quit with active runs

Thorough, well-tested change. The core design — a cancellable session_before_shutdown lifecycle event fired only on interactive quit and bypassed on signal exits — is clean, and the separation between core (coding-agent) and the workflows extension is correct. Docs, changelog (both packages), and the lifecycle diagram are all updated, files stay under the 500-line gate, and test coverage is genuinely strong (unit + integration + regression + manual E2E). Nice work.

I reviewed the full diff against CLAUDE.md conventions. A few notes, mostly minor.

Correctness — looks solid

• The shutdownConfirmationPending guard with try/finally correctly releases on a thrown handler, and the post-await if (this.isShuttingDown) return; re-check handles a SIGTERM/SIGHUP racing in (via fromSignal) while the prompt is mounted. Good defensive ordering.
• Fail-open semantics are consistent across both entry points: session_before_shutdown treats undefined as "allow quit" (shouldQuit !== false), and the graph-q path in requestQuit also quits on undefined (if (confirmed === false) return; → only an explicit cancel blocks). So a degraded/headless host never wedges, and graph q on a non-custom host still quits as before.
• blocksAppShutdown correctly excludes graph-quit resumable paused runs (exitReason === "quit" && status === "paused" && resumable === true), which is the spurious-confirmation bug the PR set out to fix.

Minor: dead field
OverlayUISurface.confirm? is added at packages/workflows/src/tui/overlay-adapter.ts:54, but the adapter no longer calls openKillConfirm and routes requestQuit through openWorkflowQuitConfirm, which takes a plain UiSurface and never reads .confirm. The field appears unused — consider removing it (or wiring it) to avoid implying a fallback that does not exist.

Consistency observation: inFlightRunCount vs blocksAppShutdown
The new blocksAppShutdown predicate is more precise than inFlightRunCount() in packages/workflows/src/extension/workflow-targets.ts:32-34, which still counts any endedAt === undefined run as in-flight. inFlightRunCount backs the session_before_switch (/new, /resume) prompt and the reload-blocked message — so the same class of bug you just fixed (a graph-quit resumable paused run counted as an active blocker) can still surface in those flows. Not in scope here, but worth a follow-up issue or a confirmation that the broader count is intentional for switch/reload.

Nit: settle() duplication
In openKillConfirm/openWorkflowQuitConfirm, the inner finish() still inlines settled = true; done(undefined); resolve(result) rather than delegating to the shared settle() helper. It is correct (both close over the same settled flag), but finish could be done(undefined); settle(result); to remove the duplicated guard the PR description says was unified.

Tests
Coverage is comprehensive and the assertions are meaningful (default-cancel focus, panel-token vs canvas-token rendering, fail-open on reject/never-mount, Ctrl+C/kitty-sequence cancel variants, duplicate-suppression, guard release on throw). The integration test asserting q mounts the confirmation overlay first (capturedComponents.length === 2) before killing is a good regression guard. I could not execute the suite in this environment (sandbox approval), but the PR reports 273 pass, 0 fail plus passing typecheck/lint.

Overall: approve-leaning. The dead confirm? field and the inFlightRunCount consistency note are the only items I would ask you to address or explicitly defer.

[flora131]

Closing and deleting this exit confirmation feature branch/worktree per request.