Conversation
There was a problem hiding this comment.
Pull request overview
Reverts the Phase 1.5 ChatRAGBuilder.buildContext system-prompt assembly ordering to help bisect a “silence regression” by restoring the prior injection sequence.
Changes:
- Moves HumanPresenceTracker injection back to the start of section 2.4 (before the generic RAG section loop).
- Moves tool-definitions injection back to the end of section 2.4 (after the generic RAG section loop).
- Removes the Phase 1.5 ordering rationale comments tied to prefix-KV-cache stability.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const lines = allPresence.map(p => { | ||
| const viewingThis = p.roomId === contextId; | ||
| return `- ${p.displayName} is viewing: ${p.roomName}${viewingThis ? ' (this room — they can see your response in real-time)' : ''}`; | ||
| }); | ||
| finalIdentity.systemPrompt = finalIdentity.systemPrompt + | ||
| `\n\n## HUMAN PRESENCE\n${lines.join('\n')}`; | ||
| } |
There was a problem hiding this comment.
displayName and roomName come from presence events (ultimately user/room-provided strings) and are interpolated directly into the system prompt. This allows prompt-injection via crafted names (e.g., embedded newlines/markdown headers/instructions) at the highest-priority prompt level. Consider sanitizing/escaping these fields (at least strip newlines and other control characters, enforce a tight length cap) and/or clearly delimiting them as untrusted data (e.g., quoted/escaped) before inserting into systemPrompt.
| // 2.4.1. Inject human presence awareness (which room each user is viewing) | ||
| // This is NOT a RAG source — it's lightweight synchronous state, always injected. | ||
| const allPresence = HumanPresenceTracker.allPresence; | ||
| if (allPresence.length > 0) { | ||
| const lines = allPresence.map(p => { | ||
| const viewingThis = p.roomId === contextId; | ||
| return `- ${p.displayName} is viewing: ${p.roomName}${viewingThis ? ' (this room — they can see your response in real-time)' : ''}`; | ||
| }); | ||
| finalIdentity.systemPrompt = finalIdentity.systemPrompt + | ||
| `\n\n## HUMAN PRESENCE\n${lines.join('\n')}`; | ||
| } |
There was a problem hiding this comment.
The small-context guard above says tight context models should skip non-essential injections, but human presence is still injected unconditionally. With many users this can bloat the system prompt and defeat the small-context safeguard. Consider gating this block behind !isSmallContext, or truncating/condensing presence output (e.g., cap N users / shorten text) so small-context prompts remain within budget.
| // 2.4.3. Inject XML tool definitions for text-based providers (budget-aware via ToolDefinitionsSource) | ||
| const toolDefinitionsPrompt = systemPromptSections.get('tool-definitions'); | ||
| if (!isSmallContext && toolDefinitionsPrompt) { | ||
| finalIdentity.systemPrompt += toolDefinitionsPrompt; | ||
| injectedCount++; | ||
| this.log(`🔧 ChatRAGBuilder: Injected tool definitions into system prompt (XML format)`); |
There was a problem hiding this comment.
This comment/log claim the injected tool-definitions section is "XML format" and "for text-based providers", but ToolDefinitionsSource can also emit a non-XML behavioral nudge for native-tool providers. Please update the comment and log message to avoid mislabeling what’s being appended (or branch the log based on metadata.format if you need to distinguish XML vs native).
| // 2.4.3. Inject XML tool definitions for text-based providers (budget-aware via ToolDefinitionsSource) | |
| const toolDefinitionsPrompt = systemPromptSections.get('tool-definitions'); | |
| if (!isSmallContext && toolDefinitionsPrompt) { | |
| finalIdentity.systemPrompt += toolDefinitionsPrompt; | |
| injectedCount++; | |
| this.log(`🔧 ChatRAGBuilder: Injected tool definitions into system prompt (XML format)`); | |
| // 2.4.3. Inject tool-definitions content (budget-aware via ToolDefinitionsSource) | |
| const toolDefinitionsPrompt = systemPromptSections.get('tool-definitions'); | |
| if (!isSmallContext && toolDefinitionsPrompt) { | |
| finalIdentity.systemPrompt += toolDefinitionsPrompt; | |
| injectedCount++; | |
| this.log(`🔧 ChatRAGBuilder: Injected tool definitions/instructions into system prompt`); |
Bisecting tonight's silence regression on clean main. Reverting most recent of the 4 merges (#922 Phase 1.5) first. If main responds after this lands, the regression is in #922's reordering. If still silent, next revert is #920.