feat(cli): Ctrl+B promote keybind (#3831 PR-3 of 3)#3969
Merged
Conversation
…troller (#3831 PR-3 of 3) Final piece of the foreground → background promote feature. PR-1 (#3842) landed the `signal.reason` foundation; PR-2 (#3894) wired `shell.ts` to detect a `{ kind: 'background' }` abort, snapshot output, register a `BackgroundShellEntry`, and stash the promote `AbortController` on `TrackedExecutingToolCall`. This PR exposes the user-visible surface: pressing Ctrl+B during an in-flight foreground shell command transfers ownership to a background task the user can inspect via `/tasks` or stop via `task_stop`. ## Changes - `keyBindings.ts`: new `Command.PROMOTE_SHELL_TO_BACKGROUND` bound to `Ctrl+B`. JSDoc explains the no-shell-running no-op semantics. - `useReactToolScheduler.ts`: project `promoteAbortController` from the core's `ExecutingToolCall` through `TrackedExecutingToolCall` so the React layer (AppContainer keypress handler) can find it by callId without re-plumbing through the scheduler. - `AppContainer.tsx`: `handleGlobalKeypress` gains a `PROMOTE_SHELL_TO_BACKGROUND` branch that walks `pendingToolCallsRef.current` (the ref, not the destructured array — keeps the deps list stable so the handler isn't re-bound on every tool-call status update), finds the executing tool call with a defined `promoteAbortController`, calls `.abort({ kind: 'background' })`, and returns early. No-op when no foreground shell is executing — Ctrl+B then falls through to the input layer's existing cursor-left binding. - `keyboard-shortcuts.md`: documents Ctrl+B with explicit fall-through behavior so the conflict with the prompt-area cursor-left binding is intentional + understandable. ## Tests - `keyMatchers.test.ts` (+1): Ctrl+B positive / bare-b + meta+b + Ctrl+other negatives. - `AppContainer.test.tsx` (+2): - **Ctrl+B promotes** — pendingToolCalls includes an executing shell with a stubbed `AbortController` + spy; firing Ctrl+B asserts `abort({ kind: 'background' })` is called once. - **Ctrl+B no-op** — empty `pendingToolCalls` + Ctrl+B must NOT throw (pins the safety contract for the typing-mid-prompt case where the input layer's own Ctrl+B should still fire). - 37/37 keyMatchers + 58/58 AppContainer pass; tsc + ESLint clean. ## E2E (manual, PR description guidance) The unit / integration tests cover the keybind → abort wiring and the promote handler's downstream behavior (PR-2's tests). Real-PTY E2E is intentionally manual since headless test infrastructure doesn't drive a real shell child + Ctrl+B keystroke; documented in the PR description checklist. Closes the 3-PR sequence for #3831 (Phase D part b of #3634).
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
wenshao
commented
May 8, 2026
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
Additional findings (not mapped to specific diff lines)
Suggestion — packages/core/src/tools/shell.ts:1588: setPromoteAbortControllerCallback 调用时机存在竞态窗口。该回调在 await ShellExecutionService.execute(...) 之后才调用，但核心调度器在 execute() 内部 await 之前就将状态切换为 'executing' 并触发 notifyToolCallsUpdate()。React 在该 yield 期间处理重渲染时，tracked tool call 的 promoteAbortController 为 undefined，导致用户在此期间按 Ctrl+B 静默穿透到光标左移。建议将回调移到 promoteAbortController 创建并组装 combinedSignal 之后、await execute(...) 之前的同步代码区。
Suggestion — packages/cli/src/ui/hooks/useToolScheduler.test.ts: 缺少 promoteAbortController 从 core 到 tracked 类型的投影测试。若该映射被意外移除，Ctrl+B 将静默失效。
— DeepSeek/deepseek-v4-pro via Qwen Code /review
wenshao
commented
May 8, 2026
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
[Suggestion] packages/cli/src/ui/hooks/useReactToolScheduler.ts：useReactToolScheduler 没有测试文件。promoteAbortController 在执行分支的投影逻辑以及在非执行状态转换时清零为 undefined 的逻辑从未被直接测试过。AppContainer.test.tsx 的测试通过 mock 绕过了调度器。建议添加单元测试验证投影和清零行为。
— DeepSeek/deepseek-v4-pro via Qwen Code /review
5 #3969 review threads addressed: - **AppContainer.tsx Ctrl+B handler**: documented the KeypressContext.broadcast caveat (after `return`, the same Ctrl+B is still dispatched to text-buffer cursor-left + DebugProfiler; visible cursor-left side effect is cosmetic) so future readers understand why the prompt cursor moves on a successful promote. Added `debugLogger.debug` calls on both branches (matched callId on success; streamingState + pendingToolCalls.length on no-op fall-through) so "Ctrl+B doesn't work" reports are debuggable. - **useReactToolScheduler.ts TrackedExecutingToolCall**: dropped the redundant `pid?` and `promoteAbortController?` declarations — both come through the `& ExecutingToolCall` intersection unchanged. Fixed the JSDoc that wrote `{ kind: 'background', shellId }`: callers don't generate `shellId` (it's optional on the abort-reason union and `handlePromotedForeground` produces it downstream). The corresponding executing branch in `toolCallsUpdateHandler` no longer projects pid / promoteAbortController explicitly — `...coreTc` already spreads them; the explicit-undefined clearing in the non-executing branch is also dropped (those fields aren't on coreTc when status !== 'executing', so `...coreTc` doesn't carry them). - **AppContainer.test.tsx**: replaced two `as unknown as Key` double-casts with direct `: Key` annotations on the literal — the object already conforms to the Key interface, double-cast was bypassing type safety needlessly. Tests: 37/37 keyMatchers + 58/58 AppContainer pass; tsc + ESLint clean. No behavior change beyond the new debug log lines.
…ybind # Conflicts: # packages/cli/src/ui/AppContainer.test.tsx # packages/cli/src/ui/AppContainer.tsx
wenshao
commented
May 9, 2026
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
— deepseek-v4-pro via Qwen Code /review
…lear 3 #3969 review threads addressed; 1 deferred: - AppContainer.tsx: Ctrl+B `find()` predicate now also checks `tc.request.name === ToolNames.SHELL` before matching the executing tool call. Defense-in-depth — today only the shell tool wires `promoteAbortController`, but a future copy-paste / type confusion that adds the property to a non-shell tool would otherwise let Ctrl+B mistakenly fire `abort({kind:'background'})` on a tool whose service has no promote-handoff handler. - useReactToolScheduler.ts: re-added explicit `pid: undefined` and `promoteAbortController: undefined` to the non-executing return. Previously dropped on the assumption that `...coreTc` doesn't carry these fields when the status isn't `executing` — true today, but the explicit clearing is defense-in-depth against a future core change that adds either field to a non-executing status type (would surface as a stuck PID display or a Ctrl+B handler that matches a no-longer-executing tool call). - AppContainer.test.tsx: replaced the placeholder "no-op when no pending tool calls" framing on the empty-array case (it does exercise the `executing-status` predicate but NOT the tool-name guard) with TWO tests: 1. existing empty-array no-throw test (renamed for clarity) 2. NEW: executing non-shell tool with a hostile-shape `promoteAbortController` — asserts `abortSpy` is NOT called. This is the regression test for the new tool-name guard above. Tests: 61/61 AppContainer.test.tsx pass; tsc + ESLint clean. Deferred to follow-up (replied + tracked): - `debugLogger.debug` is file-only; success-path "agent unblocks + next message says 'promoted to bg_xxx'" is the user-visible signal. Adding a synthetic history item or stderr line for the gap between keypress and agent message conflicts with Ink rendering and is better as a focused UX PR.
wenshao
commented
May 11, 2026
wenshao
commented
May 11, 2026
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
[Suggestion] packages/cli/src/ui/hooks/useReactToolScheduler.ts — The promoteAbortController: undefined defense-in-depth clearing in the non-executing branch has zero test coverage (the entire useReactToolScheduler module has no test file). Consider adding unit tests to verify promoteAbortController is undefined after executing→completed transitions.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
…ssertions #3969 review: the earlier "redundant declaration" review removed the explicit `pid?: number` and `promoteAbortController?: AbortController` from `TrackedExecutingToolCall`, relying on the `& ExecutingToolCall` intersection to inherit them. Current review flags the type-safety regression: if core renames or removes either field, the React-side build won't catch it locally — Ctrl+B handler silently breaks at runtime. Compromise: keep the type minimal (no re-declaration noise the prior review flagged) but add compile-time `extends keyof ExecutingToolCall` assertions that fail loudly + locally if either field disappears. The assertions are evaluated at compile time and zero-cost at runtime; the dummy `const` pins them so they aren't dead code. 61/61 AppContainer tests pass; tsc clean.
wenshao
commented
May 11, 2026
| if (compactToggleHasVisualEffect(historyRef.current)) { | ||
| refreshStatic(); | ||
| } | ||
| } else if (keyMatchers[Command.PROMOTE_SHELL_TO_BACKGROUND](key)) { |
Collaborator
Author
There was a problem hiding this comment.
[Suggestion] embeddedShellFocused guard missing for the new PROMOTE_SHELL_TO_BACKGROUND branch.
The Escape handler at this same level has if (embeddedShellFocused) { return; } to avoid intercepting keys during embedded PTY sessions, but the new Ctrl+B handler lacks this guard. When the user is in an embedded PTY shell and presses Ctrl+B (bash's cursor-backward key):
- If a shell tool call is executing → the global handler incorrectly promotes it instead of forwarding Ctrl+B to the PTY
- If no shell is executing →
.find()anddebugLogger.debug()still run unnecessarily
Suggested change
| } else if (keyMatchers[Command.PROMOTE_SHELL_TO_BACKGROUND](key)) { | |
| } else if (keyMatchers[Command.PROMOTE_SHELL_TO_BACKGROUND](key)) { | |
| if (embeddedShellFocused) { | |
| return; | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
tanzhenxin
approved these changes
May 11, 2026
tanzhenxin
left a comment
Collaborator
There was a problem hiding this comment.
Review
This wires Ctrl+B to abort the executing shell tool's promoteAbortController with { kind: 'background' }, which the already-merged PR-2 picks up to detach and register a background task. The end-to-end path is clean: keyBindings.ts → keypress handler → pendingToolCallsRef.current.find(...) → abort({ kind: 'background' }) → core scheduler observes the signal → ShellExecutionService promote path. The tool-name guard tc.request.name === ToolNames.SHELL is genuine defense-in-depth even though setPromoteAbortControllerCallback is already gated to ShellToolInvocation upstream. No conflicting global Ctrl+B binding exists — text-buffer.ts:2282 is the only prior consumer at the input layer, and the fall-through behavior matches what the docs promise.
Inline comments in AppContainer.tsx and useReactToolScheduler.ts are unusually thorough — they explain every non-obvious choice (ref vs state for stable deps, the tool-name guard's purpose, the broadcast-has-no-consumed-flag caveat with a follow-up tracked in-comment). Nice work.
Verdict
APPROVE.
This was referenced May 11, 2026
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Final piece of the foreground → background promote feature (Phase D part b of #3634). Builds on:
signal.reasonfoundation —ShellAbortReason = { kind: 'cancel' } | { kind: 'background'; shellId? }.shell.tsintegration — detectsresult.promoted: true, snapshots output tobg_xxx.output, registers aBackgroundShellEntry, and stashes the promoteAbortControlleron the executing tool call.This PR exposes the user-visible surface: pressing Ctrl+B during an in-flight foreground shell command transfers ownership to a background task the user can inspect via
/tasksor stop viatask_stop. The child keeps running; the agent's turn unblocks immediately.Caller flow end-to-end
Changes
keyBindings.ts: newCommand.PROMOTE_SHELL_TO_BACKGROUNDbound toCtrl+B. JSDoc explains the no-shell-running no-op semantics so future readers know why the binding fires conditionally.useReactToolScheduler.ts: projectpromoteAbortControllerfrom the core'sExecutingToolCallthroughTrackedExecutingToolCallso the React layer (AppContainer keypress handler) can find it on the executing tool call. Without this, the type contract at the React boundary doesn't carry the controller across.AppContainer.tsx:handleGlobalKeypressgains aPROMOTE_SHELL_TO_BACKGROUNDbranch that:pendingToolCallsRef.current(the ref, not the destructured array — keeps the deps list stable so the handler isn't re-bound on every tool-call status update, which is noisy).promoteAbortController(non-shell tools don't expose one, so this naturally filters to shell-tool branches)..abort({ kind: 'background' })and returns early.docs/users/reference/keyboard-shortcuts.md: documents Ctrl+B with explicit fall-through behavior so the conflict with the prompt-area cursor-left binding is intentional + understandable to users reading the reference.Tests
keyMatchers.test.ts(+1): Ctrl+B positive / bare-b+ meta+b + Ctrl+other negatives.AppContainer.test.tsx(+2):pendingToolCallsincludes an executing shell with a stubbedAbortController+ spy; firing Ctrl+B assertsabort({ kind: 'background' })is called once.pendingToolCalls+ Ctrl+B must NOT throw (pins the safety contract for the typing-mid-prompt case where the input layer's own Ctrl+B should still fire).Test plan
vitest runcli: 37/37 keyMatchers + 58/58 AppContainer passtsc --build packages/cli: cleantail -f /var/log/system.log) + Ctrl+B → child stays alive,/tasksshows the bg_xxx entry,task_stop bg_xxxkills it. Headless test infrastructure can't drive a real shell child + Ctrl+B keystroke; this is the manual gate that closes the 3-PR sequence.Related
signal.reasonfoundation, MERGED)cc @tanzhenxin