fix: include .hermes/cache in search_files results - #72473
Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing both the ripgrep and find implementations. The reported behavior is present on current main, but the current patch over-broadens the security exception.
Problems
tools/file_operations.py:2234adds globalrg --files --hidden; the two negation globs only omit.gitand.hub, so every other hidden directory becomes searchable. Current main deliberately keeps hidden descendants excluded by default (tools/file_operations.py:2130-2146), following the #1558 regression (tests/tools/test_search_hidden_dirs.py:1-13; commit7d91b436e4).- The new tests invoke standalone
find/rgcommands rather thanShellFileOperations._search_files, unlike the existing real fallback coverage intests/tools/test_file_operations.py:394-451.
Suggested changes
- Keep default hidden-directory exclusion and introduce a narrow, path-aware allowance for the intended cache location only.
- Test both actual backends and assert an unrelated hidden directory remains absent.
Automated hermes-sweeper review.
| # Exclude dangerous hidden dirs (.git, .hub) while including | ||
| # trusted ones like .hermes/cache/. | ||
| exclude_globs = ( | ||
| f"-g {self._escape_shell_arg('!.git/')} " |
There was a problem hiding this comment.
--hidden makes every hidden directory searchable, not just .hermes/cache; excluding only .git and .hub weakens the default hidden-directory exclusion introduced for #1558. Please use a narrowly scoped allowance and retain exclusion for unrelated hidden descendants.
Address review feedback from teknium1 on PR NousResearch#72473: - find: prune ALL hidden dirs except .hermes (not just .git/.hub) so unrelated hidden dirs like .secretstuff remain excluded - rg: add .hermes/cache as explicit search path instead of --hidden with broad negation globs, keeping default hidden-dir exclusion - Tests: add .secretstuff fixture to verify unrelated hidden dirs stay excluded; test both find and rg backends with actual _search_files behavior; 14 tests, all passing This is a path-aware allowlist approach: only .hermes/cache/ is explicitly permitted, all other hidden directories remain excluded by default, preserving the NousResearch#1558 security fix.
621531e to
2ebc5dd
Compare
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Summary
Nine PRs address or reference the hidden-directory search complex. #18487, #18589, #18645, and #34017 modify the unresolved grep/content path; #16672 and #19878 cover the analogous file-name fallback, #41135 and #42872 cover Windows test portability, and #72473 proposes a broader file-search policy that does not fix the reported grep cause.
Related pull requests
- #16672 [closed]
related— (+97/-5) — reference implementation: Fixes only the target='files' find fallback by allowing an explicit hidden root and post-filtering hidden descendants, leaving _search_with_grep unchanged. Although closed, it remains relevant because its implementation was salvaged into #19878 and provides the root-relative filtering model needed by the content fix. - #18487 [closed]
partial— (+20/-1) — partial duplicate: Avoids grep root self-exclusion by omitting --exclude-dir='.*' for a hidden search root, but provides no replacement filtering for hidden descendants and no regression tests. Although closed by its author, it remains the earlier implementation that #18589 was closed in favor of. - #18589 [closed]
best fix— (+96/-4) — verify-selected partial base: Implements the grep bypass and adds focused command-construction coverage for hidden, visible, relative, and dot-only roots, but does not preserve hidden-descendant exclusion with a live result test. Although closed as a duplicate of #18487, it remains relevant because its regression cases are stronger than #18487's untested diff. - #18645
partial— (+11/-3) — duplicate with blockers: Combines the grep bypass with find work already implemented by merged #19878, misclassifies '.' and '..' as hidden, and exposes hidden descendants beneath an explicit hidden root. Despite the keep_open review on #18645, its diff has no unique salvage advantage over #34017 and retains the contributor-identified path-component defect. - #19878 [merged]
related— (+97/-5) — merged reference implementation: Implements the target='files' half by permitting an explicit hidden root while filtering hidden descendants relative to it; it does not change the affected grep/content path. It remains relevant as the merged model for the missing descendant filtering, and contributor discussion explicitly confirms that #18473 remained unresolved. - #34017
best fix— (+74/-1) — verify-selected salvage base: Directly patches the current grep fallback and correctly excludes '.' and '..' from hidden-root detection, but simply dropping --exclude-dir makes .hub, .git, and other hidden descendants searchable; its command-only tests cannot detect that regression. Consistent with its keep_open review, retain it only with root-relative descendant filtering and a live forced-grep regression. - #41135 [closed]
related— (+12/-2) — separate duplicate: Changes only Windows compatibility in test_search_hidden_dirs.py by using shutil.which and skipping POSIX-only tests, without affecting the grep failure. Although closed, it remains relevant as the near-identical predecessor of #42872. - #42872
related— (+6/-2) — separate test fix: Replaces collection-time POSIX which calls with shutil.which and skips Unix find/grep tests on Windows, but does not modify search behavior. Consistent with the automated keep_open verdict, keep this independent from #18473 while recognizing that it substantially duplicates closed #41135. - #72473
related— (+135/-20) — separate policy change with salvage path: Changes target='files' to traverse hidden directories globally and exclude only .git and .hub, rather than repairing grep self-exclusion for an explicitly hidden root. Its keep_open review identifies the required salvage: narrowly allow the intended .hermes/cache location, retain exclusion for unrelated hidden descendants, and test the actual backends.
Duplicates
#18487, #18589, #18645, and #34017 share the same core grep hidden-root bypass, with #18645 additionally duplicating find work already merged through #19878; #16672 was directly salvaged as #19878. Separately, #41135 and #42872 substantially duplicate the Windows test-portability change. #72473 is not a duplicate of the #18473 fixes.
Suggested consolidation
Keep #34017 open with a salvage path: combine its current-main grep change with the useful regression cases from verify-selected #18589, preserve hidden-descendant exclusion relative to the explicit hidden root using the #19878 pattern, and add a live forced-grep test that finds visible content while excluding .hub/index-cache and another hidden descendant. Keep #18487 and #18589 closed as duplicate historical implementations; close #18645 as duplicate of #34017 despite its keep_open review because its diff also duplicates merged #19878 code, misclassifies '.' and '..', and does not solve the descendant-filtering blocker. Handle #42872 separately in accordance with its automated keep_open verdict, leave #41135 closed as its duplicate, and keep #72473 open only with the contributor-requested narrow .hermes/cache allowance rather than its current global hidden-directory traversal.
Cross-PR triage: Reviewed 9 pull requests and 1 issue in this complex. Diffs were read for 8 of 9 PRs (rest unavailable); Assessment working set: 32 kB of PR diffs, 18 kB of issue/PR text, 10 kB of discussion (22 comments), 6 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.
|
功能已由 #19878 合并实现(main 上 tools/file_operations.py 的 has_hidden_path_ancestor + 显式隐藏根搜索 + 隐藏后代过滤,L2351-2407)。本 PR 范围已被覆盖,关闭以避免重复。感谢审阅。 |
Problem
search_fileswithtarget='files'skips all hidden directories by default. This makes files in.hermes/cache/(Hermes's own cache, where user-sent files are stored) invisible when searching from parent directories.Root Cause
rg --filesexcludes hidden directories by default (no--hiddenflag)-not -path '*/.*'blanket-excludes every path containing a dot-directoryFix
Both backends now exclude only dangerous hidden directories (
.git/,.hub/) while allowing trusted ones like.hermes/cache/:_search_files_rg: Added--hiddenflag with-g '!.git/'and-g '!.hub/'negation globs_search_files(find fallback): Replaced-not -path '*/.*'with targeted-prunefor.gitand.hubdirectories onlySecurity
The #1558 security fix (adversarial content in
.hub/index-cache/) is preserved:.hub/directories are still excluded by both rg and find paths.git/directories are still excluded.hermes/cache/is now searchableTests
Updated
tests/tools/test_search_hidden_dirs.pywith:.hermes/cache/directory in the test fixtureTestFindExcludesHiddenDirs::test_find_includes_hermes_cache_files— verifies.hermes/cache/files are found by the new find prune patternTestRipgrepHiddenWithExclusionsclass — 5 tests verifyingrg --files --hiddenwith.git//.hub/exclusions includes.hermes/cache/while excluding dangerous dirsAll 13 tests pass.