Skip to content

fix: keep file-search count aligned with accessible results#754

Open
grtninja wants to merge 3 commits intoamd:mainfrom
grtninja:codex/issue-594-file-search-contract
Open

fix: keep file-search count aligned with accessible results#754
grtninja wants to merge 3 commits intoamd:mainfrom
grtninja:codex/issue-594-file-search-contract

Conversation

@grtninja
Copy link
Copy Markdown

Summary

  • keep file-search count consistent with the number of files the tool actually returns
  • keep the SSE file-list payload total consistent with the list actually included in the event
  • add focused regression coverage for both the tool result and the UI event packaging

Why

Fixes #594. The current contract can report more file-search results than the tool payload and UI event actually expose, which makes the UI claim files that are not accessible from the returned list.

Validation

  • PYTHONPATH=src python -m pytest -q tests\\unit\\test_file_tools.py::test_search_file_count_matches_returned_files
  • PYTHONPATH=src python -m pytest -q tests\\unit\\chat\\ui\\test_sse_handler.py -k file_list_limited_to_20

Risk Notes

  • narrow result-contract fix only
  • no auth, routing, or model behavior changes
  • no overlap with the active ChatView/code-index PR surfaces

@github-actions github-actions bot added agents Agent system changes tests Test changes labels Apr 11, 2026
@kovtcharov
Copy link
Copy Markdown
Collaborator

kovtcharov commented Apr 11, 2026

@claude review PR.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 11, 2026

Claude finished @kovtcharov's task —— View job


PR Review: LGTM 🟢

Summary: Clean, minimal fix that correctly aligns count/total fields with the actual returned file lists.


Code Review

file_tools.py

  • Correctly introduces limited_files = matching_files[:10] and uses it consistently for files, file_list, count, and display_message
  • Applied in both the quick search (line 333) and deep search (line 388) code paths
  • Comment clarifies intent without being verbose

sse_handler.py

  • Same pattern: limited_files = files[:20] before building the event
  • total now reflects the actual payload size rather than data.get("count", ...) which could differ
  • Clean, minimal change

Tests

  • test_search_file_count_matches_returned_files - Good regression test that creates 25 files, ensures 10 are returned, and validates count == 10
  • test_file_list_limited_to_20 updated assertion - Properly reflects the new contract

🟢 Minor Observations (non-blocking)

  1. Loss of "total found" visibility: The UI no longer knows how many files were actually found (e.g., "showing 10 of 25"). If this is desired UX, consider adding a separate total_found or truncated: true field in a follow-up.

  2. Test fixture scope: search_file_fn uses scope="module" but modifies global _TOOL_REGISTRY. This works but could cause test coupling if other tests in the same module expect a clean registry. Consider scope="function" for isolation, or document the intentional sharing.


Verdict: Approve. The fix is correct, well-tested, and addresses the contract violation described in #594.

@kovtcharov kovtcharov enabled auto-merge April 11, 2026 18:28
@kovtcharov
Copy link
Copy Markdown
Collaborator

Lint is failing on a Black formatting issue — one function signature is too long. Here's the fix:

-def test_search_file_count_matches_returned_files(search_file_fn, tmp_path, monkeypatch):
+def test_search_file_count_matches_returned_files(
+    search_file_fn, tmp_path, monkeypatch
+):

In tests/unit/test_file_tools.py line 759. You can also just run python util/lint.py --black --fix to auto-fix it.

@kovtcharov kovtcharov disabled auto-merge April 11, 2026 18:29
@github-actions github-actions bot added the devops DevOps/infrastructure changes label Apr 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agents Agent system changes devops DevOps/infrastructure changes tests Test changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

File search results capped at ~10 files despite reporting 32 found

2 participants