Move built-in tool guidance to ToolPrompt and report static request footprint#66
Move built-in tool guidance to ToolPrompt and report static request footprint#66
Conversation
Consolidate provider-facing tool names, descriptions, params, and defaults under `tool_metadata`, then update the SerdesAI and agent runtime layers to consume those grouped constants directly. This also trims the tool context text so prompts stay closer to the implementation while using fewer tokens. Changes: - move model-facing metadata into grouped per-tool modules under `tool_metadata` - remove `tool_names` and switch code, tests, docs, and examples to `tool_metadata` - share metadata-backed defaults in SerdesAI schemas and read adapters - shorten context text while preserving current behavior Benefits: - keeps names and schema metadata in one canonical location - reduces prompt overhead without changing tool behavior
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ 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). (4)
WalkthroughReplaces the legacy tool_names surface with a new tool_metadata hierarchy and per-tool modules (bash, edit, glob, grep, read, task, todo_read, todo_write, webfetch, write) exposing canonical NAME constants, descriptions, and parameter metadata. Migrates consumers across core, serdesai, agent-runtime, examples, tests, and docs to use tool_metadata (including ParamMetadata). Changes ToolContext::context() to return typed ToolPrompt variants and adds ToolPromptFacts/tool-prompt rendering; makes TaskTargetSummary public. Removes many string-based context files and updates webfetch timeout APIs to accept/validate milliseconds. Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/llm-coding-tools-serdesai/src/todo.rs (1)
28-31:⚠️ Potential issue | 🟡 MinorAdd
#[serde(deny_unknown_fields)]to reject unknown parameters.
TodoReadArgscurrently ignores extra keys during deserialization, so calls like{"foo":1}pass silently. This masks tool-call typos. The struct should reject unexpected parameters with a validation error instead.Fix
#[derive(Debug, Clone, Deserialize)] +#[serde(deny_unknown_fields)] struct TodoReadArgs {}Also applies to line 132 where the struct is deserialized.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-serdesai/src/todo.rs` around lines 28 - 31, Add serde's deny_unknown_fields to the TodoReadArgs definition so unknown keys cause a deserialization error: annotate the struct TodoReadArgs with #[serde(deny_unknown_fields)] and ensure the place where it's deserialized (the code that calls serde_json::from_value for TodoReadArgs around the existing deserialization at line ~132) continues to propagate or handle the error so malformed inputs like {"foo":1} fail validation rather than being ignored.
🧹 Nitpick comments (9)
src/llm-coding-tools-core/src/context/write_allowed.txt (1)
2-2: Clarify the parameter rule sentence for model-facing precision.Line 2 reads as a fragment and packs two constraints into one bullet. Splitting into explicit statements will make the policy less ambiguous for downstream prompt consumers.
Suggested wording
-- `file_path` can be relative to the allowed directories or an absolute path inside the allowed directories. `content` is required. +- `file_path` must be either: + - relative to an allowed directory, or + - an absolute path within an allowed directory. +- `content` is required.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-core/src/context/write_allowed.txt` at line 2, Rewrite the sentence that mentions `file_path` and `content` in write_allowed.txt into two clear statements: one stating that `file_path` may be either a path relative to the allowed directories OR an absolute path that resides inside the allowed directories, and a second stating that `content` is required and must contain the file body to be written; update wording to be model-facing and unambiguous, using the parameter names `file_path` and `content` as shown.src/llm-coding-tools-core/src/context/read_allowed.txt (1)
1-8: LGTM! Concise documentation effectively reduces token usage.The simplified bullet-point format preserves all essential information while reducing prompt overhead, which aligns well with the PR's goals of tightening context text.
📝 Optional: Minor clarity improvement for line 5
Line 5 could be slightly clearer by repositioning "only":
-- This tool reads files, not directories. Use `glob` to find files or `bash` only when you specifically need a directory listing. +- This tool reads files, not directories. Use `glob` to find files. Use `bash` only when you specifically need a directory listing.This makes it clearer that
bashshould be used only for directory listings, not as a general alternative.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-core/src/context/read_allowed.txt` around lines 1 - 8, Update the phrasing in read_allowed.txt's bullet about directory listing to move "only" for clarity: change the line that currently reads "Use `glob` to find files or `bash` only when you specifically need a directory listing." to a clearer variant such as "Use `glob` to find files, or only use `bash` when you specifically need a directory listing." so it's explicit that `bash` is only for directory listings.src/llm-coding-tools-core/src/tool_metadata/edit.rs (1)
6-6: Use&'static strfor compile-time constant strings.The coding guidelines specify using
&'static strfor compile-time constant strings. While&strworks here (the compiler infers'static), explicit annotation improves clarity and consistency.♻️ Suggested change
-pub const NAME: &str = "edit"; +pub const NAME: &'static str = "edit";Similarly for the description constants:
- pub const ABSOLUTE: &str = + pub const ABSOLUTE: &'static str = "Replace exact text in a file. Without replace_all, old_string must match exactly once."; - pub const ALLOWED: &str = + pub const ALLOWED: &'static str = "Replace exact text in a file in allowed directories. Without replace_all, old_string must match exactly once.";As per coding guidelines: "Use
&'static strfor compile-time constant strings".Also applies to: 14-15, 18-19
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-core/src/tool_metadata/edit.rs` at line 6, Change the constant string types to explicit &'static str to match coding guidelines: update the NAME constant (pub const NAME: &str = "edit";) to pub const NAME: &'static str = "edit"; and likewise update the other compile-time description constants referenced in this file (the description-related consts near the top, e.g., the two description constants noted in the review) to use &'static str so the types are explicit and consistent.src/llm-coding-tools-core/src/tool_metadata/write.rs (1)
6-16: Make the public string constants explicitly'static.These are compile-time literals, so spelling them as
&'static strkeeps the metadata API aligned with the repo convention.♻️ Suggested cleanup
-pub const NAME: &str = "write"; +pub const NAME: &'static str = "write"; @@ - pub const ABSOLUTE: &str = + pub const ABSOLUTE: &'static str = "Write a file. Creates parent directories and overwrites existing files."; @@ - pub const ALLOWED: &str = + pub const ALLOWED: &'static str = "Write a file in allowed directories. Creates parent directories and overwrites existing files.";As per coding guidelines,
Use &'static str for compile-time constant strings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-core/src/tool_metadata/write.rs` around lines 6 - 16, Change the public string constants to use explicit 'static lifetimes: update the signature of NAME to pub const NAME: &'static str = "write"; and update description::ABSOLUTE and description::ALLOWED to pub const ABSOLUTE: &'static str = "..."; and pub const ALLOWED: &'static str = "..."; respectively so these compile-time literals follow the repo convention of using &'static str for public metadata constants.src/llm-coding-tools-core/src/tool_metadata/read.rs (1)
30-45: Make the description helpers const-generic.The line-number mode is already a type-level constant in downstream readers, so exposing
absolute::<LINE_NUMBERS>()/allowed::<LINE_NUMBERS>()would keep the metadata API aligned with that shape. The call sites would then move toread_meta::description::allowed::<LINE_NUMBERS>().♻️ Suggested refactor
- pub const fn absolute(line_numbers: bool) -> &'static str { - if line_numbers { + pub const fn absolute<const LINE_NUMBERS: bool>() -> &'static str { + if LINE_NUMBERS { "Read a file and return line-numbered text." } else { "Read a file and return raw text." } } @@ - pub const fn allowed(line_numbers: bool) -> &'static str { - if line_numbers { + pub const fn allowed<const LINE_NUMBERS: bool>() -> &'static str { + if LINE_NUMBERS { "Read a file from allowed directories and return line-numbered text." } else { "Read a file from allowed directories and return raw text." } }As per coding guidelines,
Use const generics for compile-time branching (e.g., <const LINE_NUMBERS: bool>).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-core/src/tool_metadata/read.rs` around lines 30 - 45, Change the two helper functions to use const generics so callers can call absolute::<LINE_NUMBERS>() and allowed::<LINE_NUMBERS>() at compile time: convert pub const fn absolute(line_numbers: bool) -> &'static str to pub const fn absolute<const LINE_NUMBERS: bool>() -> &'static str and use LINE_NUMBERS in the conditional, and do the same for pub const fn allowed<const LINE_NUMBERS: bool>() -> &'static str using LINE_NUMBERS for branching; keep them const fns and return the same string literals.src/llm-coding-tools-core/src/tool_metadata/grep.rs (1)
18-34: Make the description helpers const-generic.These branches are chosen from compile-time modes, so exposing
absolute::<LINE_NUMBERS>()/allowed::<LINE_NUMBERS>()would better match the type-level configuration used by the tool implementations. The downstream call sites would then switch fromdescription::absolute(flag)todescription::absolute::<FLAG>().♻️ Suggested refactor
- pub const fn absolute(line_numbers: bool) -> &'static str { - if line_numbers { + pub const fn absolute<const LINE_NUMBERS: bool>() -> &'static str { + if LINE_NUMBERS { "Search file contents with a regex. Returns matching lines with line numbers, sorted newest first by file." } else { "Search file contents with a regex. Returns matching lines, sorted newest first by file." } } @@ - pub const fn allowed(line_numbers: bool) -> &'static str { - if line_numbers { + pub const fn allowed<const LINE_NUMBERS: bool>() -> &'static str { + if LINE_NUMBERS { "Search file contents with a regex in allowed directories. Returns matching lines with line numbers, sorted newest first by file." } else { "Search file contents with a regex in allowed directories. Returns matching lines, sorted newest first by file." } }As per coding guidelines,
Use const generics for compile-time branching (e.g., <const LINE_NUMBERS: bool>).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-core/src/tool_metadata/grep.rs` around lines 18 - 34, Change the runtime boolean parameters to const generics: update the signatures of absolute and allowed to use a const generic boolean (e.g., pub const fn absolute<const LINE_NUMBERS: bool>() -> &'static str and pub const fn allowed<const LINE_NUMBERS: bool>() -> &'static str) and move the if/else branching to depend on the const generic (if LINE_NUMBERS { ... } else { ... }); then update call sites to use the const-generic form (description::absolute::<true>() or ::<false>(), and description::allowed::<...>()) so the choice is resolved at compile time.src/llm-coding-tools-core/src/tool_metadata/todo_write.rs (1)
6-9: Make the public string constants explicitly'static.Both values are compile-time literals, so using
&'static strhere keeps the metadata API consistent with the repository’s preferred type spelling.♻️ Suggested cleanup
-pub const NAME: &str = "todowrite"; +pub const NAME: &'static str = "todowrite"; @@ -pub const DESCRIPTION: &str = "Replace the full todo list."; +pub const DESCRIPTION: &'static str = "Replace the full todo list.";As per coding guidelines,
Use &'static str for compile-time constant strings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-core/src/tool_metadata/todo_write.rs` around lines 6 - 9, The public string constants NAME and DESCRIPTION are declared as &str but should use the explicit &'static str lifetime; update the declarations for NAME and DESCRIPTION to use the type &'static str (keeping the same values and visibility) so they are explicitly compile-time static string constants and conform to the metadata API convention.src/llm-coding-tools-serdesai/src/todo.rs (1)
55-83: Finish the metadata migration for nested todo fields.The
requiredlist is metadata-backed, but the nestedpropertieskeys are still literal"id","content","status", and"priority". If any of those names changes intodo_write_meta, the generated schema becomes internally inconsistent again.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-serdesai/src/todo.rs` around lines 55 - 83, The nested schema uses hard-coded property keys ("id","content","status","priority") but the required array already references metadata constants; update the properties map to use the metadata-backed names from todo_write_meta::param (e.g., todo_write_meta::param::ID.name, ::CONTENT.name, ::STATUS.name, ::PRIORITY.name) and keep descriptions from the corresponding todo_write_meta::param::*.description values so the generated JSON schema stays consistent if those param names change; modify the block building the "properties" object in the function/constructor that calls .raw(...) to substitute those literal keys with the metadata names and ensure each property's "description" uses the matching metadata description.src/llm-coding-tools-serdesai/src/absolute/grep.rs (1)
85-103: Use metadata for validation field names too.
definition()is metadata-backed now, but thesevalidation_errorcalls still hardcode"pattern"and"limit". Pull those fromgrep_meta::param::*as well so the runtime diagnostics cannot drift from the published schema.♻️ Minimal follow-up
if pattern.is_empty() { return Err(ToolError::validation_error( grep_meta::NAME, - Some("pattern".to_string()), + Some(grep_meta::param::PATTERN.name.to_string()), "pattern must not be empty".to_string(), )); } @@ if limit == 0 { return Err(ToolError::validation_error( grep_meta::NAME, - Some("limit".to_string()), + Some(grep_meta::param::LIMIT.name.to_string()), "limit must be greater than zero".to_string(), )); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-serdesai/src/absolute/grep.rs` around lines 85 - 103, Replace the hardcoded parameter names in the validation_error calls with the schema-backed constants from grep_meta::param (e.g., use grep_meta::param::PATTERN and grep_meta::param::LIMIT instead of "pattern" and "limit"). Update the two affected calls to ToolError::validation_error(grep_meta::NAME, Some(grep_meta::param::PATTERN.to_string()), ...) and ToolError::validation_error(grep_meta::NAME, Some(grep_meta::param::LIMIT.to_string()), ...), ensuring any required conversions (.to_string() or .to_owned()) are applied so the argument type matches validation_error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/llm-coding-tools-core/src/context/git_workflow.txt`:
- Line 4: Update the guidance string that currently reads "Draft a concise
message that explains why the change exists. Do not commit obvious secrets." to
remove ambiguity by replacing "Do not commit obvious secrets" with an absolute
prohibition such as "Do not commit any secrets or sensitive data." Locate and
edit the exact text instance (the message string in git_workflow.txt) so the
full guidance reads clearly: "Draft a concise message that explains why the
change exists. Do not commit any secrets or sensitive data."
In `@src/llm-coding-tools-core/src/context/github_cli.txt`:
- Line 1: The text contains a typo: the command is written as `gh ` (with a
trailing space) — locate the string "Use `gh ` via `bash` for GitHub tasks." (or
any occurrence of the backticked `gh ` token) and remove the extra space so it
reads `gh` (i.e., change backticks from `` `gh ` `` to `` `gh` ``) to correct
the documentation.
In `@src/llm-coding-tools-serdesai/src/webfetch.rs`:
- Around line 81-87: The call() handler currently passes args.timeout_ms
straight through; add explicit bounds validation after deserializing
WebFetchArgs to enforce 1..=MAX_TIMEOUT_MS (use the same MAX_TIMEOUT_MS
constant) and return a ToolError::validation_error (with webfetch_meta::NAME)
when timeout_ms is 0 or > MAX_TIMEOUT_MS, rather than calling fetch_url; only
construct Duration::from_millis and call fetch_url when the timeout has been
validated.
---
Outside diff comments:
In `@src/llm-coding-tools-serdesai/src/todo.rs`:
- Around line 28-31: Add serde's deny_unknown_fields to the TodoReadArgs
definition so unknown keys cause a deserialization error: annotate the struct
TodoReadArgs with #[serde(deny_unknown_fields)] and ensure the place where it's
deserialized (the code that calls serde_json::from_value for TodoReadArgs around
the existing deserialization at line ~132) continues to propagate or handle the
error so malformed inputs like {"foo":1} fail validation rather than being
ignored.
---
Nitpick comments:
In `@src/llm-coding-tools-core/src/context/read_allowed.txt`:
- Around line 1-8: Update the phrasing in read_allowed.txt's bullet about
directory listing to move "only" for clarity: change the line that currently
reads "Use `glob` to find files or `bash` only when you specifically need a
directory listing." to a clearer variant such as "Use `glob` to find files, or
only use `bash` when you specifically need a directory listing." so it's
explicit that `bash` is only for directory listings.
In `@src/llm-coding-tools-core/src/context/write_allowed.txt`:
- Line 2: Rewrite the sentence that mentions `file_path` and `content` in
write_allowed.txt into two clear statements: one stating that `file_path` may be
either a path relative to the allowed directories OR an absolute path that
resides inside the allowed directories, and a second stating that `content` is
required and must contain the file body to be written; update wording to be
model-facing and unambiguous, using the parameter names `file_path` and
`content` as shown.
In `@src/llm-coding-tools-core/src/tool_metadata/edit.rs`:
- Line 6: Change the constant string types to explicit &'static str to match
coding guidelines: update the NAME constant (pub const NAME: &str = "edit";) to
pub const NAME: &'static str = "edit"; and likewise update the other
compile-time description constants referenced in this file (the
description-related consts near the top, e.g., the two description constants
noted in the review) to use &'static str so the types are explicit and
consistent.
In `@src/llm-coding-tools-core/src/tool_metadata/grep.rs`:
- Around line 18-34: Change the runtime boolean parameters to const generics:
update the signatures of absolute and allowed to use a const generic boolean
(e.g., pub const fn absolute<const LINE_NUMBERS: bool>() -> &'static str and pub
const fn allowed<const LINE_NUMBERS: bool>() -> &'static str) and move the
if/else branching to depend on the const generic (if LINE_NUMBERS { ... } else {
... }); then update call sites to use the const-generic form
(description::absolute::<true>() or ::<false>(), and
description::allowed::<...>()) so the choice is resolved at compile time.
In `@src/llm-coding-tools-core/src/tool_metadata/read.rs`:
- Around line 30-45: Change the two helper functions to use const generics so
callers can call absolute::<LINE_NUMBERS>() and allowed::<LINE_NUMBERS>() at
compile time: convert pub const fn absolute(line_numbers: bool) -> &'static str
to pub const fn absolute<const LINE_NUMBERS: bool>() -> &'static str and use
LINE_NUMBERS in the conditional, and do the same for pub const fn allowed<const
LINE_NUMBERS: bool>() -> &'static str using LINE_NUMBERS for branching; keep
them const fns and return the same string literals.
In `@src/llm-coding-tools-core/src/tool_metadata/todo_write.rs`:
- Around line 6-9: The public string constants NAME and DESCRIPTION are declared
as &str but should use the explicit &'static str lifetime; update the
declarations for NAME and DESCRIPTION to use the type &'static str (keeping the
same values and visibility) so they are explicitly compile-time static string
constants and conform to the metadata API convention.
In `@src/llm-coding-tools-core/src/tool_metadata/write.rs`:
- Around line 6-16: Change the public string constants to use explicit 'static
lifetimes: update the signature of NAME to pub const NAME: &'static str =
"write"; and update description::ABSOLUTE and description::ALLOWED to pub const
ABSOLUTE: &'static str = "..."; and pub const ALLOWED: &'static str = "...";
respectively so these compile-time literals follow the repo convention of using
&'static str for public metadata constants.
In `@src/llm-coding-tools-serdesai/src/absolute/grep.rs`:
- Around line 85-103: Replace the hardcoded parameter names in the
validation_error calls with the schema-backed constants from grep_meta::param
(e.g., use grep_meta::param::PATTERN and grep_meta::param::LIMIT instead of
"pattern" and "limit"). Update the two affected calls to
ToolError::validation_error(grep_meta::NAME,
Some(grep_meta::param::PATTERN.to_string()), ...) and
ToolError::validation_error(grep_meta::NAME,
Some(grep_meta::param::LIMIT.to_string()), ...), ensuring any required
conversions (.to_string() or .to_owned()) are applied so the argument type
matches validation_error.
In `@src/llm-coding-tools-serdesai/src/todo.rs`:
- Around line 55-83: The nested schema uses hard-coded property keys
("id","content","status","priority") but the required array already references
metadata constants; update the properties map to use the metadata-backed names
from todo_write_meta::param (e.g., todo_write_meta::param::ID.name,
::CONTENT.name, ::STATUS.name, ::PRIORITY.name) and keep descriptions from the
corresponding todo_write_meta::param::*.description values so the generated JSON
schema stays consistent if those param names change; modify the block building
the "properties" object in the function/constructor that calls .raw(...) to
substitute those literal keys with the metadata names and ensure each property's
"description" uses the matching metadata description.
🪄 Autofix (Beta)
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
Run ID: e477e832-b6be-4023-8af0-988ed6089b13
📒 Files selected for processing (54)
src/llm-coding-tools-agents/src/runtime/builder.rssrc/llm-coding-tools-agents/src/runtime/task.rssrc/llm-coding-tools-agents/src/runtime/tool_catalog.rssrc/llm-coding-tools-core/README.mdsrc/llm-coding-tools-core/examples/system_prompt_preview.rssrc/llm-coding-tools-core/src/context/bash.txtsrc/llm-coding-tools-core/src/context/edit_absolute.txtsrc/llm-coding-tools-core/src/context/edit_allowed.txtsrc/llm-coding-tools-core/src/context/git_workflow.txtsrc/llm-coding-tools-core/src/context/github_cli.txtsrc/llm-coding-tools-core/src/context/glob_absolute.txtsrc/llm-coding-tools-core/src/context/glob_allowed.txtsrc/llm-coding-tools-core/src/context/grep_absolute.txtsrc/llm-coding-tools-core/src/context/grep_allowed.txtsrc/llm-coding-tools-core/src/context/read_absolute.txtsrc/llm-coding-tools-core/src/context/read_allowed.txtsrc/llm-coding-tools-core/src/context/task.txtsrc/llm-coding-tools-core/src/context/todoread.txtsrc/llm-coding-tools-core/src/context/todowrite.txtsrc/llm-coding-tools-core/src/context/webfetch.txtsrc/llm-coding-tools-core/src/context/write_absolute.txtsrc/llm-coding-tools-core/src/context/write_allowed.txtsrc/llm-coding-tools-core/src/lib.rssrc/llm-coding-tools-core/src/tool_metadata/bash.rssrc/llm-coding-tools-core/src/tool_metadata/edit.rssrc/llm-coding-tools-core/src/tool_metadata/glob.rssrc/llm-coding-tools-core/src/tool_metadata/grep.rssrc/llm-coding-tools-core/src/tool_metadata/mod.rssrc/llm-coding-tools-core/src/tool_metadata/read.rssrc/llm-coding-tools-core/src/tool_metadata/task.rssrc/llm-coding-tools-core/src/tool_metadata/todo_read.rssrc/llm-coding-tools-core/src/tool_metadata/todo_write.rssrc/llm-coding-tools-core/src/tool_metadata/webfetch.rssrc/llm-coding-tools-core/src/tool_metadata/write.rssrc/llm-coding-tools-core/src/tool_names.rssrc/llm-coding-tools-serdesai/src/absolute/edit.rssrc/llm-coding-tools-serdesai/src/absolute/glob.rssrc/llm-coding-tools-serdesai/src/absolute/grep.rssrc/llm-coding-tools-serdesai/src/absolute/read.rssrc/llm-coding-tools-serdesai/src/absolute/write.rssrc/llm-coding-tools-serdesai/src/agent_runtime/build.rssrc/llm-coding-tools-serdesai/src/agent_runtime/task.rssrc/llm-coding-tools-serdesai/src/allowed/edit.rssrc/llm-coding-tools-serdesai/src/allowed/glob.rssrc/llm-coding-tools-serdesai/src/allowed/grep.rssrc/llm-coding-tools-serdesai/src/allowed/read.rssrc/llm-coding-tools-serdesai/src/allowed/write.rssrc/llm-coding-tools-serdesai/src/bash.rssrc/llm-coding-tools-serdesai/src/common/edit.rssrc/llm-coding-tools-serdesai/src/task/definition.rssrc/llm-coding-tools-serdesai/src/task/handle.rssrc/llm-coding-tools-serdesai/src/task/tool.rssrc/llm-coding-tools-serdesai/src/todo.rssrc/llm-coding-tools-serdesai/src/webfetch.rs
💤 Files with no reviewable changes (1)
- src/llm-coding-tools-core/src/tool_names.rs
📜 Review details
⏰ 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). (5)
- GitHub Check: Build and Test (Blocking) (windows-latest, x86_64-pc-windows-msvc, false)
- GitHub Check: Build and Test (Async/Tokio) (windows-latest, x86_64-pc-windows-msvc, false)
- GitHub Check: Build and Test (Blocking) (macos-latest, aarch64-apple-darwin, false)
- GitHub Check: Build and Test (Async/Tokio) (ubuntu-latest, x86_64-unknown-linux-gnu, false)
- GitHub Check: Build and Test (Async/Tokio) (macos-latest, aarch64-apple-darwin, false)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.rs
📄 CodeRabbit inference engine (src/AGENTS.md)
src/**/*.rs: Preallocate collections when size is known or estimable usingString::with_capacity(estimated_len),Vec::with_capacity(count), orBufReader::with_capacity(size, reader)
Prefer&str/&[T]returns over owned types when lifetime allows
UseCow<'_, str>for conditional ownership (e.g.,String::from_utf8_lossy)
Use&'static strfor compile-time constant strings
Reuse buffers by using.clear()and reusingVec/Stringinstead of reallocating
Use const generics for compile-time branching (e.g.,<const LINE_NUMBERS: bool>)
Use#[inline]on small, hot-path functions
Prefercoreoverstdwhere possible (e.g.,core::memoverstd::mem)
Stream data instead of loading entire files when possible
Usememchrfor fast byte searching over manual iteration
Keep modules under 500 lines (excluding tests); split if larger
Placeusestatements inside functions only for#[cfg]conditional compilation
Document public items with///and add examples in docs where helpful
Use//!for module-level documentation
Focus comments on 'why' not 'what'
Use [TypeName] rustdoc links in documentation, not backticks
Files:
src/llm-coding-tools-core/src/tool_metadata/glob.rssrc/llm-coding-tools-core/src/lib.rssrc/llm-coding-tools-core/src/tool_metadata/todo_read.rssrc/llm-coding-tools-core/src/tool_metadata/webfetch.rssrc/llm-coding-tools-serdesai/src/allowed/glob.rssrc/llm-coding-tools-serdesai/src/agent_runtime/task.rssrc/llm-coding-tools-serdesai/src/common/edit.rssrc/llm-coding-tools-agents/src/runtime/task.rssrc/llm-coding-tools-core/src/tool_metadata/edit.rssrc/llm-coding-tools-serdesai/src/absolute/glob.rssrc/llm-coding-tools-core/src/tool_metadata/grep.rssrc/llm-coding-tools-core/src/tool_metadata/read.rssrc/llm-coding-tools-core/src/tool_metadata/task.rssrc/llm-coding-tools-core/src/tool_metadata/write.rssrc/llm-coding-tools-core/src/tool_metadata/todo_write.rssrc/llm-coding-tools-serdesai/src/task/definition.rssrc/llm-coding-tools-serdesai/src/task/tool.rssrc/llm-coding-tools-serdesai/src/webfetch.rssrc/llm-coding-tools-agents/src/runtime/tool_catalog.rssrc/llm-coding-tools-core/src/tool_metadata/bash.rssrc/llm-coding-tools-core/examples/system_prompt_preview.rssrc/llm-coding-tools-serdesai/src/task/handle.rssrc/llm-coding-tools-serdesai/src/allowed/edit.rssrc/llm-coding-tools-serdesai/src/agent_runtime/build.rssrc/llm-coding-tools-serdesai/src/bash.rssrc/llm-coding-tools-serdesai/src/absolute/grep.rssrc/llm-coding-tools-serdesai/src/allowed/grep.rssrc/llm-coding-tools-serdesai/src/allowed/read.rssrc/llm-coding-tools-serdesai/src/allowed/write.rssrc/llm-coding-tools-serdesai/src/todo.rssrc/llm-coding-tools-agents/src/runtime/builder.rssrc/llm-coding-tools-serdesai/src/absolute/edit.rssrc/llm-coding-tools-core/src/tool_metadata/mod.rssrc/llm-coding-tools-serdesai/src/absolute/write.rssrc/llm-coding-tools-serdesai/src/absolute/read.rs
🧠 Learnings (1)
📚 Learning: 2026-03-17T21:06:53.909Z
Learnt from: CR
Repo: Sewer56/llm-coding-tools PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-03-17T21:06:53.909Z
Learning: Applies to src/**/*.rs : Document public items with `///` and add examples in docs where helpful
Applied to files:
src/llm-coding-tools-core/examples/system_prompt_preview.rs
🪛 LanguageTool
src/llm-coding-tools-core/src/context/webfetch.txt
[uncategorized] ~4-~4: Did you mean the formatting language “Markdown” (= proper noun)?
Context: ...ults to 30000. - HTML is converted to markdown, JSON is pretty-printed, and other cont...
(MARKDOWN_NNP)
src/llm-coding-tools-core/src/context/write_allowed.txt
[style] ~2-~2: To form a complete sentence, be sure to include a subject.
Context: ...nt directories if needed. - file_path can be relative to the allowed directories ...
(MISSING_IT_THERE)
src/llm-coding-tools-core/src/context/read_allowed.txt
[style] ~2-~2: To form a complete sentence, be sure to include a subject.
Context: ...thin allowed directories. - file_path can be relative to the allowed directories ...
(MISSING_IT_THERE)
src/llm-coding-tools-core/src/context/edit_allowed.txt
[style] ~2-~2: To form a complete sentence, be sure to include a subject.
Context: ...thin allowed directories. - file_path can be relative to the allowed directories ...
(MISSING_IT_THERE)
🔇 Additional comments (53)
src/llm-coding-tools-core/src/context/write_absolute.txt (1)
1-6: Clear and appropriately scoped tool guidance.This is a solid reduction in prompt verbosity while preserving key operational constraints (absolute path, overwrite semantics, and when to prefer
edit). No issues found in Lines 1–6.src/llm-coding-tools-core/src/context/task.txt (1)
1-7: Concise and well-scoped Task delegation contract.This rewrite is clear, enforceable, and aligned with the refactor goal of reducing prompt overhead while preserving behavior boundaries.
src/llm-coding-tools-core/src/context/grep_absolute.txt (1)
1-6: LGTM!The context text is concise and provides clear guidance for the grep tool usage, aligning with the PR objective to reduce prompt token overhead.
src/llm-coding-tools-core/src/context/todoread.txt (1)
1-4: LGTM!The simplified instructions are clear and reduce prompt token usage while preserving essential behavior guidance.
src/llm-coding-tools-core/src/tool_metadata/webfetch.rs (1)
1-31: LGTM!Well-structured metadata module following the established pattern. Constants are properly typed, documentation is clear, and the timeout values in the description match the defined constants.
src/llm-coding-tools-core/src/lib.rs (1)
19-19: LGTM!The module rename from
tool_namestotool_metadatais a clean breaking change that aligns with the PR objective to centralize metadata under per-tool modules.src/llm-coding-tools-core/src/tool_metadata/glob.rs (1)
1-40: LGTM!Well-structured metadata module with clear separation between absolute and allowed path variants. The dual description and parameter constants for the two modes provide good flexibility for different tool configurations.
src/llm-coding-tools-agents/src/runtime/builder.rs (2)
84-84: LGTM!Import correctly updated to use the new
tool_metadatamodule with descriptive aliases.
110-113: LGTM!Test correctly migrated to use
read_meta::NAMEandglob_meta::NAMEinstead of the previoustool_namesconstants.src/llm-coding-tools-serdesai/src/common/edit.rs (2)
3-3: LGTM!Import correctly aliased to
edit_metafor clarity and consistency with the refactor pattern.
14-38: LGTM!All error mappings consistently use
edit_meta::NAMEinstead of the previous hard-coded constant, maintaining the single source of truth for the tool name.src/llm-coding-tools-core/src/context/glob_absolute.txt (1)
1-6: LGTM!The context text is clear, concise, and documents important behavior including the 1000 result cap and truncation indicator.
src/llm-coding-tools-serdesai/src/task/tool.rs (1)
17-18: Metadata-based Task naming migration is consistent and safe.All updated references (runtime errors, context name, and tests) consistently use
task_meta::NAME, which keeps canonical naming centralized without changing behavior.Also applies to: 71-71, 86-86, 95-95, 112-112, 120-120
src/llm-coding-tools-serdesai/src/agent_runtime/task.rs (1)
135-137: Task runtime tests are correctly aligned to grouped tool metadata.The updated fixtures/assertions consistently use
*_meta::NAME, matching the new canonical metadata model and preserving test intent.Also applies to: 171-171, 214-214, 243-243, 249-249, 263-265, 296-297, 310-310, 327-329, 342-342, 353-354, 369-369, 375-375, 390-392, 406-406, 412-412, 423-425
src/llm-coding-tools-serdesai/src/agent_runtime/build.rs (1)
278-280: Build-path tests are cleanly migrated to metadata constants.The updated test expectations and permissions remain coherent after removing flat tool-name shims.
Also applies to: 390-390, 403-405, 426-426, 466-466, 474-474, 485-485, 508-508, 522-523, 534-534, 546-547
src/llm-coding-tools-core/src/context/edit_allowed.txt (1)
1-8: Condensed guidance retains the critical constraints for allowed-path edit behavior.This keeps prompt overhead low while preserving the key failure modes and safety boundaries.
src/llm-coding-tools-serdesai/src/task/handle.rs (1)
10-11: Task handle validation and permission checks now consistently use canonical metadata names.The migration is applied uniformly across runtime errors, permission evaluation, and tests.
Also applies to: 76-76, 86-86, 124-124, 132-132, 141-147, 150-150, 207-207, 261-261, 280-280, 293-293, 313-313, 350-350, 363-363, 383-383, 400-400, 422-422
src/llm-coding-tools-core/src/context/edit_absolute.txt (1)
1-7: Absolute-edit context is concise and still operationally clear.The shortened version retains required inputs, replacement constraints, and failure conditions.
src/llm-coding-tools-core/src/context/webfetch.txt (1)
1-7: WebFetch context is compact and preserves key behavioral guarantees.Required args, output contract, and response-size limits remain clearly specified after the rewrite.
src/llm-coding-tools-core/src/context/bash.txt (1)
1-9: Bash context rewrite keeps important execution semantics while reducing verbosity.The concise guidance still communicates required params, timeout bounds, output conventions, and when to prefer specialized tools.
src/llm-coding-tools-agents/src/runtime/tool_catalog.rs (1)
14-18: LGTM!The migration from
tool_namestotool_metadataaliases is clean and consistent. Using*_meta::NAMEconstants centralizes the canonical tool names appropriately.Also applies to: 62-73
src/llm-coding-tools-core/src/context/glob_allowed.txt (1)
1-7: LGTM!The condensed context text effectively communicates glob tool behavior while minimizing token usage. Key constraints (allowed directories, result cap, gitignore respect) are clearly stated.
src/llm-coding-tools-serdesai/src/absolute/read.rs (1)
6-6: LGTM!The read tool cleanly adopts the centralized metadata pattern. Serde defaults (
default_offset,default_limit) and schema parameters now derive fromread_meta, ensuring a single source of truth for tool configuration.Also applies to: 19-19, 22-22, 46-72
src/llm-coding-tools-core/src/tool_metadata/edit.rs (1)
22-50: LGTM!The parameter metadata definitions are well-structured with clear documentation. Using
constfor allParamMetadatafields enables compile-time evaluation.src/llm-coding-tools-serdesai/src/allowed/write.rs (1)
5-5: LGTM!The write tool correctly adopts the centralized metadata pattern. All tool identity references (
NAME,description::ALLOWED) and parameter metadata (FILE_PATH_ALLOWED,CONTENT) are sourced fromwrite_meta, maintaining consistency with the broader refactor.Also applies to: 40-56, 61-61, 64-64, 69-69
src/llm-coding-tools-core/src/context/todowrite.txt (1)
1-8: LGTM!The condensed context text clearly communicates the replacement semantics and required todo structure. The explicit enumeration of
statusandpriorityvalues helps prevent invalid tool calls.src/llm-coding-tools-core/src/context/read_absolute.txt (1)
1-7: LGTM!The context text effectively documents the read tool's behavior, defaults, and constraints. The guidance to "read the file before editing it" and suggestions for parallel reads are practical LLM instructions.
src/llm-coding-tools-core/src/context/grep_allowed.txt (1)
1-8: LGTM!The context text concisely documents grep behavior including the important single-line-only limitation. The explicit guidance to "use this instead of shell
grep/rg" helps steer LLMs toward the sandboxed tool.src/llm-coding-tools-agents/src/runtime/task.rs (4)
12-12: LGTM on metadata import refactoring.The import alias
task_metafortool_metadata::taskis clean and consistent with the PR's goal of consolidating metadata under per-tool modules.
14-21: TaskTargetSummary struct is well-designed.The struct uses
Box<str>appropriately for owned string data. The documentation clearly describes the purpose of each field.
33-49: Good use of preallocation.
Vec::with_capacity(callable.len())on line 38 follows the coding guideline for preallocating collections when size is known. As per coding guidelines: "Preallocate collections when size is known or estimable usingVec::with_capacity(count)".
155-157: Test imports cleanly updated.The grouped import of multiple metadata aliases (
bash_meta,read_meta,task_meta,write_meta) is well-organized and consistent with the refactoring pattern.src/llm-coding-tools-core/src/tool_metadata/todo_read.rs (1)
1-7: Clean metadata module structure.The module correctly uses
&'static strfor compile-time constants and follows the documentation guidelines with//!for module-level docs and///for item docs.src/llm-coding-tools-serdesai/src/allowed/glob.rs (3)
6-6: LGTM on metadata aliasing.The
glob_metaalias import is consistent with the codebase refactoring pattern.
42-59: Tool definition correctly uses centralized metadata.The schema construction and
ToolDefinitionnow pull names, descriptions, and required flags fromglob_meta, ensuring consistency with the canonical metadata source.
73-78: ToolContext implementation is correct.The
NAMEconstant correctly referencesglob_meta::NAMEas a&'static str, and the context method returns the appropriate allowed-path context.src/llm-coding-tools-serdesai/src/allowed/grep.rs (3)
31-31: Good use of const generics.The
GrepTool<const LINE_NUMBERS: bool>pattern enables compile-time branching for line number inclusion, following the coding guideline. As per coding guidelines: "Use const generics for compile-time branching".
48-80: Tool definition correctly migrated to metadata.The schema construction uses
grep_meta::param::*for parameter metadata, and theToolDefinitionusesgrep_meta::NAMEandgrep_meta::description::allowed(LINE_NUMBERS)for dynamic description generation.
95-98: Limit defaults centralized correctly.Using
grep_meta::DEFAULT_LIMITandgrep_meta::MAX_LIMITensures consistent behavior across all grep tool implementations and simplifies future changes.src/llm-coding-tools-core/README.md (3)
35-40: Documentation clearly explains the new metadata structure.The updated text accurately describes the grouped module structure with canonical names and provider-facing metadata for each tool.
88-107: Code example correctly demonstrates ToolContext usage.The example shows the updated import path (
tool_metadata) and the correct constant reference (tool_metadata::read::NAME).
231-241: Reference links updated consistently.All tool reference links now correctly point to
crate::tool_metadata::<tool>::NAMEpaths, maintaining consistency with the code changes.src/llm-coding-tools-core/src/tool_metadata/task.rs (1)
1-34: Well-structured task metadata module.The module correctly defines canonical constants with proper documentation. Parameter metadata clearly distinguishes required (
description,prompt,subagent_type) from optional (session_id,command) parameters, and theSESSION_IDdescription appropriately signals it's unsupported.src/llm-coding-tools-serdesai/src/allowed/edit.rs (3)
6-6: LGTM on metadata aliasing.The
edit_metaalias import is consistent with the codebase refactoring pattern.
48-75: Tool definition correctly uses centralized metadata.The schema construction and
ToolDefinitionnow pull all parameter names, descriptions, and required flags fromedit_meta, ensuring consistency with the canonical metadata source.
94-99: ToolContext implementation is correct.The
NAMEconstant correctly referencesedit_meta::NAME, and the context method returns the appropriate allowed-path context.src/llm-coding-tools-serdesai/src/task/definition.rs (3)
17-30: Good capacity preallocation for string building.The
String::with_capacity(32 + ordered.len() * 64)provides reasonable estimation for the output string, following the coding guideline for preallocating collections. As per coding guidelines: "Preallocate collections when size is known or estimable usingString::with_capacity(estimated_len)".
33-69: Task tool definition correctly uses centralized metadata.The implementation properly uses
task_meta::DESCRIPTION_PREFIXfor the description prefix andtask_meta::param::*for all parameter schema fields, ensuring consistency with the canonical metadata source.
73-73: Test import correctly re-exports task_meta.Using
super::task_metain tests ensures the tests use the same metadata path as the implementation, maintaining consistency.src/llm-coding-tools-serdesai/src/absolute/glob.rs (1)
40-67: LGTM on the metadata wiring.Definition building, validation errors, result tagging, and
ToolContext::NAMEnow all read fromglob_meta, which removes the remaining duplicated provider-facing strings in this adapter.Also applies to: 72-73
src/llm-coding-tools-serdesai/src/absolute/edit.rs (1)
44-75: LGTM on the metadata migration.Definition building, validation error mapping, and
ToolContext::NAMEall source fromedit_metanow, which is the right single source of truth for this adapter.Also applies to: 90-91
src/llm-coding-tools-serdesai/src/bash.rs (1)
64-111: Nice single-source metadata hookup.The schema, validation errors, fallback timeout, and
ToolContext::NAMEnow all pull frombash_meta, so provider-facing defaults stay centralized.Also applies to: 115-116
src/llm-coding-tools-serdesai/src/allowed/read.rs (1)
19-23: Nice default/source-of-truth consolidation.Using
read_meta::default_offset/read_meta::default_limitfor serde, the same metadata in the schema, andread_meta::NAMEfor errors keeps the wire contract synchronized.Also applies to: 63-99, 103-104
Trim schema facts that are already present in tool definitions from the system-prompt context, and update the preview example to count both prompt text and tool definitions. This keeps token budgeting focused on the full request shape while preserving the behavioral guidance that still belongs in context. Changes: - remove repeated parameter, default, and enum details from tool context files - update context tests for the slimmer task guidance - expand `system_prompt_preview` to print system prompt, tool definition, and combined static token estimates Benefits: - reduces prompt overhead in the preview by roughly 400 estimated tokens - makes the biggest static token contributors easier to spot
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/llm-coding-tools-core/src/context/grep_absolute.txt`:
- Line 3: Update the documentation line in grep_absolute.txt that currently
states "Returns matches grouped by file" to explicitly include the output
ordering: change it to state that matches are returned grouped by file and files
are sorted newest-first by file mtime (i.e., descending modification time) so
callers have deterministic semantics; ensure the phrase mentions "newest-first
by file mtime" and that grouping remains by file.
- Line 2: The docs in src/llm-coding-tools-core/src/context/grep_absolute.txt
claim the `pattern` must be non-empty but the grep implementation only checks
regex compilation; either enforce non-empty in the code path that
validates/compiles the regex (add an explicit check that `pattern !== ""` and
return/throw the same validation error used for invalid regex) or update
grep_absolute.txt to remove the “`pattern` must not be empty” sentence so the
contract matches current behavior; locate the check around the regex compilation
logic (the place that currently validates regex compilation for `pattern`) and
apply one of these two fixes.
🪄 Autofix (Beta)
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
Run ID: dcc2e9c2-589e-4482-81f8-8140713074e4
📒 Files selected for processing (17)
src/llm-coding-tools-core/examples/system_prompt_preview.rssrc/llm-coding-tools-core/src/context/bash.txtsrc/llm-coding-tools-core/src/context/edit_absolute.txtsrc/llm-coding-tools-core/src/context/edit_allowed.txtsrc/llm-coding-tools-core/src/context/glob_absolute.txtsrc/llm-coding-tools-core/src/context/glob_allowed.txtsrc/llm-coding-tools-core/src/context/grep_absolute.txtsrc/llm-coding-tools-core/src/context/grep_allowed.txtsrc/llm-coding-tools-core/src/context/mod.rssrc/llm-coding-tools-core/src/context/read_absolute.txtsrc/llm-coding-tools-core/src/context/read_allowed.txtsrc/llm-coding-tools-core/src/context/task.txtsrc/llm-coding-tools-core/src/context/todoread.txtsrc/llm-coding-tools-core/src/context/todowrite.txtsrc/llm-coding-tools-core/src/context/webfetch.txtsrc/llm-coding-tools-core/src/context/write_absolute.txtsrc/llm-coding-tools-core/src/context/write_allowed.txt
✅ Files skipped from review due to trivial changes (3)
- src/llm-coding-tools-core/src/context/write_absolute.txt
- src/llm-coding-tools-core/src/context/write_allowed.txt
- src/llm-coding-tools-core/src/context/task.txt
🚧 Files skipped from review as they are similar to previous changes (8)
- src/llm-coding-tools-core/src/context/todoread.txt
- src/llm-coding-tools-core/src/context/edit_allowed.txt
- src/llm-coding-tools-core/src/context/edit_absolute.txt
- src/llm-coding-tools-core/src/context/glob_absolute.txt
- src/llm-coding-tools-core/src/context/read_absolute.txt
- src/llm-coding-tools-core/src/context/bash.txt
- src/llm-coding-tools-core/src/context/grep_allowed.txt
- src/llm-coding-tools-core/examples/system_prompt_preview.rs
📜 Review details
⏰ 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). (5)
- GitHub Check: Build and Test (Blocking) (windows-latest, x86_64-pc-windows-msvc, false)
- GitHub Check: Build and Test (Async/Tokio) (macos-latest, aarch64-apple-darwin, false)
- GitHub Check: Build and Test (Async/Tokio) (ubuntu-latest, x86_64-unknown-linux-gnu, false)
- GitHub Check: Build and Test (Async/Tokio) (windows-latest, x86_64-pc-windows-msvc, false)
- GitHub Check: Build and Test (Blocking) (macos-latest, aarch64-apple-darwin, false)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.rs
📄 CodeRabbit inference engine (src/AGENTS.md)
src/**/*.rs: Preallocate collections when size is known or estimable usingString::with_capacity(estimated_len),Vec::with_capacity(count), orBufReader::with_capacity(size, reader)
Prefer&str/&[T]returns over owned types when lifetime allows
UseCow<'_, str>for conditional ownership (e.g.,String::from_utf8_lossy)
Use&'static strfor compile-time constant strings
Reuse buffers by using.clear()and reusingVec/Stringinstead of reallocating
Use const generics for compile-time branching (e.g.,<const LINE_NUMBERS: bool>)
Use#[inline]on small, hot-path functions
Prefercoreoverstdwhere possible (e.g.,core::memoverstd::mem)
Stream data instead of loading entire files when possible
Usememchrfor fast byte searching over manual iteration
Keep modules under 500 lines (excluding tests); split if larger
Placeusestatements inside functions only for#[cfg]conditional compilation
Document public items with///and add examples in docs where helpful
Use//!for module-level documentation
Focus comments on 'why' not 'what'
Use [TypeName] rustdoc links in documentation, not backticks
Files:
src/llm-coding-tools-core/src/context/mod.rs
🔇 Additional comments (6)
src/llm-coding-tools-core/src/context/grep_absolute.txt (1)
4-5: Good tool-boundary guidanceThe guidance to prefer this tool over shell
grep/rgand route filename/full-file workflows toglob/readis clear and consistent with intended tool usage.src/llm-coding-tools-core/src/context/todowrite.txt (1)
1-5: Documentation accurately reflects implementation.The condensed guidance correctly describes the todowrite tool's behavior:
- Full list replacement is confirmed by the implementation (
*state.todos.write() = todos;)- Non-empty constraints are enforced by runtime validation
The brevity aligns with the PR's goal to reduce token usage in context files.
src/llm-coding-tools-core/src/context/read_allowed.txt (1)
1-6: Nice tightening of thereadcontract.The condensed guidance stays accurate with current behavior (line-numbered output + 2000-char truncation + allowed-directory constraints) and is easier for agents to follow.
src/llm-coding-tools-core/src/context/glob_allowed.txt (1)
1-6: Looks good — concise and implementation-aligned.The capped-result and
truncated: truebehavior is clearly captured and matches currentglobbehavior.src/llm-coding-tools-core/src/context/mod.rs (1)
262-264: Test update is aligned with the new task-context contract.These assertions match the delegation-focused wording introduced in the condensed
taskguidance.src/llm-coding-tools-core/src/context/webfetch.txt (1)
3-3: The 5 MiB response size limit is correctly enforced. The constantMAX_RESPONSE_SIZEis defined as5 * 1_024 * 1_024bytes insrc/llm-coding-tools-core/src/tools/webfetch/mod.rsand thecheck_size()function validates this limit both against theContent-Lengthheader and during streaming reads in both async and blocking implementations. Documentation and code are in sync.
Move built-in tool guidance into `ToolPrompt` renderers and remove the old parallel text files so the prompt rules live in one place. This also reshapes the preview examples and docs around the slimmer, conditional prompt model. Changes: - replace built-in string context constants with `ToolPrompt` variants and conditional renderers - remove redundant built-in context text files and update SerdesAI tools to return prompt variants directly - split and rename system prompt preview helpers, add readonly/full comparison cases, and refresh docs/examples Benefits: - keeps built-in prompt behavior and documentation aligned in one source - reduces prompt overhead by only rendering shared rules when the related tools are present
Sewer56
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
src/llm-coding-tools-core/src/system_prompt.rs (1)
402-402: Usetool_metadata::*::NAMEin the built-in test helpers too.These literals reintroduce a second source of truth right after the metadata consolidation.
src/llm-coding-tools-core/examples/system_prompt/mock_tools.rsalready pulls the names fromtool_metadata; doing the same here keeps the prompt tests aligned with real tool names.🔧 Suggested cleanup
- use crate::context::PathMode; + use crate::{context::PathMode, tool_metadata};- built_in_path_tool_with_line_numbers!(BuiltInReadTool, "read", Read); - built_in_path_tool!(BuiltInWriteTool, "write", Write); - built_in_path_tool!(BuiltInEditTool, "edit", Edit); - built_in_path_tool!(BuiltInGlobTool, "glob", Glob); - built_in_path_tool_with_line_numbers!(BuiltInGrepTool, "grep", Grep); - built_in_tool!(BuiltInBashTool, "bash", ToolPrompt::Bash); - built_in_tool!(BuiltInTaskTool, "task", ToolPrompt::Task); + built_in_path_tool_with_line_numbers!(BuiltInReadTool, tool_metadata::read::NAME, Read); + built_in_path_tool!(BuiltInWriteTool, tool_metadata::write::NAME, Write); + built_in_path_tool!(BuiltInEditTool, tool_metadata::edit::NAME, Edit); + built_in_path_tool!(BuiltInGlobTool, tool_metadata::glob::NAME, Glob); + built_in_path_tool_with_line_numbers!(BuiltInGrepTool, tool_metadata::grep::NAME, Grep); + built_in_tool!(BuiltInBashTool, tool_metadata::bash::NAME, ToolPrompt::Bash); + built_in_tool!(BuiltInTaskTool, tool_metadata::task::NAME, ToolPrompt::Task);Also applies to: 482-489
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-core/src/system_prompt.rs` at line 402, Replace hard-coded tool name string literals used in the built-in prompt test helpers with the canonical constants from tool_metadata (use tool_metadata::<ToolModule>::NAME) so the tests use the same source of truth as examples/mock_tools.rs; update any helper that references raw names (and any uses near PathMode) to import the appropriate tool_metadata modules and reference their NAME constants instead of literal strings.src/llm-coding-tools-core/examples/system_prompt_preview_compare.rs (1)
29-94: Extract thesePromptCasefixtures into the shared example module.
FULL_SYSTEM_PROMPT,READONLY_SYSTEM_PROMPT,TASK_TARGETS,full_case(), andreadonly_case()are now duplicated acrosssystem_prompt_preview.rs,system_prompt_preview_compare.rs, andsystem_prompt_preview_readonly.rs. A tool toggle or prompt text tweak now has to stay in sync in three places or the comparisons stop being like-for-like.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-core/examples/system_prompt_preview_compare.rs` around lines 29 - 94, FULL_SYSTEM_PROMPT, READONLY_SYSTEM_PROMPT, TASK_TARGETS, full_case(), and readonly_case() are duplicated and should be extracted into a shared example module so all three files reuse the same fixtures; create a new module (e.g., examples::shared_prompts) that defines and exposes the constants and functions named FULL_SYSTEM_PROMPT, READONLY_SYSTEM_PROMPT, TASK_TARGETS, full_case, and readonly_case, replace the inline definitions in system_prompt_preview.rs, system_prompt_preview_compare.rs, and system_prompt_preview_readonly.rs with use/imports from that shared module, and ensure the signatures and types (PromptCase, TaskTarget, ReadConfig, PathMode, GrepConfig) match so callers compile without changes.src/llm-coding-tools-core/examples/system_prompt/build.rs (1)
1-4: Add a//!header for this shared preview module.This new helper module starts directly with imports, so it is missing the short module-level context the rest of the example helpers already carry.
As per coding guidelines, `src/**/*.rs` modules should "Use `//!` for module-level documentation".📝 Suggested addition
+//! Shared builders for the system prompt preview examples. + use llm_coding_tools_core::context; use llm_coding_tools_core::{AllowedPathResolver, SystemPromptBuilder};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-core/examples/system_prompt/build.rs` around lines 1 - 4, Add a module-level documentation header using a `//!` comment at the top of this file that briefly describes the purpose of this shared preview helper module (what it provides and how it’s used by the examples), following the pattern used by other example helpers; place the `//!` header immediately above the existing imports (before `use llm_coding_tools_core::context;`) and mention key items provided/used such as `SystemPromptBuilder`, `AllowedPathResolver`, and the helper types `PromptArtifacts`/`PromptCase`.src/llm-coding-tools-core/src/context/tool_prompt/common_rules.rs (1)
28-29: Moveusestatement to module scope.Same as in
tool_sections.rs, thisusestatement should be at the top of the file per coding guidelines.Suggested fix
Move the import to the module-level imports:
use super::{push_line, write_tool_list, ToolPromptFacts}; +use crate::tool_metadata::{edit, glob, grep, read, write};Then remove line 29.
As per coding guidelines: "Place
usestatements inside functions only for#[cfg]conditional compilation".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-core/src/context/tool_prompt/common_rules.rs` around lines 28 - 29, The local `use` inside append_bash_rule should be moved to module scope: remove the `use crate::tool_metadata::{edit, glob, grep, read, write};` from inside the function append_bash_rule and add that same import to the file's top-level imports (alongside other module-level `use` statements, similar to the change in tool_sections.rs) so the symbols edit, glob, grep, read, write are available without function-local imports; then delete the now-redundant line inside append_bash_rule.src/llm-coding-tools-core/src/context/tool_prompt/tool_sections.rs (1)
214-216: Moveusestatement to module scope.The coding guideline specifies: "Place
usestatements inside functions only for#[cfg]conditional compilation." This import should be moved to the top of the file with other imports.Suggested fix
use super::{push_block, push_line, write_tool_list, PathMode, ToolPrompt, ToolPromptFacts}; +use crate::tool_metadata::{glob, grep, read}; pub(super) fn render_tool(prompt: ToolPrompt, output: &mut String, facts: ToolPromptFacts) {Then remove line 215:
fn write_task_section(output: &mut String, facts: ToolPromptFacts) { - use crate::tool_metadata::{glob, grep, read}; - push_block(As per coding guidelines: "Place
usestatements inside functions only for#[cfg]conditional compilation".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-core/src/context/tool_prompt/tool_sections.rs` around lines 214 - 216, Move the in-function use into module scope: remove the `use crate::tool_metadata::{glob, grep, read};` from inside the function `write_task_section` and add it with the other top-level imports so `glob`, `grep`, and `read` are imported at module scope rather than within `write_task_section`.src/llm-coding-tools-serdesai/src/todo.rs (1)
62-80: Inconsistent use of metadata constants for property keys.The
requiredarray correctly uses metadata constants (e.g.,todo_write_meta::param::ID.name), but thepropertiesobject uses hardcoded string keys ("id","content", etc.). This creates a maintenance risk if parameter names are ever changed in the metadata.♻️ Suggested fix to use metadata constants consistently
"properties": { - "id": { "type": "string", "description": todo_write_meta::param::ID.description }, - "content": { "type": "string", "description": todo_write_meta::param::CONTENT.description }, + todo_write_meta::param::ID.name: { "type": "string", "description": todo_write_meta::param::ID.description }, + todo_write_meta::param::CONTENT.name: { "type": "string", "description": todo_write_meta::param::CONTENT.description }, "status": { "type": "string", "enum": ["pending", "in_progress", "completed", "cancelled"],Note: The
serde_json::json!macro may not support non-literal keys directly. You may need to build the properties object programmatically or use a helper that constructs the JSON with dynamic keys.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-serdesai/src/todo.rs` around lines 62 - 80, The properties object currently uses hardcoded keys ("id", "content", etc.) while required uses todo_write_meta::param::<...>.name constants; replace the hardcoded property keys by constructing the properties map programmatically using the same metadata constants (e.g., todo_write_meta::param::ID.name, ::CONTENT.name, ::STATUS.name, ::PRIORITY.name) and values built as JSON objects that reuse the metadata descriptions (todo_write_meta::param::*.description) and enums/types; because serde_json::json! doesn't accept dynamic keys, create a serde_json::Map<String, Value> (or a HashMap then convert) and insert each property entry, then attach that map to the overall schema so keys remain in sync with the metadata constants used by the required array.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/llm-coding-tools-core/examples/system_prompt_preview_compare.rs`:
- Around line 24-26: The preview currently hides prompt growth because
print_delta() clamps negative deltas to 0 and headings assume comparisons are
always savings; update the behavior so actual signed deltas are shown: either
modify print_delta() (or add a new print_delta_signed()) to compute delta =
other.len() - full.len() and print negative values (with sign) instead of
clamping to 0, and update the calls in system_prompt_preview_compare.rs (the
calls to print_delta for "No supplemental workflow", "Readonly" and the other
comparisons around 96-113) so the headings reflect that deltas can be increases
as well as savings.
In `@src/llm-coding-tools-serdesai/src/absolute/edit.rs`:
- Around line 61-65: The schema boolean for replace_all currently only marks it
optional and drops the provider-facing default; update the builder call that
constructs the field (the boolean invocation using
edit_meta::param::REPLACE_ALL.name/description/required) to also set the
advertised default to the canonical default from the metadata (e.g., use the
default value from edit_meta::param::REPLACE_ALL) so the schema matches
EditArgs' default=false; apply the same change to the parallel definition in
allowed/edit.rs (the boolean call around lines referencing
edit_meta::param::REPLACE_ALL).
---
Nitpick comments:
In `@src/llm-coding-tools-core/examples/system_prompt_preview_compare.rs`:
- Around line 29-94: FULL_SYSTEM_PROMPT, READONLY_SYSTEM_PROMPT, TASK_TARGETS,
full_case(), and readonly_case() are duplicated and should be extracted into a
shared example module so all three files reuse the same fixtures; create a new
module (e.g., examples::shared_prompts) that defines and exposes the constants
and functions named FULL_SYSTEM_PROMPT, READONLY_SYSTEM_PROMPT, TASK_TARGETS,
full_case, and readonly_case, replace the inline definitions in
system_prompt_preview.rs, system_prompt_preview_compare.rs, and
system_prompt_preview_readonly.rs with use/imports from that shared module, and
ensure the signatures and types (PromptCase, TaskTarget, ReadConfig, PathMode,
GrepConfig) match so callers compile without changes.
In `@src/llm-coding-tools-core/examples/system_prompt/build.rs`:
- Around line 1-4: Add a module-level documentation header using a `//!` comment
at the top of this file that briefly describes the purpose of this shared
preview helper module (what it provides and how it’s used by the examples),
following the pattern used by other example helpers; place the `//!` header
immediately above the existing imports (before `use
llm_coding_tools_core::context;`) and mention key items provided/used such as
`SystemPromptBuilder`, `AllowedPathResolver`, and the helper types
`PromptArtifacts`/`PromptCase`.
In `@src/llm-coding-tools-core/src/context/tool_prompt/common_rules.rs`:
- Around line 28-29: The local `use` inside append_bash_rule should be moved to
module scope: remove the `use crate::tool_metadata::{edit, glob, grep, read,
write};` from inside the function append_bash_rule and add that same import to
the file's top-level imports (alongside other module-level `use` statements,
similar to the change in tool_sections.rs) so the symbols edit, glob, grep,
read, write are available without function-local imports; then delete the
now-redundant line inside append_bash_rule.
In `@src/llm-coding-tools-core/src/context/tool_prompt/tool_sections.rs`:
- Around line 214-216: Move the in-function use into module scope: remove the
`use crate::tool_metadata::{glob, grep, read};` from inside the function
`write_task_section` and add it with the other top-level imports so `glob`,
`grep`, and `read` are imported at module scope rather than within
`write_task_section`.
In `@src/llm-coding-tools-core/src/system_prompt.rs`:
- Line 402: Replace hard-coded tool name string literals used in the built-in
prompt test helpers with the canonical constants from tool_metadata (use
tool_metadata::<ToolModule>::NAME) so the tests use the same source of truth as
examples/mock_tools.rs; update any helper that references raw names (and any
uses near PathMode) to import the appropriate tool_metadata modules and
reference their NAME constants instead of literal strings.
In `@src/llm-coding-tools-serdesai/src/todo.rs`:
- Around line 62-80: The properties object currently uses hardcoded keys ("id",
"content", etc.) while required uses todo_write_meta::param::<...>.name
constants; replace the hardcoded property keys by constructing the properties
map programmatically using the same metadata constants (e.g.,
todo_write_meta::param::ID.name, ::CONTENT.name, ::STATUS.name, ::PRIORITY.name)
and values built as JSON objects that reuse the metadata descriptions
(todo_write_meta::param::*.description) and enums/types; because
serde_json::json! doesn't accept dynamic keys, create a serde_json::Map<String,
Value> (or a HashMap then convert) and insert each property entry, then attach
that map to the overall schema so keys remain in sync with the metadata
constants used by the required array.
🪄 Autofix (Beta)
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
Run ID: 1d85f298-cf6b-4316-8e7c-040041a1699f
📒 Files selected for processing (44)
src/llm-coding-tools-core/README.mdsrc/llm-coding-tools-core/examples/system_prompt/build.rssrc/llm-coding-tools-core/examples/system_prompt/definitions.rssrc/llm-coding-tools-core/examples/system_prompt/mock_tools.rssrc/llm-coding-tools-core/examples/system_prompt/mod.rssrc/llm-coding-tools-core/examples/system_prompt/report.rssrc/llm-coding-tools-core/examples/system_prompt/types.rssrc/llm-coding-tools-core/examples/system_prompt_preview.rssrc/llm-coding-tools-core/examples/system_prompt_preview_compare.rssrc/llm-coding-tools-core/examples/system_prompt_preview_readonly.rssrc/llm-coding-tools-core/src/context/bash.txtsrc/llm-coding-tools-core/src/context/edit_absolute.txtsrc/llm-coding-tools-core/src/context/edit_allowed.txtsrc/llm-coding-tools-core/src/context/glob_absolute.txtsrc/llm-coding-tools-core/src/context/glob_allowed.txtsrc/llm-coding-tools-core/src/context/grep_absolute.txtsrc/llm-coding-tools-core/src/context/grep_allowed.txtsrc/llm-coding-tools-core/src/context/mod.rssrc/llm-coding-tools-core/src/context/read_absolute.txtsrc/llm-coding-tools-core/src/context/read_allowed.txtsrc/llm-coding-tools-core/src/context/task.txtsrc/llm-coding-tools-core/src/context/todoread.txtsrc/llm-coding-tools-core/src/context/todowrite.txtsrc/llm-coding-tools-core/src/context/tool_prompt/common_rules.rssrc/llm-coding-tools-core/src/context/tool_prompt/mod.rssrc/llm-coding-tools-core/src/context/tool_prompt/tool_sections.rssrc/llm-coding-tools-core/src/context/webfetch.txtsrc/llm-coding-tools-core/src/context/write_absolute.txtsrc/llm-coding-tools-core/src/context/write_allowed.txtsrc/llm-coding-tools-core/src/system_prompt.rssrc/llm-coding-tools-serdesai/src/absolute/edit.rssrc/llm-coding-tools-serdesai/src/absolute/glob.rssrc/llm-coding-tools-serdesai/src/absolute/grep.rssrc/llm-coding-tools-serdesai/src/absolute/read.rssrc/llm-coding-tools-serdesai/src/absolute/write.rssrc/llm-coding-tools-serdesai/src/allowed/edit.rssrc/llm-coding-tools-serdesai/src/allowed/glob.rssrc/llm-coding-tools-serdesai/src/allowed/grep.rssrc/llm-coding-tools-serdesai/src/allowed/read.rssrc/llm-coding-tools-serdesai/src/allowed/write.rssrc/llm-coding-tools-serdesai/src/bash.rssrc/llm-coding-tools-serdesai/src/task/tool.rssrc/llm-coding-tools-serdesai/src/todo.rssrc/llm-coding-tools-serdesai/src/webfetch.rs
💤 Files with no reviewable changes (15)
- src/llm-coding-tools-core/src/context/task.txt
- src/llm-coding-tools-core/src/context/edit_absolute.txt
- src/llm-coding-tools-core/src/context/grep_allowed.txt
- src/llm-coding-tools-core/src/context/glob_allowed.txt
- src/llm-coding-tools-core/src/context/edit_allowed.txt
- src/llm-coding-tools-core/src/context/grep_absolute.txt
- src/llm-coding-tools-core/src/context/glob_absolute.txt
- src/llm-coding-tools-core/src/context/read_absolute.txt
- src/llm-coding-tools-core/src/context/bash.txt
- src/llm-coding-tools-core/src/context/todowrite.txt
- src/llm-coding-tools-core/src/context/read_allowed.txt
- src/llm-coding-tools-core/src/context/todoread.txt
- src/llm-coding-tools-core/src/context/write_allowed.txt
- src/llm-coding-tools-core/src/context/write_absolute.txt
- src/llm-coding-tools-core/src/context/webfetch.txt
🚧 Files skipped from review as they are similar to previous changes (8)
- src/llm-coding-tools-serdesai/src/absolute/read.rs
- src/llm-coding-tools-serdesai/src/bash.rs
- src/llm-coding-tools-serdesai/src/allowed/write.rs
- src/llm-coding-tools-serdesai/src/allowed/grep.rs
- src/llm-coding-tools-serdesai/src/absolute/glob.rs
- src/llm-coding-tools-serdesai/src/absolute/write.rs
- src/llm-coding-tools-serdesai/src/webfetch.rs
- src/llm-coding-tools-serdesai/src/task/tool.rs
📜 Review details
⏰ 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). (5)
- GitHub Check: Build and Test (Async/Tokio) (windows-latest, x86_64-pc-windows-msvc, false)
- GitHub Check: Build and Test (Blocking) (macos-latest, aarch64-apple-darwin, false)
- GitHub Check: Build and Test (Async/Tokio) (macos-latest, aarch64-apple-darwin, false)
- GitHub Check: Build and Test (Async/Tokio) (ubuntu-latest, x86_64-unknown-linux-gnu, false)
- GitHub Check: Build and Test (Blocking) (windows-latest, x86_64-pc-windows-msvc, false)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.rs
📄 CodeRabbit inference engine (src/AGENTS.md)
src/**/*.rs: Preallocate collections when size is known or estimable usingString::with_capacity(estimated_len),Vec::with_capacity(count), orBufReader::with_capacity(size, reader)
Prefer&str/&[T]returns over owned types when lifetime allows
UseCow<'_, str>for conditional ownership (e.g.,String::from_utf8_lossy)
Use&'static strfor compile-time constant strings
Reuse buffers by using.clear()and reusingVec/Stringinstead of reallocating
Use const generics for compile-time branching (e.g.,<const LINE_NUMBERS: bool>)
Use#[inline]on small, hot-path functions
Prefercoreoverstdwhere possible (e.g.,core::memoverstd::mem)
Stream data instead of loading entire files when possible
Usememchrfor fast byte searching over manual iteration
Keep modules under 500 lines (excluding tests); split if larger
Placeusestatements inside functions only for#[cfg]conditional compilation
Document public items with///and add examples in docs where helpful
Use//!for module-level documentation
Focus comments on 'why' not 'what'
Use [TypeName] rustdoc links in documentation, not backticks
Files:
src/llm-coding-tools-core/examples/system_prompt_preview_compare.rssrc/llm-coding-tools-serdesai/src/absolute/edit.rssrc/llm-coding-tools-core/src/system_prompt.rssrc/llm-coding-tools-core/examples/system_prompt/mock_tools.rssrc/llm-coding-tools-serdesai/src/allowed/edit.rssrc/llm-coding-tools-core/examples/system_prompt/build.rssrc/llm-coding-tools-core/examples/system_prompt_preview_readonly.rssrc/llm-coding-tools-core/examples/system_prompt_preview.rssrc/llm-coding-tools-core/src/context/tool_prompt/tool_sections.rssrc/llm-coding-tools-core/examples/system_prompt/definitions.rssrc/llm-coding-tools-core/examples/system_prompt/mod.rssrc/llm-coding-tools-core/src/context/tool_prompt/common_rules.rssrc/llm-coding-tools-serdesai/src/allowed/glob.rssrc/llm-coding-tools-core/examples/system_prompt/report.rssrc/llm-coding-tools-serdesai/src/absolute/grep.rssrc/llm-coding-tools-serdesai/src/todo.rssrc/llm-coding-tools-core/src/context/mod.rssrc/llm-coding-tools-core/src/context/tool_prompt/mod.rssrc/llm-coding-tools-core/examples/system_prompt/types.rssrc/llm-coding-tools-serdesai/src/allowed/read.rs
🧠 Learnings (5)
📚 Learning: 2026-03-17T21:06:53.909Z
Learnt from: CR
Repo: Sewer56/llm-coding-tools PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-03-17T21:06:53.909Z
Learning: Applies to src/**/*.rs : Document public items with `///` and add examples in docs where helpful
Applied to files:
src/llm-coding-tools-core/examples/system_prompt_preview_compare.rssrc/llm-coding-tools-core/examples/system_prompt/mock_tools.rssrc/llm-coding-tools-core/examples/system_prompt/build.rssrc/llm-coding-tools-core/examples/system_prompt_preview_readonly.rssrc/llm-coding-tools-core/examples/system_prompt_preview.rssrc/llm-coding-tools-core/src/context/tool_prompt/tool_sections.rssrc/llm-coding-tools-core/examples/system_prompt/definitions.rssrc/llm-coding-tools-core/examples/system_prompt/mod.rssrc/llm-coding-tools-core/src/context/tool_prompt/common_rules.rssrc/llm-coding-tools-core/examples/system_prompt/report.rssrc/llm-coding-tools-core/examples/system_prompt/types.rs
📚 Learning: 2026-03-17T21:06:53.909Z
Learnt from: CR
Repo: Sewer56/llm-coding-tools PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-03-17T21:06:53.909Z
Learning: Applies to src/**/*.rs : Use `//!` for module-level documentation
Applied to files:
src/llm-coding-tools-core/examples/system_prompt/mock_tools.rssrc/llm-coding-tools-core/src/context/tool_prompt/tool_sections.rssrc/llm-coding-tools-core/examples/system_prompt/mod.rssrc/llm-coding-tools-core/src/context/tool_prompt/common_rules.rssrc/llm-coding-tools-core/src/context/tool_prompt/mod.rssrc/llm-coding-tools-core/examples/system_prompt/types.rs
📚 Learning: 2026-03-17T21:06:53.909Z
Learnt from: CR
Repo: Sewer56/llm-coding-tools PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-03-17T21:06:53.909Z
Learning: Applies to src/**/*.rs : Keep modules under 500 lines (excluding tests); split if larger
Applied to files:
src/llm-coding-tools-core/examples/system_prompt/mod.rssrc/llm-coding-tools-core/src/context/tool_prompt/common_rules.rs
📚 Learning: 2026-03-17T21:06:53.909Z
Learnt from: CR
Repo: Sewer56/llm-coding-tools PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-03-17T21:06:53.909Z
Learning: Applies to src/**/*.rs : Place `use` statements inside functions only for `#[cfg]` conditional compilation
Applied to files:
src/llm-coding-tools-core/examples/system_prompt/mod.rs
📚 Learning: 2026-03-17T21:06:53.909Z
Learnt from: CR
Repo: Sewer56/llm-coding-tools PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-03-17T21:06:53.909Z
Learning: Applies to src/**/Cargo.toml : Keep dependency footprint minimal
Applied to files:
src/llm-coding-tools-core/examples/system_prompt/mod.rs
🔇 Additional comments (24)
src/llm-coding-tools-core/src/system_prompt.rs (1)
1280-1335: Nice coverage for the common-rules factoring.These cases hit both the “emit once” and “omit unavailable tools” paths, which should make prompt regressions much easier to catch.
src/llm-coding-tools-core/src/context/tool_prompt/tool_sections.rs (1)
1-28: LGTM!The module structure is clean with good separation of concerns. The pattern matching in
render_toolis clear, and the delegation to per-tool writers keeps the code well-organized.src/llm-coding-tools-core/src/context/tool_prompt/common_rules.rs (1)
130-169: LGTM - Thorough test coverage.The exhaustive enumeration of all
ToolPromptFactsboolean combinations ensures theCOMMON_RULES_SECTION_MAX_SIZEconstant stays in sync with the actual rendered output. This is a solid approach for preventing size estimation drift.src/llm-coding-tools-serdesai/src/allowed/read.rs (2)
104-112: LGTM!Clean migration from static string context to structured
ToolPrompt::Read. TheLINE_NUMBERSconst generic is properly threaded through to the prompt variant.
62-91: LGTM!The schema and definition construction now consistently sources all metadata from
read_meta, eliminating duplication between the tool definition and the centralized metadata module.src/llm-coding-tools-core/src/context/mod.rs (2)
78-88: LGTM!The
ToolContexttrait signature change to returnToolPromptinstead of&'static stris the key abstraction enabling conditional prompt rendering. The#[must_use]annotation is appropriate.
1-38: Good documentation update.The module-level docs provide clear examples for both built-in tool prompts (structured
ToolPrompt::Read) and custom tools (usingToolPrompt::Static), which helps implementers understand the new pattern.src/llm-coding-tools-serdesai/src/allowed/glob.rs (1)
74-81: LGTM!Consistent migration to structured
ToolPrompt::GlobwithPathMode::Allowed, matching the pattern established in other tool implementations.src/llm-coding-tools-core/examples/system_prompt/mod.rs (1)
1-31: LGTM!The example module is well-organized with appropriate
#![allow(...)]attributes for example code. Thesort_sizes_deschelper correctly implements descending size ordering with lexicographic name tiebreaker.src/llm-coding-tools-core/examples/system_prompt/definitions.rs (2)
10-23: LGTM!Good use of
Vec::with_capacity(10)for the known maximum tool count. The conditional push pattern cleanly handles optional tools.
359-372: LGTM!The capacity estimation
128 + targets.len() * 48is a reasonable heuristic for the prefix and per-target lines, avoiding reallocations during string building.src/llm-coding-tools-serdesai/src/absolute/grep.rs (2)
130-138: LGTM!Clean migration to
ToolPrompt::Grepwith properPathMode::AbsoluteandLINE_NUMBERSconst generic threading.
95-98: LGTM!The limit handling now consistently uses
grep_meta::DEFAULT_LIMITandgrep_meta::MAX_LIMIT, eliminating magic numbers and ensuring consistency with the tool definition schema bounds.src/llm-coding-tools-core/README.md (1)
35-40: LGTM!The README updates correctly reflect the migration from
tool_namestotool_metadatamodules. The code examples properly demonstrate the newToolPromptreturn type withPathModeconfiguration, matching the actualToolPromptenum definition in the codebase.Also applies to: 82-154
src/llm-coding-tools-core/examples/system_prompt/report.rs (2)
3-6: LGTM!The token estimation using
chars.div_ceil(4)is a reasonable approximation (commonly used ~4 chars/token heuristic). The function is appropriately documented.
48-85: LGTM!The section parser correctly handles the heading structure emitted by
SystemPromptBuilder::build():
- Finds
# Tool Usage Guidelinesstart marker- Stops at next
#heading (excluding##)- Strips the " Tool" suffix and backticks to normalize section names
- Properly flushes the last section at end-of-input
The logic aligns with the prompt structure in
system_prompt.rs(context snippet 1).src/llm-coding-tools-serdesai/src/todo.rs (2)
8-11: LGTM!The imports correctly bring in
ToolPromptand the aliased metadata modules for cleaner usage throughout the file.
99-105: LGTM!The
ToolContextimplementations correctly return the newToolPrompt::TodoWriteandToolPrompt::TodoReadvariants, and use the metadataNAMEconstants.Also applies to: 139-145
src/llm-coding-tools-core/src/context/tool_prompt/mod.rs (4)
1-11: LGTM!Good module-level documentation using
//!as per guidelines. The module purpose is clearly explained.
23-63: LGTM!The
PathModeandToolPromptenums are well-documented and provide a clean abstraction for tool guidance rendering. The enum variants cover all built-in tools with appropriate configuration fields.
72-152: LGTM!
ToolPromptFactscorrectly tracks tool presence and determines when shared rules should be rendered. Thefrom_promptspattern efficiently builds facts in a single pass. Thehas_common_ruleslogic appropriately identifies when tools share common guidance (allowed paths, bash + file tools, multiple discovery tools, etc.).
163-185: LGTM!The
write_tool_listhelper correctly handles:
- Empty list (no output)
- Single item (just the name)
- Two items ("A and B")
- Three+ items with Oxford comma ("A, B, and C")
src/llm-coding-tools-core/examples/system_prompt/types.rs (2)
6-25: LGTM!The configuration structs are appropriately documented and use
Copyderivation for lightweight value semantics. Using&'static strforTaskTargetfields aligns with the coding guidelines for compile-time constant strings.
56-84: LGTM!The
PromptArtifactsstruct cleanly aggregates rendered output data. Thedefinition_sizesmethod provides useful introspection for the preview examples. Usingunwrap()on serialization is acceptable here sinceserde_json::to_stringforValueis infallible for well-formed values.
"Do not commit obvious secrets" was ambiguous — updated to "Do not commit any secrets or sensitive data" for an absolute prohibition.
Core is the authoritative validation gate across all tools (e.g., read re-checks offset/limit bounds, edit validates empty strings). Moving the timeout bounds check from the serdesai wrapper into core's fetch_url ensures any future framework wrapper gets the same protection without duplicating logic. The schema constraint in serdesai remains as defense-in-depth. Changes: - fetch_url (tokio + blocking) now accepts timeout_ms: u64 and validates 1..=MAX_TIMEOUT_MS - Serdesai wrapper passes timeout_ms directly, no longer constructs Duration
Replace hard-coded tool name string literals in the built-in prompt test macros with the canonical constants from tool_metadata, matching the pattern already used in common_rules.rs and tool_sections.rs.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/llm-coding-tools-core/src/tools/webfetch/blocking_impl.rs (1)
107-117:⚠️ Potential issue | 🟡 MinorThese blocking tests can pass without validating behavior.
The plain-text test passes on
Err, and the 404 test passes onOk, so regressions in request handling or status handling can still leave CI green. Please make the expectedOk/Errshape explicit, or move these behind a deterministic local fixture if they need to stay in the suite.Also applies to: 120-128
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-core/src/tools/webfetch/blocking_impl.rs` around lines 107 - 117, The test fetches_plain_text (and the related 404 test) currently treat both Ok and Err as passing; make the expected Result explicit by asserting the result shape instead of conditional checks: in fetches_plain_text call fetch_url and assert result.is_ok() (or use result.unwrap()) before checking output.content and output.content_type, and in the 404 test assert result.is_err() (or check status code == 404 on Ok) so CI fails on regressions; if network nondeterminism is a concern, move these into a deterministic local fixture or mock the HTTP client used by test_client()/fetch_url instead.
🧹 Nitpick comments (1)
src/llm-coding-tools-core/src/tools/webfetch/tokio_impl.rs (1)
18-25: Add direct coverage for the new timeout bounds.Nothing in this file currently exercises
timeout_ms == 0ortimeout_ms > MAX_TIMEOUT_MS, so that validation branch could regress silently. Two small tests that assertErr(ToolError::Validation(_))for those boundaries would lock down the contract.🧪 Minimal tests
+ #[tokio::test] + async fn rejects_zero_timeout() { + let client = test_client(); + let result = fetch_url(&client, "https://example.com", 0).await; + assert!(matches!(result, Err(ToolError::Validation(_)))); + } + + #[tokio::test] + async fn rejects_timeout_above_max() { + let client = test_client(); + let result = fetch_url( + &client, + "https://example.com", + crate::tool_metadata::webfetch::MAX_TIMEOUT_MS + 1, + ) + .await; + assert!(matches!(result, Err(ToolError::Validation(_)))); + }Also applies to: 100-193
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-core/src/tools/webfetch/tokio_impl.rs` around lines 18 - 25, Add two unit tests that exercise the timeout validation in the tokio_impl.rs code path (the branch that returns Err(ToolError::Validation(...)) when timeout_ms == 0 or timeout_ms > MAX_TIMEOUT_MS). In the test module call the function that constructs the timeout (the code that receives timeout_ms and computes Duration::from_millis) with timeout_ms = 0 and with timeout_ms = MAX_TIMEOUT_MS + 1, and assert each returns an Err matching ToolError::Validation(_). Place these tests near other tokio_impl.rs tests so they fail if the validation branch regresses.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/llm-coding-tools-core/src/tools/webfetch/blocking_impl.rs`:
- Around line 107-117: The test fetches_plain_text (and the related 404 test)
currently treat both Ok and Err as passing; make the expected Result explicit by
asserting the result shape instead of conditional checks: in fetches_plain_text
call fetch_url and assert result.is_ok() (or use result.unwrap()) before
checking output.content and output.content_type, and in the 404 test assert
result.is_err() (or check status code == 404 on Ok) so CI fails on regressions;
if network nondeterminism is a concern, move these into a deterministic local
fixture or mock the HTTP client used by test_client()/fetch_url instead.
---
Nitpick comments:
In `@src/llm-coding-tools-core/src/tools/webfetch/tokio_impl.rs`:
- Around line 18-25: Add two unit tests that exercise the timeout validation in
the tokio_impl.rs code path (the branch that returns
Err(ToolError::Validation(...)) when timeout_ms == 0 or timeout_ms >
MAX_TIMEOUT_MS). In the test module call the function that constructs the
timeout (the code that receives timeout_ms and computes Duration::from_millis)
with timeout_ms = 0 and with timeout_ms = MAX_TIMEOUT_MS + 1, and assert each
returns an Err matching ToolError::Validation(_). Place these tests near other
tokio_impl.rs tests so they fail if the validation branch regresses.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c81c28c4-e61d-4c9c-9c8b-5a12df3250c1
📒 Files selected for processing (5)
src/llm-coding-tools-core/src/context/git_workflow.txtsrc/llm-coding-tools-core/src/context/github_cli.txtsrc/llm-coding-tools-core/src/tools/webfetch/blocking_impl.rssrc/llm-coding-tools-core/src/tools/webfetch/tokio_impl.rssrc/llm-coding-tools-serdesai/src/webfetch.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/llm-coding-tools-core/src/context/git_workflow.txt
- src/llm-coding-tools-core/src/context/github_cli.txt
📜 Review details
⏰ 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). (3)
- GitHub Check: Build and Test (Async/Tokio) (windows-latest, x86_64-pc-windows-msvc, false)
- GitHub Check: Build and Test (Async/Tokio) (macos-latest, aarch64-apple-darwin, false)
- GitHub Check: Build and Test (Async/Tokio) (ubuntu-latest, x86_64-unknown-linux-gnu, false)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.rs
📄 CodeRabbit inference engine (src/AGENTS.md)
src/**/*.rs: Preallocate collections when size is known or estimable usingString::with_capacity(estimated_len),Vec::with_capacity(count), orBufReader::with_capacity(size, reader)
Prefer&str/&[T]returns over owned types when lifetime allows
UseCow<'_, str>for conditional ownership (e.g.,String::from_utf8_lossy)
Use&'static strfor compile-time constant strings
Reuse buffers by using.clear()and reusingVec/Stringinstead of reallocating
Use const generics for compile-time branching (e.g.,<const LINE_NUMBERS: bool>)
Use#[inline]on small, hot-path functions
Prefercoreoverstdwhere possible (e.g.,core::memoverstd::mem)
Stream data instead of loading entire files when possible
Usememchrfor fast byte searching over manual iteration
Keep modules under 500 lines (excluding tests); split if larger
Placeusestatements inside functions only for#[cfg]conditional compilation
Document public items with///and add examples in docs where helpful
Use//!for module-level documentation
Focus comments on 'why' not 'what'
Use [TypeName] rustdoc links in documentation, not backticks
Files:
src/llm-coding-tools-core/src/tools/webfetch/blocking_impl.rssrc/llm-coding-tools-core/src/tools/webfetch/tokio_impl.rssrc/llm-coding-tools-serdesai/src/webfetch.rs
🔇 Additional comments (2)
src/llm-coding-tools-core/src/tools/webfetch/blocking_impl.rs (1)
17-26: Good defense-in-depth on the raw timeout input.Validating
timeout_msin core before building the request keeps non-wrapper callers from bypassing the schema constraint and makes the blocking backend reject bad values early.src/llm-coding-tools-serdesai/src/webfetch.rs (1)
14-16: Nice single-source-of-truth cleanup.Pulling the default, schema fields, tool name, and
ToolPromptvariant fromwebfetch_metaremoves the remaining wrapper-specific copies and keeps this adapter aligned with core.Also applies to: 61-95
Follows the project guideline to place `use` inside functions only for `#[cfg]` conditional compilation.
In-function use statements without #[cfg] conditional compilation
belong at module scope per project conventions.
Changes:
- Moved `use crate::tool_metadata::{glob, grep, read}` out of `write_task_section`
Exercises the timeout_ms == 0 and timeout_ms > MAX_TIMEOUT_MS validation branches in both tokio and blocking fetch_url.
…tants Tool prompt text in tool_sections.rs and common_rules.rs now uses const_format::formatcp! with canonical constants from tool_metadata (e.g. bash::NAME, read::MAX_LINE_LENGTH, glob::MAX_RESULTS) instead of hard-coded string literals. This keeps prompt copy in sync with the metadata module and eliminates duplicate definitions. Also adds several new metadata constants (MAX_LINE_LENGTH, LINE_PREFIX_FORMAT, LINE_PREFIX_DISPLAY, MAX_RESULTS, MAX_RESPONSE_SIZE_MIB) to centralize values that were previously only embedded in prompt text. Fixed: stray backtick after `docker` in bash tool prompt (CodeRabbit).
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/llm-coding-tools-core/src/tool_metadata/webfetch.rs (2)
6-19: Use explicit&'static strfor compile-time string constants.Line 6 and Line 18 can be typed as
&'static strto match the repository convention for constant strings.♻️ Proposed diff
-pub const NAME: &str = "webfetch"; +pub const NAME: &'static str = "webfetch"; @@ -pub const DESCRIPTION: &str = +pub const DESCRIPTION: &'static str = "Fetch one URL. HTML is converted to Markdown and JSON is pretty-printed.";As per coding guidelines, use
&'static strfor compile-time constant strings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-core/src/tool_metadata/webfetch.rs` around lines 6 - 19, The NAME and DESCRIPTION constants use &str and should be explicitly typed as &'static str to follow project convention; update the declarations of the constants named NAME and DESCRIPTION to use the &'static str type (i.e., change their type annotations to &'static str) while keeping their current values unchanged so they remain compile-time string constants.
1-1: Prefer rustdoc links/plain text over backticks in documentation comments.The doc comments on Line 1 and around Line 26–30 use backticks for identifiers; consider plain text or rustdoc links for consistency with project docs style.
As per coding guidelines, use [
TypeName] rustdoc links in documentation, not backticks.Also applies to: 26-30
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-core/src/tool_metadata/webfetch.rs` at line 1, Replace backticked identifiers in the documentation comments (e.g., the top-line "the `webfetch` tool" and the identifiers in the block around lines ~26–30) with rustdoc links or plain text per project style; use the [`webfetch`] style for module/type/function names or plain text for non-code phrases so doc tooling will link types correctly and match guidelines (look for occurrences of `webfetch` and any other backticked names in the file and convert them to [`Name`] rustdoc links or plain text as appropriate).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/llm-coding-tools-core/src/tool_metadata/webfetch.rs`:
- Around line 6-19: The NAME and DESCRIPTION constants use &str and should be
explicitly typed as &'static str to follow project convention; update the
declarations of the constants named NAME and DESCRIPTION to use the &'static str
type (i.e., change their type annotations to &'static str) while keeping their
current values unchanged so they remain compile-time string constants.
- Line 1: Replace backticked identifiers in the documentation comments (e.g.,
the top-line "the `webfetch` tool" and the identifiers in the block around lines
~26–30) with rustdoc links or plain text per project style; use the [`webfetch`]
style for module/type/function names or plain text for non-code phrases so doc
tooling will link types correctly and match guidelines (look for occurrences of
`webfetch` and any other backticked names in the file and convert them to
[`Name`] rustdoc links or plain text as appropriate).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7209a048-63c4-4f5b-847b-f6553b27b6a8
⛔ Files ignored due to path filters (1)
src/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
src/llm-coding-tools-core/Cargo.tomlsrc/llm-coding-tools-core/src/context/tool_prompt/common_rules.rssrc/llm-coding-tools-core/src/context/tool_prompt/tool_sections.rssrc/llm-coding-tools-core/src/system_prompt.rssrc/llm-coding-tools-core/src/tool_metadata/bash.rssrc/llm-coding-tools-core/src/tool_metadata/glob.rssrc/llm-coding-tools-core/src/tool_metadata/grep.rssrc/llm-coding-tools-core/src/tool_metadata/read.rssrc/llm-coding-tools-core/src/tool_metadata/webfetch.rssrc/llm-coding-tools-core/src/tools/glob.rssrc/llm-coding-tools-core/src/tools/read.rssrc/llm-coding-tools-core/src/tools/webfetch/blocking_impl.rssrc/llm-coding-tools-core/src/tools/webfetch/tokio_impl.rs
✅ Files skipped from review due to trivial changes (4)
- src/llm-coding-tools-core/Cargo.toml
- src/llm-coding-tools-core/src/tools/read.rs
- src/llm-coding-tools-core/src/tool_metadata/glob.rs
- src/llm-coding-tools-core/src/tool_metadata/bash.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- src/llm-coding-tools-core/src/tools/webfetch/blocking_impl.rs
- src/llm-coding-tools-core/src/tools/webfetch/tokio_impl.rs
- src/llm-coding-tools-core/src/context/tool_prompt/tool_sections.rs
- src/llm-coding-tools-core/src/context/tool_prompt/common_rules.rs
- src/llm-coding-tools-core/src/tool_metadata/grep.rs
- src/llm-coding-tools-core/src/tool_metadata/read.rs
📜 Review details
⏰ 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). (5)
- GitHub Check: Build and Test (Async/Tokio) (ubuntu-latest, x86_64-unknown-linux-gnu, false)
- GitHub Check: Build and Test (Blocking) (windows-latest, x86_64-pc-windows-msvc, false)
- GitHub Check: Build and Test (Async/Tokio) (macos-latest, aarch64-apple-darwin, false)
- GitHub Check: Build and Test (Async/Tokio) (windows-latest, x86_64-pc-windows-msvc, false)
- GitHub Check: Build and Test (Blocking) (macos-latest, aarch64-apple-darwin, false)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.rs
📄 CodeRabbit inference engine (src/AGENTS.md)
src/**/*.rs: Preallocate collections when size is known or estimable usingString::with_capacity(estimated_len),Vec::with_capacity(count), orBufReader::with_capacity(size, reader)
Prefer&str/&[T]returns over owned types when lifetime allows
UseCow<'_, str>for conditional ownership (e.g.,String::from_utf8_lossy)
Use&'static strfor compile-time constant strings
Reuse buffers by using.clear()and reusingVec/Stringinstead of reallocating
Use const generics for compile-time branching (e.g.,<const LINE_NUMBERS: bool>)
Use#[inline]on small, hot-path functions
Prefercoreoverstdwhere possible (e.g.,core::memoverstd::mem)
Stream data instead of loading entire files when possible
Usememchrfor fast byte searching over manual iteration
Keep modules under 500 lines (excluding tests); split if larger
Placeusestatements inside functions only for#[cfg]conditional compilation
Document public items with///and add examples in docs where helpful
Use//!for module-level documentation
Focus comments on 'why' not 'what'
Use [TypeName] rustdoc links in documentation, not backticks
Files:
src/llm-coding-tools-core/src/tools/glob.rssrc/llm-coding-tools-core/src/system_prompt.rssrc/llm-coding-tools-core/src/tool_metadata/webfetch.rs
🔇 Additional comments (9)
src/llm-coding-tools-core/src/tools/glob.rs (1)
5-5: LGTM!Clean refactor moving the
MAX_RESULTSconstant to the centralizedtool_metadatamodule. This keeps the limit in sync across prompt text and runtime behavior.src/llm-coding-tools-core/src/tool_metadata/webfetch.rs (1)
22-39: Nice centralization of parameter metadata with compile-time formatting.The
parammodule keeps tool schema text aligned with timeout constants and avoids drift in prompt/tool definitions.src/llm-coding-tools-core/src/system_prompt.rs (7)
6-8: LGTM!Import changes align with the new
ToolPrompt-based architecture.
11-15: LGTM!Storing
ToolPromptinstead of a precomputed string enables context-aware rendering with shared facts.
291-297: LGTM!The facts derivation from tracked prompts and size estimation logic is clean. Using
COMMON_RULES_SECTION_MAX_SIZEfor capacity estimation is appropriate.
357-376: LGTM!The common rules are rendered once before tool sections, and each tool's prompt is rendered with shared facts context. The trailing newline handling ensures consistent formatting.
426-489: LGTM!Good use of const generics for compile-time branching in test stubs. The macros cleanly generate tool implementations that model
ToolPromptbehavior for testing.
1281-1336: LGTM!Comprehensive tests verifying common rules are emitted once and tool references are filtered based on tracked tools. The assertions are specific and validate the key behaviors of the new
ToolPromptFacts-based rendering.
24-37: LGTM!Doc examples are updated to demonstrate the new
ToolPromptAPI withPathModeandline_numbersconfiguration.
…ines Tool definitions already convey what each tool does. The system prompt guidelines now contain only usage rules, constraints, and coordination advice, eliminating duplication and reducing prompt token count. Changes: - Removed lead sentences from all 10 tool sections in tool_sections.rs - Removed now-unused PathMode parameter from section writer functions - Added print_tool_definitions to preview examples for inspection Benefits: - Fewer prompt tokens (no duplicated descriptions) - Single source of truth for tool descriptions in tool_metadata - Cleaner separation: definitions describe "what", guidelines describe "how"
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/llm-coding-tools-core/examples/system_prompt_preview_readonly.rs (1)
18-18: Prefer a static separator string over runtime.repeat()allocation.Line 18 creates a new
Stringevery run for fixed text. Use aconst&'static strinstead.♻️ Suggested tweak
+const SECTION_SEPARATOR: &str = "\n============================================================"; + fn main() { let readonly = build_case(readonly_case()); @@ - println!("\n{}", "=".repeat(60)); + println!("{SECTION_SEPARATOR}"); print_footprint("Static request footprint", &readonly);As per coding guidelines: "Use
&'static strfor compile-time constant strings."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-core/examples/system_prompt_preview_readonly.rs` at line 18, Replace the runtime allocation "=".repeat(60) with a compile-time static string constant (e.g., define a const & 'static str like SEPARATOR = "============================================================";) and use that constant in the println! invocation where the current println!("\n{}", "=".repeat(60)) call appears (search for the println! call in system_prompt_preview_readonly.rs) so the separator is a &'static str instead of being allocated each run.src/llm-coding-tools-core/src/context/tool_prompt/tool_sections.rs (1)
11-11: Add rustdoc forrender_tool
render_toolispub(super)but currently undocumented. Please add a///doc comment describing expected inputs and output formatting contract (e.g., whether callers must manage trailing newlines).Suggested patch
-pub(super) fn render_tool(prompt: ToolPrompt, output: &mut String, facts: ToolPromptFacts) { +/// Renders built-in guidance for a single [`ToolPrompt`] into `output`. +/// +/// Appends text directly to `output` without clearing it. +/// Callers are responsible for section ordering and surrounding separators. +pub(super) fn render_tool(prompt: ToolPrompt, output: &mut String, facts: ToolPromptFacts) {As per coding guidelines: Document public items with
///and add examples in docs where helpful.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-core/src/context/tool_prompt/tool_sections.rs` at line 11, Add a triple-slash rustdoc for the pub(super) function render_tool describing its purpose and the contract for its parameters and output: explain what ToolPrompt and ToolPromptFacts represent, state that output is appended to the provided &mut String and whether the function adds or expects trailing newlines (caller responsibility), mention any formatting guarantees (e.g., escaped chars, sections order), and include a short usage example showing how to call render_tool with a mutable String and how to handle newlines. Reference the function name render_tool and the parameter names (prompt: ToolPrompt, output: &mut String, facts: ToolPromptFacts) in the doc so readers can locate the behavior easily.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/llm-coding-tools-core/examples/system_prompt_preview_readonly.rs`:
- Line 18: Replace the runtime allocation "=".repeat(60) with a compile-time
static string constant (e.g., define a const & 'static str like SEPARATOR =
"============================================================";) and use that
constant in the println! invocation where the current println!("\n{}",
"=".repeat(60)) call appears (search for the println! call in
system_prompt_preview_readonly.rs) so the separator is a &'static str instead of
being allocated each run.
In `@src/llm-coding-tools-core/src/context/tool_prompt/tool_sections.rs`:
- Line 11: Add a triple-slash rustdoc for the pub(super) function render_tool
describing its purpose and the contract for its parameters and output: explain
what ToolPrompt and ToolPromptFacts represent, state that output is appended to
the provided &mut String and whether the function adds or expects trailing
newlines (caller responsibility), mention any formatting guarantees (e.g.,
escaped chars, sections order), and include a short usage example showing how to
call render_tool with a mutable String and how to handle newlines. Reference the
function name render_tool and the parameter names (prompt: ToolPrompt, output:
&mut String, facts: ToolPromptFacts) in the doc so readers can locate the
behavior easily.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ad8bb612-8a6b-450d-9451-fb9bcd21093f
📒 Files selected for processing (5)
src/llm-coding-tools-core/examples/system_prompt/mod.rssrc/llm-coding-tools-core/examples/system_prompt/report.rssrc/llm-coding-tools-core/examples/system_prompt_preview.rssrc/llm-coding-tools-core/examples/system_prompt_preview_readonly.rssrc/llm-coding-tools-core/src/context/tool_prompt/tool_sections.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/llm-coding-tools-core/examples/system_prompt/mod.rs
- src/llm-coding-tools-core/examples/system_prompt/report.rs
- src/llm-coding-tools-core/examples/system_prompt_preview.rs
📜 Review details
⏰ 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). (2)
- GitHub Check: Build and Test (Async/Tokio) (windows-latest, x86_64-pc-windows-msvc, false)
- GitHub Check: Build and Test (Async/Tokio) (macos-latest, aarch64-apple-darwin, false)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.rs
📄 CodeRabbit inference engine (src/AGENTS.md)
src/**/*.rs: Preallocate collections when size is known or estimable usingString::with_capacity(estimated_len),Vec::with_capacity(count), orBufReader::with_capacity(size, reader)
Prefer&str/&[T]returns over owned types when lifetime allows
UseCow<'_, str>for conditional ownership (e.g.,String::from_utf8_lossy)
Use&'static strfor compile-time constant strings
Reuse buffers by using.clear()and reusingVec/Stringinstead of reallocating
Use const generics for compile-time branching (e.g.,<const LINE_NUMBERS: bool>)
Use#[inline]on small, hot-path functions
Prefercoreoverstdwhere possible (e.g.,core::memoverstd::mem)
Stream data instead of loading entire files when possible
Usememchrfor fast byte searching over manual iteration
Keep modules under 500 lines (excluding tests); split if larger
Placeusestatements inside functions only for#[cfg]conditional compilation
Document public items with///and add examples in docs where helpful
Use//!for module-level documentation
Focus comments on 'why' not 'what'
Use [TypeName] rustdoc links in documentation, not backticks
Files:
src/llm-coding-tools-core/src/context/tool_prompt/tool_sections.rssrc/llm-coding-tools-core/examples/system_prompt_preview_readonly.rs
🧠 Learnings (2)
📚 Learning: 2026-03-17T21:06:53.909Z
Learnt from: CR
Repo: Sewer56/llm-coding-tools PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-03-17T21:06:53.909Z
Learning: Applies to src/**/*.rs : Use `//!` for module-level documentation
Applied to files:
src/llm-coding-tools-core/src/context/tool_prompt/tool_sections.rs
📚 Learning: 2026-03-17T21:06:53.909Z
Learnt from: CR
Repo: Sewer56/llm-coding-tools PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-03-17T21:06:53.909Z
Learning: Applies to src/**/*.rs : Document public items with `///` and add examples in docs where helpful
Applied to files:
src/llm-coding-tools-core/src/context/tool_prompt/tool_sections.rssrc/llm-coding-tools-core/examples/system_prompt_preview_readonly.rs
🔇 Additional comments (3)
src/llm-coding-tools-core/examples/system_prompt_preview_readonly.rs (2)
1-4: Nice module-level docs and runnable example command.This is clear and directly usable for local verification.
As per coding guidelines: "Use
//!for module-level documentation."
27-50: Readonly case wiring is consistent and aligned with the PR goal.The tool toggles/path modes cleanly enforce readonly behavior while still enabling prompt/footprint analysis.
src/llm-coding-tools-core/src/context/tool_prompt/tool_sections.rs (1)
12-30: Nice split of variant dispatch and per-tool writersThe
matchis exhaustive and the per-tool helper structure keeps behavior localized and readable while still allowing fact-driven prompt trimming.Also applies to: 46-234
…tests The system prompt previously used blank lines (`\n\n`) to separate major sections. This adds unnecessary token overhead for LLMs. Sections now use single newlines throughout, producing denser Markdown output. The `section_separator()` helper is removed; `build()` only appends `\n` when the preceding content lacks a trailing newline. Changes: - Removed `section_separator()` helper function - Changed all section headers from `# Section\n\n` to `# Section\n` - Removed inter-section blank line padding between Environment, Tools, and Supplemental sections - Removed duplicate and low-signal tests (empty builder duplicate, fluent API smoke tests, backwards compatibility test, overlapping supplemental ordering tests) - Merged near-identical test variants (e.g. two multi-tool newline tests, env-only smoke tests, system prompt trailing newline tests) - Added `assert_no_triple_newlines` and `assert_no_trailing_whitespace` helpers to deduplicate repeated assertions - Added `empty_environment_section_has_single_newline_boundaries` test to cover the previously-untested empty env block boundary Benefits: - Fewer tokens in generated system prompts - Test suite reduced from 47 to ~38 tests with no coverage loss
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 (1)
src/llm-coding-tools-core/src/system_prompt.rs (1)
21-63:⚠️ Potential issue | 🟡 MinorRefresh the public output example to match the new formatter.
The rustdoc example still shows blank-line-separated sections and omits the shared
## Common Rulesblock, butbuild()now emits denser Markdown and may insert that section. The current docs will mislead anyone comparing the example to actual output. As per coding guidelines, "Document public items with///and add examples in docs where helpful".Also applies to: 315-361
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-core/src/system_prompt.rs` around lines 21 - 63, Update the public rustdoc example for SystemPromptBuilder::build() to reflect the new formatter: replace the blank-line-separated sections with the denser Markdown layout the builder now emits and include the shared "## Common Rules" block that build() may insert; edit the example output under the SystemPromptBuilder docs (the code block following the example usage) so its headings, spacing and the presence of "## Common Rules" exactly match the current build() output format to avoid misleading readers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/llm-coding-tools-core/src/system_prompt.rs`:
- Around line 114-118: The code pushes a ContextEntry using T::NAME but the
prompt body (ToolPrompt/ToolPromptFacts via entry.prompt.render(...)) can
declare a different built-in action, creating a mismatch; in track<T:
ToolContext>(&mut self, tool: T) -> T you should derive or validate the display
name from the tool context instead of blindly using T::NAME: inspect the
returned ToolPrompt (or use ToolPromptFacts) to obtain the canonical built-in
display name and set ContextEntry.name to that, or assert that T::NAME matches
the prompt-derived name and panic/log on mismatch; apply the same validation or
name-derivation to the other code path that constructs ContextEntry (the other
entries.push(...) site creating ContextEntry with T::NAME) so built-in names and
prompt variants remain coupled.
---
Outside diff comments:
In `@src/llm-coding-tools-core/src/system_prompt.rs`:
- Around line 21-63: Update the public rustdoc example for
SystemPromptBuilder::build() to reflect the new formatter: replace the
blank-line-separated sections with the denser Markdown layout the builder now
emits and include the shared "## Common Rules" block that build() may insert;
edit the example output under the SystemPromptBuilder docs (the code block
following the example usage) so its headings, spacing and the presence of "##
Common Rules" exactly match the current build() output format to avoid
misleading readers.
🪄 Autofix (Beta)
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
Run ID: fa7c5608-b809-49b6-9354-5a0b937c6f5c
📒 Files selected for processing (1)
src/llm-coding-tools-core/src/system_prompt.rs
📜 Review details
⏰ 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). (5)
- GitHub Check: Build and Test (Async/Tokio) (macos-latest, aarch64-apple-darwin, false)
- GitHub Check: Build and Test (Async/Tokio) (windows-latest, x86_64-pc-windows-msvc, false)
- GitHub Check: Build and Test (Async/Tokio) (ubuntu-latest, x86_64-unknown-linux-gnu, false)
- GitHub Check: Build and Test (Blocking) (windows-latest, x86_64-pc-windows-msvc, false)
- GitHub Check: Build and Test (Blocking) (macos-latest, aarch64-apple-darwin, false)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.rs
📄 CodeRabbit inference engine (src/AGENTS.md)
src/**/*.rs: Preallocate collections when size is known or estimable usingString::with_capacity(estimated_len),Vec::with_capacity(count), orBufReader::with_capacity(size, reader)
Prefer&str/&[T]returns over owned types when lifetime allows
UseCow<'_, str>for conditional ownership (e.g.,String::from_utf8_lossy)
Use&'static strfor compile-time constant strings
Reuse buffers by using.clear()and reusingVec/Stringinstead of reallocating
Use const generics for compile-time branching (e.g.,<const LINE_NUMBERS: bool>)
Use#[inline]on small, hot-path functions
Prefercoreoverstdwhere possible (e.g.,core::memoverstd::mem)
Stream data instead of loading entire files when possible
Usememchrfor fast byte searching over manual iteration
Keep modules under 500 lines (excluding tests); split if larger
Placeusestatements inside functions only for#[cfg]conditional compilation
Document public items with///and add examples in docs where helpful
Use//!for module-level documentation
Focus comments on 'why' not 'what'
Use [TypeName] rustdoc links in documentation, not backticks
Files:
src/llm-coding-tools-core/src/system_prompt.rs
🔇 Additional comments (1)
src/llm-coding-tools-core/src/system_prompt.rs (1)
471-484: Nice regression coverage for the common-rules path.The built-in stubs plus the single-emission and unavailable-tool assertions pin down the highest-risk behavior in this refactor well.
Also applies to: 1134-1190
Explains the function's append contract, trailing-newline caveat, and links to the [`ToolPrompt`] and [`ToolPromptFacts`] types.
Move built-in tool guidance to ToolPrompt and report static request footprint
1 participant
Summary
ToolContext::context()to returnToolPrompt, then render built-in tool guidance from code so the prompt rules live in one place instead of parallelcontext/*.txtfilesSystemPromptBuilderderive sharedCommon Rulesfrom the tracked tool set, which keeps built-in guidance shorter and only emits cross-tool rules when those tools are actually presentNotes
ToolPrompt::StaticTesting
cargo run --example system_prompt_preview_compare -p llm-coding-tools-core