Skip to content

discover: a symlink must not abort a bounded count (any repo with a symlink can never auto-index) - #1803

Open
lgerard42 wants to merge 1 commit into
DeusData:mainfrom
lgerard42:fix/symlink-aborts-bounded-count
Open

discover: a symlink must not abort a bounded count (any repo with a symlink can never auto-index)#1803
lgerard42 wants to merge 1 commit into
DeusData:mainfrom
lgerard42:fix/symlink-aborts-bounded-count

Conversation

@lgerard42

Copy link
Copy Markdown

The bug

safe_stat reports a symlink as CBM_NOT_FOUND by policy (src/discover/discover.c, the S_ISLNK branch and the Windows reparse-point branch). walk_dir_process_entry then turns any CBM_NOT_FOUND into out->failed when count_only is set, which aborts the entire walk.

cbm_discover_count_bounded is what auto-index admission calls, so any repository containing any symlink can never auto-index. maybe_auto_index logs reason=unsafe_or_unavailable_path files=-1 and returns.

The label misleads in two directions, which is what cost me the time: nothing is unsafe, and the 5,000 ms count deadline is not involved either — a full find over the repository I hit this on takes 26 ms. The count simply refused at the first symlink it met.

Explicit index_repository is unaffected: count_only is false there, so the same entry is skipped rather than fatal. That asymmetry is what makes it confusing in practice — the repository indexes perfectly when you ask for it by hand, and silently never indexes itself.

Reproduction

  1. git init a scratch repo with one .js file, start an MCP session in it → auto-indexes, project appears in list_projects.
  2. delete_project, add ln -s sub linkdir, commit, start a session again → daemon.autoindex.skipped ... reason=unsafe_or_unavailable_path files=-1, no project.

Nothing else changed between the two runs.

The fix

safe_stat gains an is_link out-parameter so the caller can tell "skipped by policy" from "could not observe". A count skips symlinks and still aborts on an entry it genuinely cannot observe, which preserves the conservative behaviour the admission guard wants.

Test

discover_count_bounded_survives_a_symlink in tests/test_discover.c: a repo with two files and one symlink must give CBM_DISCOVER_OK and count 2. Guarded with #ifndef _WIN32 because it calls symlink().

Verified in this tree with HOME=$(mktemp -d) scripts/test.sh --suites discover:

  • without the one-line change: 113 passed, 1 failed (tests/test_discover.c:1473)
  • with it: 114 passed

Found while vendoring this project — a Formula -> pkg/homebrew/Formula symlink in the tree was enough to trip it.

safe_stat reports a symlink as CBM_NOT_FOUND by policy, and
walk_dir_process_entry turns any CBM_NOT_FOUND into out->failed when count_only
is set. One symlink anywhere in a tree therefore aborts the whole count.

cbm_discover_count_bounded is what auto-index admission calls, so any repository
containing any symlink could never auto-index: maybe_auto_index logs
reason=unsafe_or_unavailable_path files=-1 and returns. The label is misleading
too — nothing is unsafe, and the walk is not slow; the count simply refused at
the first symlink it met. Explicit index_repository is unaffected, because
count_only is false there and the same entry is skipped rather than fatal.

safe_stat now reports whether the entry was a symlink. A count skips symlinks and
still aborts on an entry it genuinely cannot observe, which is what the admission
guard wants.

Reproduced before fixing: a scratch git repository auto-indexed successfully,
then failed with the identical log line after one symlink was added and nothing
else changed. The regression test fails without this change (113 passed,
1 failed) and passes with it (114 passed). It is guarded for Windows because it
calls symlink().
@lgerard42
lgerard42 requested a review from DeusData as a code owner August 22, 2026 20:47
@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.

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