Skip to content

refactor(search): run POSIX unscoped scan as argv stages, no shell (#2634) - #2749

Open
tahodev wants to merge 6 commits into
DeusData:mainfrom
tahodev:argv-grep-find-2634
Open

tahodev wants to merge 6 commits into
DeusData:mainfrom
tahodev:argv-grep-find-2634

Conversation

@tahodev

@tahodev tahodev commented Oct 21, 2026

Copy link
Copy Markdown

Disclosure: this pull request was written with AI assistance on behalf of the account owner, who reviews and certifies every commit (DCO sign-off).

Summary

Implements the first phase of #2634 for the search pipeline: on POSIX, the unscoped code-search scan no longer builds a /bin/sh -c command string. Instead it runs as three supervised argv stages using the existing argv-based subprocess API:

  1. find <root> -type f [-name <file_pattern>] -print0 writes the NUL-delimited candidate list
  2. sort -z under LC_ALL=C orders the list in place
  3. grep -Hn -E|-F -f <pattern_file> -- <files...> scans it in chunks of 256 files per invocation

grep exit status 1 (no matches in a chunk) is an ordinary empty result, and an empty file list short-circuits to an empty success without spawning grep — the same contract the retired xargs wrapper encoded.

What does not change

  • Containment contract: the new mcp_run_argv_stage_cancellable_bounded runner reuses cbm_subprocess_spawn/cbm_subprocess_poll with the same cancellation latch, deadline handling, and cumulative output-limit latch as the existing shell runner, so cancel/deadline/oversized-output semantics are unchanged. find/sort failures fail closed, exactly like the old pipeline's status checks.
  • Scope: only the POSIX, unscoped, no-override scan path is routed to the argv stages. The Windows build and any user-configured command override keep the existing shell runner, so the test override hooks behave exactly as before.
  • No new system()/popen()/fork() calls — everything goes through the existing cbm_subprocess_* API, so the security allowlist is untouched.

The git helpers are left for a follow-up PR to keep this one under the project's size guidance.

Tested

  • build/c/test-runner mcp (ASan+UBSan, x86_64 Linux): 383 PASS / 0 FAIL, including the existing search_code_recursive_fallback_propagates_discovery_failures family (discovery/sort failure propagation, no-match success, empty discovery), which now runs through the argv stages.
  • Three new POSIX tests in tests/test_mcp.c, each on an unindexed project so the unscoped argv scan is exercised end to end:
    • a file whose name carries shell metacharacters (space, quote, backtick, $, ;) is discovered and searched literally;
    • a pattern with no matches returns success with "total_grep_matches":0;
    • file_pattern reaches find as a -name operand, so a non-matching extension is excluded and the same search without it matches.
  • Broader suites: the full make -f Makefile.cbm test run does not complete in my 2 GB sandbox — the ASan/UBSan process is OOM-killed partway through (4,350 tests passed, 0 failures at the kill point, across the mcp, cross_repo, index_resilience, fqn, route_canon, path_alias, watcher, lz4, zstd, sqlite_writer, artifact, scope, type_rep and go_lsp suites). No failure was observed in any suite that ran; I'm relying on CI for the complete run.

Not tested

  • Windows build (unchanged code path, still uses the shell runner).
  • Custom command overrides (unchanged code path).
  • scripts/lint.sh (no cppcheck/clang-format toolchain available in my environment).

Closes first phase of #2634.

…eusData#2634)

Signed-off-by: tahodev <85001874+tahodev@users.noreply.github.com>
@tahodev
tahodev requested a review from DeusData as a code owner October 21, 2026 09:02
@github-actions

Copy link
Copy Markdown

Thanks for opening this — it has been seen, and it is queued.

This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence.

Current review status: working through a backlog. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

  • It will not be closed for inactivity. No stale bot touches pull requests here.
  • It may still sit a while before a human reads it. That is on us, not on you.
  • Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

  • Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
  • Get CI green, or say which failures you believe are pre-existing.
  • Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
  • Every commit needs a sign-off (git commit -s) — CI enforces DCO.

If this fixes a bug, a reproduction we can run is worth more than a description of the symptom.

Thanks for contributing, and sorry in advance for the wait.

@DeusData DeusData added enhancement New feature or request security Security vulnerabilities, hardening labels Oct 25, 2026
@DeusData

DeusData commented Nov 1, 2026

Copy link
Copy Markdown
Owner

Thank you, @tahodev, for this second hardening step and for the clear write-up. We have made the direction call: we want the argv-based helpers, so the search path no longer goes through /bin/sh at all - defence in depth on top of the validator that already guards the shell path.

One change to the shape, so we do not end up with a third scanning mechanism: instead of running grep in chunks of 256 files, please feed the file list (the find / sort output) into the existing in-process scanner, search_scan_in_process, so matching keeps one implementation. The git helpers can follow the same pattern in the next step, as you planned.

Two small things the linters will ask for anyway:

  1. clang-format on the new hunks (Homebrew LLVM clang-format, make -f Makefile.cbm lint-ci shows them).
  2. The new raw allocations should go through the memory core (make -f Makefile.cbm lint-memory-core lists them), so the baseline does not grow.

Main has also moved since your last push, so a merge of current main will be needed. If you would rather we make these changes ourselves on your branch, just say so here; maintainer edits are enabled and we are happy to push them with an explanation of each commit. Thanks again for pushing this forward.

…of chunked grep

Signed-off-by: tahodev <85001874+tahodev@users.noreply.github.com>
Signed-off-by: tahodev <85001874+tahodev@users.noreply.github.com>
@tahodev
tahodev force-pushed the argv-grep-find-2634 branch from 5ba6a81 to 9a0742e Compare November 1, 2026 20:29
tahodev and others added 2 commits November 2, 2026 07:25
Signed-off-by: tahodev <85001874+tahodev@users.noreply.github.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
@DeusData

DeusData commented Nov 3, 2026

Copy link
Copy Markdown
Owner

Thank you, @tahodev, for turning this around so quickly and for taking the in-process route exactly as we asked. Feeding the find / sort list straight into search_scan_in_process means indexed and unindexed roots now share one matcher, and the POSIX search path no longer runs a shell at all. The memory-core count for src/mcp/mcp.c did not grow, and you even tightened its baseline to the real count.

Main had changed src/mcp/mcp.c since your last push, so we merged current main into your branch (no conflicts) to get a fresh run on the combined tree. Please pull the branch before pushing again so the merge commit is kept. We will merge once the required checks are green. Thanks again - the git helpers are very welcome in the same shape whenever you get to them.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request security Security vulnerabilities, hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants