-
Notifications
You must be signed in to change notification settings - Fork 2
fix(search_file): return workspace-relative paths (T0-6) #181
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,7 +38,10 @@ def name(self) -> str: | |
|
|
||
| @property | ||
| def description(self) -> str: | ||
| return "Search file paths by glob pattern (e.g. *.py, src/**/*.ts). Returns paths as absolute paths. Use paths directly for read_file." | ||
| return ( | ||
| "Search file paths by glob pattern (e.g. *.py, src/**/*.ts). " | ||
| "Returns workspace-relative paths; matches in non-primary roots are prefixed as @root[n]/." | ||
| ) | ||
|
|
||
| @property | ||
| def input_schema(self) -> dict[str, Any]: | ||
|
|
@@ -57,7 +60,11 @@ def output_schema(self) -> dict[str, Any]: | |
| return { | ||
| "type": "object", | ||
| "properties": { | ||
| "paths": {"type": "array", "items": {"type": "string"}, "description": "Absolute paths; use directly for read_file"}, | ||
| "paths": { | ||
| "type": "array", | ||
| "items": {"type": "string"}, | ||
| "description": "Workspace-relative paths. Non-primary roots are encoded as @root[n]/<path>.", | ||
| }, | ||
| "total_matches": {"type": "integer"}, | ||
| "truncated": {"type": "boolean"}, | ||
| }, | ||
|
|
@@ -143,18 +150,18 @@ def _execute_search_file(input: dict[str, Any], context: RunContext[Any]) -> Too | |
|
|
||
| if search_path.is_file(): | ||
| abs_path = search_path.resolve() | ||
| rel_path = _normalized_relative_path(abs_path, root) | ||
| rel_path = _normalized_relative_path(abs_path, root, roots) | ||
| if _match_pattern(pattern, rel_path): | ||
| matches.append(str(abs_path).replace("\\", "/")) | ||
| matches.append(rel_path) | ||
| else: | ||
| for dirpath, dirs, files in os.walk(search_path, topdown=True, followlinks=False): | ||
| dirs[:] = [d for d in sorted(dirs) if d not in ignore_dirs] | ||
| for filename in sorted(files): | ||
| abs_path = (Path(dirpath) / filename).resolve() | ||
| rel_path = _normalized_relative_path(abs_path, root) | ||
| rel_path = _normalized_relative_path(abs_path, root, roots) | ||
| if not _match_pattern(pattern, rel_path): | ||
| continue | ||
| matches.append(str(abs_path).replace("\\", "/")) | ||
| matches.append(rel_path) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Appending Useful? React with 👍 / 👎.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in What changed:
Verification:
|
||
| if len(matches) >= max_results: | ||
| truncated = True | ||
| break | ||
|
|
@@ -178,8 +185,16 @@ def _execute_search_file(input: dict[str, Any], context: RunContext[Any]) -> Too | |
| ) | ||
|
|
||
|
|
||
| def _normalized_relative_path(path: Path, root: Path) -> str: | ||
| return relative_to_root(path, root).replace("\\", "/") | ||
| def _normalized_relative_path(path: Path, root: Path, roots: list[Path]) -> str: | ||
| relative_path = relative_to_root(path, root).replace("\\", "/") | ||
| try: | ||
| root_index = roots.index(root) | ||
| except ValueError: | ||
| root_index = 0 | ||
|
|
||
| if root_index <= 0: | ||
| return relative_path | ||
| return f"@root[{root_index}]/{relative_path}" | ||
|
|
||
|
|
||
| def _match_pattern(pattern: str, relative_path: str) -> bool: | ||
|
|
@@ -200,4 +215,3 @@ def _error_result(error: ToolError) -> ToolResult: | |
| error=error.message, | ||
| evidence=[], | ||
| ) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here
rel_pathis already rewritten as@root[n]/...for secondary roots, so directory-aware patterns likepkg/*.pyorsrc/**/*.tsno longer match even when files exist under the searched secondary root. I confirmed this by exercisingSearchFileToolwith two workspace roots: searching the secondary root with patternpkg/*.pyreturns zero results because matching is done against the prefixed display path instead of the root-relative path. This is a regression introduced by the prefixing change and causes false negatives for multi-root searches.Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in
2bf92d2by separating match-path semantics from display-path formatting.What changed:
search_filenow performs glob matching against the root-relative path (for examplepkg/c.py).@root[n]/...for non-primary roots).test_search_file_matches_directory_glob_in_secondary_rootVerification:
.venv/bin/pytest -q tests/unit/test_v4_file_tools.py::test_search_file_prefixes_secondary_root_paths tests/unit/test_v4_file_tools.py::test_search_file_matches_directory_glob_in_secondary_root tests/unit/test_v4_file_tools.py::test_read_file_accepts_secondary_root_prefixed_path.venv/bin/pytest -q tests/unit/test_v4_file_tools.py