feat(tools): allow file operations outside working directory#657
Merged
Conversation
Signed-off-by: Richard Chien <stdrc@outlook.com>
stdrc
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR enables file write and edit operations outside the working directory when using absolute paths, adding a new security boundary while maintaining protection through user approval requirements.
Changes:
- Allow WriteFile and StrReplaceFile tools to operate on files outside the working directory when absolute paths are provided
- Add
EDIT_OUTSIDEaction to FileActions enum for differentiated approval requests - Update tool parameter descriptions to clarify path requirements
- Update documentation to reflect the new capabilities and requirements
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/kimi_cli/tools/file/write.py | Modified path validation to allow absolute paths outside working directory; added EDIT_OUTSIDE action selection |
| src/kimi_cli/tools/file/replace.py | Modified path validation to allow absolute paths outside working directory; added EDIT_OUTSIDE action selection |
| src/kimi_cli/tools/file/read.py | Minor cleanup: removed comment and adjusted path canonicalization order |
| src/kimi_cli/tools/file/init.py | Added EDIT_OUTSIDE enum value to FileActions |
| tests/test_write_file.py | Updated tests to verify new behavior allowing operations outside working directory |
| tests/test_str_replace_file.py | Updated tests to verify new behavior allowing operations outside working directory |
| tests/test_tool_schemas.py | Updated expected parameter descriptions for WriteFile and StrReplaceFile |
| tests/test_default_agent.py | Updated expected parameter descriptions in default agent test |
| tests/conftest.py | Changed outside_file fixture to use temporary directory instead of hardcoded paths |
| docs/en/customization/agents.md | Updated documentation to clarify working directory restrictions and absolute path requirements |
| docs/zh/customization/agents.md | Updated Chinese documentation to clarify working directory restrictions and absolute path requirements |
| docs/en/release-notes/changelog.md | Added changelog entry for new file operation capabilities |
| docs/zh/release-notes/changelog.md | Added Chinese changelog entry for new file operation capabilities |
| CHANGELOG.md | Added changelog entry for new file operation capabilities |
Comments suppressed due to low confidence (3)
src/kimi_cli/tools/file/write.py:81
- Inconsistent path usage in error message. This error message uses
params.path(the original user input), while the approval message at line 119 and diff blocks at line 103 usestr(p)(the canonicalized path). For consistency and clarity, all user-facing messages should use the same path representation, preferably the canonicalized version.
if not await p.parent.exists():
return ToolError(
message=f"`{params.path}` parent directory does not exist.",
brief="Parent directory not found",
)
src/kimi_cli/tools/file/replace.py:93
- Inconsistent path usage in error messages. These error messages use
params.path(the original user input), while the approval message at line 126 and diff blocks at line 113 usestr(p)(the canonicalized path). For consistency and clarity, all user-facing messages should use the same path representation, preferably the canonicalized version.
if not await p.exists():
return ToolError(
message=f"`{params.path}` does not exist.",
brief="File not found",
)
if not await p.is_file():
return ToolError(
message=f"`{params.path}` is not a file.",
brief="Invalid path",
)
src/kimi_cli/tools/file/read.py:174
- Inconsistent path usage in error messages. These error messages use
params.path(the original user input), while other parts of the code usestr(p)(the canonicalized path). For consistency and clarity, all user-facing messages should use the same path representation, preferably the canonicalized version.
if not await p.exists():
return ToolError(
message=f"`{params.path}` does not exist.",
brief="File not found",
)
if not await p.is_file():
return ToolError(
message=f"`{params.path}` is not a file.",
brief="Invalid path",
)
header = await p.read_bytes(MEDIA_SNIFF_BYTES)
file_type = detect_file_type(str(p), header=header)
if file_type.kind in ("image", "video"):
return await self._read_media(p, file_type)
if file_type.kind == "unknown":
return ToolError(
message=(
f"`{params.path}` seems not readable. "
"You may need to read it with proper shell commands, Python tools "
"or MCP tools if available. "
"If you read/operate it with Python, you MUST ensure that any "
"third-party packages are installed in a virtual environment (venv)."
),
brief="File not readable",
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+80
to
+82
| if err := await self._validate_path(p): | ||
| return err | ||
| p = p.canonical() |
There was a problem hiding this comment.
There is redundant path canonicalization. The path is canonicalized inside _validate_path at line 50, but then canonicalized again at line 82 after validation. Additionally, the validation is called with the expanded but non-canonical path, which could lead to inconsistent behavior. Consider either canonicalizing before validation or removing the duplicate canonicalization after validation.
Suggested change
| if err := await self._validate_path(p): | |
| return err | |
| p = p.canonical() | |
| p = p.canonical() | |
| if err := await self._validate_path(p): | |
| return err |
Comment on lines
+99
to
131
| async def test_write_with_relative_path(write_file_tool: WriteFile, temp_work_dir: KaosPath): | ||
| """Test writing with a relative path inside the work directory.""" | ||
| relative_dir = temp_work_dir / "relative" / "path" | ||
| await relative_dir.mkdir(parents=True, exist_ok=True) | ||
|
|
||
| result = await write_file_tool(Params(path="relative/path/file.txt", content="content")) | ||
|
|
||
| assert result.is_error | ||
| assert "not an absolute path" in result.message | ||
| assert not result.is_error | ||
| assert await (temp_work_dir / "relative" / "path" / "file.txt").read_text() == "content" | ||
|
|
||
|
|
||
| async def test_write_outside_work_directory(write_file_tool: WriteFile, outside_file: Path): | ||
| """Test writing outside the working directory (should fail).""" | ||
| """Test writing outside the working directory with an absolute path.""" | ||
| result = await write_file_tool(Params(path=str(outside_file), content="content")) | ||
|
|
||
| assert result.is_error | ||
| assert "outside the working directory" in result.message | ||
| assert not result.is_error | ||
| assert outside_file.read_text() == "content" | ||
|
|
||
|
|
||
| async def test_write_outside_work_directory_with_prefix( | ||
| write_file_tool: WriteFile, temp_work_dir: KaosPath | ||
| ): | ||
| """Paths sharing the same prefix as work dir should still be rejected.""" | ||
| """Paths sharing the same prefix as work dir should still be writable with absolute paths.""" | ||
| base = Path(str(temp_work_dir)) | ||
| sneaky_dir = base.parent / f"{base.name}-sneaky" | ||
| sneaky_dir.mkdir(parents=True, exist_ok=True) | ||
| sneaky_file = sneaky_dir / "file.txt" | ||
|
|
||
| result = await write_file_tool(Params(path=str(sneaky_file), content="content")) | ||
|
|
||
| assert result.is_error | ||
| assert "outside the working directory" in result.message | ||
| assert not result.is_error | ||
| assert sneaky_file.read_text() == "content" | ||
|
|
There was a problem hiding this comment.
Missing test coverage for an important edge case: relative paths that traverse outside the working directory (e.g., ../../../tmp/file.txt). The current test suite doesn't verify the behavior when a relative path is provided that would resolve to outside the working directory when the CWD equals the work directory. This test case is critical given the new feature allowing operations outside the working directory with absolute paths.
Comment on lines
+71
to
+75
| p = KaosPath(params.path).expanduser() | ||
|
|
||
| # Validate path safety | ||
| path_error = await self._validate_path(p) | ||
| if path_error: | ||
| return path_error | ||
| if err := await self._validate_path(p): | ||
| return err | ||
| p = p.canonical() |
There was a problem hiding this comment.
There is redundant path canonicalization. The path is canonicalized inside _validate_path at line 47, but then canonicalized again at line 75 after validation. Additionally, the validation is called with the expanded but non-canonical path, which could lead to inconsistent behavior. Consider either canonicalizing before validation or removing the duplicate canonicalization after validation.
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
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.
Allow editing/writing files outside the working directory when using absolute paths. Previously, these operations were restricted to the working directory only.