fix: correct code-review findings across verify, introspect, and CLI surface

CHANGELOG.md

@@ -37,6 +37,39 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 - `allowed_tools` hint is now keyed off `has_cli` (pure-library skills no
   longer get a `Bash` hint), and derived marketplace keywords are capped at
   16 so a deep CLI subcommand tree can't bloat the marketplace entry.
+- **`add --target` help text matched the code**: the flag's help claimed it
+  defaults to `claude`, but the implementation delegates to `update`'s
+  default (every target already present, falling back to `all`). The help
+  now describes the real behavior.
+- **GPL license detection distinguishes v2 from v3** — every GPL variant
+  used to collapse to `GPL-3.0`, so a GPLv2 project shipped plugin.json with
+  the wrong SPDX id. Detection reads the "Version N" header line and emits
+  the modern `GPL-2.0-only` / `GPL-3.0-only` ids.
+- **`--target list --format sarif|github|junit` now errors** instead of
+  silently degrading the listing to human output (init/update/diff/add/remove).
+- A hand-written SKILL.md whose flag-bearing fenced block is left unclosed
+  at EOF is still detected as documenting a CLI (the fallback parser only
+  evaluated closed fences).
+- `discovery.empty`'s failure message lists `.trae/rules/` and Trae, which
+  were missing while every other rules directory was named.
+- Windows PATH lookup falls back to cmd.exe's default executable extensions
+  when `PATHEXT` is unset, so bare-name probes (`node`, `go`) resolve in
+  minimal environments.
+- `resolve_targets` now actually dedups into canonical declaration order
+  (`--target cursor --target all` used to reorder Cursor ahead of the pack).
+
+### Changed
+
+- Dropped the `once_cell` dependency (`std::sync::LazyLock` — MSRV is 1.85)
+  and dialoguer's unused `fuzzy-select`/`editor` features (only `Input` is
+  used). Removed stale `Cargo.toml` exclude entries.
+- Corrected several doc comments that a refactor left attached to the wrong
+  items (`is_meta_flag` ↔ `check_subcommand_drift`, `run_help` ↔
+  `extract_documented_invocation`, `spawn_capture` ↔ `check_flag_drift`,
+  `find_kv_colon` ↔ `claude_present`, `canonicalize_for_argv`, and the
+  `SkillConfig::author` field), rewrote the garbled `verify --watch` flag
+  doc, updated the stale language list on `ProjectProfile::language`, and
+  reworded the internal "Ponytail" notes.
 
 ## [0.13.2](https://github.com/nordicnode/skillpack/compare/v0.13.1...v0.13.2) - 2026-08-15
 

Cargo.toml

@@ -15,10 +15,10 @@ readme = "README.md"
 exclude = [
     "/.claude",
     "/.github",
+    # The 770KB logo stays out of the crate package (crates.io renders README
+    # images against the repo); only the markdown docs ship.
     "/docs/logo.png",
     "/scripts",
-    "/memory:",
-    "2026-07-09-skillpack-design.md",
 ]
 
 [lib]
@@ -32,14 +32,15 @@ path = "src/main.rs"
 [dependencies]
 clap = { version = "4.5", features = ["derive"] }
 clap_complete = "4"
-dialoguer = { version = "0.11", default-features = false, features = ["fuzzy-select", "editor"] }
+# Only `Input` is used (interview prompts); every other dialoguer item is
+# feature-gated, so default-features = false keeps the build lean.
+dialoguer = { version = "0.11", default-features = false }
 toml = "0.8"
 serde = { version = "1", features = ["derive"] }
 serde_json = { version = "1", features = ["preserve_order"] }
 tera = { version = "1", default-features = false }
 anyhow = "1"
 regex = "1"
-once_cell = "1"
 tempfile = "3"
 notify = "8"
 tracing = "0.1"

src/cli.rs

