fix(core): insert skill/agent content literally in system prompt substitutions#27994
Conversation
|
📊 PR Size: size/S
|
🛑 Action Required: Evaluation ApprovalSteering changes have been detected in this PR. To prevent regressions, a maintainer must approve the evaluation run before this PR can be merged. Maintainers:
Once approved, the evaluation results will be posted here automatically. |
Summary of ChangesHello, 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 bug where dynamic content containing dollar-sign sequences was being incorrectly processed by Highlights
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 the 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 counterproductive. 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. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request fixes an issue where special $ sequences in prompt substitutions (such as ${AgentSkills}, ${SubAgents}, and ${AvailableTools}) were being incorrectly interpreted by String.prototype.replace by switching to the function-based replacement form. Unit tests were also added to verify literal insertion of strings containing $ sequences. The reviewer noted that the subsequent loop replacing tool-specific variables (${toolName}_ToolName) was missed and should also be updated to use the function form of replace to prevent the same issue when tool names contain $ characters.
| ? allToolNames.map((name) => `- ${name}`).join('\n') | ||
| : 'No tools are currently available.'; | ||
| result = result.replace(/\${AvailableTools}/g, availableToolsList); | ||
| result = result.replace(/\${AvailableTools}/g, () => availableToolsList); |
There was a problem hiding this comment.
While fixing the substitution for ${AvailableTools}, the subsequent loop that replaces tool-specific variables (e.g., ${toolName}_ToolName) was missed.
If a tool name contains $ characters (such as the weird$&tool used in your new test case), the replacement of ${weird$&tool_ToolName} will also suffer from the exact same String.prototype.replace behavior where $ sequences are interpreted.
To prevent this, the tool-specific replacement should also use the function form of replace:
for (const toolName of allToolNames) {
const varName = `${toolName}_ToolName`;
result = result.replace(
new RegExp(`\\\\\\\${\\\\b${varName}\\\\b}`, 'g'),
() => toolName,
);
}There was a problem hiding this comment.
consistency fix applied; note that the branch isn't reachable via $ tool names because the dynamically-built regex won't match them
Summary
applySubstitutions()inpackages/core/src/prompts/utils.tsinjectsskill / sub-agent / tool content into the system prompt using the string
form of
String.prototype.replace:When the replacement argument is a plain string, JavaScript interprets
$replacement patterns inside it (
$$,$&,$`,$',$n). TheskillsPrompt/ sub-agents content is rendered from user- andextension-authored skill and agent descriptions (
SKILL.md, agent.mdfiles), which can legitimately contain shell snippets such as
$'…',$$, or$VAR. When they do, the substituted system prompt is silently mangled —no error, just a degraded instruction that can subtly worsen model behavior.
The worst case is
$'(dollar-apostrophe, common in shell), whichString.replacetreats as "insert everything after the match" — so theentire remainder of the system prompt is spliced in and duplicated.
This runs whenever a custom system-prompt override is in use
(
applySubstitutionsis called frompromptProvider.ts).Details
The fix switches the three substitutions to the function form of
replace, so the value is inserted literally and$sequences are nevertreated as replacement patterns:
This mirrors the pattern the repo already uses for exactly this reason in
HookRunner.expandCommand(packages/core/src/hooks/hookRunner.ts):.replace(/\$GEMINI_PROJECT_DIR/g, () => escapedCwd). The change makesutils.tsconsistent with that existing in-repo practice.Platform-independent defect (language-level
String.prototype.replacesemantics).
Related Issues
Closes #27993
How to Validate
This PR adds three regression tests to the existing
describe('applySubstitutions')block inutils.test.ts, asserting that$-containing values are inserted literally for each of the threeplaceholders. The pre-existing suite only used alphanumeric replacement
values, so it could not see this bug.
${SubAgents}casereports
expected 'A:see :B here:B' to be "A:see $' here:B"(the prompt tailis spliced in by
$'), and the${AvailableTools}case reportsexpected 'T: - weird${AvailableTools}tool' to be 'T: - weird$&tool'($&re-inserts the matched placeholder). The suite reports
3 failed | 30 passed.reports
33 passed.Pre-Merge Checklist