diff --git a/CHANGELOG.md b/CHANGELOG.md index 7f54dc5..749ca8b 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/Cargo.lock b/Cargo.lock index cee779d..0e8e9b3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -237,9 +237,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "658bce805d770f407bc62102fca7c2c64ceef2fbcb2b8bd19d2765ce093980de" dependencies = [ "console 0.15.11", - "fuzzy-matcher", "shell-words", - "tempfile", "thiserror", ] @@ -301,15 +299,6 @@ dependencies = [ "libc", ] -[[package]] -name = "fuzzy-matcher" -version = "0.3.7" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "54614a3312934d066701a80f20f15fa3b56d67ac7722b39eea5b4c9dd1d66c94" -dependencies = [ - "thread_local", -] - [[package]] name = "getrandom" version = "0.3.4" @@ -893,7 +882,6 @@ dependencies = [ "insta", "libc", "notify", - "once_cell", "predicates", "proptest", "regex", diff --git a/Cargo.toml b/Cargo.toml index 02483d3..b522299 100644 --- a/Cargo.toml +++ b/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" diff --git a/src/cli.rs b/src/cli.rs index cf41788..fc639f0 100644 --- a/src/cli.rs +++ b/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, /// 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, @@ -595,15 +596,21 @@ pub fn resolve_targets(raw: &[String]) -> anyhow::Result> { })?); } } - // 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; diff --git a/src/commands/add.rs b/src/commands/add.rs index aa94b84..14b0fee 100644 --- a/src/commands/add.rs +++ b/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; } diff --git a/src/commands/diff.rs b/src/commands/diff.rs index 41aca78..add087c 100644 --- a/src/commands/diff.rs +++ b/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; } diff --git a/src/commands/init.rs b/src/commands/init.rs index 533771a..2d4c61d 100644 --- a/src/commands/init.rs +++ b/src/commands/init.rs @@ -296,6 +296,13 @@ pub(crate) fn run_init( import: Option, 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; } diff --git a/src/commands/remove.rs b/src/commands/remove.rs index 0003fb6..7c7ba88 100644 --- a/src/commands/remove.rs +++ b/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; } diff --git a/src/commands/update.rs b/src/commands/update.rs index 43a87e8..a8f7b1b 100644 --- a/src/commands/update.rs +++ b/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; } diff --git a/src/config.rs b/src/config.rs index 086406c..900ef38 100644 --- a/src/config.rs +++ b/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, - /// SPDX license id (e.g. `MIT`). + /// Author display name (e.g. `Jane Doe`). #[serde(default, skip_serializing_if = "Option::is_none")] pub author: Option, + /// SPDX license id (e.g. `MIT`). #[serde(default, skip_serializing_if = "Option::is_none")] pub license: Option, /// Stdin bytes to feed the CLI during `verify` spawns (e.g. `--help`, diff --git a/src/generate.rs b/src/generate.rs index a69b448..8db24e5 100644 --- a/src/generate.rs +++ b/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 = Lazy::new(|| { +static TERA: LazyLock = 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(), diff --git a/src/introspect/cli_candidates.rs b/src/introspect/cli_candidates.rs index d73d91c..c6e11b4 100644 --- a/src/introspect/cli_candidates.rs +++ b/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 { - let exts: Vec = std::env::var("PATHEXT") - .ok() - .map(|p| p.split(';').map(|s| s.to_string()).collect()) - .unwrap_or_default(); + let exts: Vec = 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() diff --git a/src/introspect/cli_probe.rs b/src/introspect/cli_probe.rs index c8c81cb..4b0c6c3 100644 --- a/src/introspect/cli_probe.rs +++ b/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 diff --git a/src/introspect/repo.rs b/src/introspect/repo.rs index 97520ff..37f747a 100644 --- a/src/introspect/repo.rs +++ b/src/introspect/repo.rs @@ -90,7 +90,16 @@ pub(crate) fn detect_license(root: &Path) -> Option { return Some("BSD-3-Clause".to_string()); } if lower.contains("gnu general public license") { - return Some("GPL-3.0".to_string()); + // The GPL family shares the header text; distinguish v2 from + // v3 via the "Version N" line under the title (both canonical + // texts put it in the first three lines). Emit the modern + // `-only` SPDX form — bare `GPL-3.0`/`GPL-2.0` are deprecated + // ids, and guessing `or-later` from the license file alone + // would over-claim. + if lower.contains("version 2") && !lower.contains("version 2.1") { + return Some("GPL-2.0-only".to_string()); + } + return Some("GPL-3.0-only".to_string()); } } } @@ -186,9 +195,71 @@ pub(crate) fn repo_url_name(repo_url: &Option) -> Option { #[cfg(test)] mod tests { //! README-hint tests that assert the `skip_while` predicate drops raw - //! HTML and lands on first prose. + //! HTML and lands on first prose, plus LICENSE heuristics tests. - use super::read_readme_hint; + use super::{detect_license, read_readme_hint}; + + #[test] + fn detect_license_distinguishes_gpl_v2_from_v3() { + // Regression: every GPL variant used to collapse to `GPL-3.0`, so a + // GPLv2 project shipped plugin.json with the wrong SPDX id. + let dir = std::env::temp_dir().join(format!( + "skillpack-gpl-{}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + + std::fs::write( + dir.join("LICENSE"), + " GNU GENERAL PUBLIC LICENSE\n Version 2, June 1991\n", + ) + .unwrap(); + assert_eq!(detect_license(&dir).as_deref(), Some("GPL-2.0-only")); + + std::fs::write( + dir.join("LICENSE"), + "GNU GENERAL PUBLIC LICENSE\nVersion 3, 29 June 2007\n", + ) + .unwrap(); + assert_eq!(detect_license(&dir).as_deref(), Some("GPL-3.0-only")); + + let _ = std::fs::remove_dir_all(&dir); + } + + #[test] + fn detect_license_mit_and_apache_unchanged() { + let dir = std::env::temp_dir().join(format!( + "skillpack-lic-{}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + + std::fs::write( + dir.join("LICENSE"), + "MIT License\n\nPermission is hereby granted, free of charge.\n", + ) + .unwrap(); + assert_eq!(detect_license(&dir).as_deref(), Some("MIT")); + + std::fs::write( + dir.join("LICENSE"), + "Apache License\nVersion 2.0, January 2004\n", + ) + .unwrap(); + assert_eq!(detect_license(&dir).as_deref(), Some("Apache-2.0")); + + let _ = std::fs::remove_dir_all(&dir); + } #[test] fn read_readme_hint_skips_leading_html_div() { diff --git a/src/spawn.rs b/src/spawn.rs index 61f8189..662893e 100644 --- a/src/spawn.rs +++ b/src/spawn.rs @@ -21,8 +21,8 @@ use std::time::{Duration, Instant}; /// GOCACHE build + AV scan under parallel-test load) and any `node_modules` /// resolution, while still bounding a hung CLI. History: raised 8s → 15s for /// the same Windows `go run .` flake, then 15s → 30s when it recurred under -/// heavier parallel CI load. Ponytail: ceiling is CI cold-cache; if a real CLI -/// genuinely needs >30s to print `--help` the agent shouldn't invoke it +/// heavier parallel CI load. Caveat: the ceiling is CI cold-cache; if a real +/// CLI genuinely needs >30s to print `--help` the agent shouldn't invoke it /// anyway, so this cap is also the fail-safe. pub const HELP_TIMEOUT: Duration = Duration::from_secs(30); diff --git a/src/types.rs b/src/types.rs index e3ef219..0e3dea9 100644 --- a/src/types.rs +++ b/src/types.rs @@ -17,9 +17,11 @@ pub struct ProjectProfile { /// Best-effort tool name, derived from the project manifest or repo dir. /// Always coerced to kebab-case before it reaches a generated file. pub name: String, - /// Detected ecosystem: one of `rust`, `node`, `python`, `go`, `ruby`, + /// Detected ecosystem. One of `rust`, `node`, `python`, `go`, `ruby`, /// `php`, `csharp`, `jvm`, `zig`, `swift`, `c_cpp`, `elixir`, `deno`, - /// or `unknown`. + /// `nix`, `dart`, `haskell`, `lua`, `julia`, `crystal`, `clojure`, + /// `ocaml`, `erlang`, `r`, `perl`, `shell`, `powershell`, or + /// `unknown`. pub language: Language, /// Any additional languages detected alongside the dominant one (a /// polyglot monorepo, e.g. a Rust CLI with a TypeScript frontend). Empty diff --git a/src/verify/discovery/claude.rs b/src/verify/discovery/claude.rs index 0b0fa51..bfdcd68 100644 --- a/src/verify/discovery/claude.rs +++ b/src/verify/discovery/claude.rs @@ -12,6 +12,9 @@ use crate::introspect::{detect_language, project_manifest_version}; use crate::types::DiagTrace; use crate::verify::result::CheckResult; +/// True if the Claude Code distribution directory (`.claude-plugin/`) is +/// present. Gates the marketplace.json / plugin.json / SKILL.md checks so a +/// `--target cursor`-only pack doesn't false-fail on their absence. pub(crate) fn claude_present(root: &Path) -> bool { root.join(schema::CLAUDE_PLUGIN_DIR).is_dir() } diff --git a/src/verify/discovery/mod.rs b/src/verify/discovery/mod.rs index 1c636a3..f9b9e58 100644 --- a/src/verify/discovery/mod.rs +++ b/src/verify/discovery/mod.rs @@ -11,14 +11,14 @@ use std::path::Path; use anyhow::Result; -use once_cell::sync::Lazy; use regex::Regex; +use std::sync::LazyLock; use super::result::CheckResult; use super::schema; -static NAME_RE: Lazy = - Lazy::new(|| Regex::new(schema::NAME_KEBAB_REGEX).expect("compiled constant regex")); +static NAME_RE: LazyLock = + LazyLock::new(|| Regex::new(schema::NAME_KEBAB_REGEX).expect("compiled constant regex")); mod agentsmd; mod claude; @@ -329,8 +329,8 @@ pub fn run( if out.is_empty() { out.push(CheckResult::fail( "discovery.empty", - "at least one ecosystem is present (Claude / Codex / Cursor / OpenCode / Copilot / AGENTS.md / CLAUDE.md / GEMINI.md / Windsurf / Aider / Cline / Roo / Kilo / Goose / Qoder / Continue / Augment / Amazon Q)", - "no distribution files found (none of: .claude-plugin/, .claude/skills/, .codex/skills/, .cursor/rules/, .windsurf/rules/, .opencode/agents/, .github/copilot-instructions.md, AGENTS.md, CLAUDE.md, GEMINI.md, CONVENTIONS.md, .clinerules/, .roo/rules/, .kilocode/rules/, .goose/instructions.md, .qoder/rules/, .continue/rules/, .augment/rules/, .amazonq/rules/)", + "at least one ecosystem is present (Claude / Codex / Cursor / OpenCode / Copilot / AGENTS.md / CLAUDE.md / GEMINI.md / Windsurf / Aider / Cline / Roo / Kilo / Goose / Qoder / Continue / Augment / Amazon Q / Trae)", + "no distribution files found (none of: .claude-plugin/, .claude/skills/, .codex/skills/, .cursor/rules/, .windsurf/rules/, .opencode/agents/, .github/copilot-instructions.md, AGENTS.md, CLAUDE.md, GEMINI.md, CONVENTIONS.md, .clinerules/, .roo/rules/, .kilocode/rules/, .goose/instructions.md, .qoder/rules/, .continue/rules/, .augment/rules/, .amazonq/rules/, .trae/rules/)", "To fix: run `skillpack init --target ` first.", )); } @@ -338,7 +338,10 @@ pub fn run( Ok(out) } -/// True if the Claude Code distribution files (`.claude-plugin/`) are present. +/// Find the index of the `key:` colon separator in one frontmatter line — +/// the first `:` not inside quotes. Handles escaped quotes (`\"`, `\'`) so a +/// value like `description: "test: here"` yields the key's colon, not the +/// one inside the string. Returns `None` when the line has no unquoted colon. pub(crate) fn find_kv_colon(line: &str) -> Option { // First `:` not inside quotes. Handles escaped quotes (\", \'). let mut in_s = false; diff --git a/src/verify/fix.rs b/src/verify/fix.rs index 5d9bfc4..16ac411 100644 --- a/src/verify/fix.rs +++ b/src/verify/fix.rs @@ -1,6 +1,6 @@ //! `verify --fix`: typed fix actions + appliers for mechanical drift. //! -//! Ponytail scope: one variant per mechanical drift class that verify already +//! Scope note: one variant per mechanical drift class that verify already //! detects. Adding a new fixable drift is a compile-driven extension — the //! exhaustive `apply` match below means forgetting an applier is a build //! failure, not a silent runtime gap. @@ -172,7 +172,7 @@ fn apply_regen_skill_md_frontmatter( // Derive the target from the location path: Codex skills live under // `.codex/skills/`, Claude skills under `skills/`. Render ONLY the // ecosystem whose file drifted — surgical: we don't touch the other path. - // ponytail: ceiling is two path-prefixes (Codex + default Claude). When a + // Scope note: the ceiling is two path prefixes (Codex + default Claude). When a // third ecosystem path appears (or a non-standard skill location), extend // this `if/else` into a match on a thread-friendly enum (or a mapping from // Target → canonical path prefix). The current prefix inference is fine diff --git a/src/verify/invocation/drift.rs b/src/verify/invocation/drift.rs index dfe0827..e8476ec 100644 --- a/src/verify/invocation/drift.rs +++ b/src/verify/invocation/drift.rs @@ -120,10 +120,12 @@ pub(crate) fn reverse_drift( )); } -/// True for the universal help/version meta-flags that every CLI implicitly -/// supports but does not (and should not) list among its own passable flags. -/// These are excluded from flag-drift comparison so a SKILL.md instruction like -/// "Run ` --help`" doesn't read as drift. +/// For each subcommand the SKILL.md documents, spawn ` --help` and +/// set-diff the documented flags against the real `--help`. Pushes one +/// `invocation.subcommand_drift` result per documented subcommand. A documented +/// subcommand whose `--help` won't spawn here fails (honest, like +/// `invocation.help_present`); a documented flag the real help omits fails; +/// reverse drift (help advertises a flag the skill doesn't) warns. pub(crate) fn check_subcommand_drift( base_cmd: &[String], spawn_cwd: &Path, diff --git a/src/verify/invocation/mod.rs b/src/verify/invocation/mod.rs index 6552a28..3661c5e 100644 --- a/src/verify/invocation/mod.rs +++ b/src/verify/invocation/mod.rs @@ -156,20 +156,12 @@ pub fn run(input: &InvocationInput, report: &mut VerifyReport) -> Result<()> { Ok(()) } -/// Pull the text of the SKILL.md that documents the CLI invocation, so flag- -/// drift extraction reads only the documented invocation area (not the templated -/// prose/footguns/metadata). Returns `None` when the skill is a pure library. -/// -/// Two signals, in order: -/// 1. A `## Invocation` heading — the section the skillpack CLI template emits. -/// skillpack *libraries* use `## Usage` (never `## Invocation`), so this -/// cleanly separates the two for generated packs. -/// 2. A fenced code block containing a `--flag` token — the fallback for -/// *hand-written* skills (e.g. the `broken-cli` fixture) that document a CLI -/// without the `## Invocation` heading. A pure-library import block -/// (`import { parse } from 'x'`) has no `--flag`, so it correctly stays a -/// library (Bug 2 + Improvement F, without the prose false-positives that -/// scoping to the *whole* body would reintroduce). +/// Spawn ` [cmd[1..]]` (e.g. `chronicle --help`) under the hard +/// `HELP_TIMEOUT`, push the outcome as a `invocation.help_present` check, and +/// return the captured stdout+stderr (empty string on any failure outcome so +/// the drift checks degrade to a no-op). Spawn calls are emitted as +/// `tracing::debug!` events by the shared spawn core (design §8.2 --debug / +/// --log-level debug). fn run_help( cmd: &[String], root: &Path, @@ -264,8 +256,11 @@ pub(crate) fn snippet(s: &str, max: usize) -> String { out } -/// Compare documented flags in SKILL.md against those advertised in `--help`, -/// flagging any documented flag that does not actually exist (drift). +/// Spawn `cmd` (argv, with the working directory set to `root`) under +/// `timeout` and return the captured stdout+stderr, or `None` when the spawn +/// could not run at all (not found / spawn failure / timeout). Non-zero exits +/// still return the output — some CLIs print `--version`/`--help` and exit 1. +/// Unlike [`run_help`], this pushes no checks; the caller owns the diff. fn spawn_capture( cmd: &[String], root: &Path, @@ -286,12 +281,6 @@ fn spawn_capture( } } -/// For each subcommand the SKILL.md documents, spawn ` --help` and -/// set-diff the documented flags against the real `--help`. Pushes one -/// `invocation.subcommand_drift` result per documented subcommand. A documented -/// subcommand whose `--help` won't spawn here fails (honest, like -/// `invocation.help_present`); a documented flag the real help omits fails; -/// reverse drift (help advertises a flag the skill doesn't) warns. #[cfg(test)] mod checks { use super::drift::{diff_one_subcommand, reverse_drift}; @@ -379,6 +368,20 @@ mod checks { assert!(extract_documented_invocation(skill).is_some()); } + /// A flag-bearing fence left unclosed at EOF still documents a CLI — the + /// old parser only evaluated a block at its closing fence, so a malformed + /// hand-written skill fell through to the pure-library path. + #[test] + fn documented_invocation_from_unterminated_fence() { + let skill = "# x\n\n```\nx --new\n"; // no closing ``` + let block = extract_documented_invocation(skill).expect("unterminated fence"); + assert!(block.contains("--new")); + + // An unclosed fence WITHOUT flags stays a pure library. + let lib = "# x\n\n```\nimport { parse } from 'fastcsv'\n"; + assert!(extract_documented_invocation(lib).is_none()); + } + /// Regression: `command_from_documented` must extract the program from the /// FENCED command line, never from the surrounding prose. The old code /// returned "The" (the first word of "The exact command an agent should diff --git a/src/verify/invocation/parse.rs b/src/verify/invocation/parse.rs index 1fb57cc..3ea6e55 100644 --- a/src/verify/invocation/parse.rs +++ b/src/verify/invocation/parse.rs @@ -1,3 +1,17 @@ +/// Pull the text of the SKILL.md that documents the CLI invocation, so flag- +/// drift extraction reads only the documented invocation area (not the templated +/// prose/footguns/metadata). Returns `None` when the skill is a pure library. +/// +/// Two signals, in order: +/// 1. A `## Invocation` heading — the section the skillpack CLI template emits. +/// skillpack *libraries* use `## Usage` (never `## Invocation`), so this +/// cleanly separates the two for generated packs. +/// 2. A fenced code block containing a `--flag` token — the fallback for +/// *hand-written* skills (e.g. the `broken-cli` fixture) that document a CLI +/// without the `## Invocation` heading. A pure-library import block +/// (`import { parse } from 'x'`) has no `--flag`, so it correctly stays a +/// library (Bug 2 + Improvement F, without the prose false-positives that +/// scoping to the *whole* body would reintroduce). pub fn extract_documented_invocation(skill_md: &str) -> Option { // (1) Prefer an explicit `## Invocation` section. if let Some(block) = heading_block(skill_md, "invocation") { @@ -5,6 +19,8 @@ pub fn extract_documented_invocation(skill_md: &str) -> Option { } // (2) Fallback: any fenced ``` block whose text contains a `--flag`. + // A fence left unclosed at EOF still counts — a hand-written skill with a + // missing closing fence documents a CLI just as much as a well-formed one. let mut in_fence = false; let mut block = String::new(); for line in skill_md.lines() { @@ -25,6 +41,10 @@ pub fn extract_documented_invocation(skill_md: &str) -> Option { block.push('\n'); } } + // Unterminated final fence: the block accumulated but never closed. + if in_fence && extract_flags(&block).iter().any(|f| !is_meta_flag(f)) { + return Some(block); + } None } @@ -101,10 +121,10 @@ pub(crate) fn heading_block(skill_md: &str, heading: &str) -> Option { } } -/// Spawn ` [cmd[1..]]` (e.g. `chronicle --help`) under a hard timeout, -/// push the outcome as a check, and return the captured stdout+stderr on -/// success. Spawn calls are emitted as `tracing::debug!` events by the shared -/// spawn core (design §8.2 --debug / --log-level debug). +/// True for the universal help/version meta-flags that every CLI implicitly +/// supports but does not (and should not) list among its own passable flags. +/// These are excluded from flag-drift comparison so a SKILL.md instruction like +/// "Run ` --help`" doesn't read as drift. pub fn is_meta_flag(flag: &str) -> bool { matches!(flag, "--help" | "-h" | "--version" | "-V" | "--help-all") }