@@ -223,9 +223,9 @@ pub enum Commands {
         /// Watch for file changes and re-run verify on each change (debounced).
         /// Useful during iterative SKILL.md / skillpack.toml edits — get
         /// instant feedback without manually re-running verify each time.
-        /// Ctrl-C stops the watcher (terminates the process). Only valid
-        /// mode prints a new report per cycle; JSON output isn't meaningful
-        /// for a streaming watcher).
+        /// Ctrl-C stops the watcher (terminates the process). Only valid with
+        /// `--format human`: the watcher prints a fresh report per cycle, and
+        /// JSON/SARIF/GitHub/JUnit output isn't meaningful for a stream.
         #[arg(long)]
         watch: bool,
 
@@ -378,8 +378,9 @@ pub enum Commands {
         license: Option<String>,
 
         /// Agent ecosystem(s) to regenerate after appending. Defaults to
-        /// `claude`; `all` refreshes every target, `list` prints the canonical
-        /// target names and exits. Repeats.
+        /// every target already present in the repo (falling back to `all`
+        /// when none are found), matching `update`; `all` refreshes every
+        /// target, `list` prints the canonical target names and exits. Repeats.
         #[arg(long, num_args = 1.., value_name = "ECOSYSTEM")]
         target: Vec<String>,
 
@@ -595,15 +596,21 @@ pub fn resolve_targets(raw: &[String]) -> anyhow::Result<Vec<Target>> {
             })?);
         }
     }
-    // Dedup preserving canonical order — `--target all --target claude`
-    // must not emit Claude twice (double-writes files).
-    let mut seen = Vec::new();
-    for t in out {
-        if !seen.contains(&t) {
-            seen.push(t);
-        }
-    }
-    Ok(seen)
+    // Dedup, then reorder into canonical (declaration) order. Dedup alone
+    // keeps first-seen order, so `--target cursor --target all` would emit
+    // Cursor's files before everything else; sorting by the ALL_TARGETS index
+    // makes the emitted file sequence identical no matter how the flags were
+    // spelled. `--target all --target claude` must also not emit Claude twice
+    // (double-writes files) — dedup handles that.
+    let mut seen = std::collections::HashSet::new();
+    out.retain(|t| seen.insert(*t));
+    out.sort_by_key(|t| {
+        ALL_TARGETS
+            .iter()
+            .position(|c| c == t)
+            .unwrap_or(usize::MAX)
+    });
+    Ok(out)
 }
 
 #[cfg(test)]
@@ -625,6 +632,26 @@ mod tests {
         assert!(all.contains(&Target::Trae));
     }
 
