Conversation
- Adds a configuration option to enable or disable user rules. - This allows users to control whether custom rules are included in the agent's system prompt. Commit generated by The Saturn Pipeline
- Implements loading user-defined rules from a file. - Rules are included in the system prompt within <user_rules> tags. - Adds XML escaping to user rules content for safety. - Limits user rules file size to 1MB and content length to 50k characters. Commit generated by The Saturn Pipeline
- Modifies AgentManager to include user rules in the system prompt. - Adds a flag to control including user rules in the system prompt. Commit generated by The Saturn Pipeline
- Includes user rules in the main agent's system prompt. - Ensures the main agent respects user-defined rules. Commit generated by The Saturn Pipeline
- Adds a dialog for editing user-defined rules. - Allows users to define custom rules that influence agent behavior. - Provides a text editor with markdown support for defining rules. - Adds UI elements to enable/disable user rules and save/clear rules. Commit generated by The Saturn Pipeline
- Integrates user rules into the chat interface. - Adds a menu item to edit user rules. - Dynamically updates the system prompt when user rules are modified. - Monitors the rules file for changes and updates the system prompt accordingly. - Extracts the base system prompt to avoid duplicating directory/rules sections. Commit generated by The Saturn Pipeline
- Remove project-specific rules and response format guidelines from the user rules editor dialog. - This simplifies the dialog and focuses on general guidelines. Commit generated by The Saturn Pipeline
WalkthroughAdds opt-in "user rules" support: introduces EnableUserRules across Mode, AgentConfiguration, and persisted config; adds UserRulesManager to load/sanitize .saturn/rules.md; extends SystemPrompt.Create to include user rules; updates agent creation, review flows, Program, UI (editor, prompt refresh, menu), and related config plumbing. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as ChatInterface
participant SP as SystemPrompt
participant UR as UserRulesManager
participant Agent
rect rgb(238,245,255)
note over UI,SP: Build or refresh system prompt
User->>UI: Start chat / change mode / rules edited
UI->>SP: Create(basePrompt, includeDirectories: true, includeUserRules: cfg.EnableUserRules)
alt includeUserRules = true
SP->>UR: LoadUserRules()
UR-->>SP: (content / empty / error)
SP-->>UI: base + directories + <user_rules>...</user_rules>
else disabled
SP-->>UI: base + directories (no user rules)
end
UI->>Agent: Update Configuration.SystemPrompt (+ update first system message)
end
sequenceDiagram
autonumber
actor User
participant Dialog as UserRulesEditorDialog
participant UR as UserRulesManager
participant CI as ChatInterface
participant SP as SystemPrompt
participant Agent
rect rgb(243,255,243)
note over Dialog,UR: Edit and persist rules
User->>Dialog: Open "Edit User Rules..."
Dialog->>UR: LoadUserRules()
User->>Dialog: Save (enabled/disabled + content)
alt Disabled or cleared
Dialog->>UR: DeleteRulesFile()
else Enabled
Dialog->>UR: SaveRulesAsync(content, createBackup: true)
end
Dialog-->>CI: Notify rules updated / changed
CI->>SP: Create(basePrompt, includeDirectories: true, includeUserRules: cfg.EnableUserRules)
SP-->>CI: New system prompt
CI->>Agent: Update Configuration.SystemPrompt and chat system message
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
Agents/MultiAgent/AgentManager.cs (2)
75-76: High collision risk: Substring applied to prefixed ID truncates most of GUID
$"agent_{Guid.NewGuid():N}".Substring(0, 12)includes the"agent_"prefix in the substring, leaving only ~6 hex chars of entropy. Collisions are likely under load.Apply this diff:
- var agentId = $"agent_{Guid.NewGuid():N}".Substring(0, 12); + var agentId = "agent_" + Guid.NewGuid().ToString("N"); // full 32 hex chars for uniquenessIf you need shorter IDs, truncate the GUID part only, e.g.,
"agent_" + Guid.NewGuid().ToString("N").Substring(0, 12).
364-365: Same ID truncation bug for reviewerId
$"reviewer_{Guid.NewGuid():N}".Substring(0, 16)leaves ~7 hex chars of entropy after the"reviewer_"prefix.Apply this diff:
- var reviewerId = $"reviewer_{Guid.NewGuid():N}".Substring(0, 16); + var reviewerId = "reviewer_" + Guid.NewGuid().ToString("N");Agents/Core/AgentConfiguration.cs (1)
29-61: AddEnableUserRulesto the persistence layerThe new
EnableUserRulesflag is never persisted or restored, so user settings will be lost on reload. Update the following files:
- Configuration/Objects/PersistedAgentConfiguration.cs
- Configuration/ConfigurationManager.cs
- In
FromAgentConfiguration(...)- In
ApplyToAgentConfiguration(...)Suggested changes:
--- a/Configuration/Objects/PersistedAgentConfiguration.cs +++ b/Configuration/Objects/PersistedAgentConfiguration.cs @@ public class PersistedAgentConfiguration public bool? RequireCommandApproval { get; set; } + public bool? EnableUserRules { get; set; }--- a/Configuration/ConfigurationManager.cs +++ b/Configuration/ConfigurationManager.cs @@ public static PersistedAgentConfiguration FromAgentConfiguration(AgentConfiguration agentConfig) return new PersistedAgentConfiguration { Name = agentConfig.Name, Model = agentConfig.Model, Temperature = agentConfig.Temperature, MaxTokens = agentConfig.MaxTokens, TopP = agentConfig.TopP, EnableStreaming = agentConfig.EnableStreaming, MaintainHistory = agentConfig.MaintainHistory, MaxHistoryMessages = agentConfig.MaxHistoryMessages, EnableTools = agentConfig.EnableTools, ToolNames = agentConfig.ToolNames, RequireCommandApproval = agentConfig.RequireCommandApproval, + EnableUserRules = agentConfig.EnableUserRules }; @@ public static void ApplyToAgentConfiguration(AgentConfiguration target, PersistedAgentConfiguration source) target.Name = source.Name ?? target.Name; target.Model = source.Model ?? target.Model; target.Temperature = source.Temperature ?? target.Temperature; target.MaxTokens = source.MaxTokens ?? target.MaxTokens; target.TopP = source.TopP ?? target.TopP; target.EnableStreaming = source.EnableStreaming ?? target.EnableStreaming; target.MaintainHistory = source.MaintainHistory ?? target.MaintainHistory; target.MaxHistoryMessages = source.MaxHistoryMessages ?? target.MaxHistoryMessages; target.EnableTools = source.EnableTools ?? target.EnableTools; target.ToolNames = source.ToolNames ?? target.ToolNames; - target.RequireCommandApproval = source.RequireCommandApproval ?? true; + target.RequireCommandApproval = source.RequireCommandApproval ?? true; + target.EnableUserRules = source.EnableUserRules ?? target.EnableUserRules;UI/ChatInterface.cs (2)
52-65: Initialize EnableUserRules from the agent configuration
currentConfigdoesn't copyEnableUserRules, so it silently reverts to the default (true) even if the agent config disables it.Add the missing copy:
RequireCommandApproval = agent.Configuration.RequireCommandApproval }; + currentConfig.EnableUserRules = agent.Configuration.EnableUserRules;
1324-1338: Rebuild the system prompt consistently to reflect EnableUserRules changesCurrent logic only wraps when no directory markers exist; it won’t remove
<user_rules>when the flag is turned off (or add them when turned on) if the prompt is already wrapped.Normalize by extracting the base prompt and always regenerating:
- if (!string.IsNullOrWhiteSpace(currentConfig.SystemPrompt)) - { - // Check if the prompt already contains directory information markers - if (currentConfig.SystemPrompt.Contains("<current_directory>") || - currentConfig.SystemPrompt.Contains("Current working directory:") || - currentConfig.SystemPrompt.Contains("</current_directory>")) - { - // Already wrapped, assign directly - agent.Configuration.SystemPrompt = currentConfig.SystemPrompt; - } - else - { - // Needs wrapping with directory context - agent.Configuration.SystemPrompt = await SystemPrompt.Create(currentConfig.SystemPrompt, includeDirectories: true, includeUserRules: currentConfig.EnableUserRules); - } - } + if (!string.IsNullOrWhiteSpace(currentConfig.SystemPrompt)) + { + var basePrompt = ExtractBaseSystemPrompt(currentConfig.SystemPrompt); + agent.Configuration.SystemPrompt = await SystemPrompt.Create( + basePrompt, includeDirectories: true, includeUserRules: currentConfig.EnableUserRules); + }This keeps the wrapper idempotent and ensures the rules section reliably matches the toggle.
🧹 Nitpick comments (6)
Core/UserRulesManager.cs (3)
13-21: Environment.CurrentDirectory may drift; consider a stable workspace root providerIf the app changes current directory (e.g., via tools/agents), rules path resolution will change. Prefer a stable base (e.g., repository root via GitManager, config-driven root, or AppContext.BaseDirectory) and centralize it.
I can wire this through a small WorkspacePaths utility and update call sites.
45-48: Inconsistent over-length handling between load (truncate) and save (throw)Load truncates >50k content, but Save throws. This can confuse users who can load but cannot save the same file. Choose one behavior (truncate with note or hard limit) and apply consistently.
Apply this diff to optionally truncate on save (default: truncate):
- public static async Task<bool> SaveRulesAsync(string content, bool createBackup = true) + public static async Task<bool> SaveRulesAsync(string content, bool createBackup = true, bool truncateIfTooLong = true) { @@ - if (content != null && content.Length > MaxContentLength) - { - throw new ArgumentException($"Content too long (max {MaxContentLength} characters)"); - } + if (content != null && content.Length > MaxContentLength) + { + if (truncateIfTooLong) + { + content = content.Substring(0, MaxContentLength) + "\n[Content truncated - rules file too long]"; + } + else + { + throw new ArgumentException($"Content too long (max {MaxContentLength} characters)"); + } + }Also applies to: 64-67
160-169: Prefer UTC timestamps for status to avoid timezone surprisesGetRulesFileStatus returns LastWriteTime (local). Use LastWriteTimeUtc to keep UI and logs consistent across environments.
Apply this diff:
- return (true, fileInfo.Length, fileInfo.LastWriteTime); + return (true, fileInfo.Length, fileInfo.LastWriteTimeUtc);Agents/MultiAgent/AgentManager.cs (1)
433-444: Timeout path should cancel the in-flight review task to avoid resource leaksWhen timeout wins, the review task continues running. If Agent/transport supports cancellation, add a CancellationTokenSource and cancel here; otherwise, at least log/track for cleanup.
If
Execute<T>supports cancellation:- var reviewTask = reviewerAgent.Execute<Message>(reviewPrompt); - var timeoutTask = Task.Delay(prefs.ReviewTimeoutSeconds * 1000); + using var cts = new CancellationTokenSource(); + var reviewTask = reviewerAgent.Execute<Message>(reviewPrompt, cts.Token); + var timeoutTask = Task.Delay(prefs.ReviewTimeoutSeconds * 1000, cts.Token); @@ - if (completedTask == timeoutTask) + if (completedTask == timeoutTask) { + cts.Cancel(); _reviewers.TryRemove(reviewerId, out _); return new ReviewDecision { Status = ReviewStatus.Approved, Feedback = "Review timed out - auto-approved" }; }UI/ChatInterface.cs (2)
1353-1375: Avoid duplicating marker strings; centralize prompt markers
ExtractBaseSystemPrompthardcodes tag strings that are also defined inAgents/SystemPrompt. To avoid drift, consider moving these markers to a shared static class (e.g.,PromptMarkers) or exposing them as internal constants.If you introduce a shared
PromptMarkers, this method becomes more robust and self-documenting.
1377-1457: Persist configuration after background rules refresh (optional)When rules change, you update
agent.Configuration.SystemPromptandcurrentConfig.SystemPrompt, but changes aren’t persisted. Consider saving so a restart uses the fresh prompt.Add:
currentConfig.SystemPrompt = freshSystemPrompt; + // Persist the refreshed prompt so it survives restarts + await ConfigurationManager.SaveConfigurationAsync( + ConfigurationManager.FromAgentConfiguration(agent.Configuration)); + // If we're in an active session, update the system message in chat history
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
Agents/Core/AgentConfiguration.cs(1 hunks)Agents/Core/Mode.cs(6 hunks)Agents/MultiAgent/AgentManager.cs(2 hunks)Agents/SystemPrompt.cs(4 hunks)Core/UserRulesManager.cs(1 hunks)Program.cs(1 hunks)UI/AgentConfiguration.cs(2 hunks)UI/ChatInterface.cs(7 hunks)UI/Dialogs/UserRulesEditorDialog.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
Agents/SystemPrompt.cs (2)
Program.cs (2)
Task(18-54)Task(57-227)Core/UserRulesManager.cs (1)
FileInfo(154-158)
Core/UserRulesManager.cs (1)
UI/Dialogs/UserRulesEditorDialog.cs (4)
GetRulesFilePath(117-125)Task(127-151)Task(165-269)GetDefaultRulesTemplate(153-163)
UI/Dialogs/UserRulesEditorDialog.cs (1)
Core/UserRulesManager.cs (5)
GetRulesFilePath(13-21)Task(28-56)Task(58-92)Task(111-124)GetDefaultRulesTemplate(126-152)
UI/ChatInterface.cs (4)
Program.cs (2)
Task(18-54)Task(57-227)Configuration/ConfigurationManager.cs (3)
Task(28-52)Task(54-70)ConfigurationManager(11-104)Agents/SystemPrompt.cs (1)
SystemPrompt(11-117)OpenRouter/Serialization/Json.cs (1)
Serialize(40-41)
Agents/MultiAgent/AgentManager.cs (1)
Agents/SystemPrompt.cs (1)
SystemPrompt(11-117)
🔇 Additional comments (13)
UI/AgentConfiguration.cs (2)
23-23: Property addition looks good; default aligns with coreEnableUserRules defaulting to true is consistent with Mode defaults.
137-139: Clone completeness: good propagation of new flagCloning EnableUserRules and RequireCommandApproval maintains behavioral parity.
Agents/MultiAgent/AgentManager.cs (1)
404-415: Reviewer prompt excludes user rules: good choiceKeeping reviewers independent of workspace rules reduces bias. No action needed.
Agents/Core/Mode.cs (6)
23-24: New EnableUserRules flag: good additionAdding this at the Mode level makes the feature first-class in presets.
42-44: Defaulting EnableUserRules = true matches UI and Core defaultsConsistency across layers is good. No action.
71-73: Propagating RequireCommandApproval and EnableUserRules in CreateDefault is correctDefaults look coherent.
93-94: ApplyToConfiguration correctly copies EnableUserRulesThis ensures runtime config respects the selected mode.
127-128: FromConfiguration copies EnableUserRules back into ModeRound-tripping keeps modes and configs aligned.
150-151: Clone includes EnableUserRulesCopy semantics are complete.
Agents/SystemPrompt.cs (2)
107-116: XML escaping is adequate for element text; no need to escape quotes
<user_rules>contains only text, so&,<,>escaping is sufficient. Leaving quotes unescaped is fine and keeps Markdown readable.
21-33: Verify default flags in the CLI invocation of SystemPrompt.CreateIt looks like every call site has been updated to pass the new
includeDirectoriesandincludeUserRulesflags explicitly—except one in your CLI startup (Program.cs). Please confirm that the defaults (includeDirectories: true,includeUserRules: true) are correct here, or update the call to use named arguments for clarity.• Program.cs (ln 88):
SystemPrompt = await SystemPrompt.Create( @"You are a CLI based coding assistant…" // currently relying on defaults: includeDirectories = true, includeUserRules = true );– If you intend to keep both features enabled, no change is strictly necessary.
– Otherwise, change to:- SystemPrompt.Create(@"You are a CLI…"); + SystemPrompt.Create( + @"You are a CLI…", + includeDirectories: false, // or true + includeUserRules: false // or true + );All other call sites already use named args consistent with the new signature:
- UI/ChatInterface.cs: 1145, 1337, 1430
- Agents/MultiAgent/AgentManager.cs: 85, 407
UI/Dialogs/UserRulesEditorDialog.cs (1)
90-116: Nice UX touchesKeyboard shortcuts, colored status, and auto-close after save are thoughtful and improve usability.
UI/ChatInterface.cs (1)
218-221: Great addition — explicit “Edit User Rules…” entryThis improves discoverability of the feature in the UI.
Core/UserRulesManager.cs
Outdated
| private static readonly string RulesFileName = "rules.md"; | ||
| private static readonly int MaxFileSize = 1024 * 1024; // 1MB | ||
| private static readonly int MaxContentLength = 50000; // 50k characters | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Expose limits and reuse this manager from SystemPrompt and UI editor to avoid duplication
Max limits here duplicate similar constants and file I/O in Agents/SystemPrompt.cs and UI/Dialogs/UserRulesEditorDialog.cs. Centralize by exposing a load API (already exists) and reusing it in SystemPrompt to ensure one source of truth (path, size checks, truncation text).
As a follow-up, update SystemPrompt.LoadUserRules to call into this manager and then perform only escaping/section wrapping. See proposed patch in SystemPrompt.cs below.
🤖 Prompt for AI Agents
In Core/UserRulesManager.cs around lines 9-12, the file defines private
constants (RulesFileName, MaxFileSize, MaxContentLength) that are duplicated
elsewhere; expose these limits and file metadata via public properties/methods
and centralize file I/O: make RulesFileName, MaxFileSize and MaxContentLength
public (or add GetLimits/GetFileInfo method) and ensure LoadUserRules returns
the raw loaded text plus an indicator if it was truncated (and the truncation
notice text). Then update Agents/SystemPrompt.LoadUserRules to call
UserRulesManager.LoadUserRules and only perform escaping/section wrapping
(remove duplicate path/size/truncation logic), and update
UI/Dialogs/UserRulesEditorDialog to reuse UserRulesManager for path, size checks
and loading/saving logic so all three consumers share one source of truth.
| public static async Task<bool> SaveRulesAsync(string content, bool createBackup = true) | ||
| { | ||
| var rulesPath = GetRulesFilePath(); | ||
|
|
||
| try | ||
| { | ||
| if (content != null && content.Length > MaxContentLength) | ||
| { | ||
| throw new ArgumentException($"Content too long (max {MaxContentLength} characters)"); | ||
| } | ||
|
|
||
| if (createBackup && File.Exists(rulesPath)) | ||
| { | ||
| var backupPath = rulesPath + ".backup"; | ||
| File.Copy(rulesPath, backupPath, overwrite: true); | ||
| } | ||
|
|
||
| if (string.IsNullOrWhiteSpace(content)) | ||
| { | ||
| if (File.Exists(rulesPath)) | ||
| { | ||
| File.Delete(rulesPath); | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| // Save content | ||
| await File.WriteAllTextAsync(rulesPath, content).ConfigureAwait(false); | ||
| return true; | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| throw new InvalidOperationException($"Error saving rules: {ex.Message}", ex); | ||
| } | ||
| } |
There was a problem hiding this comment.
Use atomic writes with File.Replace to avoid partial/corrupted saves
Current save writes directly to the target path. A crash or abrupt termination can leave a truncated/corrupted rules.md. Use a temp file + File.Replace for atomicity; it also preserves ACLs.
Apply this diff:
public static async Task<bool> SaveRulesAsync(string content, bool createBackup = true)
{
var rulesPath = GetRulesFilePath();
try
{
if (content != null && content.Length > MaxContentLength)
{
throw new ArgumentException($"Content too long (max {MaxContentLength} characters)");
}
- if (createBackup && File.Exists(rulesPath))
- {
- var backupPath = rulesPath + ".backup";
- File.Copy(rulesPath, backupPath, overwrite: true);
- }
-
if (string.IsNullOrWhiteSpace(content))
{
if (File.Exists(rulesPath))
{
File.Delete(rulesPath);
}
return true;
}
- // Save content
- await File.WriteAllTextAsync(rulesPath, content).ConfigureAwait(false);
+ // Save content atomically
+ var tmpPath = rulesPath + ".tmp";
+ await File.WriteAllTextAsync(tmpPath, content).ConfigureAwait(false);
+ if (createBackup && File.Exists(rulesPath))
+ {
+ var backupPath = rulesPath + ".backup";
+ // File.Replace is atomic on supported platforms
+ File.Replace(tmpPath, rulesPath, backupPath, ignoreMetadataErrors: true);
+ }
+ else
+ {
+ // If no original exists, move into place
+ if (File.Exists(rulesPath)) File.Delete(rulesPath);
+ File.Move(tmpPath, rulesPath);
+ }
return true;
}
catch (Exception ex)
{
throw new InvalidOperationException($"Error saving rules: {ex.Message}", ex);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static async Task<bool> SaveRulesAsync(string content, bool createBackup = true) | |
| { | |
| var rulesPath = GetRulesFilePath(); | |
| try | |
| { | |
| if (content != null && content.Length > MaxContentLength) | |
| { | |
| throw new ArgumentException($"Content too long (max {MaxContentLength} characters)"); | |
| } | |
| if (createBackup && File.Exists(rulesPath)) | |
| { | |
| var backupPath = rulesPath + ".backup"; | |
| File.Copy(rulesPath, backupPath, overwrite: true); | |
| } | |
| if (string.IsNullOrWhiteSpace(content)) | |
| { | |
| if (File.Exists(rulesPath)) | |
| { | |
| File.Delete(rulesPath); | |
| } | |
| return true; | |
| } | |
| // Save content | |
| await File.WriteAllTextAsync(rulesPath, content).ConfigureAwait(false); | |
| return true; | |
| } | |
| catch (Exception ex) | |
| { | |
| throw new InvalidOperationException($"Error saving rules: {ex.Message}", ex); | |
| } | |
| } | |
| public static async Task<bool> SaveRulesAsync(string content, bool createBackup = true) | |
| { | |
| var rulesPath = GetRulesFilePath(); | |
| try | |
| { | |
| if (content != null && content.Length > MaxContentLength) | |
| { | |
| throw new ArgumentException($"Content too long (max {MaxContentLength} characters)"); | |
| } | |
| if (string.IsNullOrWhiteSpace(content)) | |
| { | |
| if (File.Exists(rulesPath)) | |
| { | |
| File.Delete(rulesPath); | |
| } | |
| return true; | |
| } | |
| // Save content atomically | |
| var tmpPath = rulesPath + ".tmp"; | |
| await File.WriteAllTextAsync(tmpPath, content).ConfigureAwait(false); | |
| if (createBackup && File.Exists(rulesPath)) | |
| { | |
| var backupPath = rulesPath + ".backup"; | |
| // File.Replace is atomic on supported platforms | |
| File.Replace(tmpPath, rulesPath, backupPath, ignoreMetadataErrors: true); | |
| } | |
| else | |
| { | |
| // If no original exists, move into place | |
| if (File.Exists(rulesPath)) File.Delete(rulesPath); | |
| File.Move(tmpPath, rulesPath); | |
| } | |
| return true; | |
| } | |
| catch (Exception ex) | |
| { | |
| throw new InvalidOperationException($"Error saving rules: {ex.Message}", ex); | |
| } | |
| } |
| enabledCheckBox = new CheckBox("Enable user rules (uncheck to temporarily disable)") | ||
| { | ||
| X = 1, | ||
| Y = 3, | ||
| Checked = true | ||
| }; | ||
| enabledCheckBox.Toggled += (prev) => RulesEnabled = enabledCheckBox.Checked; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
RulesEnabled is never consumed by UI logic — wire it into configuration
The checkbox toggles RulesEnabled, but nothing reads it to enable/disable inclusion in prompts. Ensure ChatInterface.ShowUserRulesEditorAsync updates currentConfig.EnableUserRules from dialog.RulesEnabled. See ChatInterface diff in the next file.
Also applies to: 18-21
🏁 Script executed:
#!/bin/bash
# Show all references to RulesEnabled
rg -n -S "RulesEnabled" -C2Length of output: 853
RulesEnabled isn’t wired into the chat configuration—please propagate it after the dialog
The enabledCheckBox correctly updates UserRulesEditorDialog.RulesEnabled, but nothing reads that flag, so disabling rules there has no effect on the prompt logic. After calling ShowUserRulesEditorAsync, assign the dialog’s RulesEnabled value into your chat configuration.
• In ChatInterface.ShowUserRulesEditorAsync (or the caller), add:
// after awaiting the dialog…
currentConfig.EnableUserRules = dialog.RulesEnabled;• Verify that currentConfig.EnableUserRules is then used when composing prompts to include or skip user rules.
🤖 Prompt for AI Agents
In UI/Dialogs/UserRulesEditorDialog.cs around lines 50-57 the enabledCheckBox
correctly updates UserRulesEditorDialog.RulesEnabled but that flag is never
propagated to the chat configuration; after awaiting ShowUserRulesEditorAsync
(in ChatInterface.ShowUserRulesEditorAsync or its caller) assign the dialog's
RulesEnabled to your chat config (e.g., currentConfig.EnableUserRules =
dialog.RulesEnabled) and then ensure the prompt-composition logic reads
currentConfig.EnableUserRules to include or skip user rules accordingly.
- Adds a boolean flag to agent configuration to enable/disable user rules. - This allows agents to selectively include or exclude user-defined rules during operation. Commit generated by The Saturn Pipeline
- Moves user rules loading logic to UserRulesManager. - This provides a single source of truth for loading and managing user rules. - Adds error handling and truncation logic to the manager. Commit generated by The Saturn Pipeline
- Adds functionality for parent agents to control user rules for sub-agents. - Sub-agents can inherit the parent's EnableUserRules setting or override it. Commit generated by The Saturn Pipeline
- Adds a checkbox to the user rules editor dialog to enable/disable rules. - Updates the agent configuration based on the checkbox state. - Preserves the rules file even when disabled. Commit generated by The Saturn Pipeline
- Persists the EnableUserRules setting in the agent configuration. - The UI now correctly reflects and modifies the persisted state. Commit generated by The Saturn Pipeline
- Respect EnableUserRules from persisted config or default to true. Commit generated by The Saturn Pipeline
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
UI/ChatInterface.cs (2)
711-713: Bug: newline handling duplicated — causes incorrect spacing in non-streaming path
endsWithNewlineandendsWithDoubleNewlineboth check for “\n\n”. The first should check a single newline to mirror the streaming branch, otherwise spacing logic is wrong.- bool endsWithNewline = updatedText.EndsWith("\n\n"); - bool endsWithDoubleNewline = updatedText.EndsWith("\n\n"); + bool endsWithNewline = updatedText.EndsWith("\n"); + bool endsWithDoubleNewline = updatedText.EndsWith("\n\n");
1308-1347: Persist EnableUserRules alongside other settings in UpdateConfiguration()
UpdateConfiguration()updates many fields but omitsEnableUserRules. If modes or other UI flows toggle it, the persisted config will be stale, and subsequent launches won’t reflect the change.agent.Configuration.ToolNames = currentConfig.ToolNames ?? new List<string>(); agent.Configuration.RequireCommandApproval = currentConfig.RequireCommandApproval; + agent.Configuration.EnableUserRules = currentConfig.EnableUserRules; if (!string.IsNullOrWhiteSpace(currentConfig.SystemPrompt)) { // Check if the prompt already contains directory information markers if (currentConfig.SystemPrompt.Contains("<current_directory>") || currentConfig.SystemPrompt.Contains("Current working directory:") || currentConfig.SystemPrompt.Contains("</current_directory>")) { // Already wrapped, assign directly agent.Configuration.SystemPrompt = currentConfig.SystemPrompt; } else { // Needs wrapping with directory context - agent.Configuration.SystemPrompt = await SystemPrompt.Create(currentConfig.SystemPrompt, includeDirectories: true, includeUserRules: currentConfig.EnableUserRules); + agent.Configuration.SystemPrompt = await SystemPrompt.Create( + currentConfig.SystemPrompt, + includeDirectories: true, + includeUserRules: currentConfig.EnableUserRules); } }
♻️ Duplicate comments (3)
Core/UserRulesManager.cs (1)
69-103: Make saves atomic to prevent truncated/corrupted rules.md on crashesWriting directly to the target path risks partial files if the process crashes or the OS is interrupted mid-write. Use a temp file and File.Replace where available.
Apply this diff:
public static async Task<bool> SaveRulesAsync(string content, bool createBackup = true) { var rulesPath = GetRulesFilePath(); try { if (content != null && content.Length > MaxContentLength) { throw new ArgumentException($"Content too long (max {MaxContentLength} characters)"); } - - if (createBackup && File.Exists(rulesPath)) - { - var backupPath = rulesPath + ".backup"; - File.Copy(rulesPath, backupPath, overwrite: true); - } - if (string.IsNullOrWhiteSpace(content)) { if (File.Exists(rulesPath)) { File.Delete(rulesPath); } return true; } - // Save content - await File.WriteAllTextAsync(rulesPath, content).ConfigureAwait(false); + // Save content atomically + var tmpPath = rulesPath + ".tmp"; + await File.WriteAllTextAsync(tmpPath, content).ConfigureAwait(false); + if (createBackup && File.Exists(rulesPath)) + { + var backupPath = rulesPath + ".backup"; + File.Replace(tmpPath, rulesPath, backupPath, ignoreMetadataErrors: true); + } + else + { + if (File.Exists(rulesPath)) File.Delete(rulesPath); + File.Move(tmpPath, rulesPath); + } return true; } catch (Exception ex) { throw new InvalidOperationException($"Error saving rules: {ex.Message}", ex); } }Program.cs (1)
85-87: Good fix: rules inclusion now respects persisted configurationYou correctly derive
enableUserRulesfrompersistedConfig(defaulting to true). This addresses the earlier issue where rules were always included. Nice.UI/ChatInterface.cs (1)
1130-1163: Propagate enable/disable state and improve status text for clarityGood: you sync
dialog.RulesEnabledto bothagent.ConfigurationandcurrentConfig, rebuild the prompt, and persist. Enhance the status line to reflect both enablement and file presence so users aren’t misled when the file exists but rules are disabled. Also, propagate the updated parent default to the AgentManager for sub-agents created later in the same session.- var rulesPath = Path.Combine(Environment.CurrentDirectory, ".saturn", "rules.md"); - var statusText = File.Exists(rulesPath) ? "Rules file exists and will be included" : "No rules file - rules disabled"; + var rulesPath = Path.Combine(Environment.CurrentDirectory, ".saturn", "rules.md"); + var statusText = $"{(currentConfig.EnableUserRules ? "Rules enabled" : "Rules disabled")} • " + + $"{(File.Exists(rulesPath) ? "file present" : "no rules file")}"; UpdateAgentStatus($"Ready - {statusText}"); @@ // Update both agent and current config agent.Configuration.SystemPrompt = newSystemPrompt; currentConfig.SystemPrompt = newSystemPrompt; // Save the updated configuration await ConfigurationManager.SaveConfigurationAsync( ConfigurationManager.FromAgentConfiguration(agent.Configuration)); + + // Ensure sub-agents use the latest default + AgentManager.Instance.SetParentEnableUserRules(agent.Configuration.EnableUserRules);
🧹 Nitpick comments (11)
Core/UserRulesManager.cs (3)
24-27: Avoid side-effects in RulesFileExists (don’t create .saturn just to check existence)GetRulesFilePath creates the directory; calling RulesFileExists should not. Compute the path without creating directories.
Apply this diff:
- public static bool RulesFileExists() - { - return File.Exists(GetRulesFilePath()); - } + public static bool RulesFileExists() + { + var saturnDir = Path.Combine(Environment.CurrentDirectory, ".saturn"); + var path = Path.Combine(saturnDir, RulesFileName); + return File.Exists(path); + }Optionally mirror this “no side-effect” approach in GetRulesFileInfo/GetRulesFileStatus.
14-22: Allow overriding the workspace root for tests/embeddersHardwiring Environment.CurrentDirectory makes testing and embedding harder. Support an env var override for the workspace root; fall back to CWD.
Apply this diff:
- public static string GetRulesFilePath() - { - var saturnDir = Path.Combine(Environment.CurrentDirectory, ".saturn"); + public static string GetRulesFilePath() + { + var workspaceRoot = Environment.GetEnvironmentVariable("SATURN_WORKSPACE_DIR"); + var baseDir = string.IsNullOrWhiteSpace(workspaceRoot) ? Environment.CurrentDirectory : workspaceRoot!; + var saturnDir = Path.Combine(baseDir, ".saturn"); if (!Directory.Exists(saturnDir)) { Directory.CreateDirectory(saturnDir); } return Path.Combine(saturnDir, RulesFileName); }
9-13: Use const for immutable metadataThese fields never change at runtime; declare them const to clarify immutability and enable compile-time usage.
Apply this diff:
- public static readonly string RulesFileName = "rules.md"; - public static readonly int MaxFileSize = 1024 * 1024; // 1MB - public static readonly int MaxContentLength = 50000; // 50k characters - public static readonly string TruncationNotice = "\n[Content truncated - rules file too long]"; + public const string RulesFileName = "rules.md"; + public const int MaxFileSize = 1024 * 1024; // 1MB + public const int MaxContentLength = 50000; // 50k characters + public const string TruncationNotice = "\n[Content truncated - rules file too long]";Configuration/ConfigurationManager.cs (2)
68-75: Don’t swallow save errors silently — at least log themSaveConfigurationAsync catches and ignores exceptions. At minimum, log to stderr; ideally return a bool or throw.
Apply this minimal diff:
public static async Task SaveConfigurationAsync(PersistedAgentConfiguration config) { try { @@ var json = JsonSerializer.Serialize(config, JsonOptions); await File.WriteAllTextAsync(ConfigFilePath, json); } catch (Exception ex) { - + Console.Error.WriteLine($"[ConfigurationManager] Failed to save config: {ex}"); } }
52-56: Load errors are also swallowed — add loggingReturning null hides root causes (permissions, malformed JSON). Log the exception to aid diagnosis.
Apply this diff:
public static async Task<PersistedAgentConfiguration?> LoadConfigurationAsync() { try { @@ return config; } catch (Exception ex) { - + Console.Error.WriteLine($"[ConfigurationManager] Failed to load config from {ConfigFilePath}: {ex}"); return null; } }Agents/SystemPrompt.cs (1)
83-92: Escape quotes too (or use built-in XML escaper)To be safe in all contexts, also escape " and '. Alternatively, use System.Security.SecurityElement.Escape.
Apply this diff:
private static string EscapeXmlContent(string content) { if (string.IsNullOrEmpty(content)) return content; return content .Replace("&", "&") .Replace("<", "<") - .Replace(">", ">"); + .Replace(">", ">") + .Replace("\"", """) + .Replace("'", "'"); }Program.cs (1)
78-83: Avoid duplicate “gpt-5 temperature override” logicYou set temperature for GPT‑5 twice (pre- and post-config). Keep only the post-config block to reduce drift and surprises.
- var temperature = 0.15; - if (model.Contains("gpt-5", StringComparison.OrdinalIgnoreCase)) - { - temperature = 1.0; - } + var temperature = 0.15;Also applies to: 225-228
UI/Dialogs/UserRulesEditorDialog.cs (2)
171-173: Null-safety: guard against null Text in TextView and unused tuple elementsStatic analysis flags a possible null dereference. Guard the read and discard unused tuple items to prevent future warnings.
- var content = rulesTextView.Text.ToString(); - var (maxFileSize, maxContentLength, fileName) = UserRulesManager.GetLimits(); + var content = rulesTextView.Text?.ToString() ?? string.Empty; + var (_, maxContentLength, _) = UserRulesManager.GetLimits();After this change, the “dereference of a possibly null reference” warnings should disappear.
267-280: ShowFileInfo is unused — either wire it or remove to reduce driftConsider adding an “File Info…” button that calls this, or drop the method to avoid dead code.
UI/ChatInterface.cs (2)
1358-1380: String-slicing the prompt is fragile; consider centralizing section stripping
ExtractBaseSystemPromptduplicates the delimiter strings and depends on exact newline placement. Prefer exposing these tokens (or a “StripSections” helper) fromSystemPromptto avoid drift if the tokens change.I can add public constants for the section markers in SystemPrompt (or a
StripSections(string, bool dir, bool rules)helper) and update the call sites. Want me to open a follow-up PR?
140-147: Proactive: keep AgentManager defaults in sync with session stateYou set
SetParentEnableUserRuleson first session initialization — good. If the user toggles rules mid-session, call this again (already proposed in ShowUserRulesEditorAsync) so sub-agents created later inherit the updated default.Also applies to: 598-606
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
Agents/Core/AgentConfiguration.cs(2 hunks)Agents/MultiAgent/AgentManager.cs(5 hunks)Agents/SystemPrompt.cs(4 hunks)Configuration/ConfigurationManager.cs(3 hunks)Configuration/Objects/PersistedAgentConfiguration.cs(1 hunks)Core/UserRulesManager.cs(1 hunks)Program.cs(3 hunks)UI/ChatInterface.cs(7 hunks)UI/Dialogs/UserRulesEditorDialog.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Agents/Core/AgentConfiguration.cs
🧰 Additional context used
🧬 Code graph analysis (5)
Agents/SystemPrompt.cs (2)
Program.cs (2)
Task(18-54)Task(57-231)Core/UserRulesManager.cs (5)
Task(29-33)Task(35-67)Task(69-103)Task(122-135)UserRulesManager(7-186)
Core/UserRulesManager.cs (2)
UI/Dialogs/UserRulesEditorDialog.cs (2)
Task(118-164)Task(167-252)Agents/SystemPrompt.cs (3)
Task(20-44)Task(46-65)Task(67-81)
UI/Dialogs/UserRulesEditorDialog.cs (1)
Core/UserRulesManager.cs (8)
Task(29-33)Task(35-67)Task(69-103)Task(122-135)UserRulesManager(7-186)GetDefaultRulesTemplate(137-163)maxFileSize(165-168)exists(176-185)
UI/ChatInterface.cs (6)
Agents/MultiAgent/AgentManager.cs (1)
SetParentEnableUserRules(56-59)Program.cs (2)
Task(18-54)Task(57-231)Configuration/ConfigurationManager.cs (3)
Task(28-57)Task(59-75)ConfigurationManager(11-111)Agents/SystemPrompt.cs (1)
SystemPrompt(12-93)Core/UserRulesManager.cs (1)
FileInfo(170-174)OpenRouter/Serialization/Json.cs (1)
Serialize(40-41)
Agents/MultiAgent/AgentManager.cs (1)
Agents/SystemPrompt.cs (1)
SystemPrompt(12-93)
🪛 GitHub Check: Build and Test (macos-latest, Release)
UI/Dialogs/UserRulesEditorDialog.cs
[warning] 189-189:
Dereference of a possibly null reference.
🪛 GitHub Check: Build and Test (macos-latest, Debug)
UI/Dialogs/UserRulesEditorDialog.cs
[warning] 189-189:
Dereference of a possibly null reference.
🪛 GitHub Check: Build and Test (windows-latest, Release)
UI/Dialogs/UserRulesEditorDialog.cs
[warning] 189-189:
Dereference of a possibly null reference.
🪛 GitHub Check: Build and Test (windows-latest, Debug)
UI/Dialogs/UserRulesEditorDialog.cs
[warning] 189-189:
Dereference of a possibly null reference.
🪛 GitHub Check: Build and Test (ubuntu-latest, Debug)
UI/Dialogs/UserRulesEditorDialog.cs
[warning] 189-189:
Dereference of a possibly null reference.
🪛 GitHub Check: Build and Test (ubuntu-latest, Release)
UI/Dialogs/UserRulesEditorDialog.cs
[warning] 189-189:
Dereference of a possibly null reference.
🔇 Additional comments (11)
Configuration/Objects/PersistedAgentConfiguration.cs (1)
22-22: Add persisted EnableUserRules toggle — LGTMProperty addition is consistent with existing nullable-boolean flags (e.g., RequireCommandApproval) and works well with the JSON options (camelCase + ignore nulls). No concerns.
Configuration/ConfigurationManager.cs (2)
91-93: Correct mapping of EnableUserRules — LGTMPlumbing from AgentConfiguration to persisted config and back is consistent with other settings.
Also applies to: 109-110
45-48: Default EnableUserRules is always true — please confirm opt-in vs. opt-out semanticsI see that whenever
EnableUserRulesis missing or null, the code unconditionally defaults it totruein multiple places:• In ConfigurationManager (Configuration/ConfigurationManager.cs:45-48, 109) it sets
config.EnableUserRules = trueif the persisted value isnull.
• In the persisted-to-runtime mapping (Program.cs:85-86) it doespersistedConfig?.EnableUserRules ?? true.
• BothAgents/Core/AgentConfigurationandUI/AgentConfigurationmodel classes initializeEnableUserRulestotrueby default.
• New Modes (Agents/Core/Mode.cs) also defaultEnableUserRules = truein their constructors.Since the PR text describes “opt-in” semantics (which usually implies a default of
false), please confirm the intended behavior:
- If all agents should have user rules enabled by default (opt-out), then the code is correct but the PR summary should reflect “opt-out” semantics.
- If users must opt in before rules can run, we should change these defaults to
false(and update tests/UI accordingly).Let me know which behavior is desired so we can align code, tests, and documentation.
Agents/SystemPrompt.cs (1)
33-40: Conditional inclusion of user rules — LGTMThe feature flag is cleanly threaded into the prompt assembly and respects the caller’s choice.
Agents/MultiAgent/AgentManager.cs (3)
74-76: Per-subagent includeUserRules override — LGTMOptional parameter with fallback to the parent flag keeps behavior flexible and backward-compatible.
Also applies to: 97-97
419-419: Good call: disable user rules for reviewer agentReviewers should be unbiased; forcing includeUserRules: false makes sense.
56-64: No further action needed: SetParentEnableUserRules is invoked by callersThe search confirms that
SetParentEnableUserRulesis called inUI/ChatInterface.cs(line 610), passing the loadedagent.Configuration.EnableUserRules. This ensures parent-level user rules are correctly applied before sub-agents are initialized.Program.cs (1)
194-206: SystemPrompt now parameterized by EnableUserRules — aligned with configPassing
includeUserRules: enableUserRulesand persistingEnableUserRules = enableUserRuleskeeps runtime and persisted state in sync.UI/Dialogs/UserRulesEditorDialog.cs (3)
50-57: Enable/disable toggle behavior is correct and statefulTying the checkbox to
RulesEnabledviaToggledis clean. The dialog cleanly communicates intent (“temporarily disable”) without deleting the file.
117-164: Nice: centralized reads via Core.UserRulesManager with robust UXLeveraging
UserRulesManager.LoadUserRules()and the default template keeps behavior consistent with the prompt builder. The truncation and colored status UX is helpful.
167-186: Disable without deletion flow is correct; save path uses centralized helperThe early return when disabled plus
SaveRulesAsync(content, createBackup: true)is the right balance of safety and consistency.Also applies to: 206-225
- Escape XML content in error messages to prevent XML parsing issues. - This ensures that any special characters in the error message are properly handled when included in the system prompt. Commit generated by The Saturn Pipeline
1 participant
Summary by CodeRabbit