Add untracked local file detection#52
Open
eljulians wants to merge 5 commits into
Open
Conversation
SUMMARY Detect files in install target directories not covered by any Skillfile entry. Surfaces untracked files as warnings in `validate`, as errors with `validate --strict`, and as an opt-in section in `status --untracked`. RATIONALE Users who create skills by hand (e.g. `.claude/skills/docker/`) but forget to declare them in the Skillfile leave teammates without access after cloning. Ghost files also linger after `skillfile remove`. This feature closes the "source of truth" gap: the Skillfile now reports what it does and does not manage. The `--strict` flag enables CI enforcement for teams that require all skills to be declared. CHANGES - `crates/deploy/src/paths.rs`: add `UntrackedFile` struct, `covered_paths()`, `find_untracked()`, `DirScanner`, `ScanSpec`, `for_each_local_adapter()` helper. Scans all local install targets (not just the first). Lenient mode: extra files inside managed nested dirs are not flagged. 17 new unit tests. - `crates/cli/src/commands/validate.rs`: add `check_untracked()`, wire `--strict` flag. Warnings print to stderr, `--strict` promotes to errors and exits non-zero. Summary line appends `(N untracked)`. 3 new unit tests. - `crates/cli/src/commands/status.rs`: add `--untracked` flag and `print_untracked()`. Appends an "Untracked:" section when flag is set. - `crates/cli/src/main.rs`: add `--strict` to `Validate`, `--untracked` to `Status` clap definitions. - `tests/cli.rs`: 4 new functional tests covering `validate` warnings, `validate --strict` failure, `status --untracked` output, and the negative case (no flag hides section). - `roadmap.md`: replace old untracked detection entry with full adversarial-reviewed spec. Remove completed "dirty change tracking" and "strip doc comments" entries from the idea pool.
**SUMMARY** Post-review cleanup from adversarial review of the untracked local detection feature. Two minor fixes in `crates/deploy/src/paths.rs`. **RATIONALE** `covered_paths` has no external consumer outside the file where it is defined, so `pub` visibility was unnecessarily broad. `sort_by` was replaced with `sort_unstable_by` to match the codebase convention since path uniqueness makes sort stability irrelevant. **CHANGES** - `covered_paths` visibility reduced from `pub` to private - `DirScanner::into_sorted` switched from `sort_by` to `sort_unstable_by`
**SUMMARY**
Replaced the bare `is_dir: bool` on `UntrackedFile` with a typed
`UntrackedKind` enum and eliminated the `check_untracked` middle-man
function in `validate.rs`.
**RATIONALE**
Adversarial architectural review flagged two issues: (1) `is_dir: bool`
carried adapter-specific semantics (Nested vs Flat) erased into a bare
bool, with the `"/"` suffix logic duplicated across two CLI consumers.
(2) `check_untracked` mixed validation concerns with presentation
formatting by collecting strings into a `Vec<String>` that was then
iterated again for display.
`UntrackedKind::suffix()` centralizes the display logic, and
`cmd_validate` now calls `find_untracked` directly, passing the data
to a `print_untracked_warnings` presentation function.
**CHANGES**
- `UntrackedKind` enum (`File`/`Directory`) with `suffix()` method
added to `crates/deploy/src/paths.rs`
- `UntrackedFile.is_dir` replaced with `UntrackedFile.kind`
- `check_untracked` removed from `validate.rs`, replaced with
`print_untracked_warnings` that takes `&[UntrackedFile]` directly
- `cmd_validate` calls `find_untracked` directly instead of through
a middle-man function
- `scan_adapter` iterator chain simplified: `map`+`filter`+`collect`
replaced with `filter_map`+`then_some` (no intermediate `Vec`)
- Both CLI consumers (`validate.rs`, `status.rs`) now use
`f.kind.suffix()` instead of inline `if f.is_dir { "/" } else { "" }`
**SUMMARY** Fixed two bugs where `entry_covered_paths` computed wrong covered paths for flat-deployed directory entries and local directory entries, causing false positive untracked file reports. **RATIONALE** Adversarial QA review found that `entry_covered_paths` registered `target_dir/name` for flat dir entries, but flat deploy places individual files directly in `target_dir/`. Files like `a.md`, `b.md` were missing from the covered set. Separately, `is_dir_entry()` only inspects Github `path_in_repo` and returns false for Local entries, so local directory skills got `target_dir/name.md` instead of the actual `target_dir/name/` directory path. The deploy code already handled this at `adapter.rs:150` with `is_dir_entry(entry) || source.is_dir()` but `covered_paths` did not follow the same pattern. **CHANGES** - `entry_covered_paths` now checks `is_dir_entry(entry) || is_local_dir(entry, repo_root)` to detect directory entries regardless of source type - For flat-mode dir entries, calls `adapter.installed_dir_files()` to cover the individual deployed file paths instead of the parent directory - Added `is_local_dir` helper that checks the filesystem, mirroring the deploy code's `req.source.is_dir()` pattern - Added `github_agent_dir` test helper for agent directory entries - Two regression tests: `flat_agent_dir_entry_no_false_positives` and `local_dir_skill_not_false_positive`
eljulians
force-pushed
the
untracked-local-detectino
branch
from
0ef8c9a to
b0937b2
Compare
**SUMMARY** Six fixes from adversarial code review of the untracked local detection feature. Fixes a false-positive bug for nested single-file skills, improves public API ergonomics, help text discoverability, and code clarity. **RATIONALE** Adversarial review found that `entry_covered_paths` only added the leaf file (e.g. `browser/SKILL.md`) to the covered set for nested single-file skills, but the scanner walks one level deep and sees the parent dir (`browser/`). Every nested single-file skill would be flagged as untracked. Additionally, `UntrackedFile` was a public struct without `Debug`, the help text for `validate` and `status` didn't mention the new flags, `find_untracked` ran unnecessary I/O when structural errors already existed, and minor code quality issues. **CHANGES** - `crates/deploy/src/paths.rs`: add parent dir to covered set for nested single-file entries in `entry_covered_paths`, fixing false positives - `crates/deploy/src/paths.rs`: add `#[derive(Debug)]` to `UntrackedFile` - `crates/deploy/src/paths.rs`: move `scan_target_dir` into `DirScanner` as methods (`scan_target_dir` + `consider_entry`), split to satisfy the `excessive_nesting` clippy lint - `crates/deploy/src/paths.rs`: simplify `relative_to` to use `unwrap_or` instead of `map_or_else` - `crates/cli/src/commands/validate.rs`: move `find_untracked()` after the structural error check to avoid unnecessary I/O on error paths - `crates/cli/src/main.rs`: update `validate` long help to describe untracked detection and `--strict`, add example - `crates/cli/src/main.rs`: update `status` long help to describe `--untracked`, add example - Update test assertions for nested covered paths (`browser/SKILL.md` and `browser/` instead of `browser.md`)
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #52 +/- ##
==========================================
+ Coverage 91.15% 91.41% +0.25%
==========================================
Files 36 36
Lines 15509 16012 +503
==========================================
+ Hits 14137 14637 +500
- Misses 1372 1375 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SUMMARY
Detect files in install target directories not covered by any Skillfile entry. Surfaces untracked files as warnings in
validate, as errors withvalidate --strict, and as an opt-in section instatus --untracked. Includes type-safeUntrackedKindenum, false-positive fixes for flat-deployed and local directory entries, and post-review cleanup.RATIONALE
Users who create skills by hand (e.g.
.claude/skills/docker/) but forget to declare them in the Skillfile leave teammates without access after cloning. Ghost files also linger afterskillfile remove. This feature closes the "source of truth" gap: the Skillfile now reports what it does and does not manage. The--strictflag enables CI enforcement for teams that require all skills to be declared.CHANGES
crates/deploy/src/paths.rs: addUntrackedFilestruct with typedUntrackedKindenum (File/Directory) andsuffix()method,covered_paths()(private),find_untracked(),DirScanner,ScanSpec,for_each_local_adapter()helper,is_local_dirhelper that checks the filesystem mirroring the deploy code'sreq.source.is_dir()pattern. Scans all local install targets (not just the first). Lenient mode: extra files inside managed nested dirs are not flagged.entry_covered_pathschecksis_dir_entry(entry) || is_local_dir(entry, repo_root)to detect directory entries regardless of source type. For flat-mode dir entries, callsadapter.installed_dir_files()to cover the individual deployed file paths instead of the parent directory.DirScanner::into_sortedusessort_unstable_byto match codebase convention. 17 new unit tests plus 2 false-positive regression tests (flat_agent_dir_entry_no_false_positives,local_dir_skill_not_false_positive).crates/cli/src/commands/validate.rs: addprint_untracked_warningspresentation function (replaces removedcheck_untrackedmiddle-man that mixed validation with formatting).cmd_validatecallsfind_untrackeddirectly. Wire--strictflag. Warnings print to stderr,--strictpromotes to errors and exits non-zero. Summary line appends(N untracked). 3 new unit tests.crates/cli/src/commands/status.rs: add--untrackedflag andprint_untracked(). Appends an "Untracked:" section when flag is set. Usesf.kind.suffix()instead of inline bool check.crates/cli/src/main.rs: add--stricttoValidate,--untrackedtoStatusclap definitions.tests/cli.rs: 4 new functional tests coveringvalidatewarnings,validate --strictfailure,status --untrackedoutput, and the negative case (no flag hides section).roadmap.md: replace old untracked detection entry with full adversarial-reviewed spec. Remove completed "dirty change tracking" and "strip doc comments" entries from the idea pool.