Skip to content

fix(ck-cli): don't reduce the search root past the operands' shared parent - #184

Open
ak2k wants to merge 1 commit into
BeaconBay:mainfrom
ak2k:fix/search-root-filesystem-walk
Open

fix(ck-cli): don't reduce the search root past the operands' shared parent#184
ak2k wants to merge 1 commit into
BeaconBay:mainfrom
ak2k:fix/search-root-filesystem-walk

Conversation

@ak2k

@ak2k ak2k commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

The bug

ck <query> docs/ notes.md — a directory operand plus a file beside it — roots the walk at / and traverses the whole filesystem. On current main:

mkdir -p /tmp/ckdemo/docs && cd /tmp/ckdemo
echo needle > docs/a.txt; echo needle > notes.md
ck needle docs notes.md      # hangs — walking /
ck needle notes.md docs      # instant (order-dependent)

Where / isn't writable it surfaces as Read-only file system (os error 30) instead — ck trying to index /. I found it after a ck process spent 4.5h walking a mounted cloud drive, downloading files as it went.

Cause

In find_search_root(), when the candidate is an ancestor of the current root, the second loop walks the candidate up looking for an ancestor contained by the (deeper) root — a condition that never holds, so it runs to /, which the trailing if root.starts_with(&candidate) then adopts.

Fix

Reduce to the operands' longest common path prefix instead of walking either path up one parent at a time. Both containment fast-paths are kept as short-circuits.

Scope

This makes the root the operands' actual shared parent; it doesn't bound how wide that parent may legitimately be. ck <query> /tmp/a /var/b still roots at /, because / genuinely is their shared parent — unchanged here, and pinned by a test. If you'd want disjoint operands handled differently (per-operand walks, or an error), that's a separate behavioral decision.

Verification

Six unit tests, including both operand orders and order-independence for N>2. Differential-tested against the old walk over 500k random operand sets: narrower in some, broader in none. cargo test (91), clippy, fmt clean. CHANGELOG + UNEXPECTED.md entries included.

Two things worth your call

  • Operands sharing no component (relative a/x + b/y, or differing Windows drives) reduce to nothing. I keep the current root so one operand is still covered, rather than the old fall-through to . which covered neither. The dropped operand is silent — happy to add a warning if you'd prefer.
  • The containment fast-paths are strictly redundant with the reduction (verified exhaustively). Kept as an optimization; happy to collapse the loop body if you'd rather have the smaller code.

@ak2k
ak2k force-pushed the fix/search-root-filesystem-walk branch from e7f5534 to 243904a Compare July 29, 2026 03:39
@ak2k ak2k changed the title fix(ck-cli): don't reduce the search root to / for unrelated operands fix(ck-cli): don't reduce the search root past the operands' shared parent Jul 29, 2026
@ak2k
ak2k force-pushed the fix/search-root-filesystem-walk branch 2 times, most recently from 80ac541 to f124d01 Compare July 29, 2026 03:59
…arent

`ck <query> docs/ notes.md` walks the entire filesystem. In
find_search_root(), when the candidate is an *ancestor* of the current
root, a loop walked that candidate up one parent at a time looking for an
ancestor contained by the root. With the root the deeper of the two, that
condition never held, so the walk ran to "/" and the root adopted it.

The effect depends on operand order (a directory operand first triggers
it) and shows up as an apparent hang, or as "Read-only file system
(os error 30)" when ck tries to create an index at /. It needs no unusual
setup -- two ordinary operands where one is a directory and the other
lives beside it rather than inside it.

Reduce to the longest common prefix of the paths' components instead, so
the root is the operands' actual shared parent. This narrows the root to
the shared prefix; it does not bound how wide that prefix may legitimately
be, so operands whose only shared component is "/" (`/tmp/a` and `/var/b`)
still root there, correctly. When the reduction is empty -- operands
sharing no component at all -- keep the current root, which covers one
operand, instead of falling through to the cwd fallback, which would cover
neither.

Six regression tests: the reported escape (both operand orders), the
prefix reduction itself (neither operand containing the other, shared
prefix deeper than "/"), the ordinary sibling case, the disjoint case that
legitimately shares only "/", the empty-reduction guard, and
order-independence across three operands. Adds CHANGELOG and UNEXPECTED.md
entries.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant