feat(prompt): add optional user config to prompts#84
feat(prompt): add optional user config to prompts#84todaymare wants to merge 1 commit intojnsahaj:mainfrom
Conversation
Add support for loading optional markdown files as user preferences for explain, draft, and operate prompts. Also restructure prompt generation to separate markdown config from command-specific content.
📝 WalkthroughWalkthroughThe PR extends the AI prompt system to support markdown-based customization by making the configuration path lookup public and adding a helper function to load markdown snippets from the config directory, which are then injected into prompts within a Changes
Sequence DiagramsequenceDiagram
participant Prompt as ai_prompt.rs
participant Config as ConfigureCommand
participant FS as File System
Prompt->>Prompt: explain/draft/operate flow initiated
Prompt->>Prompt: get_md(name)
Prompt->>Config: get_config_path()
Config->>Config: resolve ~/.config/lumen
Config-->>Prompt: PathBuf
Prompt->>FS: read [name].md from config path
FS-->>Prompt: markdown content (or None)
Prompt->>Prompt: wrap in <UserConfig> block<br/>with cmd_prompt + content
Prompt-->>Prompt: final prompt ready
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly Related PRs
Suggested Reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/ai_prompt.rs (1)
209-218: Consider logging when config path resolution fails.The
.ok()on line 211 silently convertsget_config_path()errors (e.g., unable to determine home directory) toNone, which then returns an empty string. While this makes the feature gracefully optional, it could mask legitimate filesystem or configuration issues from users.Additionally, consider adding file size limits to prevent accidentally loading large files that could bloat prompts and hit token limits with AI providers.
💡 Potential improvements
Option 1: Add logging for errors
fn get_md(cmd: &str) -> Result<String, AIPromptError> { - ConfigureCommand::get_config_path() + let path_result = ConfigureCommand::get_config_path(); + if let Err(e) = &path_result { + eprintln!("Warning: Could not access config path: {}", e); + } + path_result .ok() .map(|p| p.join(cmd)) .filter(|p| p.exists()) .map(std::fs::read_to_string) .transpose() .map_err(|e| AIPromptError(format!("'{cmd}': {e}"))) .map(|v| v.unwrap_or_default()) }Option 2: Add file size validation
fn get_md(cmd: &str) -> Result<String, AIPromptError> { const MAX_CONFIG_SIZE: u64 = 10_000; // 10KB limit ConfigureCommand::get_config_path() .ok() .map(|p| p.join(cmd)) .filter(|p| { p.exists() && p.metadata().map(|m| m.len() <= MAX_CONFIG_SIZE).unwrap_or(false) }) .map(std::fs::read_to_string) .transpose() .map_err(|e| AIPromptError(format!("'{cmd}': {e}"))) .map(|v| v.unwrap_or_default()) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/ai_prompt.rssrc/command/configure.rs
🧰 Additional context used
🧬 Code graph analysis (1)
src/ai_prompt.rs (2)
src/provider/mod.rs (2)
draft(156-159)explain(151-154)src/command/configure.rs (1)
get_config_path(107-114)
🔇 Additional comments (5)
src/command/configure.rs (1)
107-107: LGTM! Visibility change enables cross-module config access.Making
get_config_path()public is a minimal change that properly enables the new markdown config feature inai_prompt.rswithout altering any behavior.src/ai_prompt.rs (4)
2-2: LGTM! Import enables config path access.Adding
ConfigureCommandto imports is necessary for theget_md()helper to locate markdown configuration files.
54-105: Clean refactoring with clear optional config structure.The prompt restructuring effectively separates user preferences (loaded from
explain.md) from command-specific content. The explicit labeling of "OPTIONAL user preferences" with the constraint "MUST NOT violate the mandatory rules" provides good guardrails for the AI model.Minor observation: If
explain.mddoesn't exist or is empty, the<UserConfig>section will be present but empty. This should function correctly but might be slightly redundant in the prompt.
176-206: Consistent pattern applied across all prompt builders.The
operateprompt follows the same structural pattern as theexplainanddraftprompts, with optional user configuration in a<UserConfig>block. This consistency makes the codebase easier to maintain and understand.The ordering places user preferences before the specific task request, maintaining a logical flow.
139-168: Excellent ordering: mandatory rules before optional preferences.The prompt structure effectively prioritizes the mandatory commit message requirements (format, character limit, etc.) before introducing optional user preferences. This hierarchy should help ensure the AI model respects the critical constraints.
The code correctly uses implicit variable capture for
context,md_prompt, anddiffwithin theformatdoc!macro. The project is configured with Rust 2021 edition, which fully supports this pattern.
|
Hi, changes look good to me I think we should also make a couple of changes for consistence
|
|
Deprecating I’m not entirely sure what you mean by deprecating
|
|
yes correct. I don't think we need a rigid system for defining the commit types specifically for the draft command. The user can define the constraints in also, I think these should be under |
Add support for loading optional markdown files as user preferences for explain, draft, and operate prompts. Also restructure prompt generation to separate markdown config from command-specific content.
Happy to adjust or drop this if it doesn’t fit the project direction.
I went ahead and implemented it to provide something concrete to review.
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.