fix(search_file): return workspace-relative paths (T0-6) - #181
Conversation
Align search_file output with existing path contract tests by returning workspace-relative matches instead of absolute filesystem paths.\n\nChanges:\n- Emit relative paths for both file and directory search branches in SearchFileTool.\n- Update tool description and output schema text to document workspace-relative output semantics.\n\nWhy:\n- Current behavior returned absolute paths while test and TODO baseline require relative paths.\n- Relative paths keep outputs deterministic across machines and CI temp directories.\n\nVerification:\n- .venv/bin/pytest -q tests/unit/test_v4_file_tools.py::test_search_file_finds_matching_paths => passed\n- .venv/bin/pytest -q tests/unit/test_v4_file_tools.py => 16 passed
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5bc4e0d3a8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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.
Appending rel_path here drops which workspace root a match came from, which breaks multi-root setups introduced by this change from absolute paths. If search_file is run against a non-primary root (for example by passing an absolute path in the second workspace root), the returned relative name is later resolved by read_file via resolve_path against roots[0], so the same path can resolve to the wrong file or fail with FILE_NOT_FOUND even though search_file reported it as a match.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ce9f26b by preserving root context in search outputs and teaching path resolution to consume it.
What changed:
search_filenow prefixes non-primary workspace roots as@root[n]/...(primary root remains unchanged).resolve_pathnow parses@root[n]/...and resolves against the selected workspace root.- Added regression tests for both behaviors:
test_search_file_prefixes_secondary_root_pathstest_read_file_accepts_secondary_root_prefixed_path
Verification:
.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_read_file_accepts_secondary_root_prefixed_path.venv/bin/pytest -q tests/unit/test_v4_file_tools.py
Problem:\n- search_file switched to pure workspace-relative output, which dropped root identity in multi-root runs\n- read_file resolves relative paths against workspace_roots[0], so matches from secondary roots could resolve to wrong files or fail\n\nChanges:\n- add @root[n]/<relative-path> support in resolve_path for deterministic root-targeted resolution\n- emit @root[n]/ prefix from search_file when a match belongs to a non-primary workspace root\n- update search_file description/output schema to document the root-prefixed contract\n- add regression tests covering secondary-root search_file output and read_file consumption of prefixed paths\n\nWhy this shape:\n- keeps existing single-root behavior unchanged\n- preserves deterministic relative outputs while restoring root context needed for follow-up file tools
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce9f26b459
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| rel_path = _normalized_relative_path(abs_path, root, roots) | ||
| if not _match_pattern(pattern, rel_path): |
There was a problem hiding this comment.
Here rel_path is already rewritten as @root[n]/... for secondary roots, so directory-aware patterns like pkg/*.py or src/**/*.ts no longer match even when files exist under the searched secondary root. I confirmed this by exercising SearchFileTool with two workspace roots: searching the secondary root with pattern pkg/*.py returns 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.
Fixed in 2bf92d2 by separating match-path semantics from display-path formatting.
What changed:
search_filenow performs glob matching against the root-relative path (for examplepkg/c.py).- After a match is found, output formatting is applied (
@root[n]/...for non-primary roots). - Added regression coverage for this case:
test_search_file_matches_directory_glob_in_secondary_root
Verification:
.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
Problem:\n- secondary-root matches are displayed as @root[n]/...\n- pattern matching was performed against that display string, which breaks directory-aware globs like pkg/*.py\n\nChanges:\n- split search_file path handling into two stages:\n 1) compute root-relative path for matching\n 2) format matched path for output with optional @root[n]/ prefix\n- keep prefixed output contract for non-primary roots, while restoring correct glob behavior\n- add regression test covering directory glob matching in a secondary workspace root\n\nVerification:\n- .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\n- .venv/bin/pytest -q tests/unit/test_v4_file_tools.py
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Update the project-level TODO ledger after T0-6 was already merged via PR #181. This marks claim AG10 and task T0-6 as done, replaces the stale pending evidence with merged-code verification, and removes the resolved search_file regression from the current issue cluster so the board matches implementation reality.
docs(todos): sync T0-6 status after merged PR #181
1 participant
Summary
search_fileto return workspace-relative paths instead of absolute filesystem pathsWhy
tests/unit/test_v4_file_tools.py::test_search_file_finds_matching_pathsexpects relative pathsVerification
.venv/bin/pytest -q tests/unit/test_v4_file_tools.py::test_search_file_finds_matching_paths=> passed.venv/bin/pytest -q tests/unit/test_v4_file_tools.py=>16 passed