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

[lgerard42]

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.

[github-actions]

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]

Thank you for the minimal reproduction and for documenting the explicit-index versus auto-index asymmetry. I checked current main: safe_stat() returns CBM_NOT_FOUND for a symlink, and the count-only path marks the walk failed for any nonzero safe_stat() result. That confirms why bounded discovery aborts while an explicit index can skip the same entry.

I have labeled this as a high-priority stability bug and routed it for review. The focused test shape is useful. The current DCO check is red, so the commit will need a valid Signed-off-by line before it can advance. Our review queue is full, so detailed review may take a little time. Thank you for tracing the misleading admission result to its actual source.

[DeusData]

Following up on the review note from the 24th — this is still blocked only on DCO, and nothing else. Every other required check is green.

It is a single commit (ef639cf2), so it should be one round trip: amend it with a sign-off (git commit --amend -s --no-edit) and force-push with lease.

Nothing about the change itself is in question. The symlink asymmetry you traced is real and confirmed against current main, and the commit message — particularly the before/after reproduction, and the note that explicit index_repository is unaffected because count_only is false there — is more evidence than most bug fixes arrive with. It is genuinely appreciated.

No rush from our side, and this will not be closed for inactivity. Ping here if the sign-off is a problem for any reason and we will sort it out.

[DeusData]

Heads-up on an adjacent PR, so the ordering is deliberate rather than discovered at merge time.

#1822 also changes safe_stat()'s signature, adding an is_symlink out-param so a symlink rejection can be reported through the ignored-files taxonomy — a file behind a symlinked directory currently vanishes from the index with no trace in not_indexed, skipped or parse_partial. Different bug from yours, same function, same two files.

Its author flagged the interaction themselves and characterised it as a rebase dependency rather than a conflict of claims, which I checked and agree with: your PR adds no ignored-file reporting, and theirs does not touch the count-abort behaviour.

My suggestion is that yours lands first, because "any repository containing a symlink can never auto-index" is a harder failure than an unreported skip, and #1822 then rebases to reuse your is_link out-param instead of adding a second one. That is a preference rather than a decision — if the reverse order suits you better, say so.

This PR has not been reviewed on its merits yet; it is still in the queue and I will get to it. Nothing is being asked of you here.

[DeusData]

Approved on substance. I traced the whole chain rather than taking it, and it is exactly as you describe.

discover.c:894   if (safe_stat(...) != 0)
discover.c:895     if (out->count_only) out->failed = true;    ← any failure, including the
                                                                 by-policy symlink rejection
mcp.c:12799      cbm_discover_count_bounded(...)               ← the auto-index admission path
mcp.c:12871      reason = "unsafe_or_unavailable_path"         ← when the count comes back negative

So yes: any repository containing any symlink can never auto-index, while explicit index_repository works perfectly because count_only is false there and the same entry is merely skipped. That asymmetry is the part that makes this expensive to diagnose rather than merely broken — the repository indexes on demand and silently never indexes itself, which points every investigation at the wrong place.

Your point about the label misleading in two directions is right, and it is worse than you found. The same unsafe_or_unavailable_path string is emitted from two sites — mcp.c:12871 and daemon/application.c:1980 — so both the MCP and daemon paths report a symlink as an unsafe path. And a reader who does not suspect symlinks will suspect the count deadline, which as you say is not remotely involved at 26 ms for a full walk. Neither half of that message is true in this case.

The fix keeps the conservative behaviour where it belongs. Distinguishing "skipped by policy" from "could not observe" via the is_link out-param means a count still aborts on an entry it genuinely cannot see, which is what the admission guard actually wants — the guard was never trying to refuse symlinks, it was refusing the unobservable and could not tell the two apart.

And the reproduction is the right shape: two runs, one ln -s, "nothing else changed between the two runs". That plus 113 passed, 1 failed → 114 passed on the one-line change leaves nothing to argue with. Guarding the test with #ifndef _WIN32 because it calls symlink() is correct.

Finding it while vendoring — a Formula -> pkg/homebrew/Formula symlink — is worth noting, because Homebrew taps carry exactly that shape routinely. This will have been silently affecting real users.

One thing to do

The commit has no sign-off, which is why dco is red — the only failing check. I verified with git's trailer parser rather than a text search, since a blank line before a trailer hides it from git while grep still finds it. Commit ef639cf2 has none:

git commit --amend -s --no-edit
git push --force-with-lease

Ordering

As noted above, this is sequenced ahead of #1822, which adds its own out-param to safe_stat for a different symlink defect — unreported skips. Yours lands first because a total auto-index failure outranks an unreported one, and #1822 then rebases to reuse your is_link rather than adding a second parameter. Its author flagged the interaction themselves and characterised it correctly.

Sign-off and a rebase onto current main (it moved three times yesterday) and this is ready.