refactor tool module - #233
Merged
Merged
Conversation
Contributor
|
Thanks for the pull request. A maintainer will review it when available. Please keep the PR focused, explain the why in the description, and make sure local checks pass before requesting review. Contribution guide: https://github.com/AI-Shell-Team/aish/blob/main/CONTRIBUTING.md |
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughCentralizes tool prompt metadata and adds Tool::prompt and session prompt injection; modularizes many tools and file/plan tooling; implements WebFetch (validation, fetch, sanitize, cache, secondary LLM); updates shell wiring, locales, Cargo deps, and README docs. ChangesTooling Refactor and WebFetch
Estimated code review effort:
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Contributor
|
This pull request description looks incomplete. Please update the missing sections below before review. Missing items:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (11)
crates/aish-tools/src/channel_ask_user/prompt.rs (1)
3-8: ⚡ Quick winPROMPT lacks guidance on the
kindparameter.The PROMPT instructions don't explain when to use
"text_input"vs"choice_or_text", even thoughkindappears in the schema. Consider adding usage guidance similar to howask_user/prompt.rsexplainsallow_freeform_input.📝 Suggested addition
pub(crate) const PROMPT: &str = r#"Use this tool only when a small amount of user input is needed to continue. Usage: - Ask one focused question at a time. - Prefer options when the likely answers are known. +- Use kind=choice_or_text when providing predefined options. - Do not ask for secrets such as passwords, API keys, or tokens."#;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aish-tools/src/channel_ask_user/prompt.rs` around lines 3 - 8, The PROMPT constant (pub(crate) const PROMPT) is missing guidance for the schema's kind parameter; update the PROMPT text to explain when to use kind="text_input" (use for freeform user responses) versus kind="choice_or_text" (present explicit options but allow a freeform override), mirroring the style of ask_user/prompt.rs's explanation of allow_freeform_input; mention examples and a short rule-of-thumb (choose text_input for open-ended questions, choice_or_text when likely answers are known but a custom response should be allowed) so callers of the channel_ask_user schema understand which kind to pick.crates/aish-tools/src/host_note/prompt.rs (1)
10-30: 💤 Low valueConsider adding conditional parameter requirements to the schema.
The current schema marks only
actionas required, butcontentis mandatory whenaction="store"andkeywordis mandatory whenaction="forget". While runtime validation inexecute()likely catches this, the schema would be more accurate with JSON Schemaif/thenconditionals oroneOfpatterns.If schema-level enforcement is impractical, consider adding a note in the parameter descriptions clarifying the action-dependent requirements.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aish-tools/src/host_note/prompt.rs` around lines 10 - 30, Update the parameters() JSON Schema so action-dependent fields are enforced: add JSON Schema conditionals (if/then) such that if "action" == "store" then "required": ["action","content"], and if "action" == "forget" then "required": ["action","keyword"]; place these conditionals inside the top-level schema returned by the parameters() function (or, if adding conditionals is impractical, update the "content" and "keyword" property descriptions to explicitly state they are required for action="store" and action="forget" respectively so callers see the dependency — ensure this aligns with the runtime checks in execute()).crates/aish-tools/src/web_fetch/web_fetch.rs (1)
65-65: 💤 Low valueConsider removing the empty system message.
Line 65 creates a system message with an empty string. If no system-level instructions are needed for the secondary model, consider removing this message entirely and using only the user message.
♻️ Proposed simplification
- let messages = vec![ChatMessage::system(""), ChatMessage::user(model_prompt)]; + let messages = vec![ChatMessage::user(model_prompt)];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aish-tools/src/web_fetch/web_fetch.rs` at line 65, The messages vector currently includes an empty system instruction (constructed via ChatMessage::system("")) which is unnecessary; update the construction of messages in web_fetch.rs to omit the empty system message and use only ChatMessage::user(model_prompt), i.e., remove the ChatMessage::system("") entry (or replace it with a meaningful system prompt if intended) so the secondary model receives just the intended user prompt.crates/aish-tools/src/web_fetch/preapproved.rs (1)
105-105: 💤 Low valueRemove unnecessary
continuestatement.The
continuestatement at line 105 is unnecessary because it's at the end of the loop iteration. The loop will continue to the next iteration anyway.♻️ Proposed cleanup
return true; } } - continue; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aish-tools/src/web_fetch/preapproved.rs` at line 105, Remove the redundant `continue;` at the end of the loop in the preapproved handling block in crates/aish-tools/src/web_fetch/preapproved.rs (the loop that processes each preapproved entry); simply delete the `continue;` statement so the loop naturally proceeds to the next iteration without the no-op explicit continue.crates/aish-tools/src/read_file/prompt.rs (1)
18-25: ⚡ Quick winConsider adding validation constraints to offset and limit parameters.
The
offsetandlimitparameters currently lack minimum value constraints in the schema. Adding"minimum": 0foroffsetand"minimum": 1forlimitwould prevent invalid negative values and improve input validation.🛡️ Proposed validation constraints
"offset": { "type": "integer", "description": "Line offset to start reading from, 0-based.", + "minimum": 0 }, "limit": { "type": "integer", "description": "Maximum number of lines to read.", + "minimum": 1 }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aish-tools/src/read_file/prompt.rs` around lines 18 - 25, The JSON schema for the read-file prompt currently allows negative or zero values; update the schema entries for the "offset" and "limit" fields in crates/aish-tools/src/read_file/prompt.rs so that "offset" includes "minimum": 0 and "limit" includes "minimum": 1 to enforce non-negative starting offsets and a positive read limit; locate the schema block where "offset" and "limit" are defined and add those minimum constraints to their definitions.crates/aish-tools/src/plan_tool/list_plan_templates.rs (1)
72-78: 💤 Low valueConsider making template assertions more flexible.
The test hardcodes expectations for specific template names ("default", "bugfix", "feature") and an exact count of 3. This couples the test to the implementation of
aish_core::plan::get_available_templates()and will break if templates are added, removed, or renamed. Consider asserting general properties instead (e.g., at least one template exists, each has required fields) or using a configuration-driven approach.♻️ Proposed more flexible assertions
let result = tool.execute(serde_json::json!({})); assert!(result.ok); assert!(result.output.contains("Available plan templates")); - assert!(result.output.contains("default")); - assert!(result.output.contains("bugfix")); - assert!(result.output.contains("feature")); let meta = result.meta.unwrap(); let templates = meta["templates"].as_array().unwrap(); - assert_eq!(templates.len(), 3); + assert!(!templates.is_empty(), "Should have at least one template"); for t in templates { assert!(t["name"].is_string()); assert!(t["description"].is_string()); assert!(t["content"].is_string()); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aish-tools/src/plan_tool/list_plan_templates.rs` around lines 72 - 78, The test currently rigidly asserts that result.output contains "default", "bugfix", "feature" and that templates.len() == 3 which couples it to aish_core::plan::get_available_templates(); change the assertions to be more flexible by verifying that result.meta is present, that meta["templates"] parses to an array (templates) with templates.len() >= 1, and that each element in templates contains required fields (e.g., name and description or whatever keys your Template struct exposes) rather than exact names or exact count; update references to result, meta, and templates in the test to reflect these general checks so the test passes when templates are added/removed/renamed.crates/aish-tools/src/plan_tool/enter_plan_mode.rs (2)
107-109: 💤 Low valueConsider removing the hardcoded plan_id length assertion.
The test asserts that
plan_idlength is exactly 12 characters. This couples the test to the internal implementation ofgenerate_plan_id()and will break if the ID format changes. Consider removing this assertion or replacing it with a weaker check (e.g., non-empty, matches expected format pattern).♻️ Proposed alternative assertion
assert!(meta["plan_id"].is_string()); - assert_eq!(meta["plan_id"].as_str().unwrap().len(), 12); + assert!(!meta["plan_id"].as_str().unwrap().is_empty());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aish-tools/src/plan_tool/enter_plan_mode.rs` around lines 107 - 109, The test currently ties to the internal ID format by asserting meta["plan_id"] length == 12; update the assertion to avoid fragility: remove the assert_eq!(meta["plan_id"].as_str().unwrap().len(), 12) and instead assert that meta["plan_id"] exists and is non-empty (or matches a stable pattern if you expect a specific format). Locate the assertions around meta["phase"] and meta["plan_id"] in enter_plan_mode.rs (the test that validates generate_plan_id()), replace the hard length check with a non-empty check or a regex-based pattern match so the test no longer depends on the exact length.
6-16: ⚡ Quick winUpdate synchronization:
VISIBLE_TOOLS_DURING_PLANNINGmatchesaish_core::plan::PLANNING_VISIBLE_TOOLS(same 8 tool names today); consider importing the core constant to prevent future drift.
aish_core::plan::PLANNING_VISIBLE_TOOLSis public and currently contains exactly:read_file,glob,grep,ask_user,memory,write_file,edit_file,exit_plan_mode.aish-corealready has tests assertingPLANNING_VISIBLE_TOOLScontents.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aish-tools/src/plan_tool/enter_plan_mode.rs` around lines 6 - 16, Replace the hard-coded VISIBLE_TOOLS_DURING_PLANNING slice with the canonical constant from core to avoid drift: remove the local const and re-export or alias aish_core::plan::PLANNING_VISIBLE_TOOLS (e.g. add a module-level `use aish_core::plan::PLANNING_VISIBLE_TOOLS as VISIBLE_TOOLS_DURING_PLANNING;` or `pub(crate) use ... as ...`), so the code refers to the single source of truth (`aish_core::plan::PLANNING_VISIBLE_TOOLS`) instead of duplicating the eight tool names.crates/aish-tools/src/write_file/write_file.rs (1)
62-81: ⚡ Quick winConsider adding a content size limit.
ReadFileToolenforces a 32 KiB size limit (see Line 57 inread_file.rs), butWriteFileToolhas no size restriction on the content parameter. An LLM could generate and write arbitrarily large files, potentially exhausting disk space.Consider adding a content length check before writing, especially if this tool is exposed in untrusted contexts.
🛡️ Proposed size limit
}; + + const SIZE_LIMIT: usize = 32 * 1024; // Match read_file limit + if content.len() > SIZE_LIMIT { + let mut args_map = std::collections::HashMap::new(); + args_map.insert("size".to_string(), content.len().to_string()); + args_map.insert("limit".to_string(), SIZE_LIMIT.to_string()); + return ToolResult::error(aish_i18n::t_with_args( + "tools.fs.write_file.content_too_large", + &args_map, + )); + } if let Some(parent) = Path::new(path).parent() {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aish-tools/src/write_file/write_file.rs` around lines 62 - 81, Add a content-size guard in write_file.rs before performing std::fs::write: define a MAX_WRITE_SIZE constant (e.g. 32 * 1024) to match ReadFileTool, check content.len() against it in the WriteFileTool execution path, and if it exceeds the limit return ToolResult::error (use a similar i18n key as other errors) including the attempted byte length and path in the args_map; only call std::fs::write when the size check passes.crates/aish-tools/src/edit_file/edit_file.rs (1)
73-85: ⚡ Quick winOptimize: Combine string presence and count checks.
Lines 73-80 check
content.contains(old), then Line 85 countscontent.matches(old).count(). This scans the string twice. Combine into a singlematches().count()call, then check if count is zero.⚡ Proposed optimization
- if !content.contains(old) { + let count = content.matches(old).count(); + if count == 0 { let mut args_map = std::collections::HashMap::new(); args_map.insert("path".to_string(), path.to_string()); return ToolResult::error(aish_i18n::t_with_args( "tools.fs.edit_file.old_string_not_found", &args_map, )); } let new_content = if replace_all { content.replace(old, new) } else { - let count = content.matches(old).count(); if count > 1 {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aish-tools/src/edit_file/edit_file.rs` around lines 73 - 85, Compute matches count once and use it for both presence check and replacement: call content.matches(old).count() into a variable (e.g., count), if count == 0 return the same ToolResult::error via aish_i18n::t_with_args, and then produce new_content by using content.replace(old, new) when replace_all is true, otherwise use content.replacen(old, new, 1) to replace only the first occurrence; update references to content.contains(old) and the separate count() call in edit_file.rs accordingly.crates/aish-tools/src/read_file/read_file.rs (1)
103-118: ⚡ Quick winReduce duplication in line-formatting logic.
The if-branch and else-branch differ only in the
.take(limit)call. Extract the iteration logic to reduce duplication.♻️ Proposed refactor
- let selected: Vec<String> = if let Some(limit) = limit { - lines - .iter() - .skip(offset) - .take(limit) - .enumerate() - .map(|(i, line)| format!("{:>6}\t{}", offset + i + 1, line)) - .collect() - } else { - lines - .iter() - .skip(offset) - .enumerate() - .map(|(i, line)| format!("{:>6}\t{}", offset + i + 1, line)) - .collect() - }; + let mut iter = lines.iter().skip(offset); + if let Some(limit) = limit { + iter = iter.take(limit); + } + let selected: Vec<String> = iter + .enumerate() + .map(|(i, line)| format!("{:>6}\t{}", offset + i + 1, line)) + .collect();Note: This requires storing the iterator in a variable, which may need a different approach due to type inference. Alternatively, use a helper function.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aish-tools/src/read_file/read_file.rs` around lines 103 - 118, The formatting logic for building selected lines is duplicated; refactor the block that constructs selected (in read_file.rs) by creating a single iterator over lines.iter().skip(offset) and conditionally applying .take(limit) when limit.is_some(), then map/enumerate/format in one place (or move the map/format into a helper like format_line) so you only have one .map(...).collect() call; refer to the selected variable construction and the .skip(offset)/.take(limit)/.map(|(i, line)| format!("{:>6}\t{}", offset + i + 1, line)) sequence when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/aish-llm/src/session.rs`:
- Around line 331-333: Remove the unnecessary borrow when calling
ChatMessage::system: replace the argument
&self.system_prompt_with_tool_prompts(sys) with
self.system_prompt_with_tool_prompts(sys) because
system_prompt_with_tool_prompts returns a String and ChatMessage::system accepts
impl Into<String> (remove the leading & in the messages.push call).
In `@crates/aish-shell/src/app.rs`:
- Around line 1102-1130: Long content passed into print_panel_line (used around
the confirmation dialog for ctx.tool_name, ctx.message and
t("shell.confirm_dialog_question")) can exceed the panel inner_width because
print_panel_line pads but does not truncate; fix by ensuring every string passed
to print_panel_line is wrapped/truncated to fit inner_width (accounting for any
left padding/ANSI escape lengths) before printing—for example, reuse wrap_text
or a new truncate_to_width helper to split or truncate lines from format!()
(including the "Reason:" first line and subsequent reason_lines) so you only
call print_panel_line with lines that are guaranteed to be <= inner_width.
- Around line 1115-1125: The reason text wrapping is using byte/char lengths via
wrap_text and so miscalculates width for wide (CJK/emoji) characters; update the
wrapping logic used to produce reason_lines (the wrap_text implementation) to
measure terminal display width using unicode-width (UnicodeWidthStr::width) or a
display-width-aware wrapper (or swap to textwrap configured for Unicode widths),
ensure spaces and padding account for display widths, then regenerate
reason_lines so print_panel_line and the indented continuation lines (the loop
using reason_lines.lines().skip(1)) never exceed inner_width when wide
characters are present.
- Around line 448-454: The WebFetchTool instance registered in AishShell::new
(via tool_registry.register(Box::new(aish_tools::WebFetchTool::new(...)))) does
not get updated when the runtime model/api/key change (ai_handler.update_model),
causing config drift; modify the same update paths used by handle_model_command
and run_setup_wizard to either recreate and re-register a new WebFetchTool with
the current config or call an exposed update method on the existing WebFetchTool
so its api_base, api_key, model, temperature, and max_tokens mirror the active
session settings whenever update_model is invoked.
In `@crates/aish-tools/src/channel_ask_user/prompt.rs`:
- Line 55: The schema currently lists the property "kind" inside the "required"
array but the runtime (which defaults missing kind to "text_input") contradicts
that; update the JSON schema by removing "kind" from the "required" array and
add a "default": "text_input" to the "kind" property definition so the schema
matches the implementation (search for the "required" array and the "kind"
property in the prompt schema and align them with the defaulting behavior in
channel_ask_user.rs).
In `@crates/aish-tools/src/edit_file/edit_file.rs`:
- Around line 38-117: The edit_file::execute implementation allows unrestricted
file paths (variables path in execute) which can let an LLM edit arbitrary
files; add a restricted SSH wrapper similar to SshReadFileTool/WriteFileTool to
validate paths before calling EditFileTool. Implement a new SshEditFileTool type
that accepts an allowed-paths predicate (or a PathValidator) and performs the
same SSH/context checks and path normalization used by SshReadFileTool (reuse
its validation logic), then call EditFileTool::execute only after validating the
provided path; ensure the wrapper exposes the same tool interface and returns
the same ToolResult error messages when path is disallowed.
- Around line 60-71: The EditFileTool currently calls
std::fs::read_to_string(path) without checking file size; before that call (in
the block that builds `content`), call std::fs::metadata(path) and verify
metadata.len() <= 32 * 1024 (32 KiB), returning a ToolResult::error via
aish_i18n::t_with_args (similar to the existing error path) if the file is too
large; keep the existing error map keys ("path" and "error" or use a new "size"
key) and ensure the new check runs before attempting read_to_string so you avoid
OOM on very large files.
In `@crates/aish-tools/src/read_file/prompt.rs`:
- Around line 7-8: Update the PROMPT and the `offset` parameter description to
explicitly state that `offset` is 0-based (used for indexing into the file)
while the line numbers returned in results are 1-based for human readability;
reference the PROMPT constant and the `offset` parameter in the read-file prompt
code and add a short note explaining that to map an index to a displayed line
number you add 1 (and to convert a displayed line number back to a 0-based index
you subtract 1), so the LLM and users clearly understand the conversion.
In `@crates/aish-tools/src/read_file/read_file.rs`:
- Line 176: Replace the hardcoded English string passed to ToolResult::error
with an i18n call (use aish_i18n::t or aish_i18n::t_with_args) so the
access-denied message is localized; locate the return in read_file.rs where
ToolResult::error("Access denied: path is not inside offload directory") is used
and change it to call aish_i18n::t("...") or t_with_args(...) with an
appropriate message key (and any args if needed) so the user-facing error
follows the existing i18n pattern.
- Around line 131-138: The new() constructor currently uses
std::fs::canonicalize(&offload_root).unwrap_or(offload_root) which silently
keeps a non-canonical offload_root and enables path-traversal escapes when later
comparing input paths (see the prefix check that uses offload_root). Fix by
ensuring the offload directory exists and then canonicalizing — i.e., create the
directory if missing (std::fs::create_dir_all) before calling canonicalize and
propagate or panic on canonicalize failures so offload_root is always canonical;
update the new() implementation that constructs offload_root and retain
ReadFileTool::new() unchanged.
- Around line 44-67: The code reads the entire file into raw_bytes via
std::fs::read before enforcing SIZE_LIMIT, causing potential OOM; change the
logic in read_file.rs to call std::fs::metadata(path) first, check
metadata.len() against SIZE_LIMIT (const SIZE_LIMIT) and return the same
ToolResult::error using aish_i18n::t_with_args if the file is too large, and
only then call std::fs::read(path), also handling and mapping metadata errors
similarly to how read errors are handled (include path and error in args_map).
In `@crates/aish-tools/src/web_fetch/web_fetch.rs`:
- Around line 51-78: The temperature and max_tokens fields stored on the struct
are never used because chat_completion_raw is called with hardcoded Some(0.1)
and Some(2048) inside apply_prompt_to_content; either (A) make
chat_completion_raw use the struct values by passing self.temperature and
self.max_tokens (or their .or(Some(...)) fallbacks) instead of the hardcoded
literals, ensuring LlmSession::new remains unchanged, or (B) remove the
temperature and max_tokens fields and constructor parameters entirely (and
simplify LlmSession::new/chat call to the fixed values) so there is no dead
field; update the apply_prompt_to_content call site and the struct/constructor
accordingly (refer to apply_prompt_to_content, LlmSession::new, and
chat_completion_raw for locations to change).
In `@crates/aish-tools/src/write_file/write_file.rs`:
- Around line 40-83: The WriteFileTool::execute currently writes content without
size limits so large payloads can exhaust disk; add a max-size guard (e.g.,
const MAX_WRITE_BYTES = 32 * 1024 or reuse ReadFileTool::SIZE_LIMIT) and check
content.len() before calling std::fs::write; if content.len() > MAX_WRITE_BYTES
return ToolResult::error with an i18n key like "tools.fs.write_file.too_large"
(include path/bytes in args_map similar to existing errors) and do not attempt
directory creation or file write when over the limit.
---
Nitpick comments:
In `@crates/aish-tools/src/channel_ask_user/prompt.rs`:
- Around line 3-8: The PROMPT constant (pub(crate) const PROMPT) is missing
guidance for the schema's kind parameter; update the PROMPT text to explain when
to use kind="text_input" (use for freeform user responses) versus
kind="choice_or_text" (present explicit options but allow a freeform override),
mirroring the style of ask_user/prompt.rs's explanation of allow_freeform_input;
mention examples and a short rule-of-thumb (choose text_input for open-ended
questions, choice_or_text when likely answers are known but a custom response
should be allowed) so callers of the channel_ask_user schema understand which
kind to pick.
In `@crates/aish-tools/src/edit_file/edit_file.rs`:
- Around line 73-85: Compute matches count once and use it for both presence
check and replacement: call content.matches(old).count() into a variable (e.g.,
count), if count == 0 return the same ToolResult::error via
aish_i18n::t_with_args, and then produce new_content by using
content.replace(old, new) when replace_all is true, otherwise use
content.replacen(old, new, 1) to replace only the first occurrence; update
references to content.contains(old) and the separate count() call in
edit_file.rs accordingly.
In `@crates/aish-tools/src/host_note/prompt.rs`:
- Around line 10-30: Update the parameters() JSON Schema so action-dependent
fields are enforced: add JSON Schema conditionals (if/then) such that if
"action" == "store" then "required": ["action","content"], and if "action" ==
"forget" then "required": ["action","keyword"]; place these conditionals inside
the top-level schema returned by the parameters() function (or, if adding
conditionals is impractical, update the "content" and "keyword" property
descriptions to explicitly state they are required for action="store" and
action="forget" respectively so callers see the dependency — ensure this aligns
with the runtime checks in execute()).
In `@crates/aish-tools/src/plan_tool/enter_plan_mode.rs`:
- Around line 107-109: The test currently ties to the internal ID format by
asserting meta["plan_id"] length == 12; update the assertion to avoid fragility:
remove the assert_eq!(meta["plan_id"].as_str().unwrap().len(), 12) and instead
assert that meta["plan_id"] exists and is non-empty (or matches a stable pattern
if you expect a specific format). Locate the assertions around meta["phase"] and
meta["plan_id"] in enter_plan_mode.rs (the test that validates
generate_plan_id()), replace the hard length check with a non-empty check or a
regex-based pattern match so the test no longer depends on the exact length.
- Around line 6-16: Replace the hard-coded VISIBLE_TOOLS_DURING_PLANNING slice
with the canonical constant from core to avoid drift: remove the local const and
re-export or alias aish_core::plan::PLANNING_VISIBLE_TOOLS (e.g. add a
module-level `use aish_core::plan::PLANNING_VISIBLE_TOOLS as
VISIBLE_TOOLS_DURING_PLANNING;` or `pub(crate) use ... as ...`), so the code
refers to the single source of truth (`aish_core::plan::PLANNING_VISIBLE_TOOLS`)
instead of duplicating the eight tool names.
In `@crates/aish-tools/src/plan_tool/list_plan_templates.rs`:
- Around line 72-78: The test currently rigidly asserts that result.output
contains "default", "bugfix", "feature" and that templates.len() == 3 which
couples it to aish_core::plan::get_available_templates(); change the assertions
to be more flexible by verifying that result.meta is present, that
meta["templates"] parses to an array (templates) with templates.len() >= 1, and
that each element in templates contains required fields (e.g., name and
description or whatever keys your Template struct exposes) rather than exact
names or exact count; update references to result, meta, and templates in the
test to reflect these general checks so the test passes when templates are
added/removed/renamed.
In `@crates/aish-tools/src/read_file/prompt.rs`:
- Around line 18-25: The JSON schema for the read-file prompt currently allows
negative or zero values; update the schema entries for the "offset" and "limit"
fields in crates/aish-tools/src/read_file/prompt.rs so that "offset" includes
"minimum": 0 and "limit" includes "minimum": 1 to enforce non-negative starting
offsets and a positive read limit; locate the schema block where "offset" and
"limit" are defined and add those minimum constraints to their definitions.
In `@crates/aish-tools/src/read_file/read_file.rs`:
- Around line 103-118: The formatting logic for building selected lines is
duplicated; refactor the block that constructs selected (in read_file.rs) by
creating a single iterator over lines.iter().skip(offset) and conditionally
applying .take(limit) when limit.is_some(), then map/enumerate/format in one
place (or move the map/format into a helper like format_line) so you only have
one .map(...).collect() call; refer to the selected variable construction and
the .skip(offset)/.take(limit)/.map(|(i, line)| format!("{:>6}\t{}", offset + i
+ 1, line)) sequence when making the change.
In `@crates/aish-tools/src/web_fetch/preapproved.rs`:
- Line 105: Remove the redundant `continue;` at the end of the loop in the
preapproved handling block in crates/aish-tools/src/web_fetch/preapproved.rs
(the loop that processes each preapproved entry); simply delete the `continue;`
statement so the loop naturally proceeds to the next iteration without the no-op
explicit continue.
In `@crates/aish-tools/src/web_fetch/web_fetch.rs`:
- Line 65: The messages vector currently includes an empty system instruction
(constructed via ChatMessage::system("")) which is unnecessary; update the
construction of messages in web_fetch.rs to omit the empty system message and
use only ChatMessage::user(model_prompt), i.e., remove the
ChatMessage::system("") entry (or replace it with a meaningful system prompt if
intended) so the secondary model receives just the intended user prompt.
In `@crates/aish-tools/src/write_file/write_file.rs`:
- Around line 62-81: Add a content-size guard in write_file.rs before performing
std::fs::write: define a MAX_WRITE_SIZE constant (e.g. 32 * 1024) to match
ReadFileTool, check content.len() against it in the WriteFileTool execution
path, and if it exceeds the limit return ToolResult::error (use a similar i18n
key as other errors) including the attempted byte length and path in the
args_map; only call std::fs::write when the size check passes.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ca8c8048-cbf4-48cf-8b82-c4775af6f44c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (55)
README.mdREADME_CN.mdcrates/aish-i18n/locales/de-DE.yamlcrates/aish-i18n/locales/en-US.yamlcrates/aish-i18n/locales/es-ES.yamlcrates/aish-i18n/locales/fr-FR.yamlcrates/aish-i18n/locales/ja-JP.yamlcrates/aish-i18n/locales/zh-CN.yamlcrates/aish-llm/src/agent.rscrates/aish-llm/src/session.rscrates/aish-llm/src/types.rscrates/aish-shell/src/app.rscrates/aish-tools/Cargo.tomlcrates/aish-tools/src/ask_user/ask_user.rscrates/aish-tools/src/ask_user/prompt.rscrates/aish-tools/src/bash/bash.rscrates/aish-tools/src/bash/prompt.rscrates/aish-tools/src/channel_ask_user/channel_ask_user.rscrates/aish-tools/src/channel_ask_user/prompt.rscrates/aish-tools/src/channel_bash/channel_bash.rscrates/aish-tools/src/channel_bash/prompt.rscrates/aish-tools/src/edit_file/edit_file.rscrates/aish-tools/src/edit_file/prompt.rscrates/aish-tools/src/final_answer/final_answer.rscrates/aish-tools/src/final_answer/prompt.rscrates/aish-tools/src/fs.rscrates/aish-tools/src/glob_tool/glob_tool.rscrates/aish-tools/src/glob_tool/prompt.rscrates/aish-tools/src/grep_tool/grep_tool.rscrates/aish-tools/src/grep_tool/prompt.rscrates/aish-tools/src/host_note/host_note.rscrates/aish-tools/src/host_note/prompt.rscrates/aish-tools/src/lib.rscrates/aish-tools/src/memory_tool/memory_tool.rscrates/aish-tools/src/memory_tool/prompt.rscrates/aish-tools/src/plan_tool.rscrates/aish-tools/src/plan_tool/enter_plan_mode.rscrates/aish-tools/src/plan_tool/exit_plan_mode.rscrates/aish-tools/src/plan_tool/list_plan_templates.rscrates/aish-tools/src/plan_tool/prompt.rscrates/aish-tools/src/python/prompt.rscrates/aish-tools/src/python/python.rscrates/aish-tools/src/read_file/prompt.rscrates/aish-tools/src/read_file/read_file.rscrates/aish-tools/src/secure_bash/secure_bash.rscrates/aish-tools/src/skill_tool/prompt.rscrates/aish-tools/src/skill_tool/skill_tool.rscrates/aish-tools/src/system_diagnose/prompt.rscrates/aish-tools/src/system_diagnose/system_diagnose.rscrates/aish-tools/src/web_fetch/preapproved.rscrates/aish-tools/src/web_fetch/prompt.rscrates/aish-tools/src/web_fetch/utils.rscrates/aish-tools/src/web_fetch/web_fetch.rscrates/aish-tools/src/write_file/prompt.rscrates/aish-tools/src/write_file/write_file.rs
💤 Files with no reviewable changes (6)
- crates/aish-tools/src/fs.rs
- crates/aish-tools/src/plan_tool.rs
- crates/aish-i18n/locales/ja-JP.yaml
- crates/aish-i18n/locales/de-DE.yaml
- crates/aish-i18n/locales/fr-FR.yaml
- crates/aish-i18n/locales/es-ES.yaml
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)
crates/aish-tools/src/edit_file/edit_file.rs (1)
98-110:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject empty
old_stringbefore doing match/replace.At Line 108,
replace_allwithold_string == ""can expand at every string boundary and trigger very large allocations/writes (OOM/disk pressure). Validate non-emptyold_stringup front.🔧 Suggested fix
let old = match args.get("old_string").and_then(|o| o.as_str()) { Some(o) => o, None => { return ToolResult::error(aish_i18n::t("tools.fs.edit_file.missing_old_string")) } }; + if old.is_empty() { + return ToolResult::error(aish_i18n::t("tools.fs.edit_file.missing_old_string")); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aish-tools/src/edit_file/edit_file.rs` around lines 98 - 110, The code counts and replaces using content.matches(old) and content.replace(old, new) which will catastrophically expand if old is an empty string; add an early check for old.is_empty() in the edit logic (before calling content.matches or performing replace_all) and return a ToolResult::error (using the same pattern as the existing error flow that builds args_map and calls aish_i18n::t_with_args) to reject empty `old` strings; update the branch that currently computes `count` and the `replace_all` path to assume non-empty `old`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/aish-tools/src/edit_file/edit_file.rs`:
- Around line 98-110: The code counts and replaces using content.matches(old)
and content.replace(old, new) which will catastrophically expand if old is an
empty string; add an early check for old.is_empty() in the edit logic (before
calling content.matches or performing replace_all) and return a
ToolResult::error (using the same pattern as the existing error flow that builds
args_map and calls aish_i18n::t_with_args) to reject empty `old` strings; update
the branch that currently computes `count` and the `replace_all` path to assume
non-empty `old`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 39681690-5ffe-4173-9907-647011cae0bf
📒 Files selected for processing (12)
crates/aish-i18n/locales/en-US.yamlcrates/aish-i18n/locales/zh-CN.yamlcrates/aish-shell/src/ai_handler.rscrates/aish-shell/src/app.rscrates/aish-tools/src/channel_ask_user/prompt.rscrates/aish-tools/src/edit_file/edit_file.rscrates/aish-tools/src/host_note/prompt.rscrates/aish-tools/src/read_file/prompt.rscrates/aish-tools/src/read_file/read_file.rscrates/aish-tools/src/web_fetch/preapproved.rscrates/aish-tools/src/web_fetch/web_fetch.rscrates/aish-tools/src/write_file/write_file.rs
💤 Files with no reviewable changes (1)
- crates/aish-tools/src/web_fetch/preapproved.rs
✅ Files skipped from review due to trivial changes (1)
- crates/aish-tools/src/read_file/prompt.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/aish-tools/src/host_note/prompt.rs
- crates/aish-i18n/locales/zh-CN.yaml
- crates/aish-tools/src/read_file/read_file.rs
- remove tool description from i18n crate - add webfetch tool
3 tasks
5 tasks
This was referenced Jul 7, 2026
Merged
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Change Type
Scope
User-visible Changes
Compatibility
Testing
Checklist
Summary by CodeRabbit
New Features
Improvements
Documentation