Repository navigation
fix: release complete editor synonym search in v1.21.19 - #59
Closed
AlexMikhalev wants to merge 9 commits into
Closed
AlexMikhalev wants to merge 9 commits into
AlexMikhalev wants to merge 9 commits into
Conversation
…ilePicker call site (Refs #293) fff-search 0.8.4 maps file content through `memmap2::Mmap::map` exactly once, in `FileItem::get_cached_content` (`src/types.rs:640`). That call's safety contract requires the mapped file not be modified concurrently, and the crate delegates the guarantee to a background watcher -- but `FilePicker::new` never installs one (`background_watcher: None`; the `watch` field is only consulted by `new_with_shared_state`), and all four call sites in this workspace use `FilePicker::new`. Every workspace grep therefore mapped files with no invalidation. Observed consequences (UF-039, CONFIRMED_UB by experiment EXP-104): with a concurrent same-length writer, 440 invalid-UTF-8 `line_content` strings out of 186M lines; and an in-place truncation during a grep kills the process with SIGBUS -- EXP-302 killed the shipped `terraphim_mcp_server --sse` with exit 135. The remedy is an explicit `ContentCacheBudget` whose `max_files` is 0, so `get_cached_content` returns `None` at its count guard before reaching `absolute_path` or `Mmap::map`, and `get_content_for_search` falls through to the slow path which reads each file into an owned buffer. Owned bytes cannot be mutated by another process, so both arms of the finding are gone. On Windows `get_cached_content` is already a stub returning `None`, so this makes Unix behave as Windows already does. `max_file_size` deliberately stays at 10 MiB. It also gates the slow path -- `get_content_for_search` rejects any file whose `size > max_file_size` -- so `ContentCacheBudget::zero()` or `max_file_size: 0` would silence grep entirely while looking like a working fix. `from_overrides` cannot express `max_files: 0` either (it reads 0 as "library default"), hence the struct literal. The value is hoisted into one constant per crate and shared with `GrepSearchOptions` so the two cannot drift. `follow_symlinks: false` is now stated at all four sites rather than inherited from `FilePickerOptions::default()`, so reasoning about which entries get indexed rests on an asserted value. `terraphim_mcp_server` does not depend on `terraphim_grep`, so the helper is duplicated rather than shared; coupling two binaries for eight lines is not worth it. A new `ci_guards` test walks `crates/**/*.rs` with `std::fs` (the Gitea runner's program allowlist rejects anything that is not `cargo` or a repo script) and asserts no `cache_budget: None` remains and that every `FilePicker::new` names `no_mmap_budget()` and `follow_symlinks: false` in its options block. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WjGks4DGYdDw3QLgu32S96
…names before fff-search indexes them (Refs #296) fff-search stores every relative path as `to_string_lossy().into_owned()`. On Linux each invalid byte in an entry name becomes a three-byte U+FFFD, so a hostile name inflates threefold. `ChunkedString::read_to_buf` (`src/simd_path.rs:170-186`) then copies `byte_len.min(buf.len())` bytes into a 4096-byte buffer and returns `core::str::from_utf8_unchecked(&buf[..total])` -- so the cut lands inside a replacement character and the returned `&str` is not valid UTF-8. The path runs through `FileItem::absolute_path` (`src/types.rs:271-275`) from `get_content_for_search` and from `collect_files`' git-status lookup, so it is taken for every file. UF-040, CONFIRMED_UB by experiment EXP-201: invalid-UTF-8 `PathBuf`/`str` values in five of seven hostile geometries, plus `DirItem::dir_name` panicking with "start byte index 10724 is out of bounds for string of length 4096". The base path is attacker-chosen through `terraphim-grep --paths` and the MCP tools' `path` argument. This is not covered by the zero-file cache budget from #293: that budget stops `get_cached_content` before it maps, but `get_content_for_search` then falls through to the slow path and still calls `absolute_path` on every file, and `collect_files` calls it before any grep runs. fff-search 0.8.4 offers no path-length cap, no strict-UTF-8 mode and no non-lossy storage switch -- all three verified absent from `FilePickerOptions`' seven fields -- so the remedy has to sit in front of the library. A pre-walk rejects the tree when an entry name is not valid UTF-8, or when a path relative to the search root exceeds `PATH_BUF_SIZE - 1` bytes. The second rule is a deliberate conservative extension covering deep trees of *valid* multi-byte names, which truncate mid-character by the same mechanism; EXP-201 did not test that geometry, so it is reasoned from `read_to_buf` rather than observed. The walk reads names only -- `DirEntry::file_type` uses the directory entry's type, with no `stat` -- and does not descend into symbolic links, matching the `follow_symlinks: false` the pickers now assert. An unreadable directory is skipped rather than reported, so an ordinary permission quirk does not become a refused search; `find_files_invalid_path_returns_error` still exercises fff-search's own message for a nonexistent base path. Placed as `terraphim_grep::path_guard::reject_hostile_tree`, called at the head of `HybridSearcher::search_code` before `FilePicker::new` (covering the CLI and every library caller), and as a private copy in `terraphim_mcp_server` called at the head of `find_files`, `grep_files` and `multi_grep_files`, surfacing as `ErrorData::invalid_params`. The duplication is deliberate: `terraphim_mcp_server` does not depend on `terraphim_grep`. Residual risks, documented at both copies: a TOCTOU window remains, since an entry created between the pre-walk and `collect_files` still reaches the inflation -- that needs an active concurrent writer inside the searched tree, whereas the confirmed exposure is a static hostile checkout. The walk also does not honour `.gitignore`, so `target/` and `node_modules/` are walked; both callers already pay a full `collect_files` scan immediately afterwards. Scope note, also recorded in the code: the #293 budget removes *fff-search's* mapping, not all mapping in the process. `FrecencyTracker::open` still memory-maps an LMDB data file and libgit2 maps pack files. Neither commit should be read as "this process no longer mmaps". Tests use real directories, no mocks. The hostile cases build six nested directories each named 250 raw `0xFF` bytes via `std::os::unix::ffi::OsStrExt` (1500 raw bytes, 4500 once inflated) and assert both `reject_hostile_tree` and the production entry points refuse them, naming the offending entry. They carry `#[cfg_attr(not(target_os = "linux"), ignore = ...)]` rather than `#[cfg]` so their bodies still compile and lint on this workspace's macOS hosts -- APFS refuses non-UTF-8 entry names, so they cannot run there. `plain_tree_still_greps` is the control that matters: it plants a match in an ordinary tree and asserts grep returns it end-to-end through `HybridSearcher::search`. That is what distinguishes the #293 remedy from the trap of zeroing `max_file_size`, which would also close the slow read path and return zero matches while looking like a working fix. The `ci_guards` arm now also requires a `reject_hostile_tree(` call within the 40 lines preceding every `FilePicker::new(` under `crates/`. The per-entry decision is split into `check_entry` in both copies so the two rules can be tested without asking the host filesystem for a geometry it refuses. That matters for rule (b): macOS caps a path at 1024 bytes and Linux at 4096, and `std::fs` passes absolute paths, so no tree whose *relative* path exceeds 4095 bytes can be created through it -- the first attempt at a filesystem-based test failed with ENAMETOOLONG. Rule (b) is therefore a backstop for a future fff-search that stores paths differently, not something a real tree reaches today, and the unit tests say so. Rule (a) is unit-tested through `check_entry` as well, which gives it coverage on macOS, where the integration test cannot run. `plain_tree_still_greps` also plants the needle in a 64 KiB file, not only a small one. `get_cached_content` returns `None` for anything below `MMAP_THRESHOLD` (16 KiB on aarch64, 4 KiB elsewhere) *before* it consults the budget, so a small file takes the slow path with or without the #293 fix and a control built only on one proves nothing. The large file is the geometry the zero-file budget actually diverts: threshold guard passes, `0 >= max_files` returns `None` without mapping, slow path reads it into an owned buffer, needle still matches. Setting `MAX_GREP_FILE_SIZE` to 0 was confirmed to turn this test red, so it does catch the trap it is there for. The MCP test asserts on `ErrorData::message` rather than `Display`: it can never run on macOS, so a wrong guess about rmcp's formatting would surface only as a red Linux CI. The length rule is applied to the base path and the relative path together, because fff-search spends them from one budget. `FileItem::write_absolute_path` (`src/types.rs:337-357`) writes the base path, a separator and the relative path into a single `[u8; PATH_BUF_SIZE]`, lets `read_to_buf` truncate the relative portion to whatever space is left, and returns the result through `from_utf8_unchecked`. Two consequences follow, and the first version of this guard, which measured only the relative portion, missed both: - A long search root leaves little room for the relative portion, so the truncation cuts a valid multi-byte character in half. No hostile entry name is needed; EXP-201's geometry is sufficient but not necessary. This is reachable on Linux with ordinary names -- a root of roughly 3000 bytes and relative paths of roughly 1200 each keep every absolute path inside `PATH_MAX` while their sum clears the 4096-byte buffer. - A root over 4096 bytes makes `buf[..base_len].copy_from_slice(base)` panic outright rather than truncate, which is why that case gets its own error text. So `base_prefix_len` now reproduces `write_absolute_path`'s own arithmetic (the root's encoded bytes plus one separator unless it already ends in `MAIN_SEPARATOR`), a root at or over the buffer is refused before the walk begins, and the per-entry rule is `base_end_idx + rel_len >= PATH_BUF_SIZE`. The error apportions the two contributions, because a user whose relative paths are plainly short would otherwise find the refusal baffling, and suggests searching from a shorter root. `>=` rather than `>` keeps the byte `write_relative_cstr` reserves for its NUL terminator (`types.rs:372`). The reachability note in the earlier revision of this branch -- that the length rule was a backstop no real tree could reach, because `std::fs` passes absolute paths and `PATH_MAX` is 1024 on macOS and 4096 on Linux -- was wrong for exactly this reason, and is retracted. It holds only for the relative portion in isolation. Four unit tests cover the arithmetic, and unlike the hostile-tree integration tests they run on macOS: `base_prefix_len` against the library's `sep_len` logic, an over-long root refused before any `read_dir` (so no unbuildable tree is needed), a long root with short ordinary ASCII names refused, and the `PATH_BUF_SIZE - 1` boundary accepted with one byte more refused. A search root whose own path is not valid UTF-8 is refused before anything else, because otherwise the length arithmetic above measures a different string from the one fff-search receives. `terraphim_grep`'s call site passes `base_path: search_path.to_string_lossy().to_string()`, which replaces each invalid byte with a three-byte U+FFFD, so such a root is stored up to three times longer than `base_prefix_len` counted -- enough to let an overflowing tree through, and enough for the inflated root alone to panic `buf[..base_len].copy_from_slice(base)` with no relative path involved. The root is never an entry the walk discovers, so the per-entry UTF-8 rule did not cover it. Refusing is the right answer irrespective of the arithmetic: the lossily stored name no longer refers to the directory on disk, so fff-search could not open anything beneath it. That is the same silent-drop defect the audit recorded for hostile entry names, and a named error beats searching and finding nothing. Reachable through `terraphim-grep --paths` and `HybridSearcher::with_search_paths`, since clap builds `Vec<PathBuf>` from `OsString` and invalid bytes survive on Linux. The MCP tools are immune -- their `path` arrives as JSON and is therefore already valid UTF-8, and the `String` handed to fff-search is byte for byte the one the guard checked -- but both copies carry the check so neither depends on where it was called from. `base_prefix_len`'s doc records that its raw-byte count matches what fff-search receives only because this check has already run. An audit of both call sites confirms `base_path` is the only path-ish input field of `FilePickerOptions`, and that no other differently-encoded copy of a path reaches the library: `args.paths` passes through to `search_path` unchanged, and the `relative_path` values in the MCP tools are outputs. Two further unit tests, both running on macOS: a non-UTF-8 root is refused (needing no on-disk tree, since the check precedes any `read_dir`), and a valid root containing Cyrillic, Japanese and multi-byte Latin characters is still accepted -- the check rejects an invalid root, not a non-ASCII one, and the reverse would be a real regression for users with non-Latin paths. A note on the `>=` in the length rule, since an earlier revision of this branch justified it wrongly and the wrong reason invites a future maintainer to loosen the rule while "fixing" the comment. `write_absolute_path`'s exact truncation threshold is `base_end_idx + rel_len > PATH_BUF_SIZE`: the relative portion is read into `&mut buf[base_end_idx..]` and `read_to_buf` takes `byte_len.min(buf.len())`, so a total of exactly `PATH_BUF_SIZE` fits and nothing is cut. The NUL byte `write_relative_cstr` reserves comes out of that function's own buffer (`types.rs:366-375`) and does not shrink `write_absolute_path`'s, so the earlier claim that `>=` was needed to preserve it was simply incorrect. `>=` is kept regardless, deliberately. One rule then also covers `write_relative_cstr`, whose threshold is `rel_len >= PATH_BUF_SIZE`, and which `base_end_idx >= 0` makes a subset of this condition -- no second branch needed. It also keeps the guard off the exact dependency boundary, which would otherwise have to be re-derived on every fff-search upgrade. The cost is that an absolute path summing to exactly `PATH_BUF_SIZE` bytes is refused although `write_absolute_path` alone would handle it; that over-rejection is intended, and both the code comment and `total_length_boundary_is_exact`'s doc now say so, the latter so the test is not read as pinning the library's own threshold. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WjGks4DGYdDw3QLgu32S96
… (Refs #293, #257)
Both dependent crates declared `fff-search = { version = "0.8.4" }`, i.e.
`^0.8.4`. Both now declare `=0.8.4`, and a `ci_guards` test asserts the exact
pin in each manifest.
fff-search 0.10.6 is the first release requiring a git2 that patches
RUSTSEC-2026-0183/0184, but it also adds a SECOND, unbudgeted
`memmap2::Mmap::map` at `types.rs:762` in `get_content_for_search`, gated only
by a 256 KiB threshold on Linux and reached exactly when a zero-file cache
budget makes `get_cached_content` return `None`. Upgrading would therefore
reintroduce UF-039 / #293 for every file at or above that size while leaving the
mitigation visibly in place -- the worst possible failure mode for a fix whose
whole point is that nothing maps any more.
Staying on 0.8.4 is the audit's decision (Delta 5, scored 19 against 12/10/8)
and is safe because both advisories are API-specific and neither vulnerable API
is reachable: fff-search 0.8.4 references only `git2::Repository`, `git2::Error`
and the `git2::Status` flags, with no `Remote`, `Blame` or `blame_buffer`
anywhere, and no workspace crate depends on git2 directly.
The urgency, stated accurately: `^0.8.4` already confined cargo to 0.8.x, and
crates.io has no stable 0.8.x above 0.8.4 -- only `0.8.5-nightly.*`
pre-releases, which cargo will not select. A routine `cargo update` therefore
cannot reach 0.10.6 today. The pin guards against a deliberate minor bump, and
it is what stops the reachability argument above lapsing silently. The earlier
claim that a routine update would reopen the finding was retracted during the
audit and is not repeated here.
`Cargo.lock` is unchanged: the exact requirement matches what was already
resolved.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WjGks4DGYdDw3QLgu32S96
AlexMikhalev
commented
Oct 11, 2026
AlexMikhalev
left a comment
AlexMikhalev
left a comment
Contributor
Author
There was a problem hiding this comment.
AI-assisted independent review of source 2d1820e: the six source commits preserve the canonical release/MCP changes and 32,000-token bound while porting the reviewed Gitea PR #385 fixes. Both reproduced synonym findings are addressed: complete exact membership is separate from display caps, and one literal multi_grep batch handles graph data while compound user input retains filters.
Canonical-source verification: 73 library and 9 real CLI/file integration tests pass; two hostile-name cases require Linux. Strict Clippy across all grep targets/features, formatting, and 57 release/CI/package-metadata contracts pass locally. Gitea's complete workspace build/tests passed, with its final tooling guard currently blocked by missing installed coverage-tool executables on the runner.
Merge is not yet recommended: exact-head whole CI and package metadata are pending. The packaging contract requires actual signed archive checksums, so stage-only run 38181077940 is producing the immutable v1.21.19 artifacts. A package-only follow-up must pin those exact hashes, regenerate .SRCINFO and pass the package and full Rust checks. The tag will not be moved. No check is disabled or bypassed.
AlexMikhalev
had a problem deploying
to
tsm-production-release
October 11, 2026 22:54 — with 
GitHub Actions
Failure
GitHub Actions
Failure
AlexMikhalev
commented
Oct 11, 2026
AlexMikhalev
left a comment
AlexMikhalev
left a comment
Contributor
Author
There was a problem hiding this comment.
Independent AI-assisted review of df10eca: clear. The only changes after the immutable v1.21.19 source are PKGBUILD, generated .SRCINFO and its checksum contract. All four pinned MUSL archive hashes match signed sealed artifact 11719320613 from successful run 38182446017; the tag still resolves to 2d1820e.
Actual Arch validation passed: seven contract tests with no skipped makepkg check; unchanged verify.sh built and linted both x86 split packages, installed them, checked package integrity (zero altered files), executed both version commands at 1.21.19, verified receipts, and removed them. All four x86/ARM source hashes were checked. This used the exact staged archives offline; public URL availability must still be checked after publication.
The staged Mac ARM64 grep also passed actual editor-to-search acceptance: original graph gives zero Concern matches; the editor-exported Risk/Concern graph retrieves three exact source passages; explicit --answer returns those same passages with source-grounded citations and does not claim the recorded decisions are completed. All 17 archive signatures/checksums/layouts, nine canonical Linux payloads, and eight Mac notarizations were verified. Required full CI remains a separate gate.
AlexMikhalev
added a commit
that referenced
this pull request
Oct 11, 2026
Integrate the independently reviewed source and package metadata from PR #59 with verified commit signing. The complete tree is identical to df10eca. Immutable v1.21.19 remains at 2d1820e; only Arch package metadata and its contract differ from that tag. Refs https://git.terraphim.cloud/terraphim/terraphim-clients/pulls/385
AlexMikhalev
added a commit
that referenced
this pull request
Oct 12, 2026
Integrate the independently reviewed source and package metadata from PR #59 with verified commit signing. The complete tree is identical to df10eca. Immutable v1.21.19 remains at 2d1820e; only Arch package metadata and its contract differ from that tag. Refs https://git.terraphim.cloud/terraphim/terraphim-clients/pulls/385
Contributor
Author
|
Superseded by merged PR #60. Its GitHub-verified signed commit preserves the exact reviewed tree (3311224407d963ac554f93412219e1aaa95f9483); all six executable checks passed. The immutable v1.21.19 release tag remains unchanged. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Exact editor-exported synonyms now retrieve every equivalent term as literal text, independently of display limits. Requested
--answersynthesis uses the retrieved evidence and preserves decision status; compound user queries retain their filters.This ports the reviewed Gitea PR #385 changes onto current GitHub main, preserving v1.21.18 release fixes and the 32,000-token bound. It also carries the reviewed fff-search pin, mmap-cache refusal and filesystem-name guards. Those guards remain a partial mitigation for upstream unsafe filesystem behavior.
Prepare v1.21.19 and add real synonym/answer/hostile-tree CLI regressions to GitHub CI. Local canonical validation: 73 library tests and 9 integration tests pass; two Linux-only hostile-name tests are deferred to this PR's Linux CI. Formatting and 22 release/CI/package-metadata contracts pass. The three Linux archive contract cases cannot execute on macOS because their fixture requires
/bin/true; the complete contract lane runs on Linux.Independent AI review reproduced and cleared both synonym edge-case blockers. Publication will use the existing immutable staging and promotion workflows, followed by fresh downloaded-editor and downloaded-grep verification. Zed fork work remains separate.
Gitea source: https://git.terraphim.cloud/terraphim/terraphim-clients/pulls/385