+    #[test]
+    fn resolve_targets_dedups_and_canonicalizes_order() {
+        // Repeated + mixed-order flags must collapse to the canonical
+        // declaration order — `render` emits files in this sequence, so a
+        // first-seen order would make `--target cursor --target all` write
+        // Cursor's file before everything else.
+        let mixed = resolve_targets(&[
+            "cursor".to_string(),
+            "all".to_string(),
+            "claude".to_string(),
+            "claude".to_string(),
+        ])
+        .unwrap();
+        let all = resolve_targets(&["all".to_string()]).unwrap();
+        assert_eq!(mixed, all, "mixed flags must dedup+sort to canonical `all`");
+        // Spot-check the canonical head/tail rather than the whole vector.
+        assert_eq!(mixed.first(), Some(&Target::Claude));
+        assert_eq!(mixed.last(), Some(&Target::Trae));
+    }
+
     #[test]
     fn effective_log_filter_maps_flags() {
         use clap::Parser;

src/commands/add.rs

@@ -34,6 +34,11 @@ pub(crate) fn run_add(
     template_dir: Option<&Path>,
     format: verify::OutputFormat,
 ) -> i32 {
+    // Validate the format before honoring `--target list` (see `run_init`).
+    if let Err(e) = reject_report_format(format) {
+        eprintln!("fatal: {e:#}");
+        return exit::INIT_FATAL;
+    }
     if let Some(code) = handle_list_request("add", &raw_targets, format) {
         return code;
     }

src/commands/diff.rs

@@ -19,6 +19,11 @@ pub(crate) fn run_diff(
     template_dir: Option<&Path>,
     format: verify::OutputFormat,
 ) -> i32 {
+    // Validate the format before honoring `--target list` (see `run_init`).
+    if let Err(e) = reject_report_format(format) {
+        eprintln!("fatal: {e:#}");
+        return exit::INIT_FATAL;
+    }
     if let Some(code) = handle_list_request("diff", &raw_targets, format) {
         return code;
     }

src/commands/init.rs

@@ -296,6 +296,13 @@ pub(crate) fn run_init(
     import: Option<String>,
     format: verify::OutputFormat,
 ) -> i32 {
+    // Validate the format before honoring `--target list`, so an invalid
+    // `--format sarif/github/junit` errors instead of silently degrading
+    // the listing to human output.
+    if let Err(e) = reject_report_format(format) {
+        eprintln!("fatal: {e:#}");
+        return exit::INIT_FATAL;
+    }
     if let Some(code) = handle_list_request("init", &raw_targets, format) {
         return code;
     }

src/commands/remove.rs

@@ -24,6 +24,11 @@ pub(crate) fn run_remove(
     template_dir: Option<&Path>,
     format: verify::OutputFormat,
 ) -> i32 {
+    // Validate the format before honoring `--target list` (see `run_init`).
+    if let Err(e) = reject_report_format(format) {
+        eprintln!("fatal: {e:#}");
+        return exit::INIT_FATAL;
+    }
     if let Some(code) = handle_list_request("remove", &raw_targets, format) {
         return code;
     }

src/commands/update.rs

@@ -34,6 +34,11 @@ pub(crate) fn run_update(
     template_dir: Option<&Path>,
     format: verify::OutputFormat,
 ) -> i32 {
+    // Validate the format before honoring `--target list` (see `run_init`).
+    if let Err(e) = reject_report_format(format) {
+        eprintln!("fatal: {e:#}");
+        return exit::INIT_FATAL;
+    }
     if let Some(code) = handle_list_request("update", &raw_targets, format) {
         return code;
     }

src/config.rs

@@ -50,9 +50,10 @@ pub struct SkillConfig {
     /// Import pattern for pure libraries. `None` for CLI projects.
     #[serde(default, skip_serializing_if = "Option::is_none")]
     pub import_pattern: Option<String>,
-    /// SPDX license id (e.g. `MIT`).
+    /// Author display name (e.g. `Jane Doe`).
     #[serde(default, skip_serializing_if = "Option::is_none")]
     pub author: Option<String>,
+    /// SPDX license id (e.g. `MIT`).
     #[serde(default, skip_serializing_if = "Option::is_none")]
     pub license: Option<String>,
     /// Stdin bytes to feed the CLI during `verify` spawns (e.g. `--help`,

src/generate.rs

@@ -8,8 +8,8 @@
 //! conditionally-present).
 
 use anyhow::{bail, Result};
-use once_cell::sync::Lazy;
 use std::path::Path;
+use std::sync::LazyLock;
 use tera::{Context as TeraContext, Tera};
 
 use crate::cli::Target;
@@ -37,7 +37,7 @@ const CONVENTIONS_MD_TPL: &str = include_str!("../templates/CONVENTIONS.md.tera"
 const WINDSURF_RULE_TPL: &str = include_str!("../templates/windsurf-rule.md.tera");
 const SKILL_BODY_TPL: &str = include_str!("../templates/skill_body.md.tera");
 
-static TERA: Lazy<Tera> = Lazy::new(|| {
+static TERA: LazyLock<Tera> = LazyLock::new(|| {
     let mut tera = Tera::default();
     tera.add_raw_template("marketplace.json", MARKETPLACE_TPL)
         .expect("marketplace template is valid");
@@ -552,7 +552,8 @@ fn derive_keywords(profile: &ProjectProfile, intent: &Intent, has_cli: bool) ->
 }
 
 /// Tiny stopword set for keyword extraction. Keeps "generate" but drops
-/// "this", "with", "about". Ponytail: inline set beats pulling a crate.
+/// "this", "with", "about". An inline set beats pulling a crate for seven
+/// call sites' worth of filtering.
 fn is_stopword(w: &str) -> bool {
     matches!(
         w.to_lowercase().as_str(),

src/introspect/cli_candidates.rs

@@ -58,13 +58,27 @@ pub struct CliCandidate {
 /// Windows-aware PATH lookup. cmd.exe appends `PATHEXT` (`cmd` → `cmd.exe`) to a
 /// bare name; Rust's `Command::new` does not. Probe `name` plus `name{ext}`
 /// for each ext in `PATHEXT` (e.g. `.EXE;.CMD;.BAT`) so a PATH lookup resolves
-/// `node` to `node.exe`. On Unix the bare-name probe is unchanged (no
-/// `PATHEXT`). Returns the resolved file path or `None` when not on PATH.
+/// `node` to `node.exe`. When `PATHEXT` is unset (minimal environments, some
+/// CI containers), fall back to the common executable extensions instead of
+/// failing every bare-name probe. On Unix the bare-name probe is unchanged
+/// (no `PATHEXT`). Returns the resolved file path or `None` when not on PATH.
 pub(crate) fn which_on_path(name: &str) -> Option<PathBuf> {
-    let exts: Vec<String> = std::env::var("PATHEXT")
-        .ok()
-        .map(|p| p.split(';').map(|s| s.to_string()).collect())
-        .unwrap_or_default();
+    let exts: Vec<String> = match std::env::var("PATHEXT") {
+        Ok(p) if !p.is_empty() => p.split(';').map(|s| s.to_string()).collect(),
+        _ => {
+            if cfg!(windows) {
+                // cmd.exe's built-in default when PATHEXT is not set.
+                vec![
+                    ".COM".to_string(),
+                    ".EXE".to_string(),
+                    ".BAT".to_string(),
+                    ".CMD".to_string(),
+                ]
+            } else {
+                Vec::new()
+            }
+        }
+    };
     let path = std::env::var_os("PATH")?;
     for dir in std::env::split_paths(&path) {
         let bare = dir.join(name);
@@ -98,9 +112,10 @@ pub(crate) fn primary_cli_candidate(
     super::manifest::language_spec(language).cli_candidate(root, name)
 }
 
-/// Parse `[[bin]].name` entries from `Cargo.toml`. Returns bin names in
-/// declaration order; empty when no `[[bin]]` tables (implicit single-bin
-/// crate where the artifact matches the package name).
+/// Canonicalize `p` to an absolute path string for use in a spawn argv,
+/// falling back to the lossy display form when canonicalization fails, and
+/// stripping the Windows extended-length prefix (`\\?\C:\…` → `C:\…`) that
+/// `fs::canonicalize` adds so the argv stays paste-able.
 pub(crate) fn canonicalize_for_argv(p: &Path) -> String {
     let path = std::fs::canonicalize(p)
         .ok()

src/introspect/cli_probe.rs

@@ -207,7 +207,8 @@ pub(crate) fn has_gemspec(root: &Path) -> bool {
 
 /// True if the root contains any `*.csproj` file. Solution-only repos (`.sln`
 /// at root, csproj in subdirs) are not detected — same limitation class as
-/// Cargo workspace-only roots. ponytail: add .sln directory walk when needed.
+/// Cargo workspace-only roots. Future work: add a `.sln` directory walk when
+/// needed.
 pub(crate) fn has_csproj(root: &Path) -> bool {
     fs::read_dir(root).is_ok_and(|entries| {
         entries
@@ -411,7 +412,7 @@ fn spawn_candidate(candidate: &CliCandidate, diag: &mut DiagTrace) -> DetectCli
             );
             DetectCli::none()
         }
-        // ponytail: permission-denied etc. are rare; mapping to `none()`
+        // Note: permission-denied etc. are rare; mapping to `none()`
         // means `has_cli=false` (pure-library path) rather than crashing.
         // verify's spawn will then surface the gap downstream if the CLI IS
         // documented. The honest path for V1 — doesn't crash.
@@ -569,7 +570,7 @@ mod tests {
         let _ = std::fs::remove_dir_all(root);
     }
 
-    // ponytail: walk_*_workspace skip branch (member with no name in manifest
+    // Note: the walk_*_workspace skip branch (member with no name in manifest
     // AND dir-tail file_name() None) is unreachable for non-root member paths —
     // the path-tail fallback always yields a name. These tests assert the
     // observable contract we DO hit: the walk continues past every member to the