Repository navigation
fix(memory): reject absolute filesystem paths with corrective routing - #934
Conversation
ci(staging): stop assuming main branch exists
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a user experience issue where absolute filesystem paths were incorrectly routed to memory tools, leading to confusion and failed operations. By introducing robust path validation and clearer guidance, the system now intelligently distinguishes between workspace-memory paths and local filesystem paths, ensuring that users are directed to the appropriate tools for their intended actions and preventing misinterpretation of commands. Highlights
Changelog
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
zmanian
left a comment
There was a problem hiding this comment.
Review: fix(memory): reject absolute filesystem paths with corrective routing
Verdict: APPROVE
Clean fix for a real UX problem where the LLM misroutes filesystem paths into memory tool calls.
Correctness
looks_like_filesystem_path() correctly detects:
- Unix absolute paths (
/Users/...) - Windows absolute paths (
C:\...,D:/...) - Home shorthand (
~/...)
The implementation using Path::new(path).is_absolute() for Unix paths and manual byte checking for Windows drive letters is correct. The is_absolute() call handles edge cases like //network paths on Unix.
Error messages
The error messages are actionable -- they tell the user to use read_file/write_file instead, and suggest shell open for editor opens. This is exactly the corrective guidance that helps the LLM self-correct on retry.
Tool descriptions
Updated descriptions now explicitly say "Never pass absolute filesystem paths" -- good for steering LLM tool selection upstream.
Tests
Two focused tests cover the happy paths. Could add an edge case for empty string and relative paths with .. components, but those are workspace-valid paths so the current behavior (allow) is correct.
No issues found.
zmanian
left a comment
There was a problem hiding this comment.
Review: fix(memory): reject absolute filesystem paths with corrective routing
Verdict: Approve (already approved previously).
Clean fix for a real UX problem where the LLM misroutes filesystem paths into memory tool calls.
Correctness: looks_like_filesystem_path() correctly detects Unix absolute paths, Windows drive letters (C:\..., D:/...), and home shorthand (~/...). The Path::new(path).is_absolute() call handles edge cases like //network paths on Unix.
Error messages: Actionable -- they tell the user to use read_file/write_file instead, and suggest shell open for editor opens. Good for LLM self-correction on retry.
Tool descriptions: Updated to explicitly say "Never pass absolute filesystem paths" -- good for steering LLM tool selection upstream.
Tests: Two focused tests cover the happy paths. Relative paths with .. components are workspace-valid paths so the current behavior (allow) is correct.
Nit: The PR bundles an unrelated CI change (commit 1: use default_branch instead of hardcoded main in staging-ci.yml) with the memory fix (commit 2). These should ideally be separate PRs for clean revert paths, but since the CI change is low-risk and already green, not a blocker.
zmanian
left a comment
There was a problem hiding this comment.
LGTM. The security fix is sound:
looks_like_filesystem_path()correctly detects Unix absolute paths (viaPath::is_absolute()), home-dir expansion (~/), and Windows drive-letter paths (C:\,D:/).- Both
memory_writeandmemory_readreject filesystem paths early with a clear error message that redirects to the correct tools (write_file/read_file). - Good test coverage for both positive (filesystem paths) and negative (workspace paths) cases.
- Error messages include corrective guidance, which is helpful for LLM tool callers.
Minor note: this PR also includes CI workflow changes (hardcoded main -> DEFAULT_BRANCH). That is unrelated to the security fix and ideally would be a separate commit/PR, but the CI changes themselves look correct.
No .unwrap() or .expect() in production code. Uses crate:: imports. Approved.
…nearai#934) * ci(staging): use default branch instead of hardcoded main * fix(memory): route absolute paths to filesystem tools
…nearai#934) * ci(staging): use default branch instead of hardcoded main * fix(memory): route absolute paths to filesystem tools
Summary
Fixes a local UX failure mode where absolute filesystem paths (for example
/Users/.../file.md) are incorrectly routed to memory tools and treated as missing workspace-memory docs.Repro (before)
User asks:
open the following in the default editor: '/Users/.../issue-12055-comment-draft.md'Observed behavior:
Root cause
memory_readandmemory_writeaccepted path-like inputs without rejecting obvious filesystem path forms, so the model could misroute absolute paths into memory operations.Fix
In
src/tools/builtin/memory.rs:looks_like_filesystem_path()classifier for:/Users/...)C:\...,D:/...)~/...)memory_readnow rejects filesystem-looking paths early with actionable guidance:read_filefor readsshell open "<absolute_path>"for default-editor openmemory_writenow rejects filesystem-looking targets with actionable guidance:write_filefor filesystem writesAfter
Absolute filesystem paths are explicitly redirected away from memory tools with corrective messaging, reducing memory-vs-filesystem routing confusion.
Validation
cargo test -p ironclaw path_routing_tests -- --nocapture~/.local/ironclaw/bin/ironclaw service status=>running/loaded