fix(hooks): inject SessionStart additionalContext into chat context#4115
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. |
wenshao
previously requested changes
May 14, 2026
Collaborator
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Review Summary
This PR correctly centralizes SessionStart hook firing into GeminiClient.startChat() but introduces several issues where callers that fire their own SessionStart now cause double execution with wrong source semantics, and some paths silently lose hook additionalContext.
Not covered by inline comments (pre-existing issues or out of diff scope):
useBranchCommand.ts:215 — SessionStart(Branch) fires but never calls appendSystemInstruction(). Meanwhile initialize() → startChat(resumedHistory) fires SessionStart(Resume). The Branch hook's additionalContext is silently discarded. Requires the same sessionStartSource parameter fix on startChat().
Test gaps:
appendSystemInstruction()has zero unit tests ingeminiChat.test.ts(3 code paths untested)- SessionStart hook rejection path in
startChat()is untested useBranchCommand.test.ts:245-253mocksinitializeas no-op, masking the productionBranchdouble-fire
— DeepSeek/deepseek-v4-pro via Qwen Code /review
|
初步测试 然后.qwen/settings.json的设置如下 启动qwen-code后，hook返回的结果可以成功注入上下文 |
Collaborator
Author
|
Test result: |
pomelo-nwu
reviewed
May 14, 2026
Collaborator
pomelo-nwu
left a comment
pomelo-nwu
left a comment
There was a problem hiding this comment.
Review notes:
setSessionStartContext()can drop existing system prompt suffixes.
packages/core/src/core/geminiChat.ts splits the current systemInstruction on \n\n---\n\n and keeps only the first segment before appending SessionStart context. That delimiter is already used by packages/core/src/core/prompts.ts to append user memory and append-system-prompt content. As a result, whenever SessionStart.additionalContext is present, existing user memory / project rules / appended system instructions after the first delimiter can be silently removed from the model-visible system prompt. This is a blocking regression risk for users relying on memory or custom prompt suffixes.
- ACP can still double-fire
SessionStart.
newSessionConfig() calls config.initialize(), and Config.initialize() initializes the GeminiClient, which now fires SessionStart through startChat(). Then createAndStoreSession() checks geminiClient.isInitialized() and, when true, directly calls fireSessionStartEvent() again. The new ACP tests mock initialize() as a no-op, so they do not cover the production path where the first hook execution already happened during config initialization. The second execution also does not inject additionalContext into the active chat.
- Auto compact appears to lose
SessionStart(Compact).
The PR removes SessionStart(Compact) from ChatCompressionService and relies on GeminiClient.tryCompressChat() calling startChat(..., SessionStartSource.Compact). That covers the manual compact path, but automatic compression goes through GeminiChat.sendMessageStream() -> this.tryCompress() and then mutates history in place with setHistory(). It does not go through GeminiClient.tryCompressChat(), so automatic compact no longer fires the compact SessionStart hook or injects returned additionalContext.
The motivation is sound: centralize SessionStart and make additionalContext model-visible. I would not merge this version until the delimiter handling, ACP double-fire path, and auto-compact behavior are fixed or explicitly re-scoped with tests.
Reviewed by qwen-code / gpt-5.5
Collaborator
Author
Resolved |
pomelo-nwu
reviewed
May 14, 2026
Collaborator
pomelo-nwu
left a comment
pomelo-nwu
left a comment
There was a problem hiding this comment.
Follow-up review finding:
- ACP still fires SessionStart outside the real ACP session.
runAcpAgent() initializes the bootstrap Config before any ACP newSession / loadSession request. With this PR, Config.initialize() now reaches GeminiClient.initialize() -> startChat() -> fireSessionStartEvent(), so ACP process startup fires SessionStart(startup) even though no ACP session has been created yet. Then newSessionConfig() creates and initializes the per-session config, which fires SessionStart again for the actual session. That means ACP can still duplicate hook side effects, and the first execution uses the bootstrap config/session/cwd rather than the client-requested session. The new ACP tests mock Config.initialize() as a no-op, so they miss this production path.
Please either add a way for the ACP bootstrap initialization to skip chat SessionStart, or restructure ACP initialization so only the real per-session config can fire it. A regression test should let initialize() call through to a GeminiClient-like object that fires SessionStart, rather than only asserting that createAndStoreSession() no longer calls the hook directly.
Reviewed by qwen-code / gpt-5.5
wenshao
previously requested changes
May 14, 2026
Collaborator
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Additional finding (file not in this PR's diff)
[Critical] packages/core/src/hooks/types.ts:298 — Hook output allows system instruction delimiter injection. getAdditionalContext() only escapes < and >, but does not strip \n\n---\n\n — the exact delimiter used by geminiChat.ts to separate system instruction sections. A malicious hook script can embed this delimiter in its additionalContext to inject fake system-level instructions. Consider also stripping or escaping this delimiter:
context.replace(/\n\n---\n\n/g, '\n\n - - - \n\n');— DeepSeek/deepseek-v4-pro via Qwen Code /review
Collaborator
Author
Resolved |
wenshao
reviewed
May 14, 2026
wenshao
reviewed
May 14, 2026
wenshao
previously requested changes
May 14, 2026
Collaborator
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
[Critical] refreshSystemInstruction() silently strips SessionStart additionalContext.
refreshSystemInstruction() (line 476) calls this.chat.setSystemInstruction() which is a blind assignment, silently erasing any SessionStart additionalContext previously injected by hooks. When the user runs /language, the hook's injected context is lost for the rest of the session with no warning.
Suggested fix: After setSystemInstruction(), re-apply the last known SessionStart context by storing lastSessionStartContext/lastSessionStartSource on GeminiClient and calling applySessionStartContext() again.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
Collaborator
Author
|
Test result-ACP: |
yiliang114
approved these changes
May 15, 2026
Merged
6 tasks
TaimoorSiddiquiOfficial
added a commit
to TaimoorSiddiquiOfficial/HopCode
that referenced
this pull request
May 15, 2026
…wenLM#4115) * inject addContext for SessionStart * resolve comment * resolve comment * resolve comment * fix comment * unfiy function and resolve comment * resolve comment (cherry picked from commit 7c2b51d) 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
6 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
SessionStarthook handling forstartup/resumeinto the mainGeminiClient.startChat()path.SessionStart.hookSpecificOutput.additionalContextinto the active chatsystemInstruction.systemMessageas non-model-visible output only.SessionStartfiring from TUIAppContainerand ACPacpAgent.clearandcompactflows.packages/cli/src/serve/server.test.tsfrom CLI build sotsc --builddoes not fail on that test-onlysupertestdependency.
SessionStart.additionalContextwas not reliably injected into model context across session entry points.SessionStarttwice, causing inconsistent behavior and duplicate hook execution.SessionStartbehavior consistent: inject once, through the real chat initialization path, acrossCLI/TUI and ACP.
packages/core/src/core/client.tspackages/core/src/core/geminiChat.tspackages/cli/src/ui/AppContainer.tsxpackages/cli/src/ui/commands/clearCommand.tspackages/core/src/services/chatCompressionService.tspackages/cli/src/acp-integration/acpAgent.tsclient.test.ts,clearCommand.test.ts,chatCompressionService.test.ts,acpAgent.test.tsValidation
npx vitest run packages/core/src/core/client.test.ts packages/core/src/hooks/hookSystem.test.ts npx vitest run packages/cli/src/acp-integration/acpAgent.test.ts npx vitest run packages/cli/src/ui/commands/clearCommand.test.ts packages/core/src/services/chatCompressionService.test.ts packages/cli/src/acp-integration/acpAgent.test.ts npm run buildSessionStartinjectionSessionStartinjectionSessionStartinjectionnewSessionno longer directly firing duplicateSessionStart~/.qwen/settings.jsonfor aSessionStartcommand hook returning:hookSpecificOutput.hookEventName = "SessionStart"hookSpecificOutput.additionalContext = "E2E_SESSIONSTART_TOKEN=alpha-42"SessionStart.additionalContextis injected exactly once into the model-visible chat context.systemMessageremains non-model-visible.SessionStartfor startup/resume.SessionStartand append returned additional context.npm run buildpassed successfully after excludingsrc/serve/server.test.tsfrom CLI TypeScript build inputs.GeminiClient.startChat()and confirmSessionStartis fired there forstartup/resume.AppContainer.tsxacpAgent.tsappendSystemInstruction(...)is used for:packages/core/src/core/client.test.ts+packages/core/src/hooks/hookSystem.test.ts:167 passedpackages/cli/src/acp-integration/acpAgent.test.ts:35 passedpackages/cli/src/ui/commands/clearCommand.test.ts+packages/core/src/services/chatCompressionService.test.ts+Scope / Risk
SessionStartinjection semantics were not changed here.Testing Matrix
Testing matrix notes:
Linked Issues / Bugs
#4111