fix(core): merge IDE context into user prompt#3980
Merged
Conversation
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
Dogfooding / Manual IDE Smoke TestI used the following manual flow to dogfood the IDE-mode behavior against this PR branch:
Expected result: IDE context is attached to the current user request as a Observed result: manual IDE dogfooding with the PR bundle behaved as expected. The lower-level request-shape cases, including pending tool-call skipping, arena cancellation, and Test resultin vscode terminal: outside of ide terminal: |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates core request construction so IDE/editor context is wrapped in a <system-reminder> envelope and merged into the current user request, instead of being injected as a separate user-history entry via chat.addHistory(). This keeps API history turn shapes stable in IDE mode and adds test coverage for the revised assembly and guard behavior.
Changes:
- Add helpers to wrap IDE context and prepend it into the first text part of the outgoing request.
- Delay IDE-context state updates until after the pre-turn arena cancellation check to avoid marking unsent context as sent.
- Update/sendMessageStream unit tests to assert merged-context behavior, closing-tag escaping, pending-tool-call skipping, and arena cancellation behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/core/src/core/client.ts | Merges IDE context into the outgoing request parts (wrapped in <system-reminder>), and delays IDE context state updates until after arena cancellation checks. |
| packages/core/src/core/client.test.ts | Updates and expands tests to validate merged IDE context in the request, correct escaping, and arena cancellation behavior without addHistory() injection. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
wenshao
reviewed
May 10, 2026
This was referenced May 10, 2026
wenshao
requested changes
May 11, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
[Critical] In packages/core/src/tools/{monitor,send-message,task-stop,web-fetch}.ts, the 5 trailing super() arguments were dropped during the merge of origin/main (commit f980f57). This removes shouldDefer: true (reverts to default false), searchHint keywords, and explicit isOutputMarkdown/canUpdateOutput/alwaysLoad values. These 4 tools will now appear in every API request instead of being discovered on-demand via ToolSearch, increasing per-request token consumption. origin/main still has these args — merging this PR as-is would revert them.
Please restore the 5 arguments in all 4 tools:
true, false, true, false, 'monitor watch tail log stream background'(monitor.ts)true, false, true, false, 'send message task communicate notify'(send-message.ts)true, false, true, false, 'task stop cancel kill background'(task-stop.ts)true, false, true, false, 'web fetch url http download content'(web-fetch.ts)
— DeepSeek/deepseek-v4-pro via Qwen Code /review
IDE mode now wraps editor context in a <system-reminder> block and prepends it to the current user request instead of inserting a separate user history entry via addHistory(). This preserves the API history turn shape and avoids extra user turns in IDE mode. Key changes: - IDE context merged into user request via prependToFirstTextPart() - State update deferred until after arena cancellation check - escapeClosingSystemReminderTags() hardens against tag injection including zero-width/control character variants - forceFullIdeContext reset on stream errors for correct resend - Context prompt updated to encourage active use of editor context Refs #3712
884ba78 to
a41dc8d
Compare
resetChat only cleared GeminiClient's new perModelGeneratorCache but dropped the BaseLlmClient.clearPerModelGeneratorCache() call that was present before the refactoring. sideQuery.ts and sessionTitle.ts still route through BaseLlmClient with the fast model, so stale generators survived session reset.
Collaborator
Author
|
The 4 tool files ( |
…iClient The per-model ContentGenerator resolution logic (resolveModelAcrossAuthTypes, createRetryAuthTypeForModel, createContentGeneratorForModel, perModelGeneratorCache) was inadvertently duplicated from BaseLlmClient into GeminiClient. This restores the original one-line delegation to BaseLlmClient.resolveForModel() and removes ~130 lines of redundant code to keep the PR focused on IDE context merging only.
wenshao
requested changes
May 11, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Critical coreToolScheduler.ts (~line 2143) still uses the old inline regex .replace(/<\/system-reminder>/gi, '<\\/system-reminder>') for closing-tag scrub instead of the new escapeClosingSystemReminderTags. The new function handles whitespace/zero-width/control-char variants that the old literal regex misses, leaving the rule/skill activation <system-reminder> envelope vulnerable to obfuscated closing-tag injection.
Suggestion packages/core/src/utils/xml.ts has no direct unit test file. The new escapeClosingSystemReminderTags function and its helpers are only indirectly covered via client.test.ts. Consider creating xml.test.ts with dedicated tests for tag variants, ignorable char edge cases, and boundary conditions.
— deepseek-v4-pro via Qwen Code /review
wenshao
previously requested changes
May 11, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
[Suggestion] escapeClosingSystemReminderTags (new in packages/core/src/utils/xml.ts) lacks a standalone unit test file (xml.test.ts). The function is only indirectly covered through client.test.ts integration tests. As an exported utility that may gain additional callers, it should have dedicated unit tests covering: no-tag inputs, opening-tag-only inputs, edge code-point ranges in isSystemReminderTagIgnorable, and performance with large HTML/JSX content.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
Collaborator
Author
|
Updated in
Validation run locally: cd packages/core && npx vitest run src/utils/xml.test.ts src/utils/partUtils.test.ts src/core/client.test.ts
npm run typecheck
npm run build
git diff --checkFollow-up: I re-ran full workspace |
Collaborator
Author
|
All review threads are resolved now, CI is green, and I re-ran full workspace |
# Conflicts: # packages/core/src/core/client.ts
Aligns the rule-activation envelope scrub with the IDE-context path — both now route through `escapeSystemReminderTags`, which neutralizes whitespace, zero-width, and control-character variants of `<system-reminder>` tag boundaries. The previous narrow regex only matched the literal `</system-reminder>` sequence, so a rule body containing `</system-reminder >`, `</ system-reminder>`, or `</system-reminder>` could still close the envelope mid-content.
Two adjacent comments described the pre-merge model: one called the system-reminder block "append" while the code prepends, and the tryCompressChat note still talked about "the previous context turn" which no longer exists once IDE context is merged into the user prompt. Rewrite both to match what the code actually does so future readers do not get a misleading mental model of prompt assembly or post-compression resend behavior.
The block-level comment still labeled the sanitization step as "closing-tag scrub", which described the old narrow regex. The shared escapeSystemReminderTags helper now neutralizes opening / self-closing / obfuscated variants too, so name the helper directly to keep the rationale and the call site in agreement.
The end-to-end scheduler scrub test only exercised a literal </system-reminder> body. Now that the rule-activation envelope routes through escapeSystemReminderTags, extend the integration coverage to the obfuscated closing-tag variants the helper was introduced to catch (whitespace before/after the slash, ZWSP / WJ / VS-16 inside the name) and to opening-tag injection. Each case asserts that the envelope still has exactly one </system-reminder> closer and that the raw obfuscated form (or unescaped opening tag) does not survive into the model-facing payload. Refactor the existing test's mock setup into a shared runSchedulerWithRule helper so the new it.each variants stay focused on the assertion shape.
wenshao
reviewed
May 13, 2026
- Add debug log when IDE context parts are empty for diagnosability - Add safety comment in xml.ts explaining why no fast-path pre-check is used in getSystemReminderTagKind (zero-width obfuscation bypass)
TaimoorSiddiquiOfficial
added a commit
to TaimoorSiddiquiOfficial/HopCode
that referenced
this pull request
May 14, 2026
- IDE context wrapped in <system-reminder> block and prepended to user request instead of inserting a separate user history entry - prependToFirstTextPart() and escapeClosingSystemReminderTags() added - escapeSystemReminderTags() neutralizes obfuscated tag variants - forceFullIdeContext reset on stream errors for correct resend - BaseLlmClient.clearPerModelGeneratorCache() restored on resetChat - New utilities: partUtils.ts (prependToFirstTextPart), xml.ts (escapeSystemReminderTags) Cherry-picked from upstream QwenLM/qwen-code commit 796de4d. Resolved conflict in coreToolScheduler.test.ts (merged with QwenLM#4058 changes). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
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
<system-reminder>block and prepends it to the current user request instead of inserting a separate user history entry viaaddHistory().</system-reminder>inside IDE-provided text.Validation
</system-reminder>.chat.addHistory()user turn is added, unsafe closing tags from IDE text cannot break the reminder envelope, and a pre-turn arena cancellation does not mark IDE context as sent.client.test.tspassed all 102 tests; lint, typecheck, build, diff whitespace checks, and manual IDE smoke passed locally.cd packages/core && npx vitest run src/core/client.test.tsand inspect thesendMessageStreamIDE context tests.Scope / Risk
/rewindin IDE mode and does not change scheduler/tool-result<system-reminder>escaping; those are follow-up scopes.Testing Matrix
Testing matrix notes:
Linked Issues / Bugs
Refs #3712
This PR handles the core IDE context merge only. Re-enabling
/rewindin IDE mode and any related CLI UI cleanup should be handled in a separate follow-up PR.