Skip to content

fix(cli): queue /plan, /interview, /review mid-turn instead of interrupting - #1256

Open
kavish-19 wants to merge 2 commits into
CodebuffAI:mainfrom
kavish-19:fix/queue-plan-interview-review-mid-turn
Open

fix(cli): queue /plan, /interview, /review mid-turn instead of interrupting#1256
kavish-19 wants to merge 2 commits into
CodebuffAI:mainfrom
kavish-19:fix/queue-plan-interview-review-mid-turn

Conversation

@kavish-19

@kavish-19 kavish-19 commented Sep 3, 2026

Copy link
Copy Markdown

What

/plan <text>, /interview <text>, and /review <text> — and their input-mode counterparts when submitted without inline args — called sendMessage() directly with no check for whether a run was already in progress.

Why this is a bug

Every other mid-turn submit path in this file (plain text via the composer, /skill:<name>) checks isStreaming || streamMessageIdRef.current || isChainInProgressRef.current and falls back to addToQueue() when busy, so a message typed while the agent is still working waits its turn.

/plan, /interview, and /review never got that treatment. Firing one of them while a previous message is still streaming calls sendMessage() immediately, which registers a new active-run owner in registerActiveRun — and that force-stops the in-flight run ('user-interrupt') to make room for the new one. The current job is interrupted and lost instead of being queued behind it. Related: #1211, where a user reports a follow-up message "overwriting" the job in progress instead of queuing.

Repro (no race required — deterministic):

  1. Send any message; while it's still streaming,
  2. type /plan add dark mode (or /interview ..., /review ...) and submit.
  3. Expected: queued behind the current run.
  4. Actual: the current run is interrupted; the in-progress job is lost.

Fix

dispatchSkillPrompt (used by /skill:<name>) already implements the correct pattern. Extracted its busy-check-then-queue-else-send logic into a shared sendOrQueuePrompt() helper in command-registry.ts, and routed all six call sites through it:

  • command-registry.ts: the /interview, /plan, /review command handlers (inline-args form)
  • router.ts: the plan, interview, review input-mode submit handlers

dispatchSkillPrompt itself is now a thin wrapper over sendOrQueuePrompt, so there's one place that owns "send now vs. queue" for every prompt-dispatching command going forward.

Testing

Added regression tests in router-steering.test.ts covering both entry paths (input mode and inline slash-command args) for all three commands, mid-turn and idle:

  • Confirmed red against the unfixed code (sendMessage was called, run interrupted) by temporarily reverting the source changes and re-running.
  • Green after the fix.
bun test cli/src/commands/__tests__/router-steering.test.ts
 13 pass / 0 fail

Also ran the full cli/src/commands/ suite (198/199 pass; the one pre-existing failure — an OSC 52 clipboard test — reproduces identically on unmodified main and is untouched by this change) and tsc --noEmit on the cli package (no new errors; the only typecheck errors present are pre-existing environment issues — missing @types/react-dom and the tar package types — unrelated to the files this PR touches).

…upting

/plan <text>, /interview <text>, and /review <text> (and their input-mode
counterparts when submitted without inline args) called sendMessage()
directly with no check for whether a run was already in progress. Firing
one of these while a previous message was still streaming registered a new
active-run owner, which force-stops the in-flight run ('user-interrupt')
instead of queuing behind it -- so the current job was interrupted and lost
rather than queued, matching what CodebuffAI#1211 describes.

/skill:<name> already gets this right via dispatchSkillPrompt, which checks
isStreaming/streamMessageIdRef/isChainInProgressRef and falls back to
addToQueue when busy. Extract that logic into a shared sendOrQueuePrompt()
helper and route all six call sites (three in command-registry.ts, three in
router.ts) through it so they can't drift out of sync with the busy check
again.

Added a failing-first regression test covering both entry paths (input mode
and inline slash-command args) for all three commands, confirmed red
against the unfixed code, green after.

Claude-Session: https://claude.ai/code/session_018vPhyqaaoKa8cgs7GEnyq5
@PiBOH

PiBOH commented Sep 4, 2026

Copy link
Copy Markdown

Please codebuff-team merge this PR, i need this fix, and thank you @kavish-19 for tha patch

@codebuff-team

Copy link
Copy Markdown
Contributor

