feat: system prompt - #16
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe change adds role-based runner configuration, project technology detection, first-message analysis, and deterministic system-prompt construction. Prompts combine role text, project context, conventions, and registered tool fragments. ChangesRole-Based Project-Aware Prompting
Sequence Diagram(s)sequenceDiagram
participant FirstMessage
participant analyze_message
participant ProjectConfig
participant with_context
participant ToolRegistry
FirstMessage->>analyze_message: first-message text
analyze_message-->>ProjectConfig: suggested role and languages
ProjectConfig-->>with_context: merged technology profiles and conventions
with_context->>ToolRegistry: request prompt fragments
ToolRegistry-->>with_context: sorted optional fragments
with_context-->>FirstMessage: assembled system prompt
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
crates/dch-loop/src/project.rs (3)
405-417: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMerge the two
impl TechProfileblocks.
apply_overridelives in animpl TechProfileblock at Line 60.from_techopens a second block here. Movefrom_techinto the first block so the type's inherent methods stay in one place.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/dch-loop/src/project.rs` around lines 405 - 417, Merge the second impl TechProfile block containing from_tech into the existing impl TechProfile block that defines apply_override. Preserve from_tech’s behavior and remove the now-empty duplicate impl block.
670-678: 📐 Maintainability & Code Quality | 🔵 TrivialTrack the loopctl
MockApiClientgap.The comment records that
MockApiClientdoes not honor theresponse_formatoption, and that the gap still needs to be filed. Two tests stay#[ignore]d until then, soanalyze_messagehas no automated coverage.Do you want me to open an issue to track adding
response_formatsupport toMockApiClientand un-ignoring these tests?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/dch-loop/src/project.rs` around lines 670 - 678, Track the documented loopctl MockApiClient gap by opening an issue for response_format support, referencing the ignored analyze_message integration tests and the need to un-ignore them once supported. No code changes are required in the MessageAnalysis or analyze_message tests.
300-321: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe schema enum duplicates the
Rolevariant list and can drift.The
enumarray hardcodes the sevensnake_caserole names. If someone adds aRolevariant incrates/dch-config/src/lib.rs, this schema still advertises the old set, and the model can never suggest the new role. Nothing fails at compile time.Derive the list from
Roleinstead. For example, expose aRole::ALL: [Role; N]constant indch-configand serialize each variant into the schema. A cheaper alternative is a test that asserts the schema enum length matches the variant count.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/dch-loop/src/project.rs` around lines 300 - 321, Update schema() to derive suggested_role enum values from the canonical Role definition in dch-config instead of hardcoding strings. Expose and reuse a Role::ALL collection (or equivalent complete variant source), serialize each role to its snake_case representation, and ensure the generated schema remains synchronized when Role gains variants.crates/dch-loop/src/lib.rs (1)
18-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
with_contextandwith_roleare ambiguous at the crate root.
dch_loop::with_contextanddch_loop::with_roledo not say what they build. Thepromptmodule is alreadypub(Line 7), so callers can usedch_loop::prompt::with_context, which reads clearly. Removing or renaming these two re-exports is cheap now and becomes a breaking change after release.♻️ Proposed change
pub use prompt::build_system_prompt; -pub use prompt::with_context; -pub use prompt::with_role; +pub use prompt::with_context as build_system_prompt_with_context; +pub use prompt::with_role as build_system_prompt_with_role;Dropping both re-exports and relying on the
promptmodule path is also acceptable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/dch-loop/src/lib.rs` around lines 18 - 20, Remove the crate-root re-exports for with_context and with_role from lib.rs, while retaining prompt::build_system_prompt and the public prompt module so callers use dch_loop::prompt::with_context and dch_loop::prompt::with_role.crates/dch-loop/src/prompt.rs (3)
50-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the developer-facing aside from the model-facing prompt.
The clause "(the caller appends both paths to the prompt)" describes the implementation to a reader of the source, not to the model. The model reads this text and gains nothing from it. Keep the note in the Rust doc comment at Line 102 and drop it from the prompt string.
♻️ Proposed change
FILE PATHS - Write within the working directory unless the user names a specific location. - Do not write to system directories. Use the temp directory for scratch and - intermediate output (the caller appends both paths to the prompt)."; + Do not write to system directories. Use the temp directory for scratch and + intermediate output.";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/dch-loop/src/prompt.rs` around lines 50 - 53, Remove the parenthetical caller-implementation aside from the model-facing prompt text in the prompt definition, while preserving the instruction to write within the working directory and avoid system directories. Keep the corresponding note in the Rust doc comment around the existing documentation unchanged.
302-345: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider asserting the section order, not only the presence of each section.
The current tests check that
PROJECT, the role body, and the fragment headers appear. They do not check that they appear in the documented order: shared discipline, then role body, thenPROJECT, then## <Tool>sections. A future edit towith_contextcould reorder the sections and every test would still pass.Add one test that compares the byte offsets of the four markers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/dch-loop/src/prompt.rs` around lines 302 - 345, The with_context tests only verify marker presence, not the documented section order. Add an assertion in the relevant with_context test that compares the byte offsets of the shared-discipline marker, role-body marker, "PROJECT", and the first "## <Tool>" section, requiring them to appear in that sequence.
105-111: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftTwo adjacent
Option<&str>parameters can be swapped without a type error.
project_conventionsandrole_prompt_overridehave the same type and sit next to each other. A caller that transposes them compiles cleanly and produces a wrong prompt: the conventions text replaces the role body, and the override text lands in thePROJECTsection. The public re-export atcrates/dch-loop/src/lib.rsLine 19 widens the exposure.Group the inputs in a struct so each field is named at the call site.
♻️ Proposed refactor
+/// The inputs that shape one assembled system prompt. +pub struct PromptContext<'a> { + pub role: Role, + pub techs: &'a [TechProfile], + pub project_conventions: Option<&'a str>, + pub role_prompt_override: Option<&'a str>, +} + #[must_use] -pub fn with_context( - role: Role, - techs: &[TechProfile], - project_conventions: Option<&str>, - role_prompt_override: Option<&str>, - tools: &ToolRegistry, -) -> String { +pub fn with_context(ctx: &PromptContext<'_>, tools: &ToolRegistry) -> String {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/dch-loop/src/prompt.rs` around lines 105 - 111, Refactor the public with_context function to accept a named configuration struct containing project_conventions and role_prompt_override instead of adjacent Option<&str> parameters. Define and expose the struct alongside with_context, update all callers and the dch-loop public re-export, and access the values through named fields while preserving prompt behavior.crates/dch-config/src/lib.rs (1)
560-573: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate
role_overridesentries for one role resolve silently to the first.
role_overridereturns the first match. If a user declares two[[runner.role_overrides]]entries with the samerole, the second is ignored without a warning. Consider documenting the first-wins rule in the doc comment, or logging a warning during load.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/dch-config/src/lib.rs` around lines 560 - 573, The role_override method currently resolves duplicate role_overrides entries using the first match without documenting that behavior. Update its doc comment to explicitly state that when multiple entries share a role, the first declaration wins; do not change the lookup logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/dch-config/src/lib.rs`:
- Around line 774-776: Update the documentation and related test name/callsite
for to_session_config to state that it maps only context-window,
compact-settings, and auto-compact values, since system_prompt is hardcoded to
None. Alternatively, wire the runner’s main() flow to construct and assign the
system prompt to SessionConfig::system_prompt, ensuring the documented behavior
matches the implementation.
- Around line 230-242: Update the prompt text in CODING_ROLE to remove the
unregistered WebFetch reference, replacing it with a registered URL lookup tool
if one exists or omitting the tool name. Keep the existing MultiEdit guidance
unchanged.
In `@crates/dch-loop/src/project.rs`:
- Around line 197-203: Update the Elixir marker’s file value in the marker
definitions used by detect_tech_stack from “Mix.exs” to lowercase “mix.exs” so
literal, case-sensitive detection matches real projects. Add a detection test
that creates a mix.exs file and verifies the Elixir stack is detected.
- Around line 391-401: Update merge_by_language to normalize both detected
profile languages and configured tech.language values before matching, while
preserving the existing override and append behavior. Apply the same
case-insensitive language matching in MessageAnalysis::tech_profiles and its
language_profile lookup so model-returned values such as "Rust" resolve to the
existing lowercase profile instead of creating duplicates.
- Around line 239-244: Update detect_tech_stack to construct the profile from
the matched Marker instead of re-resolving it through language_profile, while
retaining language_profile for the message-analysis path without a marker.
Refactor language_profile to delegate to marker_profile so both paths use
consistent profile data, and add a test containing only build.gradle that
asserts build == "gradle build".
---
Nitpick comments:
In `@crates/dch-config/src/lib.rs`:
- Around line 560-573: The role_override method currently resolves duplicate
role_overrides entries using the first match without documenting that behavior.
Update its doc comment to explicitly state that when multiple entries share a
role, the first declaration wins; do not change the lookup logic.
In `@crates/dch-loop/src/lib.rs`:
- Around line 18-20: Remove the crate-root re-exports for with_context and
with_role from lib.rs, while retaining prompt::build_system_prompt and the
public prompt module so callers use dch_loop::prompt::with_context and
dch_loop::prompt::with_role.
In `@crates/dch-loop/src/project.rs`:
- Around line 405-417: Merge the second impl TechProfile block containing
from_tech into the existing impl TechProfile block that defines apply_override.
Preserve from_tech’s behavior and remove the now-empty duplicate impl block.
- Around line 670-678: Track the documented loopctl MockApiClient gap by opening
an issue for response_format support, referencing the ignored analyze_message
integration tests and the need to un-ignore them once supported. No code changes
are required in the MessageAnalysis or analyze_message tests.
- Around line 300-321: Update schema() to derive suggested_role enum values from
the canonical Role definition in dch-config instead of hardcoding strings.
Expose and reuse a Role::ALL collection (or equivalent complete variant source),
serialize each role to its snake_case representation, and ensure the generated
schema remains synchronized when Role gains variants.
In `@crates/dch-loop/src/prompt.rs`:
- Around line 50-53: Remove the parenthetical caller-implementation aside from
the model-facing prompt text in the prompt definition, while preserving the
instruction to write within the working directory and avoid system directories.
Keep the corresponding note in the Rust doc comment around the existing
documentation unchanged.
- Around line 302-345: The with_context tests only verify marker presence, not
the documented section order. Add an assertion in the relevant with_context test
that compares the byte offsets of the shared-discipline marker, role-body
marker, "PROJECT", and the first "## <Tool>" section, requiring them to appear
in that sequence.
- Around line 105-111: Refactor the public with_context function to accept a
named configuration struct containing project_conventions and
role_prompt_override instead of adjacent Option<&str> parameters. Define and
expose the struct alongside with_context, update all callers and the dch-loop
public re-export, and access the values through named fields while preserving
prompt behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: df33c7f6-92bd-4d66-bdfe-f5b4cd59319e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
crates/dch-config/src/lib.rscrates/dch-loop/Cargo.tomlcrates/dch-loop/src/lib.rscrates/dch-loop/src/project.rscrates/dch-loop/src/prompt.rscrates/dch-loop/tests/system_prompt.rs
1 participant
No description provided.