feat(tools): make read tool useful standalone, disable by default#1266
Conversation
Reworked the read tool from a shell passthrough into a proper standalone tool: - Implements actual file reading without shell dependency - Adds line number output (cat -n style) - Supports line range via kwargs (start_line, end_line) - Handles errors gracefully (binary files, missing files, permissions) - Disabled by default since it was redundant with shell - Enables sandboxed read/write workflows with --tools read,patch,save Closes #1259
|
@greptileai review |
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to eb0c22b in 11 seconds. Click for details.
- Reviewed
306lines of code in3files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_d3D3dHoL4nzaNOHC
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Greptile SummaryThis PR rewrites the
Confidence Score: 3/5
Important Files Changed
Flowchartflowchart TD
A[execute_read called] --> B{Path provided?}
B -- No --> C[Return: No path provided]
B -- Yes --> D{File exists?}
D -- No --> E[Return: File not found]
D -- Yes --> F{Is a file?}
F -- No --> G[Return: Not a file]
F -- Yes --> H[Parse start_line / end_line kwargs]
H --> I[Read file text]
I -- UnicodeDecodeError --> J[Return: Cannot read binary file]
I -- PermissionError --> K[Return: Permission denied]
I -- Success --> L[Split into lines]
L --> M[Apply line range selection]
M --> N[Format with line numbers]
N --> O[Return formatted output with range info]
Last reviewed commit: eb0c22b |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…declarations - Add path traversal protection consistent with save.py and patch.py - Declare start_line and end_line in parameters list for tool schema
Review Feedback AddressedBoth Greptile review comments fixed in commit 0caa71f:
All 10 read tool tests pass, mypy clean. |
When a tool has optional parameters (like read's start_line/end_line), the positional arg mapping would crash with IndexError if fewer args were provided than total parameters. Now skips optional params that have no corresponding arg.
Summary
readtool from a shell passthrough into a proper standalone file reader--tools read,patch,saveChanges
gptme/tools/read.py: Full implementation withexecute_read()functionstart_line/end_linekwargsdisabled_by_default=Trueto remove from default toolsetreadblock type for markdown codeblock executiontests/test_tools_read.py: 10 tests covering all functionalitytests/test_prompt_tools.py: Updated expectations (read no longer references shell)Motivation
From #1259: when restricting tools (e.g.
--tools patch,save), having areadtool that actually works independently completes the read/write workflow without needing shell. Previously the read tool just said "use shell instead", which is useless when shell is disabled.Usage
Closes #1259
Important
Reworks
readtool to function independently with error handling and line range support, and updates tests accordingly.readtool ingptme/tools/read.pyto function as a standalone file reader withexecute_read().start_line/end_line.readtool by default usingdisabled_by_default=True.readblock type for markdown codeblock execution.tests/test_tools_read.pywith 10 tests covering full functionality.tests/test_prompt_tools.pyto reflect changes inreadtool behavior.This description was created by
for eb0c22b. You can customize this summary. It will automatically update as commits are pushed.