Bugfix - Subagents do not include base rules#678
Conversation
Summary of ChangesHello @jutaz, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug where subagents, when configured with a custom system prompt, failed to inherit essential base rules. The fix ensures that all agents consistently incorporate foundational project rules by explicitly merging them with any custom prompts, thereby preventing unintended deviations in agent behavior. New tests have been added to guarantee the robustness of this prompt merging mechanism. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Code Review SummaryStatus: 2 New Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
Files Reviewed (9 files)
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully addresses the issue where subagents were missing base rules and project-level rule files when a custom system prompt was provided, ensuring the base system prompt is always fetched and merged. However, this change increases exposure to a pre-existing prompt injection vulnerability: rule files are embedded in the prompt using CDATA sections without sanitizing the ]]> termination sequence, which could allow an attacker to hijack the agent's behavior. Additionally, the current implementation in agent.ts has inefficiencies and potential prompt duplication, and new unit tests need to be reorganized.
| if (!systemPrompt) { | ||
| systemPrompt = await this.promptsManager.getSystemPrompt(this.store.getSettings(), task, profile); | ||
| // Get the base system prompt (project rules) | ||
| const baseSystemPrompt = await this.promptsManager.getSystemPrompt(this.store.getSettings(), task, profile); |
There was a problem hiding this comment.
This change at lines 732-739 increases the attack surface for a prompt injection vulnerability. The baseSystemPrompt embeds rule files using CDATA sections without sanitizing the ]]> termination sequence in PromptsManager.getSystemPrompt. An attacker could exploit this to inject arbitrary instructions. Additionally, the current merging logic always fetches the base system prompt, leading to inefficiency and potential duplication if systemPrompt is already a full system prompt. Consider a more refined merging approach, such as checking if the provided prompt is a full override and using the additionalInstructions parameter of getSystemPrompt for merging. Remediation for the prompt injection vulnerability involves sanitizing rule file content in src/main/prompts/prompts-manager.ts by escaping or replacing ]]> before wrapping it in a CDATA section.
| const baseSystemPrompt = await this.promptsManager.getSystemPrompt(this.store.getSettings(), task, profile); | |
| if (systemPrompt) { | |
| // If it's not a full system prompt, merge it with the base prompt (rules, persona, etc.) | |
| if (!systemPrompt.startsWith('<AiderDeskSystemPrompt')) { | |
| systemPrompt = await this.promptsManager.getSystemPrompt(settings, task, profile, undefined, systemPrompt); | |
| } | |
| } else { | |
| // No custom prompt provided, fetch the default system prompt | |
| systemPrompt = await this.promptsManager.getSystemPrompt(settings, task, profile); | |
| } |
| }); | ||
| }); | ||
|
|
||
| describe('systemPrompt merging', () => { |
There was a problem hiding this comment.
This new describe block for system prompt merging is nested inside the Agent - getContextFilesAsToolCallMessages block. Since these tests are testing the runAgent logic and prompt merging rather than context file formatting, they should be moved to a separate top-level describe block or a block dedicated to runAgent to maintain proper test organization.
jutaz
force-pushed
the
fix/subagent-system-prompt-rules
branch
from
71b58a5 to
dd3d494
Compare
|
Just wanted to confirm this bug from a real-world multi-agent setup. I'm running a project with Architect (main agent) delegating to Scout (GLM-4.5-air) and Builder subagents. I have project hygiene rules configured as Skills with checkboxes enabled for both agents. Despite this, subagents consistently ignore file placement rules — creating temp scripts, diagnostic files, and test artifacts directly in the project root. Debugging the session logs revealed the root cause matches exactly what this PR describes: the Scout subagent received only ~2,100 tokens in its context — just the bare task prompt with zero rules or skills attached. The Architect sees everything correctly, but the moment it delegates via This has been a significant pain point — the project accumulated dozens of misplaced files (test_.js, check_.py, temp_.ts, fix_.js scattered across root directories) because subagents had no awareness of the file structure conventions. Great catch @jutaz — looking forward to this being merged. |
|
One thought for a potential follow-up (not for this PR): now that base rules are correctly propagated, it might be worth considering per-profile rule filtering for subagents in the future. In my setup, the Architect profile has extensive rules (deploy runbook, db-safety, release protocol, methodology docs) that are irrelevant for a Scout subagent doing read-only file navigation, and they eat into the attention budget of smaller models like GLM-4.5-air. Something like a |
|
@aresstokrat |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| systemPrompt = `${wrappedRules}\n\n${customSystemPrompt}`; | ||
| } else if (!customSystemPrompt) { | ||
| systemPrompt = wrappedRules || ''; | ||
| } |
There was a problem hiding this comment.
CRITICAL: Missing else clause causes systemPrompt to be undefined
When includeRules is true, customSystemPrompt exists, but wrappedRules is empty/falsy, the systemPrompt variable remains undefined. This will cause line 1520 to use an empty string instead of the custom prompt.
Add an else clause:
} else if (customSystemPrompt) {
// Use custom prompt even when rules are empty
systemPrompt = customSystemPrompt;
}- Add toggle option to SubagentConfig (default: false) - When enabled, subagents receive project rules (AGENTS.md, .aider-desk/rules) - Create rules-block.hbs template for standardized rules wrapping - Add UI checkbox in subagent settings section - Add i18n translations for the new option - Add tests for includeRules behavior (on/off/undefined)
jutaz
force-pushed
the
fix/subagent-system-prompt-rules
branch
from
ce12d69 to
b1a28f7
Compare
| } | ||
|
|
||
| const messages = await this.prepareMessages(task, profile, await task.getContextMessages(), await task.getContextFiles()); | ||
| <<<<<<< HEAD |
There was a problem hiding this comment.
CRITICAL: Unresolved merge conflict
This file contains merge conflict markers that must be resolved before merging. The conflict is between HEAD and commit ce12d69 in the estimateTokens method.
The conflict appears to be about:
- HEAD:
getAvailableTools(task, 'agent', profile, provider, profile.model) - Branch:
getAvailableTools(task, profile, provider)plus the new includeRules logic
You need to:
- Resolve the conflict by choosing the correct method signature for
getAvailableTools - Integrate the includeRules logic from the branch
- Remove the conflict markers (
<<<<<<<,=======,>>>>>>>) - Test that the merged code works correctly
The mock was missing the dispatchEvent method which is called by extensionManager in agent.ts. Added mock that returns the data passed to it to simulate the extension event system.
| } else if (!systemPrompt) { | ||
| systemPrompt = wrappedRules || ''; | ||
| } | ||
| } else if (!systemPrompt) { |
There was a problem hiding this comment.
CRITICAL: Logic bug - this condition will never be true
The condition else if (!systemPrompt) at line 890 will never execute because systemPrompt is already set at line 734-736 (before this new code block). When systemPrompt is initially undefined, it gets set to baseSystemPrompt at line 735, so by the time execution reaches line 890, systemPrompt will never be falsy.
This means:
- For main agents (non-subagents), the base system prompt won't be applied as intended
- The logic at lines 873-893 is redundant with the earlier logic at 734-736
Suggested fix: Remove the earlier assignment at line 734-736, or restructure this logic to handle all systemPrompt assignment in one place.
| } else if (!customSystemPrompt) { | ||
| systemPrompt = wrappedRules || ''; | ||
| } | ||
| } else if (customSystemPrompt) { |
There was a problem hiding this comment.
CRITICAL: Same logic bug as in the other location
This has the same issue as the first location (line 890). The condition else if (customSystemPrompt) at line 1738 should work correctly, but the fallback at line 1741-1743 (else { systemPrompt = baseSystemPrompt; }) may not work as intended if there's similar earlier logic in this function that pre-populates systemPrompt.
Verify that systemPrompt is not already set before this code block executes. If it is, this fallback logic will never execute.
| ${systemPrompt}`; | ||
| } else if (!systemPrompt) { | ||
| systemPrompt = wrappedRules || ''; | ||
| } |
There was a problem hiding this comment.
CRITICAL: Missing else clause for the case when includeRules is true, systemPrompt is provided, but wrappedRules is empty/falsy
The current logic:
if (systemPrompt && wrappedRules) {
systemPrompt = `${wrappedRules}\n\n${systemPrompt}`;
} else if (!systemPrompt) {
systemPrompt = wrappedRules || '';
}This doesn't handle the case where systemPrompt exists but wrappedRules is empty. In this scenario, systemPrompt remains unchanged, which may not be the intended behavior.
Consider adding an else clause:
if (systemPrompt && wrappedRules) {
systemPrompt = `${wrappedRules}\n\n${systemPrompt}`;
} else if (!systemPrompt) {
systemPrompt = wrappedRules || '';
} else {
// systemPrompt exists but wrappedRules is empty - use systemPrompt as-is
// (no change needed, but makes intent explicit)
}| ${customSystemPrompt}`; | ||
| } else if (!customSystemPrompt) { | ||
| systemPrompt = wrappedRules || ''; | ||
| } |
There was a problem hiding this comment.
CRITICAL: Same missing else clause issue as in the other location (line 891)
When includeRules is true, customSystemPrompt is provided, but wrappedRules is empty/falsy, the code doesn't explicitly handle this case. Consider adding an else clause to make the intent clear:
if (customSystemPrompt && wrappedRules) {
systemPrompt = `${wrappedRules}\n\n${customSystemPrompt}`;
} else if (!customSystemPrompt) {
systemPrompt = wrappedRules || '';
} else {
// customSystemPrompt exists but wrappedRules is empty - use customSystemPrompt as-is
systemPrompt = customSystemPrompt;
}
3 participants
As per my investigation, subagents do not receive AGENTS.md and
.aider-desk/rules/files when they have a customsystemPromptconfigured. The bug was inagent.tswheregetSystemPrompt()was only called whensystemPromptwas undefined - but subagents pass their custom prompt, causing the base prompt with rules to be skipped entirely.