fix(cli): inject plan/subagent/arena system reminders in ACP (#1151)#3479
Merged
Conversation
The ACP Session sends user messages via chat.sendMessageStream() directly, bypassing GeminiClient.sendMessageStream() where the CLI/TUI path injects its per-turn system reminders. As a result: - Plan mode is silently inert in ACP: the model never sees the reminder that tells it to avoid edits and call exit_plan_mode, so it tries to run edit tools and triggers the plan-mode block-check only as a fallback. - User-level subagents registered in the workspace are invisible to the model for the same reason. - Arena sessions started via the ACP path lose their session-dir context. Mirror the subagent / plan / arena branches from client.ts:848-878 in a new private helper #buildInitialSystemReminders, and prepend its output to the initial user-query message in #executePrompt as well as the cron path in #executeCronPrompt. The helper intentionally skips the managed auto-memory reminder — that one needs the GeminiClient prefetch pipeline and will be tackled as part of broader middleware alignment. Tests cover plan-mode on/off and non-builtin subagent filtering on the first-turn message fed into chat.sendMessageStream.
This was referenced Apr 20, 2026
The system-reminder fix added an unconditional `config.getToolRegistry().ensureTool(ToolNames.AGENT)` call inside `#buildInitialSystemReminders`, which now runs on every `session.prompt()` and every cron fire. The new system-reminder tests stub `ensureTool` via their own `stubEmptySubagents` helper, but the default `mockToolRegistry` at the top of the file still only carries `getTool`. As a result all 13 pre-existing tests that exercise `session.prompt()` blow up with `TypeError: this.config.getToolRegistry(...).ensureTool is not a function`, and the cascaded `StopFailure` assertion fails because the test never reaches the assertion point. Move both `ensureTool` (on the tool registry) and `getSubagentManager` (on the config) into the default beforeEach mocks so every test that calls `session.prompt()` can traverse `#buildInitialSystemReminders` without the caller having to know about it. Defaulting `listSubagents` to an empty array is the harmless zero case — tests that care about subagent reminders already override it. The existing `stubEmptySubagents` helper still works unchanged (its explicit overrides take precedence over the defaults), so the new system-reminder tests in this PR keep expressing intent locally.
The previous commit added `ensureTool` to the `mockToolRegistry`
declaration but left the `afterEach` reset casting to the old
`{ getTool: ReturnType<typeof vi.fn> }` shape, which TS 5 now rejects
under strict mode:
error TS2741: Property 'ensureTool' is missing in type
'{ getTool: Mock<Procedure>; }' but required in type
'{ getTool: Mock<Procedure>; ensureTool: Mock<Procedure>; }'.
Use `typeof mockToolRegistry` so the cast tracks the declaration
automatically and future additions don't need a second edit.
The previous commits wired `#buildInitialSystemReminders` into every `session.prompt()` entry, which also reads `this.config.getApprovalMode()` to decide whether to prepend the plan-mode reminder. The default `mockConfig` never provided `getApprovalMode`, so the five pre-existing prompt-level tests that don't set it locally (passes resolved paths, runtime output dir context, UserPromptSubmit/Stop/StopFailure hooks) crash with `TypeError: this.config.getApprovalMode is not a function` on every platform + node version. Default to `ApprovalMode.DEFAULT` so tests that don't care about approval mode still traverse the helper. The ~10 tests that exercise plan/yolo/auto-edit already reassign `mockConfig.getApprovalMode` locally, and the reassignment wins over the default.
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. |
Collaborator
Author
End-to-end validation resultsScripted ACP client + manual Zed walk-through. 1. Scripted test — plan reminder +
|
tanzhenxin
approved these changes
Apr 22, 2026
tanzhenxin
left a comment
Collaborator
There was a problem hiding this comment.
Review
This PR faithfully ports the plan/subagent/arena system-reminder injection block from GeminiClient.sendMessageStream into the ACP #executePrompt and #executeCronPrompt paths. Scope is well-chosen: minimum fix for #1151 without routing ACP through GeminiClient.sendMessageStream. The filter condition, prepend ordering relative to UserPromptSubmit additionalContext, and one-shot injection (not re-added inside the tool-response continuation loop) all match the reference.
Verdict
APPROVE — minimal, correct port; nits only (optional chaining mismatch, redundant test helper, some uncovered branches for cron/arena/hasAgentTool=false).
TaimoorSiddiquiOfficial
pushed a commit
to TaimoorSiddiquiOfficial/HopCode
that referenced
this pull request
Apr 23, 2026
…1151) (QwenLM#3479) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
chiga0
pushed a commit
that referenced
this pull request
Apr 24, 2026
…3479) * fix(cli): inject plan/subagent/arena system reminders in ACP (#1151) The ACP Session sends user messages via chat.sendMessageStream() directly, bypassing GeminiClient.sendMessageStream() where the CLI/TUI path injects its per-turn system reminders. As a result: - Plan mode is silently inert in ACP: the model never sees the reminder that tells it to avoid edits and call exit_plan_mode, so it tries to run edit tools and triggers the plan-mode block-check only as a fallback. - User-level subagents registered in the workspace are invisible to the model for the same reason. - Arena sessions started via the ACP path lose their session-dir context. Mirror the subagent / plan / arena branches from client.ts:848-878 in a new private helper #buildInitialSystemReminders, and prepend its output to the initial user-query message in #executePrompt as well as the cron path in #executeCronPrompt. The helper intentionally skips the managed auto-memory reminder — that one needs the GeminiClient prefetch pipeline and will be tackled as part of broader middleware alignment. Tests cover plan-mode on/off and non-builtin subagent filtering on the first-turn message fed into chat.sendMessageStream. * test(acp): add ensureTool + getSubagentManager to default mockConfig The system-reminder fix added an unconditional `config.getToolRegistry().ensureTool(ToolNames.AGENT)` call inside `#buildInitialSystemReminders`, which now runs on every `session.prompt()` and every cron fire. The new system-reminder tests stub `ensureTool` via their own `stubEmptySubagents` helper, but the default `mockToolRegistry` at the top of the file still only carries `getTool`. As a result all 13 pre-existing tests that exercise `session.prompt()` blow up with `TypeError: this.config.getToolRegistry(...).ensureTool is not a function`, and the cascaded `StopFailure` assertion fails because the test never reaches the assertion point. Move both `ensureTool` (on the tool registry) and `getSubagentManager` (on the config) into the default beforeEach mocks so every test that calls `session.prompt()` can traverse `#buildInitialSystemReminders` without the caller having to know about it. Defaulting `listSubagents` to an empty array is the harmless zero case — tests that care about subagent reminders already override it. The existing `stubEmptySubagents` helper still works unchanged (its explicit overrides take precedence over the defaults), so the new system-reminder tests in this PR keep expressing intent locally. * test(acp): update afterEach cast to match new mockToolRegistry type The previous commit added `ensureTool` to the `mockToolRegistry` declaration but left the `afterEach` reset casting to the old `{ getTool: ReturnType<typeof vi.fn> }` shape, which TS 5 now rejects under strict mode: error TS2741: Property 'ensureTool' is missing in type '{ getTool: Mock<Procedure>; }' but required in type '{ getTool: Mock<Procedure>; ensureTool: Mock<Procedure>; }'. Use `typeof mockToolRegistry` so the cast tracks the declaration automatically and future additions don't need a second edit. * test(acp): add getApprovalMode to default mockConfig The previous commits wired `#buildInitialSystemReminders` into every `session.prompt()` entry, which also reads `this.config.getApprovalMode()` to decide whether to prepend the plan-mode reminder. The default `mockConfig` never provided `getApprovalMode`, so the five pre-existing prompt-level tests that don't set it locally (passes resolved paths, runtime output dir context, UserPromptSubmit/Stop/StopFailure hooks) crash with `TypeError: this.config.getApprovalMode is not a function` on every platform + node version. Default to `ApprovalMode.DEFAULT` so tests that don't care about approval mode still traverse the helper. The ~10 tests that exercise plan/yolo/auto-edit already reassign `mockConfig.getApprovalMode` locally, and the reassignment wins over the default.
This was referenced Apr 26, 2026
Merged
xaelistic
pushed a commit
to xaelistic/qwen-code
that referenced
this pull request
Jun 7, 2026
…1151) (QwenLM#3479) * fix(cli): inject plan/subagent/arena system reminders in ACP (QwenLM#1151) The ACP Session sends user messages via chat.sendMessageStream() directly, bypassing GeminiClient.sendMessageStream() where the CLI/TUI path injects its per-turn system reminders. As a result: - Plan mode is silently inert in ACP: the model never sees the reminder that tells it to avoid edits and call exit_plan_mode, so it tries to run edit tools and triggers the plan-mode block-check only as a fallback. - User-level subagents registered in the workspace are invisible to the model for the same reason. - Arena sessions started via the ACP path lose their session-dir context. Mirror the subagent / plan / arena branches from client.ts:848-878 in a new private helper #buildInitialSystemReminders, and prepend its output to the initial user-query message in #executePrompt as well as the cron path in #executeCronPrompt. The helper intentionally skips the managed auto-memory reminder — that one needs the GeminiClient prefetch pipeline and will be tackled as part of broader middleware alignment. Tests cover plan-mode on/off and non-builtin subagent filtering on the first-turn message fed into chat.sendMessageStream. * test(acp): add ensureTool + getSubagentManager to default mockConfig The system-reminder fix added an unconditional `config.getToolRegistry().ensureTool(ToolNames.AGENT)` call inside `#buildInitialSystemReminders`, which now runs on every `session.prompt()` and every cron fire. The new system-reminder tests stub `ensureTool` via their own `stubEmptySubagents` helper, but the default `mockToolRegistry` at the top of the file still only carries `getTool`. As a result all 13 pre-existing tests that exercise `session.prompt()` blow up with `TypeError: this.config.getToolRegistry(...).ensureTool is not a function`, and the cascaded `StopFailure` assertion fails because the test never reaches the assertion point. Move both `ensureTool` (on the tool registry) and `getSubagentManager` (on the config) into the default beforeEach mocks so every test that calls `session.prompt()` can traverse `#buildInitialSystemReminders` without the caller having to know about it. Defaulting `listSubagents` to an empty array is the harmless zero case — tests that care about subagent reminders already override it. The existing `stubEmptySubagents` helper still works unchanged (its explicit overrides take precedence over the defaults), so the new system-reminder tests in this PR keep expressing intent locally. * test(acp): update afterEach cast to match new mockToolRegistry type The previous commit added `ensureTool` to the `mockToolRegistry` declaration but left the `afterEach` reset casting to the old `{ getTool: ReturnType<typeof vi.fn> }` shape, which TS 5 now rejects under strict mode: error TS2741: Property 'ensureTool' is missing in type '{ getTool: Mock<Procedure>; }' but required in type '{ getTool: Mock<Procedure>; ensureTool: Mock<Procedure>; }'. Use `typeof mockToolRegistry` so the cast tracks the declaration automatically and future additions don't need a second edit. * test(acp): add getApprovalMode to default mockConfig The previous commits wired `#buildInitialSystemReminders` into every `session.prompt()` entry, which also reads `this.config.getApprovalMode()` to decide whether to prepend the plan-mode reminder. The default `mockConfig` never provided `getApprovalMode`, so the five pre-existing prompt-level tests that don't set it locally (passes resolved paths, runtime output dir context, UserPromptSubmit/Stop/StopFailure hooks) crash with `TypeError: this.config.getApprovalMode is not a function` on every platform + node version. Default to `ApprovalMode.DEFAULT` so tests that don't care about approval mode still traverse the helper. The ~10 tests that exercise plan/yolo/auto-edit already reassign `mockConfig.getApprovalMode` locally, and the reassignment wins over the default.
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.
Summary
Closes #1151.
The ACP Session sends user messages via
chat.sendMessageStream()directly, bypassingGeminiClient.sendMessageStream()where the CLI/TUI path injects its per-turn system reminders (client.ts:848-878). Consequences:exit_plan_mode, so it tries to run edit tools; the plan-mode block-check inSession.runToolcatches them only as a fallback, producing a confusing UX.Changes
Session.#buildInitialSystemRemindersmirrors the subagent / plan / arena branches fromclient.ts:848-878.#executePrompt, and to the cron-fired prompt in#executeCronPrompt.relevantAutoMemoryreminder is intentionally not included here. Managed auto-memory requires the prefetch pipeline that lives inGeminiClient, which will be handled as part of broader middleware alignment (see notes below).Why not switch ACP to
GeminiClient.sendMessageStreamoutrightThat would be the structurally correct fix for all ACP/TUI parity gaps, but it requires:
UserPromptSubmit+Stophook loops inSession.ts(otherwise they would double-fire).ServerGeminiStreamEvent→ the rawStreamEventshape ACP currently consumes).SendMessageType.Retry / Cron / Notificationbranch semantics.That's a much larger refactor. This PR is the minimal fix for #1151; the structural work is tracked separately in a follow-up issue (opening after this PR).
Relationship to #3108
#3108 claims a broader hook-alignment gap. On HEAD the hook wiring is already complete:
UserPromptSubmitStop/StopFailurePreToolUse/PostToolUse/PostToolUseFailurerunToolPermissionRequest/NotificationThe remaining divergence is non-hook middleware (system reminders, IDE context, loop detection, auto-memory recall, arena control-signal). This PR covers the system-reminder slice; the rest is tracked by the follow-up.
Test plan
Session.test.ts > prompt > system reminders: plan-mode on vs default, plus non-builtin subagent filtering. Captures the first-turn message fed intochat.sendMessageStreamand asserts reminder presence + ordering.acp-integration/after rebuildingpackages/core.qwen --experimental-acp --approval-mode plan, ask for a code change, verify the model responds with a plan viaexit_plan_modeinstead of trying to useedit.