fix(grep): respect cancellation token#1839
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements cancellation support for the GrepFilesTool by passing a CancellationToken through the file collection and search logic. It includes a new test case to ensure the tool respects cancellation. The reviewer suggested generalizing the error message in the check_cancelled helper function to improve its reusability across different search tools.
| fn check_cancelled(cancel_token: Option<&CancellationToken>) -> Result<(), ToolError> { | ||
| if cancel_token.is_some_and(CancellationToken::is_cancelled) { | ||
| return Err(ToolError::execution_failed( | ||
| "grep_files cancelled before search completed", | ||
| )); | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
The check_cancelled helper function uses a hardcoded error message that specifically mentions grep_files. Since the recursive file collection logic and this helper might be reused by other search tools in this module (such as the file_search tool mentioned in the PR description), it would be better to use a more generic message to avoid misleading error messages in the future.
| fn check_cancelled(cancel_token: Option<&CancellationToken>) -> Result<(), ToolError> { | |
| if cancel_token.is_some_and(CancellationToken::is_cancelled) { | |
| return Err(ToolError::execution_failed( | |
| "grep_files cancelled before search completed", | |
| )); | |
| } | |
| Ok(()) | |
| } | |
| fn check_cancelled(cancel_token: Option<&CancellationToken>) -> Result<(), ToolError> { | |
| if cancel_token.is_some_and(CancellationToken::is_cancelled) { | |
| return Err(ToolError::execution_failed( | |
| "Search operation cancelled before completion", | |
| )); | |
| } | |
| Ok(()) | |
| } |
99db638 to
04b5781
Compare
|
Addressed the Gemini review suggestion in 04b5781 by generalizing the cancellation helper error message from grep-specific wording to a reusable search cancellation message. |
- grep_files now respects cancellation token (Hmbown#1839, thanks @LING71671) - Ctrl+Z restores last cleared composer draft (Hmbown#1911, thanks @LING71671) - Clipboard works on non-wlroots Wayland via wl-copy (Hmbown#1938, thanks @ousamabenyounes)
…c, receipt truncation fix - SWE-bench: codewhale swebench run/export writes prediction JSONL from working-tree diff, with untracked-file inclusion via git add -N - CLI: --workspace / -C global flag forwards to TUI for file ops - CLI: codewhale exec --auto semantics clarified in help text - Markdown: table pipes inside inline code no longer create phantom columns (split_table_cells with backtick-awareness) - Receipt: floor_char_boundary prevents multibyte UTF-8 slice panic - Contributors: Ling (LING71671 Hmbown#1839 Hmbown#1911), Ben Younes (ousamabenyounes Hmbown#1938), jeoor npm fix (Hmbown#1860) credited across all 3 READMEs - ja-JP README: 19 contributors synced to parity with EN/zh-CN (80 each) - Docs: SWEBENCH.md, RECURSIVE_SELF_IMPROVEMENT.md, MODES.md exec clarification - Sub-agent footer: Alt+V hint now says 'details' not 'raw'
|
Thanks @LING71671 — this landed in #1988 and is on main via commit 8878ac0. The shipped change makes grep_files observe the ToolContext cancellation token and includes the regression test test_grep_files_respects_cancel_token.\n\nI am closing this PR now that the code is on main. The release notes and README Thanks list both credit you for #1839. Apologies that the harvest commit missed the exact 'Harvested from PR #1839 by @LING71671' trailer, which is why the auto-close workflow did not close this at merge time. |
Summary
grep_filesobserve the activeToolContextcancellation token while collecting files and scanning linesgrep_filesinvocationRelated issue
#1791 tracks cancellation for synchronous search tools. This PR covers the
grep_filespart; #1790 coversfile_searchseparately.Validation
cargo fmt --all -- --checkcargo test -p deepseek-tui tools::search::tests::test_grep_files_respects_cancel_tokencargo test -p deepseek-tui tools::search::testscargo clippy -p deepseek-tui --all-targets --all-features(passes with pre-existing warnings inschema_migration.rsandtui/ui.rs)