fix: guard queued prompt draining across modes

[mikemikimike]

Summary / Problem

Follow-up to PR #1210. Queued prompts could start concurrently when a plan-review action dismissed the review bar, and a failed queued dispatch could be retried in a tight loop.

Changes

• Keep queue draining disabled until the client and tool manager are ready.
• Stop re-dispatching the same queue head after a failed or throwing dispatch while keeping the message queued for an explicit retry/edit.
• Keep the queue blocked while plan clarification or plan modification is being requested.
• Signal conversation completion when chat dependencies are unavailable so a queued prompt cannot remain blocked indefinitely.
• Strengthen regression coverage with the real user-message queue, pending-plan guards, failed dispatches, and both busy and idle queue editing.
• Clarify the changeset issue reference as Refs #1060.

Tests

• Focused AVA suites for interactive-app, useAppHandlers, useChatHandler, and user-input: all passed (31, 22, 24, and 52 tests respectively).
• pnpm run test:format, pnpm run test:types, pnpm run test:types:vscode, pnpm run test:lint, and pnpm run test:changesets: passed.
• pnpm run test:audit against the official npm registry: passed with no new vulnerabilities.
• pnpm run test:knip: passed with existing hints about ignored .github/** and unused tags.
• pnpm run test:ava: attempted on Windows; the changed suites pass, while platform-sensitive CLI, ACP, filesystem, subprocess, and network suites report existing failures/timeouts (90 failures, 245 pending after timeout, and one uncaught exception).
• pnpm run build: TypeScript compilation and alias rewriting passed; the final copy/chmod step cannot run on Windows because the package script uses Unix cp and chmod.

Compatibility / Known limitations

The queue remains intact after a failed dispatch, but the same queue head is not automatically retried until the user edits or otherwise changes that queued item. This prevents an unavailable or failing setup from hot-looping.

Issue

Refs #1060

[will-lamerton]

/re-review

[github-actions]

nc-review: comments — 1 important

The replacement of PR #1069 cleanly relocates queued-prompt draining into an effect in InteractiveApp, gated by the full set of in-flight states (generation, cancellation, tool execution, modal modes, plan review, pending plan proceed). The duplicate onCommandComplete signal in /retry is removed, idle navigation of queued messages is preserved, and the new tests genuinely exercise the drain path with real React state transitions. One scope-creep concern: this PR also changes .github/workflows/pr-labeler.yml from base.sha to head.sha, which is unrelated to the queue fix and is likely to break the workflow for fork PRs.

🟠 important · scope · .github/workflows/pr-labeler.yml:35

The PR title is "fix: guard queued prompt draining across modes" but the diff also flips ref: context.payload.pull_request.base.sha to ref: context.payload.pull_request.head.sha in this workflow, with no mention in the description. That change is unrelated to the queue fix and is best split into its own PR.

Beyond the scope concern, the change also regresses the workflow for fork PRs. pr-labeler.yml runs on pull_request_target with pull-requests: write, and GITHUB_TOKEN there is scoped to the base repo — head.sha for a fork PR points at commits that only exist on the fork, so github.rest.repos.getContent({ ref: head.sha, ... }) will 404 and the script will throw (Error: labeler.yml had no top-level label keys or labeler.yml is not a file). Same-repo branch PRs are fine; fork PRs would have a broken labeler step.

If the intent was to detect new labels added to .github/labeler.yml in the PR itself, that is a real fix worth landing, but it needs its own discussion and likely a fork-aware path (e.g. fall back to base.sha on 404).

---

_{🔴 blocking · 🟠 a reviewer would ask for a change · ⚪ optional}

_{Automated code review — correctness, security, design, tests, plus duplicates and scope. A human still decides; this is not a substitute for review and is not exhaustive. The required status checks separately cover lint, formatting, types, unused dependencies, the test suite and the build. This bot never merges. Maintainers can rerun with /re-review.}

[will-lamerton]

/re-review

[github-actions]

nc-review: comments — 1 nit

@mikemikimike — a few things worth a look, none blocking.

This is a clean replacement for PR #1069: the queued-prompt drain is moved out of App.tsx (where it fired on every onConversationComplete) into a properly gated effect inside InteractiveApp that blocks on generation, cancellation, tool execution, all modal modes, plan review, and pending plan proceed. The duplicate onCommandComplete signal from /retry is removed, the macrotask defer that protects the dispatched turn's abortController/isGenerating reset is preserved, and the new tests genuinely exercise the drain path with real state transitions. The only outstanding items are minor wording inconsistencies between the changeset, the PR description, and the bot's context.

Addressed since the last review

• ✅ the .github/workflows/pr-labeler.yml scope-creep finding from the previous review is gone — the PR no longer touches that workflow, so the queue-drain diff is now scope-clean.

⚪ nit · changeset · .changeset/calm-queues-compact.md

The changeset body says Closes #1060, the PR description says Refs #1060, and the bot context says the PR closes no issue. Pick one. Refs is the most accurate framing given the PR text.

---

_{🔴 blocking · 🟠 a reviewer would ask for a change · ⚪ optional}

_{Automated code review — correctness, security, design, tests, plus duplicates and scope. A human still decides; this is not a substitute for review and is not exhaustive. The required status checks separately cover lint, formatting, types, unused dependencies, the test suite and the build. This bot never merges. Maintainers can rerun with /re-review.}

[will-lamerton]

The design is right and the fix is real. /compact is genuinely covered end to end: compact-handler.ts wraps summariseWithLLM in setIsToolExecuting, so isConversationComplete flips false -> true at the correct moment and the effect drains. I verified the new planReviewState guard is load-bearing by deleting the clause, and the suite breaks. 173 focused tests pass locally.

One defect remains, in the same class the PR set out to close.

Blocking: the drain still starts a concurrent turn when the plan review bar is dismissed

queueDrainBlocked (interactive-app.tsx:192) covers planReviewState?.show and pendingPlanProceed, which closes the "proceed" path. It does not cover the other two exits from that bar.

handlePlanAskMore (useAppHandlers.tsx:645) clears planReviewState and dispatches through handleChatMessage, not handleMessageSubmit. Only handleMessageSubmit resets isConversationComplete (:675), and handleChatMessage awaits import('@/stats/record') (useChatHandler.tsx:341-352) before flipping any generating flag. React commits a render where the bar is gone, nothing is generating, and the conversation still reads complete. The drain fires there.

Confirmed with your own makeProps harness rather than inferred. Control with nothing blocking drains ["plain"]. A probe replicating the ask-more body exactly (clear the bar, then an async function awaiting a dynamic import before setting isGenerating) drains ["queued while planning"], a second turn dispatched underneath the clarifying-questions turn.

handlePlanModify (:654) has the same shape: it clears the bar, prints "Plan Mode remains active. Tell Nanocoder what to change.", and the drain immediately submits a stale queued prompt into that moment.

Not a regression, since pre-PR the drain fired even earlier while the bar was still up. But it is the same failure mode one step downstream. Cheapest fix is for handlePlanAskMore to call setIsConversationComplete(false) before clearing the bar, since it really is starting a turn. handlePlanModify is a judgement call.

The pendingPlanProceed guard has zero coverage

Replacing appState.pendingPlanProceed !== null with false leaves all 29 tests in interactive-app.spec.tsx passing. The guard is correct and necessary, since handlePlanProceed clears planReviewState and sets pendingPlanProceed in the same batch (useAppHandlers.tsx:634-636), but nothing pins it.

The three "does not drain" tests assert nothing

t.false(submitted) holds whether or not the guard works, because the default drainNextMessage mock is () => false. They only detect a regression by crashing the file with TypeError: userMessageQueue.drainNextMessage(...).then is not a function. When I removed the planReviewState clause, the plan-review test itself still reported as passing; only the non-zero file exit revealed the break. Make the default mock async and count dispatch attempts. This was flagged on #1069 and carried over unchanged.

Smaller notes

drainInProgressRef is released only inside .then. Nothing throws synchronously today, but if drainNextMessage ever did, the ref would latch true and the queue would stop draining silently.

The second commit touching .github/workflows/pr-labeler.yml nets to zero against main (the file list is 10 files, no labeler entry). Worth squashing.

The create-handler.ts await-then-complete asymmetry raised on #1069 is still unexplained. The new retry-handler.ts comment justifies removing the signal there but not why the sibling keeps it.

user-input.spec.tsx still has a trailing-newline-only hunk at EOF.

The first finding is what I want addressed before merge. The next two are cheap in the same pass.

[will-lamerton]

Thanks for the clean rewrite of #1069. The core idea is right and it does fix #1060: keying off state rather than the onConversationComplete edge is what lets onCommandComplete (slash commands, /compact) resume the queue, and it lets a blocked drain retry once the blocker clears. Two things need fixing before merge.

1. A failed dispatch becomes an unbounded retry loop

drainNextMessage pops the message and, on a falsy or throwing dispatch, pushes it back at the front and returns false (useUserMessageQueue.ts:57-63). Under the old edge-triggered drain that was safe: the message waited for the next completion edge. Under the new effect the restore mutates queuedMessages, the effect re-runs, and it re-dispatches the same message immediately.

Measured on this branch with an instrumented render:

• handleUserSubmit rejecting: 65 re-dispatches in 300 ms
• toolManager null (the return false at interactive-app.tsx:225): 101 re-dispatches in 300 ms

Both spin for as long as the condition holds, no backoff, no attempt cap, two re-renders of the queued list per iteration. The reachable trigger is a rejection out of handleUserSubmit: it awaits runLifecycleHooks('user-prompt-submit', ...) and handleMessageSubmission (useAppHandlers.tsx:711,746), and handleChatMessage does real work before its own try block (useChatHandler.tsx:347-392). Anything throwing there turns one bad queued prompt into a hot loop that may also append a chat bubble per attempt.

Suggested fix, both parts:

• Hoist the !appState.client || !appState.toolManager check into the effect's early return instead of doing it inside the dispatch. Both are already deps, so the drain resumes on its own once they populate and the queue is never popped-then-restored.
• Do not re-enter for a message that just failed. A lastFailedIdRef that is skipped while it is still at the head of the queue is enough, since any real state change can reset it.

2. The three new negative tests cannot fail

does not drain queued prompts while a turn is generating, ... while a modal mode is active, and ... while plan review is active all use makeProps's default drainNextMessage: () => false, which never invokes the dispatch callback, so submitted stays false regardless of the guards.

Confirmed by deleting queueDrainBlocked, activeMode, isSettingsMode, and isConversationComplete from the effect's early return: all 29 tests in interactive-app.spec.tsx still pass. The guards this PR exists to add have no coverage.

Please give those three the real useUserMessageQueue (the harness pattern the two positive tests already use). Also make the default mock async () => false: production calls .then() on the result (interactive-app.tsx:233) and the as never cast on props hides the mismatch from tsc.

Smaller notes (non-blocking)

• Dropping onCommandComplete from /retry is correct for the normal path, but handleChatMessage early-returns at useChatHandler.tsx:346 on !client || !toolManager without signalling completion. That leaves isConversationComplete stuck at false and the queue permanently undrainable. Cheapest fix is to signal completion in that early return, which also closes the same gap on the plain chat path.
• user-input.spec.tsx:1165 gains a CRLF line ending. biome.json excludes **/*.spec.tsx, so test:format will not catch it despite lineEnding: "lf". Same exclusion is why the new @/constants and @/hooks/... imports sitting above strip-ansi in interactive-app.spec.tsx pass unsorted.
• The isBusy removal in user-input.tsx is fine: ChatInput renders UserInput only in the else branch of the approval/confirmation/question ternary (chat-input.tsx:211-232) and the composer is unmounted for modals, settings, plan review, and input-capturing live components, so there is no arrow-key contention. But the spec swapped the busy case for the idle case rather than covering both, so loadSelectedQueuedMessage while busy is now untested.
• If you keep the void drainAttempt; line, say in the comment that it is there to satisfy useExhaustiveDependencies.
• The changeset says "Closes #1060", the PR body says "Refs #1060". Pick one.

Verified locally: ava on the five touched specs passes on macOS (109 tests), biome check on the changed non-spec files is clean.

[mikemikimike]

/re-review

[mikemikimike]

Addressed the review feedback in c96949b:

• Guard queue draining until both the client and tool manager are available.
• Keep a failed queue head queued without immediately retrying the same message, including synchronous throws.
• Mark plan clarification and plan modification as incomplete before dismissing the review bar.
• Signal completion when chat dependencies are unavailable.
• Replace the no-op drain mocks with the real queue in the negative tests, cover pending plan proceed, and cover busy queue editing.
• Changed the changeset wording to Refs #1060.

Focused tests and all required remote checks pass. The only remaining red check is the unrelated Linux sandbox-jail job, whose AVA command filters out the requested source/services/bash-executor-sandbox.spec.ts file before running it.