fix(repo-fleet-hygiene): disclose skipped intermediate symlink dirs (#2711)

[kyle-sexton]

Closes #2711

Summary

Discovery under --root still does not follow symbolic links or Windows directory junctions, but skipped intermediate paths are no longer silent — they become visible UNKNOWN findings and appear on the discovery-skips header.

Fix

• Record symlinked/junctioned intermediate directories in DISCOVERY_SYMLINK_PATHS during discover_repositories and emit discovery-symlink-skip findings (evidence notes that Windows junctions test as symlinks under Git Bash).
• Extend the Discovery skips: header with a symlink count.
• Update security-review.md, confidence-model.md, and skill prose so the non-following bound is disclosed rather than sounding complete-and-silent.
• Bump repo-fleet-hygiene to 0.22.5 with CHANGELOG entry.
• Symlinked discovery roots remain refused (CLI hard-fail / configured stale-config-entry); unchanged.

Verification

• bash scripts/check-fleet-finding-test-coverage.sh --check — passed (40 kinds)
• bash plugins/repo-fleet-hygiene/skills/audit/scripts/audit-fleet.test.sh — all collector tests passed, including intermediate symlink under --root is disclosed without descending
• bash scripts/check-changelog-parity.sh --check and --check-bump origin/main

Related

Refs #2711 (closed by keyword above)

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[claude]

Claude finished @kyle-sexton's task in 1m 26s —— View job

---

Security review complete ✅

• Gather context (PR diff, changed files, CLAUDE.md)
• Understand the request (/review:security-review per custom instructions)
• Review diff for security-relevant surface
• Report findings or confirm none found

No security issues found in this PR.

Scope reviewed: plugins/repo-fleet-hygiene/skills/audit/scripts/audit-fleet.sh (the only behavior-changing file; the other six changes are version/CHANGELOG/docs).

What the change does: audit-fleet.sh#L1219-L1227 reworks the discover_repositories walk so a symlinked/junctioned intermediate directory is recorded in DISCOVERY_SYMLINK_PATHS and reported via a discovery-symlink-skip finding, instead of being silently dropped by the old [[ -d "$child" && ! -L "$child" ]] || continue guard.

Checked specifically for the failure modes this kind of change usually introduces:

• Symlink traversal / path escape — the new code still refuses to descend into a symlinked or junctioned directory ([[ -L "$child" && -d "$child" ]] → record + continue, never recurse). Confirmed by the new test case (audit-fleet.test.sh new block) asserting buried-repo (reachable only via the symlink) never appears in output. This is a disclosure-only change, not a behavior change to the sandboxing.
• Injection via attacker-controlled path names — $child/DISCOVERY_SYMLINK_PATHS values only ever reach printf '%s'-style formatting (print_field/display_value) or json_escape() before being embedded in JSON output (audit-fleet.sh#L2469-L2476). No eval/unquoted expansion/command substitution on these values anywhere downstream.
• Quoting regressions in the glob/loop rewrite — $child stays double-quoted through every test (-e, -L, -d) and array append; no new word-splitting or globbing exposure versus the prior version.
• TOCTOU between the -L/-d checks and use — present in principle (as in the pre-existing code) but not exploitable here: the tool never opens/reads through the child after classifying it as a symlink, and this is a local, read-only audit script, not a privilege boundary.

Given the diff only changes disclosure behavior in a read-only discovery walk, and non-descent into symlinks is unchanged and test-covered, there's no exploitable path in this change.

[claude]

Claude finished @kyle-sexton's task in 2m 51s —— View job

---

Code review — /review:code-review (CI code-review lane)

• Gather context (PR diff, branch, changed files)
• Read plugins/review/skills/code-review/SKILL.md and apply its criteria
• Review audit-fleet.sh discovery-walk changes (core logic)
• Review doc/reference updates (SKILL.md, confidence-model.md, security-review.md, CHANGELOG.md)
• Review new test case in audit-fleet.test.sh
• Post findings

Summary

Reviewed the diff (git diff origin/main...HEAD, 7 files) against the correctness/maintainability criteria for this lane (security findings are out of scope here since this repo carries a dedicated security-review lane).

No blocking findings. The core change in discover_repositories (audit-fleet.sh#L1213-L1230) is sound:

• The old [[ -d "$child" && ! -L "$child" ]] filter is correctly split into an existence/symlink probe, followed by a dedicated -L "$child" && -d "$child" arm that records the path into DISCOVERY_SYMLINK_PATHS and continues without descending — matching the stated intent (don't follow, but don't stay silent).
• Excluded names (node_modules, vendor, .venv, etc.) are still filtered before the symlink check, so a symlinked directory with one of those names is skipped the same way it always was (no new disclosure for those) — consistent with existing behavior, not a regression.
• Broken/dangling symlinks (-L true, -e/-d false) fall through both the new symlink-dir arm and the -d check, so they remain silently skipped — same as before this change (the old code never followed or reported them either), so this isn't a regression, just a pre-existing gap outside this PR's stated scope.
• The empty-scope hard-fail guard, the Discovery skips: header, and the new discovery-symlink-skip finding kind are threaded consistently through confidence-model.md, security-review.md, and SKILL.md; the CHANGELOG.md/plugin.json version bump (0.22.4 → 0.22.5) is self-consistent.
• The new test (audit-fleet.test.sh L1841-1863) exercises the intermediate-symlink case with a real fixture (via-link → hidden dir containing buried-repo/.git) and asserts both the finding text and that discovery does not descend into it (! grep -Fq "buried-repo") — good coverage of the actual regression risk (accidentally following the link).

Note on verification: this lane's allowed-tools only grants Bash(gh pr diff/view/comment/review:*), Read, Glob, Grep — I could not execute audit-fleet.test.sh, check-fleet-finding-test-coverage.sh --check, or check-changelog-parity.sh myself. The PR description's listed passing runs for those commands are author-claimed and not independently verified by this review; the analysis above is based on static reading of the diff and script logic only.

[github-actions]

Last security-reviewed head: 7da900a5557d19f90509b0c0a481ae1025325a26. On the next push, the relevance gate compares only the commits since this SHA; delete this comment to force a full re-review.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7da900a555

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[github-actions]

Claude has reviewed this PR 2 times. The lane skips further automatic reviews after 5; deleting this comment resets the count.

[claude]

Claude finished @cursor[bot]'s task in 3m 3s —— View job

---

Code review — /review:code-review

• Gather context (PR diff, branch, changed files)
• Invoke /review:code-review skill and follow its instructions
• Review discovery-walk changes in audit-fleet.sh
• Follow up on Codex's P1 finding about fleet verdict propagation
• Post findings

Summary

This synchronize event adds one commit (06dba3c) on top of the already-reviewed 7da900a. That new commit exists specifically to fix the P1 gap chatgpt-codex-connector flagged on the prior head (inline comment on audit-fleet.sh:2473): a fleet-level discovery-symlink-skip (or any other fleet-level UNKNOWN finding — stale-config-entry, discovery-skip, worktree-root-pluginconfigs-unreadable) was excluded from the overall Fleet verdict calculation, so a root containing only a skipped symlink could still report Fleet verdict: CLEAN.

Verified the fix is correct (audit-fleet.sh#L2573-L2591):

• fleet_level_verdict="$(repo_verdict -1)" is now computed and fed into the same case ... BLOCKED* classification used for per-repo verdicts, incrementing fleet_blocked before fleet_verdict is derived at L2593-L2605.
• fleet_verdict is computed before both the human rollup print and the machine-readable plan JSON's "fleet_verdict" field (L2744), so both surfaces get the corrected value — not just the terminal report.
• Existing LOW-confidence-only fleet-level rows (duplicate-checkout, worktree-root-conformance-summary, worktree-root-unconfigured) still resolve to CLEAN via repo_verdict -1 and correctly fall through the CLEAN) ;; no-op arm, so this doesn't regress those cases into false positives.
• New test coverage in audit-fleet.test.sh (L1837-1863) exercises exactly the scenario Codex called out — a root whose only content is a skipped intermediate symlink — and asserts Fleet verdict: BLOCKED.

No blocking findings. One non-blocking observation: the *) fleet_candidates=... arm at L2589 is currently unreachable — none of the fleet-level finding kinds match branch_action_kind/worktree_action_kind, so repo_verdict -1 can only ever return BLOCKED (evidence gap) or CLEAN, never "N candidates". Harmless (mirrors the per-repo case shape for consistency) and not worth blocking on.

Not independently verified (this lane's tools are Read/Glob/Grep/gh pr diff/view/comment/review — no Bash execution): the PR description's audit-fleet.test.sh, check-fleet-finding-test-coverage.sh --check, and check-changelog-parity.sh runs are author-claimed, not confirmed by this review.

Doc/version updates (SKILL.md, confidence-model.md, security-review.md, CHANGELOG.md, plugin.json 0.22.4 → 0.22.5) are self-consistent with the code change.
· branch cursor/2711-fleet-symlink-8f91