Merged
Conversation
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts the chatmanager system instruction prompt to always embed the character configuration and to conditionally include conversation summaries only when memory abstraction is enabled, and bumps the project patch version. Sequence diagram for updated system instruction prompt constructionsequenceDiagram
participant ChatManager
participant Config
participant ConfigLLM as ConfigLLM
participant ConversationData
ChatManager->>ChatManager: _run()
ChatManager->>Config: get llm
Config->>ConfigLLM: return llm
ChatManager->>ConfigLLM: read enable_memory_abstract
alt enable_memory_abstract is true
ChatManager->>ConversationData: read abstract
ConversationData-->>ChatManager: abstract
ChatManager->>ChatManager: build prompt with SYSTEM_INSTRUCTIONS and SUMMARY
else enable_memory_abstract is false
ChatManager->>ChatManager: build prompt with SYSTEM_INSTRUCTIONS only
end
ChatManager-->>ChatManager: send prompt to LLM
Updated class diagram for ChatManager prompt construction logicclassDiagram
class ChatManager {
dict train
_run()
}
class Config {
ConfigLLM llm
}
class ConfigLLM {
bool enable_memory_abstract
}
class ConversationData {
string abstract
}
ChatManager --> Config : uses
ChatManager --> ConversationData : reads
Config --> ConfigLLM : has
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The conditional omission of the
block when enable_memory_abstractis false may break any downstream logic or prompts that currently assume the tags are always present (even if empty); consider keeping empty tags if consumers rely on their presence. - Accessing
self.train["content"]without a default or guard will raise if the key is missing orNone; consider usingself.train.get("content", "")or validating this earlier to avoid runtime errors. - The interpolated summary string adds a trailing space (
f"\n{data.abstract} \n"); consider trimming or normalizing this to avoid subtle formatting issues in the final prompt.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The conditional omission of the <SUMMARY> block when `enable_memory_abstract` is false may break any downstream logic or prompts that currently assume the tags are always present (even if empty); consider keeping empty tags if consumers rely on their presence.
- Accessing `self.train["content"]` without a default or guard will raise if the key is missing or `None`; consider using `self.train.get("content", "")` or validating this earlier to avoid runtime errors.
- The interpolated summary string adds a trailing space (`f"\n{data.abstract} \n"`); consider trimming or normalizing this to avoid subtle formatting issues in the final prompt.
## Individual Comments
### Comment 1
<location> `src/amrita_core/chatmanager.py:571` </location>
<code_context>
+ + "Your character setting is in the <SYSTEM_INSTRUCTIONS> tags, and the summary of previous conversations is in the <SUMMARY> tags (if provided)."
+ "\n</SCHEMA>\n"
+ "<SYSTEM_INSTRUCTIONS>\n"
+ + self.train["content"]
+ "\n</SYSTEM_INSTRUCTIONS>"
- + f"\n<SUMMARY>\n{data.abstract if config.llm.enable_memory_abstract else ''}\n</SUMMARY>"
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Consider whether logging `self.train["content"]` is appropriate given it may contain sensitive system instructions.
Because this value is embedded directly into `<SYSTEM_INSTRUCTIONS>`, the `logger.debug(self.train["content"])` call will log those instructions verbatim and could expose sensitive context depending on your logging setup. Consider removing this log, gating it behind a stricter debug flag, or sanitizing/truncating the content before logging.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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 by Sourcery
Update chat manager system instructions handling and bump project patch version.
Enhancements:
Build: