fix(core): remove proactive subagent system-reminder injection - #4587
Merged
Conversation
Remove getSubagentSystemReminder and its runtime injection points. This function injected a system-reminder message every turn that commanded the model to 'PROACTIVELY use the Agent tool', causing excessive subagent spawning. Agent information is already available via the static tool description — the runtime push is unnecessary and harmful for tasks that benefit from direct tool use. Benchmark evidence (django__django-15280): - Before: 6 subagent calls, only fixed 1/3 locations, FAILED - After: 1 subagent call, fixed all 3 locations, PASSED
Contributor
📋 Review SummaryThis PR removes the runtime 🔍 General Feedback
🎯 Specific Feedback🔵 Low
✅ Highlights
|
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
requested changes
May 29, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
[Critical] Test failure — Session.test.ts still has a test for the removed subagent reminder
The test 'prepends subagent reminder when user-level subagents exist' in packages/cli/src/acp-integration/session/Session.test.ts (lines 2930–2962) was not removed. It asserts that a system reminder containing 'researcher' and 'planner' appears in the first-turn message parts, but the production code that generates this reminder was deleted by this PR.
FAIL Session > prompt > system reminders > prepends subagent reminder when user-level subagents exist
AssertionError: expected undefined to be truthy
at Session.test.ts:2960 expect(reminder).toBeTruthy();
This causes CI failures on all 3 platforms (macOS, Ubuntu, Windows). Fix: delete the entire it('prepends subagent reminder when user-level subagents exist', ...) block.
[Suggestion] Dead test infrastructure left behind (3 files)
Several test files contain stale mocks and comments that reference the removed subagent reminder code path. These are silently unused but misleading for future readers:
Session.test.tslines 218–219, 242–245: comments about#buildInitialSystemRemindersiteratinglistSubagents()andensureTool(AGENT)— no longer accurateSession.test.tslines 2877–2888:stubEmptySubagents()helper is now dead setup code (the two surviving plan-mode tests don't need it)Session.worktree.test.tslines 119–125:getSubagentManager/ensureToolmocks with stale commentsclient.test.tslines 421–423, 476:mockSubagentManager/listSubagentsmock no longer exercised
Consider cleaning these up in this PR or a follow-up.
— qwen3.7-max via Qwen Code /review
Collaborator
|
One thing I wanted to check with you on the benchmark evidence — I might be misreading the code, so correct me if so. It looks like the reminder only gets injected when there's at least one non-built-in subagent configured (the candidate list filters out built-ins before the count check). And If that's right, then removing it wouldn't explain the 6→1 / 1-of-3→3-of-3 change, and the before/after would be picking up variance or something else in the setup. But I could easily be missing how your benchmark harness configures agents — were there any custom agents in play for those runs? Just want to make sure the evidence is pointing at this change before reading too much into it. |
wenshao
previously requested changes
Jun 1, 2026
Collaborator
|
Following up on my earlier question — having sat with this longer, I think the change is worth making regardless of how the benchmark question resolves, and there's a cleaner way to justify it than the SWE-bench numbers. The Agent tool's own description already does the two useful things the removed reminder did: it lists the available agents so the model knows they exist, and it already tells the model to proactively use any agent whose description says it should be used proactively. So the only thing the reminder uniquely added on top of that was the blanket "PROACTIVELY delegate any matching task" push. Dropping it removes that one aggressive nudge plus some duplication, while leaving discoverability and per-agent proactivity fully intact — which is a reasonable thing to want on its own, independent of any benchmark. If you reframe the PR's motivation around that — the reminder is redundant with the tool description, and the only thing it added was an over-aggressive blanket push — it's a much more solid case than the single-case trajectory, and it sidesteps the question I raised earlier entirely. One concrete thing to fix before this can land: there's still a test asserting that the subagent reminder appears in the first-turn messages (it sets up a couple of user-level agents and expects the reminder text). Since this PR removes the code that produces it, that test fails in CI across all platforms — it needs to be updated or removed along with the production code. Thanks for digging into this area — I think the direction is right. |
tanzhenxin
approved these changes
Jun 3, 2026
tanzhenxin
left a comment
tanzhenxin
left a comment
Collaborator
There was a problem hiding this comment.
Review
The latest commit cleanly resolves the test breakage I flagged — the stale assertion expecting the old reminder, plus the now-unused subagent mocks, are all removed, and there's no leftover code path the removed mocks were serving. This is a tidy, self-contained prompt-only deletion: it drops the per-turn "proactively delegate" nudge while leaving agent discoverability intact through the tool description.
On the benchmark evidence I asked about earlier — I'm still happy to take the change regardless of those numbers, as I noted before. Removing an always-on delegation nudge is sound on its own merits, so no need to block on that.
Verdict
APPROVE — the blocking test failure is fixed and the change stands on its own.
tanzhenxin
added a commit
that referenced
this pull request
Jun 3, 2026
…-startup-context Resolve conflicts after merging origin/main: 1. packages/core/src/core/prompts.test.ts — both sides deleted a function the other still tested. main (#4587) removed getSubagentSystemReminder (function + injection sites + tests) to stop excessive subagent spawning; this branch removed buildDeferredToolsSection when moving startup context into system reminders. Neither function exists in the merged prompts.ts, so both obsolete describe blocks are dropped. 2. packages/cli/src/acp-integration/session/Session.test.ts — main switched rewindToTurn from getHistory() to getHistoryShallow() and updated its rewind tests to mock both. This branch's new MCP-reminder rewind test predated that change and mocked only getHistory(); add the getHistoryShallow() mock so it exercises the post-merge code path. Verified: npm install + build + typecheck pass; prompts.test.ts (56) and Session.test.ts (79) green.
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
4 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.
What this PR does
Removes the runtime system-reminder that was injected every conversation turn, which commanded the model to "PROACTIVELY use the Agent tool to delegate user's task to appropriate agent." This eliminates a redundant and overly aggressive prompt injection while keeping agent discoverability and per-agent proactivity fully intact through the static tool description.
Why it's needed
The Agent tool's own description already does the two useful things this reminder did:
The only thing the runtime reminder uniquely added on top of that was a blanket "PROACTIVELY delegate any matching task" push, applied every turn regardless of context. This is problematic for two reasons:
Removing the reminder eliminates this one aggressive nudge and reduces prompt duplication, while leaving the model's ability to discover and use agents — including proactive ones — completely unchanged.
Reviewer Test Plan
How to verify
Evidence (Before & After)
None
Tested on
Environment (optional)
Risk & Scope
Linked Issues
N/A — discovered through SWE-bench trajectory analysis comparing Qwen Code vs Claude Code.