Fix: report symlinked recursive skill skips in completeness (GH-495)

[bniladridas]

Description

Follow-up to #499 (which closed #495).

detect_skills deliberately refuses to follow symlinked entries during
recursive discovery. Since #499 those skips record a generic limitation;
this follow-up counts them explicitly so the aggregate report states how
many symlinked skill directories were omitted, with a distinct reason and
warning, instead of only the generic entry.

This change keeps the existing behavior of not following directory symlinks
(there is an existing test asserting that skip is deliberate) but makes the
omission visible and truthful:

1. MultiSkillDetectionResult gains omitted_symlink_entries, incremented
   wherever a symlink (or junction/reparse point) is skipped by
   detect_skills.
2. The recursive scan threads that count into omitted_skill_count, so it is
   reflected in skills_omitted, analysis_completeness.is_complete,
   coverage, and the aggregate limitations.
3. A terminal warning lists how many symlinked skill directories were skipped.
4. Tests cover both the detection counter and the combined JSON report
   (skills_omitted == 1, is_complete is False, and a "symlinked recursive
   skill(s) omitted" limitation).

Changes

• src/skillspector/multi_skill.py: count skipped symlink entries in
  detect_skills and expose them on MultiSkillDetectionResult.
• src/skillspector/cli.py: account for omitted_symlink_entries when
  computing the aggregate omitted count, completeness, and limitations, and
  warn in the terminal when symlinked skill directories were skipped.

Validation summary

• Detection counter verified in tests/test_multi_skill.py for the existing
  symlink skip paths.
• Combined JSON report verified in tests/unit/test_cli.py with
  omitted_symlink_entries=1: skills_omitted == 1, is_complete is False,
  and the symlink limitation text is present.
• Full non-integration suite: 5975 passed, 17 skipped (live provider tests
  requiring API keys), 4 xfailed.
• make lint and make format clean on changed files. mypy reports only
  pre-existing errors that are unchanged from main.

[rng1995]

[SkillSpector Review]

Changes requested at current head f62d938c0b962115dcc4865fd2edfcbc53767409.

Reviewed the full four-file diff, prior resolved author comments, discovery control flow, CLI aggregation, and new tests. The symlink counter reaches aggregate JSON/completeness, but it counts entries before applying discovery's explicit name exclusions. An intentionally ignored .git or node_modules symlink therefore creates omitted skills and incomplete coverage. Apply _SKIP_DIRS before incrementing omissions and add an ignored-name regression alongside an eligible linked skill. All six hosted checks pass; the branch conflicts with main. Tests were inspected, not run locally.

[rng1995]

[SkillSpector Review]

Re-reviewed exact head 1fcdb7890149c24832ee542dd48f379381a536d4: all four changed files, recursive-discovery and aggregate-output context, prior reviews/replies, and regression assertions.

Resolved: _SKIP_DIRS is checked before incrementing the symlink omission counter, so ignored dependency/VCS links no longer inflate omitted skills or force a false partial verdict. Eligible links remain untraversed and explicitly counted. Aggregate completeness, CAUTION recommendation, omitted totals, terminal warning, and JSON symlink_not_followed reason stay consistent. Tests pair an ignored node_modules link with an eligible link and assert the partial JSON/report contract. The earlier output-reason and wording comments are also addressed. No required code or test change remains.

All six hosted checks pass and the existing review threads are resolved. Coordinate the overlapping discovery changes with #499. Static inspection and hosted CI evidence only; contributor code/tests were not executed locally. No merge performed.

[rng1995]

[SkillSpector Review]

Corrective completion of this re-review at exact head 1fcdb7890149c24832ee542dd48f379381a536d4. This supersedes my approval earlier in this run: the final caller/completeness cross-check found a missed fallback case that the direct aggregate regression does not exercise.

The prior ignored-name, JSON-reason, and wording findings are resolved. However, when recursive discovery finds no real child skill and only eligible symlinked children, omitted_symlink_entries is positive while detection.complete remains true. The CLI therefore adds no pre-scan limitation and bypasses _scan_multi_skill, the only consumer of this new counter. Its monolithic fallback records the symlinks as out-of-scope boundaries, not partial-discovery exceptions. The scan can consequently report complete coverage despite the same omitted children that force partial coverage when a real sibling exists.

Please preserve the omission as a PARTIAL discovery/system event through the fallback (or emit a truthful zero-child aggregate), keep ignored-name links exempt, and add a CLI-level zero-real-child regression covering JSON completeness and --fail-on-incomplete. The inline comment provides the code locations.

All six hosted checks pass, but the current regression calls _scan_multi_skill directly with a nonempty skills list and cannot detect this dispatch gap. Source and test inspection only, independently cross-checked; contributor code/tests were not executed locally. Changes are required; no merge performed.

[rng1995]

[SkillSpector Review]

@bniladridas, thank you for your patience working through several rounds of review on this PR, and for rebasing onto main and checking the fallback case against #499 yourself. It's much appreciated.

Re-reviewed exact head 16a1c5456dc2a59c9df4d496491656294c7fbabc.

Prior review comments: all addressed. All five review threads are resolved. Ignored-name symlinks (.git, node_modules) are no longer counted (907895f), and the JSON reason and wording comments are addressed. You were right about the zero-real-child fallback gap from my last review: #499 on main closes it. Your two new end-to-end CLI tests for that case pass against main's source unchanged, so they are good regression coverage for that behavior.

Is this PR still needed? #499 merged today and closed #495, so the fail-closed behavior is already on main. I tested a root with two real skills, an eligible symlinked skill and a symlinked node_modules, and a second root whose only child is a symlinked skill. On both, main already reports is_complete: false, status: partial and CAUTION, and --fail-on-incomplete exits 1. This PR still adds reporting accuracy: skills_omitted counts the skipped symlinks (main reports 0, which #495 called out), plus a symlink_not_followed JSON reason and a terminal warning and summary row. That is worth keeping, but it needs a narrower scope and the fixes below before it can merge.

Requested changes

1. [P1] Completeness totals don't add up. _scan_multi_skill adds the symlink count to omitted_skills but still passes total_skills=len(skills). On the first root above at this head, the output is fully_inspected_files: 2, entirely_uninspected_files: 1, total_files: 2, coverage_percent: 100.0. The PR description says coverage reflects the omission, but it does not. Please include the symlink count in total_skills (giving 3 total files and 66.67% coverage) and assert total_files and coverage_percent in the aggregate test.
2. [P2] Duplicate limitation. The JSON now lists #499's generic recursive discovery multi_skill_symlinked_entry limit reached and this PR's 1 symlinked recursive skill(s) omitted (directory symlinks are not followed) for the same skipped entry. Please keep one; the new text is clearer.
3. [P3] Dead code. Moving the _SKIP_DIRS check to the top of the loop in detect_skills makes #499's identical check inside the symlink branch, and its comment, unreachable. Please remove one of them.
4. [P3] Test setup. test_recursive_symlinked_skills_are_reported_as_omitted builds a detection with omitted_symlink_entries=1 but no multi_skill_symlinked_entry limitation. Since #499, real discovery always records both, so the fixture should too. That would also have caught item 2.
5. Description. #495 is already closed by #499. Please describe this PR as a follow-up to #499 rather than "Fixes #495".

All six hosted checks pass on this head and the branch merges cleanly with main. How I verified: I ran main and this head locally against both roots above. I also ran the PR's symlink tests against both sources. All 11 pass on this head; on main, only the 4 that reference the new omitted_symlink_entries field fail. No merge performed.

[bniladridas]

Addressing the remaining items from the 17:40 re-review (16a1c54): P1 totals and P3 dead code were fixed in e91f1e7 with thread replies. This push (d19ef96) covers the rest: the generic multi_skill_symlinked_entry aggregate message is now suppressed when the omission counter carries the clearer text (dedupe), the aggregate fixture builds the limitation alongside the counter as real discovery does since #499 with an assertion that the generic text is absent, and the PR description now frames this as a follow-up to #499 instead of fixing #495. 248 passed on the affected files, ruff check and format clean.

[rng1995]

[SkillSpector Review]

@bniladridas, thank you for turning these around so quickly, and for your patience across all the review rounds on this PR. The thread replies made each fix easy to verify.

Re-reviewed exact head d19ef965858a7918fb626e9c7c497a4058767382.

All requested changes from my last review are addressed:

1. [P1] Completeness totals: fixed in e91f1e7. total_skills now includes the symlink omissions. With two real skills and one eligible symlinked skill, the aggregate reports fully_inspected_files: 2, entirely_uninspected_files: 1, total_files: 3 and coverage_percent: 66.67. The aggregate test asserts all three fields.
2. [P2] Duplicate limitation: fixed in d19ef96. The generic multi_skill_symlinked_entry message is suppressed only when the omission counter carries the clearer text, so the JSON now lists a single 1 symlinked recursive skill(s) omitted (directory symlinks are not followed) limitation. #499's generic text never shipped in a release (it landed after 2.12.0), so dropping it here doesn't break any published output.
3. [P3] Dead code: fixed in e91f1e7. The unreachable inner _SKIP_DIRS check is gone, and its comment now sits with the loop-top check.
4. [P3] Test setup: fixed in d19ef96. The aggregate fixture builds the multi_skill_symlinked_entry limitation alongside the counter, as real discovery does since #499, and asserts that the generic text is absent.
5. Description: updated to frame this as a follow-up to #499.

All seven review threads are resolved. Fail-closed behavior is unchanged: both the mixed root and a root whose only child is a symlinked skill still report is_complete: false, status: partial and CAUTION, and --fail-on-incomplete exits 1. An ignored-name node_modules symlink still adds no omission.

How I verified: I merged this head into current main (8bf9c67) locally. On the merged tree, ruff check and ruff format --check are clean, tests/test_multi_skill.py and tests/unit/test_cli.py pass (248 tests), and the full non-integration suite passes (8389 passed, 14 skipped, 4 xfailed). I also ran both fixture roots above through skillspector scan --recursive --no-llm --format json --fail-on-incomplete. On this head, changes, lint, OpenCode TypeScript Tests, DCO Check and docker-smoke pass; hosted test-unit was still running when I posted this, so it needs to go green before merge.

Approving. No merge performed.