Good bug fix. The diagnosis is correct: /plan, /interview, and /review bypassed the busy-check that /skill:<name> already implements via dispatchSkillPrompt, so firing one of these mid-turn hit sendMessage() directly and force-stopped the in-flight run through registerActiveRun. Extracting that logic into sendOrQueuePrompt() in command-registry.ts and routing all six call sites (three command handlers + three router input-mode paths) through it is the right fix — it matches the pattern already established for plain-text and skill submits, and it closes off future drift by having dispatchSkillPrompt become a thin wrapper over the shared helper.

The test additions in router-steering.test.ts are appropriate: they cover both entry paths (inline args and input-mode submit) for all three commands, both busy and idle, and assert on sendMessage/addToQueue call counts rather than just presence, which would have caught this bug before it shipped.

One thing worth double-checking before porting: sendOrQueuePrompt now takes an attachments parameter defaulting to [] for the plan/interview/review paths, while dispatchSkillPrompt passes capturePendingAttachments(). Confirm that plan/interview/review intentionally don't need pending attachments captured — if a user has staged image/file attachments before typing /plan foo, this change would silently drop them where the old code (which also didn't capture attachments) had the same gap, so it's not a regression, but it's worth a comment or a follow-up since the two call sites now diverge in an unobvious way.

Small, well-scoped, tested, and addresses a real filed complaint (#1211). This is a solid first PR.

@codebuff-team codebuff-team added bot:triaged Classified by the community triage bot pr:port-candidate Worth porting into the private source tree labels Sep 4, 2026
Review feedback on CodebuffAI#1256 asked whether plan/interview/review needed to
capture pending attachments. Checking it turned up two real defects, one
of them introduced by that PR.

prepareUserMessage resolves attachments as
`attachments ?? useChatStore.getState().pendingAttachments`, so passing an
explicit array suppresses the store fallback and passing no key at all uses
it. sendOrQueuePrompt got that backwards on both branches:

- The queue branch defaulted to `[]`, so a mid-turn /plan, /interview or
  /review queued with no attachments and left the staged ones in the store,
  where they attached to whatever the user sent next. Before CodebuffAI#1256 these
  paths called sendMessage with no attachments key and picked them up via
  the fallback, so this was a regression, not a pre-existing gap.
- dispatchSkillPrompt passed capturePendingAttachments() as an argument,
  which evaluates before the busy check. An idle /skill:<name> therefore
  cleared the store and then sent without the captured value, dropping the
  attachments outright.

Capture inside the queue branch instead, where the skill path already had
it, and drop the parameter so neither call site can reintroduce the split.

Both defects are covered by tests that fail against the previous commit.

Claude-Session: https://claude.ai/code/session_018vPhyqaaoKa8cgs7GEnyq5
@kavish-19

Copy link
Copy Markdown
Author

Good catch on the attachments question — I checked it and it turned up two real defects, one of them introduced by this PR. Pushed a fix in dffc4bf.

The mechanism is in prepareUserMessage (cli/src/hooks/helpers/send-message.ts:151):

const allAttachments = attachments ?? useChatStore.getState().pendingAttachments

Passing an explicit array suppresses the pendingAttachments fallback; passing no key at all uses it. sendOrQueuePrompt had that backwards on both branches:

  1. Queue branch defaulted to []. A mid-turn /plan, /interview or /review queued with no attachments and left the staged ones in the store — and the queue drain passes attachments: message.attachments explicitly (cli/src/contexts/chat-runtime-context.tsx:131), so they aren't picked up there either. They stay staged and land on whatever the user sends next.

    One correction to your read: this was a regression, not the same gap the old code had. Pre-PR, /plan <text> mid-turn called sendMessage({ content, agentMode }) with no attachments key, so the fallback attached them to the plan message. Routing those paths onto the queue is what suppressed it. Worth flagging since "not a regression" is the part that would have let it through the port.

  2. dispatchSkillPrompt passed capturePendingAttachments() as an argument, so it evaluated before the busy check. An idle /skill:<name> cleared the store and then called sendMessage without the captured value — attachments dropped outright. That one is on me, and it's the more visible of the two.

Both are fixed by capturing inside the queue branch, where the skill path already had it, and dropping the parameter so neither call site can reintroduce the split. dispatchSkillPrompt stays a thin wrapper.

Verification: both new tests confirmed red against 0413433 before the fix, green after (15 pass / 0 fail in router-steering.test.ts). Full suite is 39 failures, identical to the pre-change baseline — no new ones. tsc --noEmit clean on the touched files. command-registry.ts still fails prettier --check, but it does so on unmodified main too, so I've left it rather than bury the diff in an unrelated reformat.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:triaged Classified by the community triage bot pr:port-candidate Worth porting into the private source tree

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants