Skip to content

fix(acp): apply session/set_mode to the turn that is running - #1905

Open
addyCooks wants to merge 3 commits into
Nano-Collective:mainfrom
addyCooks:fix/acp-mid-turn-set-mode
Open

addyCooks wants to merge 3 commits into
Nano-Collective:mainfrom
addyCooks:fix/acp-mid-turn-set-mode

Conversation

@addyCooks

Copy link
Copy Markdown
Collaborator

Description

A session/set_mode sent while a prompt was running was accepted ({} returned) but ignored: runTurn read the mode once at the start of the prompt, so later tool calls in the same prompt kept the old mode's availability and approval rules. Switching from yolo or auto-accept back to normal still let the next commands and file edits run without a permission prompt.

session.developmentMode is now read live:

  • for the tools offered on each model request;
  • for the plan-mode write_plan fallback;
  • for every approval decision.

A call the model had already returned that the new mode no longer offers (for example an edit after switching to plan) is refused with Error: <tool> is not available in <mode> mode instead of being run or put to the user. This only applies when the mode changed during the request. A call that is already running is not interrupted, and a permission request already pending stays valid for that call; the new mode applies from the next call.

The TUI already behaves this way (it reads the mode from a ref on every tool call); this brings ACP in line.

Closes #1857

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update

Changeset

  • Added a changeset (pnpm changeset) describing this change for the changelog

Testing

Automated Tests

  • New features include passing tests in .spec.ts/tsx files
  • All existing tests pass (pnpm test:all completes successfully)
  • Tests cover both success and error scenarios

Manual Testing

  • Tested with Ollama
  • Tested with OpenRouter
  • Tested with OpenAI-compatible API
  • Tested MCP integration (if applicable)

Checklist

  • If this was for an open issue, I was assigned to it
  • Code follows project style guidelines
  • Self-review completed
  • Documentation updated (if needed)
  • No breaking changes (or clearly documented)
  • Appropriate logging added using structured logging (see CONTRIBUTING.md)

runTurn read the session's mode once per prompt, so a switch made while
the prompt was running was accepted but ignored: later tool calls kept
the old mode's approval rules until the whole prompt ended. Moving from
yolo or auto-accept back to normal still let the next commands and file
edits run without a permission prompt.

Read session.developmentMode live instead: for the tools offered on each
model request, for the plan-mode fallback, and for every approval. A call
the model already returned that the new mode no longer offers (e.g. an
edit after switching to plan) is refused rather than run. A call that is
already running is not interrupted.

Closes Nano-Collective#1857
@github-actions

Copy link
Copy Markdown
Contributor

nc-review: comments — 1 important, 1 nit

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

The PR correctly fixes the reported bug: session.developmentMode is now read live for the tool list on each model request, for the plan-mode write_plan fallback, and for every approval decision, so a mid-turn session/set_mode takes effect on the next call — which is what issue #1857 asks for, and the new specs cover the yolo→normal and normal→yolo repros. The one defect is in the new refusal path: it runs before the single-call emitToolCall(..., 'pending', ...) and gates its failed update on announcedBatch, so a refused non-batch tool call produces no ACP client notification at all, and the added test only exercises the batch path. The change is worth having and is close to ready; it needs the refusal to notify the client for the single-call case too.

🟠 important · completeness · source/acp/acp-conversation.ts:537

A refused tool call is never announced to the client when the model returned it on its own (the common one-call-per-response case).

The new refusal block sits before the emitToolCall(session, conn, toolCall, 'pending', meta, ...) call that normally announces a tool call, and its notification is gated on announcedBatch:

if (announcedBatch) {
    await emitToolCallUpdate(session, conn, toolCall, 'failed', reason);
}
toolResults.push({...});
continue;

announcedBatch is validToolCalls.length > 1 (source/acp/acp-conversation.ts:489), so for a single tool call the continue skips the announcement entirely: the client receives no tool_call and no tool_call_update for that id. The refusal exists only as a tool result in the model's context, so the VS Code chat panel renders no tool card at all — the user switched to plan and the model attempted an edit, but the transcript shows nothing, and any other ACP client has the same hole. Every other refusal path in this file emits unconditionally — the plugin-deny path (:582), the user-deny path (:634) and the permission-cancel path (:606) all call emitToolCallUpdate(..., 'failed', ...) because the call was already announced; only this new path acts before the announcement.

The batch path happens to work because the batch announcement loop (:491-506) has already sent tool_call/pending for every call in the batch, so the failed update lands on an existing card.

The new spec a call the model already returned is refused once the mode no longer offers it only covers the batch path — it returns two tool calls in one turn (read_file + write_file), so announcedBatch is true. Nothing exercises the single-call refusal, which is why the gap survives the suite.

Fix: emit the notification for the non-batch case as well — either move the refusal block below the emitToolCall(..., 'pending', ...) so there is always a call to update, or drop the announcedBatch guard and emit a tool_call/failed for the single-call case. A regression test with one tool call per turn that asserts the client received a failed update for the refused id would pin it down.

⚪ nit · correctness · source/acp/acp-conversation.ts:450

The plan-mode write_plan fallback now mixes a live mode check with a request-time tool list:

if (
    session.developmentMode === 'plan' &&   // live
    !wrotePlan &&
    availableNames.includes('write_plan') &&  // request-time
    cleanedContent.trim().length > 0
) {

availableNames is built from requestMode at the top of the turn, and write_plan is only in it when the request was issued in plan mode (getAvailableToolNames adds it only for developmentMode === 'plan', source/tools/tool-manager.ts:297-303). So a switch from normal to plan mid-request — the model answers in prose with no tool calls — satisfies the live check but fails availableNames.includes('write_plan'), and the plan is never persisted. The VS Code plan-review card never appears and the prose is lost.

This errs in the safe direction (it does not write a plan artifact in a mode that does not offer one), and the next turn recovers because the request is then built in plan mode. It is only inconsistent rather than dangerous, so it is not worth blocking on — but computing both sides from the live mode (e.g. toolManager.getAvailableToolNames(undefined, session.developmentMode).includes('write_plan')) would make the fallback match the stated "read the mode live" intent.


🔴 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.

@github-actions github-actions Bot added the agent:comments nc-review left non-blocking findings label Oct 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent:comments nc-review left non-blocking findings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] ACP ignores session/set_mode sent during a turn; tools keep the old mode's approval rules

1 participant