feat: implement built-in file system tools (#18)#151
Conversation
Add 5 file system tools (read_file, write_file, edit_file, list_directory, delete_file) as a tools/file_system/ subpackage. - PathValidator enforces workspace-scoped path safety (symlink-aware) - BaseFileSystemTool sets FILE_SYSTEM category + shared validator - All tools use asyncio.to_thread for non-blocking file I/O - ReadFileTool: line-range selection, 1MB size guard, binary detection - WriteFileTool: create/overwrite with optional parent dir creation - EditFileTool: first-occurrence search-and-replace with LLM context - ListDirectoryTool: glob filtering, recursive listing, 1000-entry cap - DeleteFileTool: files only, require_elevated=True for permission gating - 10 structured observability event constants (TOOL_FS_*) - 59 unit tests covering happy paths, error handling, and security Closes #18 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Pre-reviewed by 10 agents, 28 findings addressed - Add PathValidator for workspace-scoped path sandboxing with symlink resolution - Implement ReadFileTool with bounded reads, line-range selection, binary detection - Implement WriteFileTool with parent directory creation, size guard (10 MB) - Implement EditFileTool with search-and-replace, size guard (1 MB), noop detection - Implement DeleteFileTool with TOCTOU-safe deletion, elevated permission flag - Implement ListDirectoryTool with glob filtering, recursive listing, truncation - Add BaseFileSystemTool ABC with shared _map_os_error helper - Add cross-platform is_dir() checks for Windows compatibility - Add unsafe glob pattern rejection to prevent workspace escape - Add 5 new observability event constants for filesystem operations - Update DESIGN_SPEC.md §15.3 project structure and §15.5 tool sandboxing - Update CLAUDE.md tools/ description to include built-in tools - Comprehensive test coverage: 44 tests across 6 test files Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (15)
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughAdds a new ai_company.tools.file_system package with a BaseFileSystemTool and PathValidator, implements five file-system tools (read, write, edit, list, delete) with workspace-bound path validation and observability events, and includes comprehensive unit tests and test fixtures. Documentation and package exports updated. Changes
Sequence Diagram(s)sequenceDiagram
participant Agent as Agent/User
participant Tool as FileSystem Tool
participant Validator as PathValidator
participant Workspace as Workspace (root)
participant FS as Filesystem
participant Logger as Observability
Agent->>Tool: execute(arguments)
activate Tool
Tool->>Validator: validate(user_path)
activate Validator
Validator->>Workspace: resolve & ensure containment
alt Path valid
Validator-->>Tool: resolved_path
else Path escapes or parent missing
Validator->>Logger: TOOL_FS_PATH_VIOLATION / TOOL_FS_PARENT_NOT_FOUND
Validator-->>Tool: raise ValueError
end
deactivate Validator
alt Validation failed
Tool->>Logger: log TOOL_FS_ERROR
Tool-->>Agent: ToolExecutionResult(error)
else Validation succeeded
Tool->>FS: perform operation (read/write/edit/list/delete)
activate FS
alt Success
FS-->>Tool: result (content/metadata)
Tool->>Logger: log TOOL_FS_* event with metadata
Tool-->>Agent: ToolExecutionResult(success, metadata)
else OS error / binary / size issue
FS-->>Tool: OSError / UnicodeDecodeError
Tool->>Logger: log TOOL_FS_ERROR / TOOL_FS_SIZE_EXCEEDED / TOOL_FS_BINARY_DETECTED
Tool-->>Agent: ToolExecutionResult(error, message)
end
deactivate FS
end
deactivate Tool
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
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 significantly expands the agent's capabilities by integrating a robust set of built-in file system tools. These tools enable secure and controlled interactions with the workspace, providing functionalities like reading, writing, editing, listing, and deleting files. The implementation prioritizes security through strict path validation, ensures cross-platform compatibility, and includes comprehensive observability features to monitor file operations. Highlights
Changelog
Activity
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
|
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive and well-designed set of built-in file system tools. While the implementation includes robust security controls like PathValidator and generally enforces size limits, a potential Denial of Service (DoS) vulnerability was identified in ReadFileTool. Specifically, the 1 MB file size limit is bypassed when line ranges are requested, which could lead to memory exhaustion if the tool is used to read very large files. Additionally, I have a couple of suggestions to enhance structured logging and improve memory efficiency for large file reads.
| if start is not None or end is not None: | ||
| raw = resolved.read_text(encoding="utf-8") | ||
| lines = raw.splitlines(keepends=True) | ||
| s = (start - 1) if start is not None else 0 | ||
| e = end if end is not None else len(lines) | ||
| return "".join(lines[s:e]) |
There was a problem hiding this comment.
The current implementation for reading a file with a line range (start_line/end_line) loads the entire file into memory using resolved.read_text(). This can lead to excessive memory consumption and a potential MemoryError if an agent requests a line range from a very large file (e.g., >1GB), even if the range itself is small. This appears to be an intentional design choice, but it presents a potential denial-of-service risk. For improved robustness, consider a more memory-efficient implementation that reads the file line-by-line, for example, using itertools.islice on the file handle.
| # When oversized and no line range, read only MAX_FILE_SIZE_BYTES | ||
| # to avoid loading the whole file into memory. | ||
| max_bytes = MAX_FILE_SIZE_BYTES if oversized and not has_line_range else None |
There was a problem hiding this comment.
The ReadFileTool implements a 1 MB size limit (MAX_FILE_SIZE_BYTES) to prevent memory exhaustion. However, this limit is bypassed when a line range (start_line or end_line) is specified. In this case, the tool reads the entire file into memory using read_text() before performing line slicing. If a very large file is present in the workspace, an attacker could trigger a Denial of Service (DoS) by requesting a line range from it, leading to memory exhaustion. Additionally, the use of splitlines() on the full file content further increases memory consumption.
# Enforce size limit even with line ranges to prevent DoS
if oversized and has_line_range:
return ToolExecutionResult(
content=(
f"File too large to read with line range: {user_path} "
f"({size_bytes:,} bytes, max {MAX_FILE_SIZE_BYTES:,})"
),
is_error=True,
)
# When oversized and no line range, read only MAX_FILE_SIZE_BYTES
# to avoid loading the whole file into memory.
max_bytes = MAX_FILE_SIZE_BYTES if oversized and not has_line_range else None| return "is_directory", f"Path is a directory, not a file: {user_path}" | ||
| if isinstance(exc, PermissionError): | ||
| return "permission_denied", f"Permission denied: {user_path}" | ||
| return str(exc), f"OS error {verb} file: {user_path}" |
There was a problem hiding this comment.
The fallback case in _map_os_error uses str(exc) as the log_key. This is not ideal for structured logging, as exception messages can be variable and are not suitable for aggregation. Using the exception's class name would provide a more consistent key for log analysis. Additionally, the user-facing message could include the exception details to be more informative.
| return str(exc), f"OS error {verb} file: {user_path}" | |
| return exc.__class__.__name__, f"OS error {verb} file '{user_path}': {exc}" |
Greptile SummaryThis PR implements the full suite of built-in file system tools (
The Confidence Score: 3/5
Last reviewed commit: 485d5ff |
There was a problem hiding this comment.
Pull request overview
Implements a first-class, workspace-scoped file system tool suite under ai_company.tools.file_system, including path sandboxing, consistent observability events, and a comprehensive unit test suite to support built-in read/write/edit/list/delete operations.
Changes:
- Added
PathValidator+BaseFileSystemToolto enforce workspace containment (incl. symlink resolution) across file system tools. - Implemented
ReadFileTool,WriteFileTool,EditFileTool,ListDirectoryTool, andDeleteFileToolwith size guards and structured observability events. - Added unit tests + updated docs/specs to reflect the new tool package and sandboxing status.
Reviewed changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/ai_company/tools/file_system/_base_fs_tool.py |
Introduces shared FS tool base + _map_os_error helper. |
src/ai_company/tools/file_system/_path_validator.py |
Adds workspace-scoped path validation with symlink containment checks. |
src/ai_company/tools/file_system/read_file.py |
Adds bounded file reads with optional line ranges + binary detection. |
src/ai_company/tools/file_system/write_file.py |
Adds guarded writes with optional directory creation + size limit. |
src/ai_company/tools/file_system/edit_file.py |
Adds first-occurrence search/replace with size guard and noop handling. |
src/ai_company/tools/file_system/list_directory.py |
Adds listing with glob filtering, recursion, truncation, and symlink annotation. |
src/ai_company/tools/file_system/delete_file.py |
Adds single-file deletion with elevated intent and TOCTOU-aware flow. |
src/ai_company/tools/file_system/__init__.py |
Exposes file system tool public API via package exports. |
src/ai_company/tools/__init__.py |
Re-exports file system tools at the top-level ai_company.tools package. |
src/ai_company/observability/events/tool.py |
Adds file-system-related event constants. |
tests/unit/tools/file_system/conftest.py |
Adds shared workspace + tool fixtures for FS tool tests. |
tests/unit/tools/file_system/test_read_file.py |
Unit tests for read behavior, truncation, binary, and line ranges. |
tests/unit/tools/file_system/test_write_file.py |
Unit tests for create/overwrite, parent handling, and size guard. |
tests/unit/tools/file_system/test_edit_file.py |
Unit tests for replace/noop/not-found, binary, and size guard. |
tests/unit/tools/file_system/test_list_directory.py |
Unit tests for listing modes, glob safety, recursion, truncation, symlinks. |
tests/unit/tools/file_system/test_delete_file.py |
Unit tests for delete success/failure modes and error mapping. |
DESIGN_SPEC.md |
Updates project structure + tool sandboxing status for FS tools. |
CLAUDE.md |
Updates tools directory description to mention built-in FS tools. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if oversized and not has_line_range: | ||
| content += ( | ||
| f"\n\n[Truncated: file is {size_bytes:,} bytes, " | ||
| f"showing first {MAX_FILE_SIZE_BYTES:,}]" | ||
| ) | ||
|
|
||
| line_count = content.count("\n") + ( | ||
| 1 if content and not content.endswith("\n") else 0 | ||
| ) |
There was a problem hiding this comment.
line_count is computed after appending the truncation notice, so the metadata/logged line_count includes the extra notice lines rather than reflecting just the file content (or the returned slice). This can make observability and consumers relying on line_count misleading. Consider computing line_count before appending the truncation marker, or tracking separate fields (e.g., file_line_count vs returned_line_count).
| glob_pattern = pattern or "*" | ||
| if recursive: | ||
| raw_iter = resolved.rglob(glob_pattern) | ||
| elif pattern: | ||
| raw_iter = resolved.glob(glob_pattern) | ||
| else: | ||
| raw_iter = resolved.iterdir() | ||
|
|
||
| # Cap at MAX_ENTRIES + 1 to detect truncation without | ||
| # materialising the entire iterator. | ||
| entries = sorted(itertools.islice(raw_iter, MAX_ENTRIES + 1)) | ||
|
|
||
| lines: list[str] = [] | ||
| for entry in entries: | ||
| try: | ||
| display = str(entry.relative_to(resolved)) if recursive else entry.name | ||
|
|
||
| if entry.is_symlink(): | ||
| target = entry.resolve() | ||
| if not target.is_relative_to(workspace_root): | ||
| lines.append(f"[SYMLINK] {display} -> (outside workspace)") | ||
| continue | ||
|
|
||
| if entry.is_dir(): | ||
| lines.append(f"[DIR] {display}/") | ||
| else: | ||
| try: |
There was a problem hiding this comment.
Symlink containment is only checked when the entry itself is a symlink. If the workspace contains a symlink directory pointing outside the workspace, glob() / rglob() can yield children under that symlink that are not symlinks themselves, and those entries will be treated as normal files/dirs (potentially listing outside-workspace content). Harden this by validating entry.resolve() is within workspace_root for every entry (and/or avoid descending into symlinked directories by using a custom walk with follow_symlinks=False).
| from ai_company.observability import get_logger | ||
| from ai_company.tools.base import BaseTool | ||
| from ai_company.tools.file_system._path_validator import PathValidator | ||
|
|
||
| if TYPE_CHECKING: | ||
| from pathlib import Path | ||
|
|
||
| logger = get_logger(__name__) | ||
|
|
||
|
|
There was a problem hiding this comment.
_base_fs_tool.py defines logger = get_logger(__name__) but never uses it. This will trip linting (unused import/variable). Remove the logger (and get_logger import) or use it for shared logging in this module.
| from ai_company.observability import get_logger | |
| from ai_company.tools.base import BaseTool | |
| from ai_company.tools.file_system._path_validator import PathValidator | |
| if TYPE_CHECKING: | |
| from pathlib import Path | |
| logger = get_logger(__name__) | |
| from ai_company.tools.base import BaseTool | |
| from ai_company.tools.file_system._path_validator import PathValidator | |
| if TYPE_CHECKING: | |
| from pathlib import Path |
| "old_text": { | ||
| "type": "string", | ||
| "description": "Exact text to find", | ||
| }, | ||
| "new_text": { | ||
| "type": "string", | ||
| "description": "Replacement text (empty string to delete)", | ||
| }, | ||
| }, | ||
| "required": ["path", "old_text", "new_text"], | ||
| "additionalProperties": False, |
There was a problem hiding this comment.
old_text is allowed to be an empty string (no runtime check and no minLength in the JSON schema). In that case content.count(old_text) becomes len(content)+1 and the replace will insert new_text at the start, which is almost certainly unintended and can massively rewrite files. Add validation that old_text is non-empty (and ideally enforce minLength: 1 in the parameters schema).
| if start is not None or end is not None: | ||
| raw = resolved.read_text(encoding="utf-8") | ||
| lines = raw.splitlines(keepends=True) | ||
| s = (start - 1) if start is not None else 0 | ||
| e = end if end is not None else len(lines) | ||
| return "".join(lines[s:e]) | ||
|
|
There was a problem hiding this comment.
When start_line/end_line is provided, _read_sync() reads the entire file into memory (resolved.read_text() + splitlines()), bypassing the size guard. For large files this can be a big memory/time spike even if the caller only needs a few lines. Consider streaming lines (e.g., iterate over the file and islice the requested range) so partial reads remain bounded.
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 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/ai_company/tools/file_system/_path_validator.py`:
- Around line 60-86: The validate method in _path_validator.py currently allows
absolute inputs because (self._workspace_root / path) ignores the root when path
is absolute; update validate (the validate(self, path: str) function) to first
check whether the incoming path string is absolute (e.g.,
Path(path).is_absolute()) and immediately treat absolute inputs as violations:
log TOOL_FS_PATH_VIOLATION with user_path and raise ValueError("Absolute paths
not allowed" or similar) before attempting to resolve; only join and resolve
when the input is relative and then keep the existing containment check using
is_relative_to(self._workspace_root).
In `@src/ai_company/tools/file_system/delete_file.py`:
- Around line 71-79: The dispatcher currently ignores the delete_file Tool's
require_elevated flag; update the permission check in ToolInvoker (the
permission validation block that inspects tool.name and tool.category) to also
reject tools with tool.require_elevated=True unless the invoking agent has an
elevated/explicit-approval capability (e.g., an AgentPermission like "elevated"
or "can_execute_elevated_tools"); modify the permission logic in
ToolInvoker.check/authorize (or the method around lines 116–141) to consult
tool.require_elevated and the agent's permission set and return a
permission-denied result when the agent lacks elevated rights, or alternatively
add a clear comment and documentation stub if you choose to defer enforcement.
In `@src/ai_company/tools/file_system/edit_file.py`:
- Around line 78-96: The parameters_schema currently allows an empty "old_text",
which leads to destructive edits in _edit_sync; update validation to reject
empty search strings by adding "minLength": 1 to the "old_text" schema property
(and likewise to the similar schema referenced around the other block mentioned)
and also add a defensive check at the start of _edit_sync (and its async
counterpart if present) to raise/return a validation error when old_text == ""
to ensure system-boundary validation.
- Around line 28-39: _edit_sync currently writes directly to the target file
which can leave it corrupted on partial failure; change it to write the updated
content to a same-directory temporary file, flush and fsync the temp file, copy
the original file's permissions to the temp (use resolved.stat().st_mode and
os.chmod), then atomically replace the original with Path.replace(temp_path,
resolved) (or temp_path.replace(resolved)) and ensure the temp is removed on
error; keep the existing behavior of counting occurrences and replacing only one
occurrence with content.replace(old_text, new_text, 1), and return the original
count.
- Around line 189-194: The logger call in the edit_file error branch currently
emits the searched text (old_text_preview=old_text[:100]) at INFO and must not
leak content; change the logger invocation in the branch where count == 0 to use
logger.warning (or logger.error), remove the raw text preview, and instead log
non-sensitive metadata such as the length of old_text (e.g.,
old_text_len=len(old_text)) or a stable hash of old_text, while retaining
path=user_path and other context before returning is_error=True/raising; update
the message constant TOOL_FS_EDIT_NOT_FOUND usage accordingly in the surrounding
function (the block referencing count, user_path, old_text, and logger).
In `@src/ai_company/tools/file_system/list_directory.py`:
- Around line 166-176: The current code treats any non-directory the same,
collapsing missing paths and existing files into "Not a directory"; change the
branching after calling path_validator.validate(user_path) so you first check
whether the resolved path exists (e.g., resolved.exists()) and if it does not,
log with TOOL_FS_ERROR and error="not_found" and return a ToolExecutionResult
indicating the path was not found; otherwise (if it exists but not
resolved.is_dir()) keep the logger.warning with error="not_a_directory" and
return the existing "Not a directory" ToolExecutionResult. Ensure you use the
same symbols: path_validator.validate, resolved.exists()/resolved.is_dir(),
logger.warning, TOOL_FS_ERROR, and ToolExecutionResult.
- Around line 155-185: The code currently only rejects ".." traversal via
_UNSAFE_GLOB_RE but doesn't reject absolute/non-relative glob patterns (e.g.,
"/tmp/*"), which cause pathlib.Path.glob()/rglob() to raise NotImplementedError;
fix by validating the incoming pattern is relative before calling _list_sync: if
pattern is set and pathlib.Path(pattern).is_absolute() (or otherwise not a
relative path), log TOOL_FS_GLOB_REJECTED with pattern and return a
ToolExecutionResult with an "Unsafe glob pattern rejected" error message; add
this check alongside the existing _UNSAFE_GLOB_RE check (affecting the same
block that references pattern, _UNSAFE_GLOB_RE, TOOL_FS_GLOB_REJECTED, and the
call site _list_sync) so absolute patterns are rejected early and
NotImplementedError cannot propagate.
In `@src/ai_company/tools/file_system/read_file.py`:
- Around line 31-55: _read_sync currently bypasses the byte cap: when max_bytes
is set it uses fh.read(max_bytes) on a text stream (reads characters, not bytes)
and when start/end are provided it calls resolved.read_text() which materialises
the whole file; fix by performing byte-bounded reads in binary mode and decoding
safely to UTF-8 without breaking multibyte sequences (e.g., use a binary open +
incremental decoder or decode up to the last valid boundary) and for ranged
reads stream bytes line-by-line in binary until the requested start/end lines
are collected (stop once max_bytes is reached) instead of calling
resolved.read_text(); apply the same binary-byte-bounded approach to the
analogous code path elsewhere in this module.
In `@src/ai_company/tools/file_system/write_file.py`:
- Around line 23-34: The _write_sync function currently uses Path.write_text
which can truncate the target in-place; instead, write the content to a
temporary file in the same directory (use a unique temp name), ensure the parent
dir exists (respecting create_dirs), open the temp file and write bytes, call
os.fsync on the file descriptor to flush data, close it, then atomically replace
the destination with pathlib.Path.replace(temp_path) and finally fsync the
containing directory if possible; preserve the returned tuple of (bytes_written,
created) and reference _write_sync, Path.replace, and create_dirs when making
the change.
In `@tests/unit/tools/file_system/test_edit_file.py`:
- Around line 13-15: The TestEditFileExecution async unit tests are missing the
30-second timeout marker; add the pytest timeout marker to the test class
declaration so the class is decorated with pytest.mark.timeout(30) (i.e., apply
the marker to TestEditFileExecution) to enforce a 30-second per-test timeout and
prevent hung filesystem calls from stalling CI.
In `@tests/unit/tools/file_system/test_list_directory.py`:
- Around line 20-22: Add the missing 30-second timeout marker to the async
unit-test classes by decorating both TestListDirectoryExecution and the other
test class at lines 132-134 with `@pytest.mark.timeout`(30); ensure pytest is
imported and place the decorator directly above each class definition so each
test inherits the repository’s 30-second timeout guard.
In `@tests/unit/tools/file_system/test_path_validator.py`:
- Around line 29-33: The test uses Path.write_text without an explicit encoding
in test_file_as_workspace_raises (creating f and calling f.write_text("x")),
which is inconsistent with other tests; update the write_text call(s) in this
file (including the occurrences in test_file_as_workspace_raises and the other
tests that create tmp files) to pass encoding="utf-8" so all write_text
invocations are consistent; locate usages around the PathValidator-related tests
and change write_text(...) to write_text(..., encoding="utf-8").
In `@tests/unit/tools/file_system/test_read_file.py`:
- Around line 66-76: The test test_read_start_line_only is too loose—change its
assertions to verify the exact returned slice from ReadFileTool instead of using
"in"; after calling read_tool.execute with arguments {"path": "multi.txt",
"start_line": 2}, assert result.content equals the expected string (only the
lines starting at line 2, e.g. "b\nc\n") and that result.is_error is False.
Apply the same tightening to the other similar test referenced (lines 120-132)
so it asserts equality against the exact five-line slice rather than membership
checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9032b086-bef6-4185-9848-2cf65fd4abe6
📒 Files selected for processing (20)
CLAUDE.mdDESIGN_SPEC.mdsrc/ai_company/observability/events/tool.pysrc/ai_company/tools/__init__.pysrc/ai_company/tools/file_system/__init__.pysrc/ai_company/tools/file_system/_base_fs_tool.pysrc/ai_company/tools/file_system/_path_validator.pysrc/ai_company/tools/file_system/delete_file.pysrc/ai_company/tools/file_system/edit_file.pysrc/ai_company/tools/file_system/list_directory.pysrc/ai_company/tools/file_system/read_file.pysrc/ai_company/tools/file_system/write_file.pytests/unit/tools/file_system/__init__.pytests/unit/tools/file_system/conftest.pytests/unit/tools/file_system/test_delete_file.pytests/unit/tools/file_system/test_edit_file.pytests/unit/tools/file_system/test_list_directory.pytests/unit/tools/file_system/test_path_validator.pytests/unit/tools/file_system/test_read_file.pytests/unit/tools/file_system/test_write_file.py
📜 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: Agent
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Python 3.14+ with PEP 649 native lazy annotations
Do NOT usefrom __future__ import annotations—Python 3.14 has PEP 649
Useexcept A, B:syntax (no parentheses) for exception handling on Python 3.14—ruff enforces this
Add type hints to all public functions in Python; mypy strict mode is enforced
Use Google-style docstrings on all public classes and functions—ruff D rules enforce this
Create new objects instead of mutating existing ones; usecopy.deepcopy()at construction for non-Pydantic internal collections andMappingProxyTypewrapping for read-only enforcement
Use frozen Pydantic models for config/identity; use separate mutable-via-copy models (usingmodel_copy(update=...)) for runtime state that evolves. Never mix static config fields with mutable runtime fields in one model.
Use Pydantic v2 withBaseModel,model_validator,computed_field, andConfigDict
Use@computed_fieldfor derived values instead of storing + validating redundant fields (e.g.TokenUsage.total_tokens)
UseNotBlankStr(fromcore.types) for all identifier/name fields—including optional (NotBlankStr | None) and tuple (tuple[NotBlankStr, ...]) variants—instead of manual whitespace validators
Preferasyncio.TaskGroupfor fan-out/fan-in parallel operations in new code (e.g. multiple tool invocations, parallel agent calls); prefer structured concurrency over barecreate_task
Enforce line length of 88 characters (ruff enforces this)
Functions should be less than 50 lines, files less than 800 lines
Handle errors explicitly; never silently swallow errors in Python code
Validate at system boundaries (user input, external APIs, config files)
Files:
src/ai_company/observability/events/tool.pytests/unit/tools/file_system/test_path_validator.pytests/unit/tools/file_system/test_delete_file.pysrc/ai_company/tools/file_system/write_file.pytests/unit/tools/file_system/test_edit_file.pytests/unit/tools/file_system/test_write_file.pytests/unit/tools/file_system/conftest.pysrc/ai_company/tools/file_system/_base_fs_tool.pysrc/ai_company/tools/file_system/delete_file.pysrc/ai_company/tools/file_system/__init__.pysrc/ai_company/tools/file_system/_path_validator.pytests/unit/tools/file_system/test_list_directory.pysrc/ai_company/tools/file_system/read_file.pysrc/ai_company/tools/file_system/list_directory.pysrc/ai_company/tools/__init__.pytests/unit/tools/file_system/test_read_file.pysrc/ai_company/tools/file_system/edit_file.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Every module with business logic MUST importfrom ai_company.observability import get_loggerthenlogger = get_logger(__name__)
Never useimport logging,logging.getLogger(), orprint()in application code
Always useloggeras the variable name for loggers (not_logger, notlog)
Use event name constants from domain-specific modules underai_company.observability.events(e.g.PROVIDER_CALL_STARTfromevents.provider,BUDGET_RECORD_ADDEDfromevents.budget). Import directly:from ai_company.observability.events.<domain> import EVENT_CONSTANT
Always use structured logging withlogger.info(EVENT, key=value)format—neverlogger.info('msg %s', val)
All error paths must log at WARNING or ERROR with context before raising
All state transitions must log at INFO level
DEBUG level logging should be used for object creation, internal flow, entry/exit of key functions
Pure data models, enums, and re-exports do NOT need logging
Files:
src/ai_company/observability/events/tool.pysrc/ai_company/tools/file_system/write_file.pysrc/ai_company/tools/file_system/_base_fs_tool.pysrc/ai_company/tools/file_system/delete_file.pysrc/ai_company/tools/file_system/__init__.pysrc/ai_company/tools/file_system/_path_validator.pysrc/ai_company/tools/file_system/read_file.pysrc/ai_company/tools/file_system/list_directory.pysrc/ai_company/tools/__init__.pysrc/ai_company/tools/file_system/edit_file.py
{src/**/*.py,tests/**/*.py,src/**/*.yaml,src/**/*.yml,tests/**/*.yaml,tests/**/*.yml,examples/**/*.yaml,examples/**/*.yml}
📄 CodeRabbit inference engine (CLAUDE.md)
NEVER use real vendor names (Anthropic, OpenAI, Claude, GPT, etc.) in project-owned code, docstrings, comments, tests, or config examples. Use generic names:
example-provider,example-large-001,example-medium-001,example-small-001,large/medium/smallas aliases. Vendor names may only appear in: (1) DESIGN_SPEC.md provider list, (2).claude/skill/agent files, (3) third-party import paths/module names (e.g.litellm.types.llms.openai). Tests must usetest-provider,test-small-001, etc.
Files:
src/ai_company/observability/events/tool.pytests/unit/tools/file_system/test_path_validator.pytests/unit/tools/file_system/test_delete_file.pysrc/ai_company/tools/file_system/write_file.pytests/unit/tools/file_system/test_edit_file.pytests/unit/tools/file_system/test_write_file.pytests/unit/tools/file_system/conftest.pysrc/ai_company/tools/file_system/_base_fs_tool.pysrc/ai_company/tools/file_system/delete_file.pysrc/ai_company/tools/file_system/__init__.pysrc/ai_company/tools/file_system/_path_validator.pytests/unit/tools/file_system/test_list_directory.pysrc/ai_company/tools/file_system/read_file.pysrc/ai_company/tools/file_system/list_directory.pysrc/ai_company/tools/__init__.pytests/unit/tools/file_system/test_read_file.pysrc/ai_company/tools/file_system/edit_file.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Mark unit tests with@pytest.mark.unit, integration tests with@pytest.mark.integration, e2e tests with@pytest.mark.e2e, and slow tests with@pytest.mark.slow
Useasyncio_mode = 'auto'for pytest async tests—no manual@pytest.mark.asyncioneeded
Set a 30-second timeout per test
Files:
tests/unit/tools/file_system/test_path_validator.pytests/unit/tools/file_system/test_delete_file.pytests/unit/tools/file_system/test_edit_file.pytests/unit/tools/file_system/test_write_file.pytests/unit/tools/file_system/conftest.pytests/unit/tools/file_system/test_list_directory.pytests/unit/tools/file_system/test_read_file.py
🧠 Learnings (1)
📚 Learning: 2026-03-06T21:51:55.175Z
Learnt from: CR
Repo: Aureliolo/ai-company PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-06T21:51:55.175Z
Learning: Applies to src/**/*.py : Use event name constants from domain-specific modules under `ai_company.observability.events` (e.g. `PROVIDER_CALL_START` from `events.provider`, `BUDGET_RECORD_ADDED` from `events.budget`). Import directly: `from ai_company.observability.events.<domain> import EVENT_CONSTANT`
Applied to files:
src/ai_company/observability/events/tool.py
🧬 Code graph analysis (13)
tests/unit/tools/file_system/test_path_validator.py (3)
src/ai_company/tools/file_system/_path_validator.py (4)
PathValidator(24-109)workspace_root(56-58)validate(60-86)validate_parent_exists(88-109)src/ai_company/tools/file_system/_base_fs_tool.py (1)
workspace_root(73-75)tests/unit/tools/file_system/conftest.py (1)
workspace(18-25)
tests/unit/tools/file_system/test_delete_file.py (2)
src/ai_company/tools/file_system/delete_file.py (3)
DeleteFileTool(34-142)require_elevated(72-79)execute(81-142)tests/unit/tools/file_system/conftest.py (2)
delete_tool(53-55)workspace(18-25)
src/ai_company/tools/file_system/write_file.py (4)
src/ai_company/observability/_logger.py (1)
get_logger(8-28)src/ai_company/tools/base.py (3)
ToolExecutionResult(24-53)description(115-117)parameters_schema(120-128)src/ai_company/tools/file_system/_base_fs_tool.py (3)
BaseFileSystemTool(41-80)workspace_root(73-75)path_validator(78-80)src/ai_company/tools/file_system/_path_validator.py (2)
workspace_root(56-58)validate(60-86)
tests/unit/tools/file_system/test_edit_file.py (2)
src/ai_company/tools/file_system/edit_file.py (2)
EditFileTool(42-223)execute(99-223)tests/unit/tools/file_system/conftest.py (2)
workspace(18-25)edit_tool(41-43)
tests/unit/tools/file_system/test_write_file.py (1)
src/ai_company/tools/file_system/write_file.py (2)
WriteFileTool(37-178)execute(89-178)
src/ai_company/tools/file_system/_base_fs_tool.py (4)
src/ai_company/core/enums.py (1)
ToolCategory(213-227)src/ai_company/observability/_logger.py (1)
get_logger(8-28)src/ai_company/tools/base.py (4)
BaseTool(56-161)description(115-117)parameters_schema(120-128)category(110-112)src/ai_company/tools/file_system/_path_validator.py (2)
PathValidator(24-109)workspace_root(56-58)
src/ai_company/tools/file_system/delete_file.py (3)
src/ai_company/tools/base.py (3)
ToolExecutionResult(24-53)description(115-117)parameters_schema(120-128)src/ai_company/tools/file_system/_base_fs_tool.py (3)
BaseFileSystemTool(41-80)workspace_root(73-75)path_validator(78-80)src/ai_company/tools/file_system/_path_validator.py (2)
workspace_root(56-58)validate(60-86)
src/ai_company/tools/file_system/__init__.py (7)
src/ai_company/tools/file_system/_base_fs_tool.py (1)
BaseFileSystemTool(41-80)src/ai_company/tools/file_system/_path_validator.py (1)
PathValidator(24-109)src/ai_company/tools/file_system/delete_file.py (1)
DeleteFileTool(34-142)src/ai_company/tools/file_system/edit_file.py (1)
EditFileTool(42-223)src/ai_company/tools/file_system/list_directory.py (1)
ListDirectoryTool(85-231)src/ai_company/tools/file_system/read_file.py (1)
ReadFileTool(58-207)src/ai_company/tools/file_system/write_file.py (1)
WriteFileTool(37-178)
src/ai_company/tools/file_system/_path_validator.py (3)
src/ai_company/observability/_logger.py (1)
get_logger(8-28)src/ai_company/tools/file_system/_base_fs_tool.py (1)
workspace_root(73-75)src/ai_company/providers/routing/resolver.py (1)
resolve(125-152)
src/ai_company/tools/file_system/read_file.py (6)
src/ai_company/observability/_logger.py (1)
get_logger(8-28)src/ai_company/tools/base.py (3)
ToolExecutionResult(24-53)description(115-117)parameters_schema(120-128)src/ai_company/tools/file_system/_base_fs_tool.py (3)
BaseFileSystemTool(41-80)workspace_root(73-75)path_validator(78-80)src/ai_company/tools/file_system/_path_validator.py (2)
workspace_root(56-58)validate(60-86)src/ai_company/tools/file_system/delete_file.py (1)
execute(81-142)src/ai_company/tools/file_system/list_directory.py (1)
execute(137-231)
src/ai_company/tools/file_system/list_directory.py (4)
src/ai_company/observability/_logger.py (1)
get_logger(8-28)src/ai_company/tools/base.py (3)
ToolExecutionResult(24-53)description(115-117)parameters_schema(120-128)src/ai_company/tools/file_system/_base_fs_tool.py (3)
BaseFileSystemTool(41-80)workspace_root(73-75)path_validator(78-80)src/ai_company/tools/file_system/_path_validator.py (2)
workspace_root(56-58)validate(60-86)
src/ai_company/tools/__init__.py (9)
src/ai_company/tools/file_system/_base_fs_tool.py (1)
BaseFileSystemTool(41-80)src/ai_company/tools/file_system/delete_file.py (1)
DeleteFileTool(34-142)src/ai_company/tools/file_system/edit_file.py (1)
EditFileTool(42-223)src/ai_company/tools/file_system/list_directory.py (1)
ListDirectoryTool(85-231)src/ai_company/tools/file_system/_path_validator.py (1)
PathValidator(24-109)src/ai_company/tools/file_system/read_file.py (1)
ReadFileTool(58-207)src/ai_company/tools/file_system/write_file.py (1)
WriteFileTool(37-178)src/ai_company/tools/invoker.py (1)
registry(100-102)src/ai_company/tools/registry.py (1)
ToolRegistry(30-122)
src/ai_company/tools/file_system/edit_file.py (4)
src/ai_company/observability/_logger.py (1)
get_logger(8-28)src/ai_company/tools/base.py (3)
ToolExecutionResult(24-53)description(115-117)parameters_schema(120-128)src/ai_company/tools/file_system/_base_fs_tool.py (4)
BaseFileSystemTool(41-80)_map_os_error(21-38)workspace_root(73-75)path_validator(78-80)src/ai_company/tools/file_system/_path_validator.py (2)
workspace_root(56-58)validate(60-86)
🔇 Additional comments (11)
CLAUDE.md (1)
57-57: LGTM!The documentation accurately reflects the new file-system tools package structure.
src/ai_company/observability/events/tool.py (1)
26-41: LGTM!The new file-system event constants follow the established naming conventions and cover all necessary operations and error conditions for the new tools.
tests/unit/tools/file_system/test_write_file.py (1)
1-104: LGTM!Comprehensive test coverage for
WriteFileToolincluding happy paths, error conditions, path traversal protection, and size limits. Tests properly use fixtures and follow project conventions.tests/unit/tools/file_system/test_path_validator.py (1)
94-111: LGTM!The symlink tests properly handle cross-platform considerations with the
_SYMLINK_SUPPORTEDcheck and appropriateskipifmarkers. Test logic correctly validates that symlinks pointing outside the workspace are rejected.tests/unit/tools/file_system/conftest.py (1)
1-55: LGTM!Well-structured test fixtures providing consistent workspace setup and tool instantiation across the test suite. The workspace fixture creates representative test files covering common scenarios.
src/ai_company/tools/__init__.py (2)
12-20: LGTM!Clean import structure from the new
file_systemsubpackage with all tool classes andPathValidatorproperly exposed at the top-leveltoolspackage.
25-44: LGTM!The
__all__list is properly alphabetized and includes all new file-system exports.tests/unit/tools/file_system/test_delete_file.py (1)
1-103: LGTM!Comprehensive test coverage for
DeleteFileToolincluding property verification, successful deletion scenarios, and proper error handling tests using monkeypatching for OS-level errors.src/ai_company/tools/file_system/__init__.py (1)
1-23: LGTM!Clean package initialization with proper docstring, explicit imports from internal modules (
_base_fs_tool,_path_validator) and public tool modules, and an alphabetized__all__list. This provides a well-defined public API surface for the file-system tools.src/ai_company/tools/file_system/read_file.py (1)
144-147: This suggestion would introduce a syntax error and should not be implemented.The code already uses the correct Python 3.14 exception syntax. When binding the exception object with
as exc:, parentheses are mandatory in Python 3.14 (PEP 758). The syntaxexcept A, B as exc:is aSyntaxError; the correct form isexcept (A, B) as exc:, which is what the code already uses.The parenthesized form is required whenever exception binding is present. The unparenthesized
except A, B:form is only valid without anasclause.> Likely an incorrect or invalid review comment.src/ai_company/tools/file_system/edit_file.py (1)
150-153: The current exception syntax is correct for Python 3.14 and does not require changes.PEP 758 allows omitting parentheses around exception lists only when
as NAMEis not used. Since both lines 150–153 and 184–187 bind the exception withas exc:, the parenthesized formexcept (...) as exc:is required and is the correct style for Python 3.14.> Likely an incorrect or invalid review comment.
| @property | ||
| def require_elevated(self) -> bool: | ||
| """Whether this tool requires elevated permissions. | ||
|
|
||
| Indicates this tool requires explicit approval before execution | ||
| due to its destructive nature. Not yet consumed by the engine; | ||
| defined for forward-compatibility. | ||
| """ | ||
| return True |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate the DeleteFileTool class and understand its structure
cd /tmp && find . -name "delete_file.py" -type f 2>/dev/null | head -20Repository: Aureliolo/ai-company
Length of output: 46
🏁 Script executed:
# Search for require_elevated usage across the codebase
rg "require_elevated" --type py -B 2 -A 2Repository: Aureliolo/ai-company
Length of output: 1314
🏁 Script executed:
# Look for dispatcher or executor that might handle tool execution
rg "dispatcher\|executor\|execute_tool" --type py -l | head -20Repository: Aureliolo/ai-company
Length of output: 46
🏁 Script executed:
# Search for issue `#18` references
rg "#18\|issue.*18" --type py --type mdRepository: Aureliolo/ai-company
Length of output: 46
🏁 Script executed:
# Look for Tool base class to understand the interface
rg "class.*Tool.*:" --type py -A 5 | grep -A 5 "require_elevated" | head -40Repository: Aureliolo/ai-company
Length of output: 455
🏁 Script executed:
# Search for Tool base class definition and interface
find . -name "*.py" -type f -exec grep -l "class.*Tool" {} \; | grep -E "(base|core)" | head -10Repository: Aureliolo/ai-company
Length of output: 302
🏁 Script executed:
# Look for tool execution in engine or dispatcher modules
fd "engine\|dispatch" --type f --extension pyRepository: Aureliolo/ai-company
Length of output: 46
🏁 Script executed:
# Search for any enforcement of elevated permissions
rg "elevated" --type py -B 3 -A 3Repository: Aureliolo/ai-company
Length of output: 2261
🏁 Script executed:
# Check the delete_file.py file structure around require_elevated
cat -n src/ai_company/tools/file_system/delete_file.py | sed -n '1,100p'Repository: Aureliolo/ai-company
Length of output: 3866
🏁 Script executed:
# Examine the Tool base class
cat -n src/ai_company/tools/base.py | head -100Repository: Aureliolo/ai-company
Length of output: 4359
🏁 Script executed:
# Check if there's any agent or engine code that reads require_elevated
rg "require_elevated" -C 5Repository: Aureliolo/ai-company
Length of output: 2703
🏁 Script executed:
# Look at the core/enums.py to understand permission levels
cat -n src/ai_company/core/enums.py | head -50Repository: Aureliolo/ai-company
Length of output: 1516
🏁 Script executed:
# Search for any permission checking in agent or executor
rg "permission\|ELEVATED" --type py -B 3 -A 3 | head -80Repository: Aureliolo/ai-company
Length of output: 46
🏁 Script executed:
# Let's check the agent.py to see if it handles tool execution
cat -n src/ai_company/core/agent.py | head -150Repository: Aureliolo/ai-company
Length of output: 5383
🏁 Script executed:
# Search for tool invocation or dispatch code
rg "execute.*tool\|invoke.*tool\|dispatch.*tool" --type py -B 2 -A 5Repository: Aureliolo/ai-company
Length of output: 46
🏁 Script executed:
# Check if there's any permission gating in BaseTool or subclasses
cat -n src/ai_company/tools/base.py | sed -n '56,150p'Repository: Aureliolo/ai-company
Length of output: 3885
🏁 Script executed:
# Search for any "issue `#18`" or references to elevated permissions requirements
rg "issue\|#18\|elevated.*permission\|permission.*elevated" --type py --type md -B 2 -A 2Repository: Aureliolo/ai-company
Length of output: 46
🏁 Script executed:
# Find ToolAccessLevel definition
rg "class ToolAccessLevel" --type py -A 15Repository: Aureliolo/ai-company
Length of output: 1062
🏁 Script executed:
# Search for ToolPermissions usage to see if it actually gates execution
rg "ToolPermissions\|access_level" --type py -B 2 -A 5 | head -100Repository: Aureliolo/ai-company
Length of output: 46
🏁 Script executed:
# Check if BaseTool has require_elevated property
rg "require_elevated" --type py --files-with-matchesRepository: Aureliolo/ai-company
Length of output: 160
🏁 Script executed:
# Let's look at the full BaseTool class
cat -n src/ai_company/tools/base.py | tail -100Repository: Aureliolo/ai-company
Length of output: 4071
🏁 Script executed:
# Check BaseFileSystemTool
cat -n src/ai_company/tools/file_system/_base_fs_tool.pyRepository: Aureliolo/ai-company
Length of output: 3232
🏁 Script executed:
# Search for ToolInvoker or tool execution dispatcher
rg "ToolInvoker\|tool.*invoke\|invoke.*tool" --type py -B 2 -A 5 | head -150Repository: Aureliolo/ai-company
Length of output: 46
🏁 Script executed:
# Look for ToolPermissionChecker mentioned in the ToolAccessLevel docstring
rg "ToolPermissionChecker" --type py -B 2 -A 15Repository: Aureliolo/ai-company
Length of output: 41866
🏁 Script executed:
# Search for any code checking require_elevated or referencing elevated access
rg "\.require_elevated\|elevated" --type py -B 3 -A 3Repository: Aureliolo/ai-company
Length of output: 46
🏁 Script executed:
# Check agent execute or run tool methods
rg "execute.*tool\|run.*tool" --type py -B 3 -A 8 | head -200Repository: Aureliolo/ai-company
Length of output: 46
🏁 Script executed:
# Look at the full ToolInvoker implementation
cat -n src/ai_company/tools/invoker.py | head -200Repository: Aureliolo/ai-company
Length of output: 7989
🏁 Script executed:
# Search for where require_elevated might be referenced
rg "require_elevated" -B 5 -A 5Repository: Aureliolo/ai-company
Length of output: 2703
🏁 Script executed:
# Check if ToolInvoker has any code that checks require_elevated
cat -n src/ai_company/tools/invoker.py | tail -200Repository: Aureliolo/ai-company
Length of output: 8532
require_elevated is not enforced by the dispatcher.
The ToolInvoker permission check (line 116–141 in src/ai_company/tools/invoker.py) only validates tool.name and tool.category against agent permissions; it does not check the require_elevated property. As a result, delete_file can be executed by any agent with FILE_SYSTEM access, even though the property explicitly signals it requires elevated approval. Before release, implement enforcement of this flag in the permission checker or document that per-tool elevated-access gating is deferred to a future release.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ai_company/tools/file_system/delete_file.py` around lines 71 - 79, The
dispatcher currently ignores the delete_file Tool's require_elevated flag;
update the permission check in ToolInvoker (the permission validation block that
inspects tool.name and tool.category) to also reject tools with
tool.require_elevated=True unless the invoking agent has an
elevated/explicit-approval capability (e.g., an AgentPermission like "elevated"
or "can_execute_elevated_tools"); modify the permission logic in
ToolInvoker.check/authorize (or the method around lines 116–141) to consult
tool.require_elevated and the agent's permission set and return a
permission-denied result when the agent lacks elevated rights, or alternatively
add a clear comment and documentation stub if you choose to defer enforcement.
| try: | ||
| resolved = self.path_validator.validate(user_path) | ||
| except ValueError as exc: | ||
| return ToolExecutionResult(content=str(exc), is_error=True) | ||
|
|
||
| # Let _delete_sync handle all filesystem checks atomically | ||
| # rather than pre-checking exists()/is_dir() — avoids TOCTOU. | ||
| try: | ||
| size_bytes = await asyncio.to_thread(_delete_sync, resolved) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, examine the delete_file.py file to see the full context
cat -n src/ai_company/tools/file_system/delete_file.pyRepository: Aureliolo/ai-company
Length of output: 5787
🏁 Script executed:
# Check the _path_validator.py to understand what validate() returns
cat -n src/ai_company/tools/file_system/_path_validator.pyRepository: Aureliolo/ai-company
Length of output: 4540
🏁 Script executed:
# Find the _delete_sync function implementation
rg -n "_delete_sync" --type py -A 20Repository: Aureliolo/ai-company
Length of output: 6646
Use fd-based deletion to eliminate symlink dereference and TOCTOU vulnerability.
PathValidator.validate() resolves symlinks to their targets (via .resolve()), so passing a symlink path will delete the target file rather than the link itself. Additionally, the TOCTOU race already documented in _path_validator.py (lines 72–77) remains: a concurrent process can swap a parent directory component for a symlink between validation and the unlink() call, redirecting deletion outside the workspace. Switching to fd-anchored operations (e.g., os.unlinkat with a directory file descriptor) eliminates both the symlink dereference and the race.
| parameters_schema={ | ||
| "type": "object", | ||
| "properties": { | ||
| "path": { | ||
| "type": "string", | ||
| "description": "File path relative to workspace", | ||
| }, | ||
| "old_text": { | ||
| "type": "string", | ||
| "description": "Exact text to find", | ||
| }, | ||
| "new_text": { | ||
| "type": "string", | ||
| "description": "Replacement text (empty string to delete)", | ||
| }, | ||
| }, | ||
| "required": ["path", "old_text", "new_text"], | ||
| "additionalProperties": False, | ||
| }, |
There was a problem hiding this comment.
Reject empty old_text up front.
An empty search string is currently valid input. In _edit_sync(), content.count("") returns len(content) + 1 and replace("", new_text, 1) just prefixes the file, so this turns into a destructive edit instead of a validation error.
Suggested fix
"old_text": {
"type": "string",
+ "minLength": 1,
"description": "Exact text to find",
},
@@
old_text: str = arguments["old_text"]
new_text: str = arguments["new_text"]
+ if old_text == "":
+ return ToolExecutionResult(
+ content="old_text must not be empty",
+ is_error=True,
+ )
+
if old_text == new_text:As per coding guidelines, "Validate at system boundaries (user input, external APIs, config files)".
Also applies to: 113-117
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ai_company/tools/file_system/edit_file.py` around lines 78 - 96, The
parameters_schema currently allows an empty "old_text", which leads to
destructive edits in _edit_sync; update validation to reject empty search
strings by adding "minLength": 1 to the "old_text" schema property (and likewise
to the similar schema referenced around the other block mentioned) and also add
a defensive check at the start of _edit_sync (and its async counterpart if
present) to raise/return a validation error when old_text == "" to ensure
system-boundary validation.
…iewers Security fixes: - Block absolute paths in PathValidator before resolve (path traversal) - Wrap resolve() in try/except OSError for malformed paths - Check containment for ALL directory entries, not just symlinks - Reject absolute glob patterns in list_directory - Block oversized files from line-range reads (DoS prevention) - Add minLength:1 + runtime check for empty old_text in edit (destructive no-op) Reliability fixes: - Atomic writes via tempfile+fsync+replace in edit_file and write_file - Stable "os_error" log key + detailed user message in _map_os_error - Distinguish missing path from non-directory in list_directory - Validate start_line <= end_line in read_file - Compute line_count before appending truncation notice Information leak fix: - Log old_text length instead of preview content in edit_file Documentation: - Add docstrings to _read_sync, _edit_sync, _write_sync, _list_sync - Update DESIGN_SPEC.md file system tool category to include "list" - Update list_directory class docstring with all type prefixes Tests: - Add pytestmark timeout(30) to all 6 test modules - Add test_empty_old_text_rejected, test_start_line_greater_than_end_line - Fix test assertions for new error messages and behavior - Add encoding="utf-8" to write_text calls in path validator tests Reviewed-by: CodeRabbit, Gemini, Greptile, Copilot Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| MAX_ENTRIES: Final[int] = 1000 | ||
|
|
||
| # Reject glob patterns that could traverse above the workspace. | ||
| _UNSAFE_GLOB_RE = re.compile(r"(^|[/\\])\.\.[/\\]|^\.\.$") |
There was a problem hiding this comment.
_UNSAFE_GLOB_RE misses trailing .. without separator
The regex (^|[/\\])\.\.[/\\]|^\.\.$ requires a / or \ after .., so a pattern like subdir/.. (terminating with ..) is not rejected. The second operand ^\.\.$ only matches the bare .. string.
On Python 3.12+ pathlib raises ValueError: Pattern cannot contain '..' when .. appears anywhere in a glob argument. Because execute() (lines 234–245) only catches PermissionError and OSError, this ValueError would escape the tool boundary as an unhandled exception.
| _UNSAFE_GLOB_RE = re.compile(r"(^|[/\\])\.\.[/\\]|^\.\.$") | |
| _UNSAFE_GLOB_RE = re.compile(r"(^|[/\\])\.\.([/\\]|$)|^\.\.$") |
This change adds $ as a valid end-anchor so that subdir/.. is caught. The existing containment check inside _list_sync still acts as a secondary defence, but the early rejection is cleaner and avoids the propagation risk entirely.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ai_company/tools/file_system/list_directory.py
Line: 27
Comment:
`_UNSAFE_GLOB_RE` misses trailing `..` without separator
The regex `(^|[/\\])\.\.[/\\]|^\.\.$` requires a `/` or `\` _after_ `..`, so a pattern like `subdir/..` (terminating with `..`) is not rejected. The second operand `^\.\.$` only matches the bare `..` string.
On Python 3.12+ pathlib raises `ValueError: Pattern cannot contain '..'` when `..` appears anywhere in a glob argument. Because `execute()` (lines 234–245) only catches `PermissionError` and `OSError`, this `ValueError` would escape the tool boundary as an unhandled exception.
```suggestion
_UNSAFE_GLOB_RE = re.compile(r"(^|[/\\])\.\.([/\\]|$)|^\.\.$")
```
This change adds `$` as a valid end-anchor so that `subdir/..` is caught. The existing containment check inside `_list_sync` still acts as a secondary defence, but the early rejection is cleaner and avoids the propagation risk entirely.
How can I resolve this? If you propose a fix, please make it concise.| glob_pattern = pattern or "*" | ||
| if recursive: | ||
| raw_iter = resolved.rglob(glob_pattern) | ||
| elif pattern: | ||
| raw_iter = resolved.glob(glob_pattern) | ||
| else: | ||
| raw_iter = resolved.iterdir() |
There was a problem hiding this comment.
recursive=False does not prevent recursion when pattern contains **
When pattern is something like "**/*.py", the elif pattern: branch (line 58) calls resolved.glob("**/*.py"), which still performs a full recursive walk — the ** wildcard is interpreted by pathlib as a glob-level recursion operator regardless of which method is used. A caller who passes recursive=False with a ** pattern gets recursive behaviour without any warning.
Consider detecting (and rejecting) ** in the pattern when recursive=False:
if pattern and not recursive and ("**" in pattern):
return ToolExecutionResult(
content="Use recursive=true with ** glob patterns.",
is_error=True,
)Alternatively, strip or escape ** segments when recursive=False to enforce the caller's intent.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ai_company/tools/file_system/list_directory.py
Line: 54-60
Comment:
`recursive=False` does not prevent recursion when pattern contains `**`
When `pattern` is something like `"**/*.py"`, the `elif pattern:` branch (line 58) calls `resolved.glob("**/*.py")`, which still performs a full recursive walk — the `**` wildcard is interpreted by pathlib as a glob-level recursion operator regardless of which method is used. A caller who passes `recursive=False` with a `**` pattern gets recursive behaviour without any warning.
Consider detecting (and rejecting) `**` in the pattern when `recursive=False`:
```python
if pattern and not recursive and ("**" in pattern):
return ToolExecutionResult(
content="Use recursive=true with ** glob patterns.",
is_error=True,
)
```
Alternatively, strip or escape `**` segments when `recursive=False` to enforce the caller's intent.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 20 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from typing import TYPE_CHECKING | ||
|
|
||
| import pytest | ||
|
|
||
| from ai_company.tools.file_system._path_validator import PathValidator | ||
|
|
||
| if TYPE_CHECKING: | ||
| from pathlib import Path | ||
|
|
||
| pytestmark = pytest.mark.timeout(30) | ||
|
|
There was a problem hiding this comment.
Path is only imported under TYPE_CHECKING, but it’s used in runtime annotations (tmp_path: Path). This will raise NameError during module import unless Path is imported at runtime (pytest evaluates annotations).
| from typing import TYPE_CHECKING | |
| import pytest | |
| from ai_company.tools.file_system._path_validator import PathValidator | |
| if TYPE_CHECKING: | |
| from pathlib import Path | |
| pytestmark = pytest.mark.timeout(30) | |
| from pathlib import Path | |
| import pytest | |
| from ai_company.tools.file_system._path_validator import PathValidator | |
| pytestmark = pytest.mark.timeout(30) |
| max_bytes: When set, only the first *max_bytes* characters are | ||
| read via text-mode ``read()`` (approximate byte cap for | ||
| oversized files without a line range). | ||
|
|
||
| Returns: | ||
| The file content (possibly sliced or truncated). | ||
|
|
||
| Raises: | ||
| UnicodeDecodeError: If the file contains non-UTF-8 bytes. | ||
| FileNotFoundError: If the file does not exist. | ||
| PermissionError: If the process lacks read permission. | ||
| OSError: For other OS-level I/O failures. | ||
| """ | ||
| if start is not None or end is not None: | ||
| raw = resolved.read_text(encoding="utf-8") | ||
| lines = raw.splitlines(keepends=True) | ||
| s = (start - 1) if start is not None else 0 | ||
| e = end if end is not None else len(lines) | ||
| return "".join(lines[s:e]) | ||
|
|
||
| if max_bytes is not None: | ||
| with resolved.open(encoding="utf-8") as fh: | ||
| return fh.read(max_bytes) | ||
|
|
There was a problem hiding this comment.
max_bytes is treated as a byte cap in the API/variable naming, but _read_sync uses text-mode fh.read(max_bytes) which limits characters, not bytes. For non-ASCII UTF-8, this can read several MB even when max_bytes=1_048_576, undermining the DoS/memory guard. Consider reading bytes in binary mode (e.g., fh.buffer.read(max_bytes)) and then decoding, so the cap is enforced in bytes.
| ValueError: If the resolved path escapes the workspace or | ||
| the parent directory does not exist. | ||
| """ | ||
| resolved = self.validate(path) | ||
| if not resolved.parent.exists(): | ||
| logger.warning( | ||
| TOOL_FS_PARENT_NOT_FOUND, | ||
| path=path, | ||
| ) | ||
| msg = f"Parent directory does not exist: {path}" | ||
| raise ValueError(msg) |
There was a problem hiding this comment.
validate_parent_exists() only checks resolved.parent.exists(). If the parent path exists but is a file (e.g., writing to hello.txt/new.txt), validation passes and the write later fails with a generic OS error. Consider checking resolved.parent.is_dir() and returning a clearer error message (and event) for “parent is not a directory”.
| ValueError: If the resolved path escapes the workspace or | |
| the parent directory does not exist. | |
| """ | |
| resolved = self.validate(path) | |
| if not resolved.parent.exists(): | |
| logger.warning( | |
| TOOL_FS_PARENT_NOT_FOUND, | |
| path=path, | |
| ) | |
| msg = f"Parent directory does not exist: {path}" | |
| raise ValueError(msg) | |
| ValueError: If the resolved path escapes the workspace, the | |
| parent directory does not exist, or the parent is not a | |
| directory. | |
| """ | |
| resolved = self.validate(path) | |
| parent = resolved.parent | |
| if not parent.exists(): | |
| logger.warning( | |
| TOOL_FS_PARENT_NOT_FOUND, | |
| path=path, | |
| ) | |
| msg = f"Parent directory does not exist: {path}" | |
| raise ValueError(msg) | |
| if not parent.is_dir(): | |
| logger.warning( | |
| TOOL_FS_PARENT_NOT_FOUND, | |
| path=path, | |
| ) | |
| msg = f"Parent path is not a directory: {path}" | |
| raise ValueError(msg) |
| if not resolved.exists(): | ||
| logger.warning(TOOL_FS_ERROR, path=user_path, error="not_found") | ||
| return ToolExecutionResult( | ||
| content=f"Path not found: {user_path}", | ||
| is_error=True, | ||
| ) | ||
|
|
||
| if not resolved.is_dir(): | ||
| logger.warning(TOOL_FS_ERROR, path=user_path, error="not_a_directory") | ||
| return ToolExecutionResult( | ||
| content=f"Not a directory: {user_path}", | ||
| is_error=True, | ||
| ) | ||
|
|
There was a problem hiding this comment.
Using Path.exists() / Path.is_dir() can silently return False on PermissionError, which means permission issues may be reported as “Path not found” or “Not a directory”. Consider using stat() (or iterdir() in a try block) and mapping PermissionError explicitly so the user gets an accurate error.
| from typing import TYPE_CHECKING | ||
|
|
||
| import pytest | ||
|
|
||
| from ai_company.core.enums import ToolCategory | ||
| from ai_company.tools.file_system.read_file import MAX_FILE_SIZE_BYTES, ReadFileTool | ||
|
|
||
| if TYPE_CHECKING: | ||
| from pathlib import Path | ||
|
|
||
| pytestmark = pytest.mark.timeout(30) |
There was a problem hiding this comment.
Path is only imported under TYPE_CHECKING, but the test function signatures use Path annotations. Import Path from pathlib at runtime to avoid NameError during test module import (pytest evaluates annotations).
| def require_elevated(self) -> bool: | ||
| """Whether this tool requires elevated permissions. | ||
|
|
||
| Indicates this tool requires explicit approval before execution | ||
| due to its destructive nature. Not yet consumed by the engine; | ||
| defined for forward-compatibility. | ||
| """ | ||
| return True |
There was a problem hiding this comment.
DeleteFileTool.require_elevated is not currently enforced anywhere (no other references in the repo), and ToolPermissionChecker grants ToolCategory.FILE_SYSTEM even at SANDBOXED/RESTRICTED levels. As implemented, delete_file will be permitted by default for most agents, which conflicts with the “elevated access only” requirement. Consider integrating require_elevated into permission checking / invocation gating (or otherwise ensuring delete_file is denied unless explicitly allowed/elevated).
| from typing import TYPE_CHECKING | ||
|
|
||
| import pytest | ||
|
|
||
| from ai_company.tools.file_system.delete_file import DeleteFileTool | ||
| from ai_company.tools.file_system.edit_file import EditFileTool | ||
| from ai_company.tools.file_system.list_directory import ListDirectoryTool | ||
| from ai_company.tools.file_system.read_file import ReadFileTool | ||
| from ai_company.tools.file_system.write_file import WriteFileTool | ||
|
|
||
| if TYPE_CHECKING: | ||
| from pathlib import Path | ||
|
|
There was a problem hiding this comment.
Path is only imported under TYPE_CHECKING, but it’s used in runtime-evaluated annotations (pytest evaluates annotations). This will raise NameError at import time. Import Path from pathlib at runtime (see pattern in tests/unit/tools/git/conftest.py which uses # noqa: TC003).
| from typing import TYPE_CHECKING | ||
|
|
||
| import pytest | ||
|
|
||
| if TYPE_CHECKING: | ||
| from pathlib import Path | ||
|
|
||
| from ai_company.tools.file_system.write_file import WriteFileTool | ||
|
|
There was a problem hiding this comment.
Path is only imported under TYPE_CHECKING, but it’s referenced in runtime annotations (e.g., workspace: Path). Pytest evaluates these annotations, so this will fail with NameError at import time unless Path is imported at runtime.
| from typing import TYPE_CHECKING | |
| import pytest | |
| if TYPE_CHECKING: | |
| from pathlib import Path | |
| from ai_company.tools.file_system.write_file import WriteFileTool | |
| from pathlib import Path | |
| import pytest | |
| from ai_company.tools.file_system.write_file import WriteFileTool |
|
|
||
| import pytest | ||
|
|
||
| if TYPE_CHECKING: | ||
| from pathlib import Path | ||
|
|
There was a problem hiding this comment.
Path is only imported under TYPE_CHECKING, but it’s used in runtime annotations for test args. This will raise NameError when importing the test module unless Path is imported at runtime.
| import pytest | |
| if TYPE_CHECKING: | |
| from pathlib import Path | |
| from pathlib import Path | |
| import pytest | |
| if TYPE_CHECKING: |
|
|
||
| import pytest | ||
|
|
||
| if TYPE_CHECKING: | ||
| from pathlib import Path | ||
|
|
There was a problem hiding this comment.
Path is only imported under TYPE_CHECKING, but runtime-evaluated annotations reference it (e.g., workspace: Path). Import Path from pathlib at runtime to avoid NameError when pytest imports this file.
| import pytest | |
| if TYPE_CHECKING: | |
| from pathlib import Path | |
| from pathlib import Path | |
| import pytest | |
| if TYPE_CHECKING: |
Summary
require_elevatedflag for future permission system_map_os_errorhelper for consistent error mappingTOOL_FS_STAT_FAILED,TOOL_FS_WORKSPACE_INVALID,TOOL_FS_PARENT_NOT_FOUND,TOOL_FS_GLOB_REJECTED,TOOL_FS_NOOP)is_dir()checks for Windows compatibility (Windows raisesPermissionErrorinstead ofIsADirectoryError)..traversalTest plan
../and absolute paths)..patterns rejected)Review coverage
Pre-reviewed by 10 agents, 28 findings consolidated and addressed:
Polish pass by code-simplifier agent (extracted shared
_map_os_error, simplified return types, restructured try blocks for consistency).Closes #18