robustness: polish parsers, cap reads, hide $HOME (Phase 8.4)

Phase 8.4 of the post-firebaguette audit closes 9 of the 17 remaining LOW
findings, plus adds a Trust model section to SECURITY.md.

Why: the LOW tier doesn't include single-shot exploits but it is a steady
trickle of bad-UX-becoming-bad-security if left untouched (cleartext HTTP
clones, orphan post-push commits, $HOME paths in CI logs, unbounded
SKILL.md reads). Knocking them out now keeps the audit closure rate ahead
of the next audit cycle.

Code changes:

* config::slug_for_url rejects http:// with a clear "use HTTPS" message
  (L1). MITM-on-clone surface gone.
* skill::parse_frontmatter strips a leading UTF-8 BOM before checking
  the opening --- fence; files with a BOM are no longer treated as
  "no frontmatter" (L2).
* skill::clean_value only strips quotes when they balance ("foo' is
  left as-is rather than silently coerced to foo) (L4).
* git::reset_hard_to_parent helper; push.rs and detect.rs roll back
  the orphan commit when git push fails after git commit succeeded (L7).
* skill::read_skill_md_bounded caps SKILL.md reads at 1 MiB and surfaces
  a per-skill warning instead of slurping a 5 GiB file (L8).
* git::clone passes --no-recurse-submodules; cargo-dist's release
  workflow now uses submodules: false on actions/checkout (L12 + L13).
* add.rs apply loop wrapped in per-skill IIFE: a single failure logs +
  continues + saves partial state (L15). Matches pull.rs (v0.1.4) and
  push.rs (v0.1.5).
* fs_util::display_path renders a $HOME-rooted path as ~/<rest>;
  applied at every "library cache not found" site + the init JSON's
  cache_path field so /Users/<operator>/ no longer leaks to CI logs
  or agent-mode JSON (L17).
* list.rs replaces a bare eprintln! with ui::log_warning (which is
  JSON-aware) (L18).

Docs:

* SECURITY.md gains a "Trust model" section that explicitly names the
  three trust boundaries (Trusted / Semi-trusted / Adversarial) plus
  an Out-of-scope list (compromised git binary, side-channel attacks).
  External auditors can now know where to look without reverse-engineering
  the code.

Deferred items (recorded with reasons in the audit notes):

* L3 (homograph warning) — needs unicode-confusables dep
* L5 (NFC normalisation) — needs unicode-normalization dep
* L6 (APFS case-insensitive collision warn) — UX-focused release
* L9 (Cargo.toml caret-semantics doc) — contributor-docs pass
* L10 (SLSA provenance) — release-workflow PR with its own dry-run
* L11 (cache-slug uniqueness) — pre-v1 single-library, revisit later
* L14 (fork destination prompt) — UX question for a broader fork review

11 new unit tests; cargo test: 147 pass; clippy clean; cargo audit clean.

Author: Fernando Pinho

.github/workflows/release.yml

@@ -59,7 +59,7 @@ jobs:
       - uses: actions/checkout@v6
         with:
           persist-credentials: false
-          submodules: recursive
+          submodules: false
       - name: Install dist
         # we specify bash to get pipefail; it guards against the `curl` command
         # failing. otherwise `sh` won't catch that `curl` returned non-0
@@ -119,7 +119,7 @@ jobs:
       - uses: actions/checkout@v6
         with:
           persist-credentials: false
-          submodules: recursive
+          submodules: false
       - name: Install Rust non-interactively if not already installed
         if: ${{ matrix.container }}
         run: |
@@ -178,7 +178,7 @@ jobs:
       - uses: actions/checkout@v6
         with:
           persist-credentials: false
-          submodules: recursive
+          submodules: false
       - name: Install cached dist
         uses: actions/download-artifact@v7
         with:
@@ -228,7 +228,7 @@ jobs:
       - uses: actions/checkout@v6
         with:
           persist-credentials: false
-          submodules: recursive
+          submodules: false
       - name: Install cached dist
         uses: actions/download-artifact@v7
         with:
@@ -340,4 +340,4 @@ jobs:
       - uses: actions/checkout@v6
         with:
           persist-credentials: false
-          submodules: recursive
+          submodules: false

CHANGELOG.md

@@ -6,6 +6,35 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
 
 ## [Unreleased]
 
+## [0.1.6] - 2026-05-25
+
+### Robustness & hygiene
+
+Close the audit's Phase 8.4 "low-impact polish" batch: 9 of the 17 remaining LOW findings, plus a new "Trust model" section in SECURITY.md that documents the boundaries underlying all the v0.1.2 → v0.1.6 hardening work. The 8 deferred LOW items either need a new runtime dependency (homograph detection, Unicode normalization), require a release-workflow change (SLSA provenance), or interact with pre-v1 design questions (slug-collision uniqueness, fork-destination UX).
+
+- **Force HTTPS in library URLs** (L1). `skillctl init http://github.com/owner/repo` was previously accepted and silently downgraded to cleartext for the initial clone. A network attacker on the operator's link could MITM the response and serve modified content. Now `slug_for_url` rejects `http://` with a clear "use HTTPS instead" message. SSH (`git@host:`, `ssh://`) is unchanged.
+- **UTF-8 BOM stripped before frontmatter parse** (L2). Some editors (Notepad on Windows, occasionally VS Code) prepend a `\u{feff}` BOM to UTF-8 files. The frontmatter parser saw `\u{feff}---` instead of `---` and treated the whole SKILL.md as "no frontmatter." Now the parser strips a leading BOM before checking the opening fence.
+- **Balanced quotes enforced in `clean_value`** (L4). `clean_value` was using `trim_matches(|c| c == '"' || c == '\'')` which silently stripped mismatched quotes — `"foo'` became `foo`. Mismatched quotes now pass through unchanged so the operator sees the malformed value and can fix it.
+- **`git push` failure rolls back the just-created commit** (L7). When `git commit` succeeds but `git push` fails (network blip, auth expiry), the local commit sat orphaned in the cache, ahead of upstream. The next `fetch_and_fast_forward` would silently `reset --hard @{upstream}` it away — or, post-M10, refuse to refresh because the working tree happened to get dirty in between. New `git::reset_hard_to_parent` helper, wired into both `push` and `detect`, restores the cache to a clean state on push failure.
+- **SKILL.md read capped at 1 MiB** (L8). `std::fs::read_to_string` for SKILL.md was unbounded — a 5 GiB file would be slurped silently into RAM during `discover`. New `read_skill_md_bounded` helper refuses to load more than 1 MiB and surfaces a per-skill warning instead.
+- **Submodule recursion disabled** (L12 + L13). `git clone` now passes `--no-recurse-submodules` explicitly so a malicious library with a `.gitmodules` pointing at attacker-controlled repos cannot pull-through during `skillctl init`. The cargo-dist release workflow's `actions/checkout` steps switched from `submodules: recursive` to `submodules: false` (we have no submodules; this is defense-in-depth that survives the next `cargo dist init` regeneration). Skills do not use submodules; if a legitimate use case appears, it can be opt-in via an explicit flag later.
+- **`add` continues on per-skill failure** (L15). The apply loop in `add` used `?` for `fs::remove_dir_all`, `copy_dir_all`, and the `source_path` strip-prefix — a single per-skill failure aborted the whole batch, and `.skills.toml` was only saved at the end, so partial successes were untracked. Now each skill is wrapped in an IIFE that logs a warning + continues on failure, and `project_config::save` always runs (capturing partial state). Same pattern as `pull` (v0.1.4) and `push` (v0.1.5).
+- **`$HOME` rendered as `~/` in displayed paths** (L17). Absolute paths in error messages and JSON output (`library cache not found at /Users/<operator>/Library/Caches/...`) leaked the operator's Unix username into CI logs and agent-mode JSON. New `fs_util::display_path(&path)` swaps a leading `$HOME` with `~/` and is applied at every "library cache not found" / cache-path-display site.
+- **`list`'s `eprintln!` routed through `ui::log_warning`** (L18). A single bare `eprintln!("warning: could not refresh library cache (...)")` in `list` bypassed the `--json` gating, polluting JSON consumers' stderr with non-JSON text. Now routed through the shared `ui::log_warning` helper, which is JSON-aware.
+- **SECURITY.md trust-model section**. New section that explicitly names the three trust boundaries — Trusted (operator's machine, interactive flags, the binary itself), Semi-trusted (library URL and cache), Adversarial (frontmatter, `.skills.toml`, git working tree, non-interactive flag values) — plus an explicit Out-of-scope list (compromised git binary, side-channel attacks). External auditors and contributors can now know where to look without reverse-engineering the code.
+
+11 new unit tests (1 HTTPS-required, 1 BOM strip, 4 balanced-quote, 2 SKILL.md size cap, 3 `$HOME` rendering). `cargo test`: 147 pass; clippy clean; `cargo audit` clean.
+
+**Deferred to a future release** (with reasons, since v0.1.6 explicitly chose to keep the scope minimal):
+
+- **L3** (homograph warning, e.g. Cyrillic `а` vs Latin `a` in skill names). Needs a `unicode-confusables` (or similar) dep; warrants its own decision before adding a runtime crate.
+- **L5** (NFC normalisation of paths/names). Needs `unicode-normalization`; same reasoning.
+- **L6** (case-insensitive FS collision warning on APFS-CI). No new dep but ~30 lines of runtime logic; deferred to a UX-focused release.
+- **L9** (Cargo.toml caret-semantics doc). Documentation-only; will land alongside a broader contributor-docs pass.
+- **L10** (SLSA provenance / cosign attestations on release binaries). Release-workflow change; deserves its own PR + dry-run on a tag.
+- **L11** (cache-slug collision via hash suffix). Pre-v1 with one-library-at-a-time, slug collisions are theoretical only; revisit if multi-library support lands.
+- **L14** (prompt operator on fork destination instead of inheriting the source's parent). UX question best decided alongside a broader `fork` flow review.
+
 ## [0.1.5] - 2026-05-22
 
 ### Security & robustness

Cargo.lock

@@ -1,6 +1,6 @@
 [package]
 name = "skillctl"
-version = "0.1.5"
+version = "0.1.6"
 edition = "2024"
 rust-version = "1.85"
 description = "CLI to manage your personal agent skills library across projects"

Cargo.toml

@@ -7,6 +7,35 @@
 - It reads YAML-like frontmatter from `SKILL.md` files and writes TOML to `.skills.toml` and the global config file.
 - There is no network surface beyond what `git` does, no privileged operations, and no telemetry.
 
+## Trust model
+
+`skillctl`'s internal validation and sanitisation is shaped by the following trust boundaries. External auditors and contributors should focus probes on the **adversarial** category — anything in the **trusted** category is taken at face value.
+
+**Trusted (taken at face value):**
+
+- The operator's machine — filesystem, `$PATH`, environment variables, git binary, git's configured credentials.
+- Flags typed by the operator on an interactive TTY. When skillctl runs interactively, `--dest <absolute-path>` and other path-accepting flags are accepted as-is.
+- The skillctl binary itself, its dependencies (audited via `cargo audit` on every release), and the configuration written to the per-user config file (which only skillctl writes).
+
+**Semi-trusted (the operator chose the source but its content is treated as adversarial):**
+
+- The library repository URL passed to `skillctl init`. The host (GitHub) is trusted; the *content* it serves is not.
+- The library cache (`~/Library/Caches/dev.umanio-agency.skills-cli/<slug>/`). Skillctl owns the directory but treats every file under it as adversarial after the initial clone.
+
+**Adversarial (treated as untrusted in every code path that touches them):**
+
+- `SKILL.md` frontmatter (`name`, `description`, `tags`) from any source — library cache, project tree, fork-locally targets. Sanitised at the discovery boundary; control bytes / ANSI / NUL / CRLF are rejected; oversize files (> 1 MiB) are refused.
+- `.skills.toml` entries, especially those that arrive via PR. `name`, `source_path`, `source_sha`, `destination` are validated at load: identifier-class for `name`, hex regex for `source_sha`, lexical subpath check (no `..`, no absolute, no Windows prefix) for the two `PathBuf` fields. Duplicates and unknown fields are rejected.
+- The library cache's git working tree and submodules. `git clone --no-recurse-submodules` blocks submodule pull-through; every git invocation runs with `-c core.hooksPath=/dev/null` so a malicious library cannot ship hook scripts.
+- Skill folder contents: symlinks, hardlinks (Unix), FIFOs, devices, and sockets are refused at copy time. File modes are masked to `0o644 | (src_mode & 0o100)` on Unix — only the user-execute bit propagates.
+- Non-interactive flag values (`--dest <path>` in `--json` / `--no-interaction` mode, where the "operator" may be an LLM running on attacker-supplied input). Absolute paths are rejected; `..` is always rejected.
+
+**Out of scope (not defended against):**
+
+- A compromised git binary or local credential helper. Skillctl uses whatever git you point it at; if `which git` returns a trojan, all bets are off.
+- A compromised library repository owned by a third party that the operator explicitly trusts (e.g. corporate skills repo). Skillctl reduces blast radius via the controls above, but cannot make a malicious-but-trusted library safe.
+- Side-channel attacks via filesystem timing, memory analysis, or OS-level surveillance. The threat model assumes a normal single-user developer machine.
+
 ## Reporting a vulnerability
 
 If you find a security issue (e.g. a way to make `skillctl` write outside the destinations it's supposed to, or to leak credentials in error messages), please report it privately:

SECURITY.md

@@ -58,7 +58,7 @@ pub fn run(args: AddArgs, ctx: &Context) -> Result<()> {
     if !library_root.exists() {
         return Err(AppError::Config(format!(
             "library cache not found at {} — run `skillctl init{}` again",
-            library_root.display(),
+            fs_util::display_path(&library_root),
             library.url
         ))
         .into());
@@ -104,20 +104,43 @@ pub fn run(args: AddArgs, ctx: &Context) -> Result<()> {
     let mut aborted = false;
 
     for skill in selected {
-        let folder_name = skill.path.file_name().ok_or_else(|| {
-            AppError::Config(format!(
-                "skill has no folder name: {}",
-                skill.path.display()
-            ))
-        })?;
-        let dest = dest_root.join(folder_name);
+        let folder_name = match skill.path.file_name() {
+            Some(n) => n.to_os_string(),
+            None => {
+                let _ = ui::log_warning(
+                    ctx,
+                    format!(
+                        "skipping {}: source has no folder name ({})",
+                        skill.name,
+                        skill.path.display()
+                    ),
+                );
+                results.push(json!({
+                    "name": skill.name,
+                    "status": "failed",
+                    "reason": "source has no folder name",
+                }));
+                continue;
+            }
+        };
+        let dest = dest_root.join(&folder_name);
 
         if dest.exists() {
             let action = resolve_conflict(ctx, &dest, conflict_policy.clone())?;
             match action {
                 ConflictAction::Overwrite => {
-                    fs::remove_dir_all(&dest)
-                        .with_context(|| format!("removing {}", dest.display()))?;
+                    if let Err(e) = fs::remove_dir_all(&dest)
+                        .with_context(|| format!("removing {}", dest.display()))
+                    {
+                        let _ =
+                            ui::log_warning(ctx, format!("add failed for `{}`: {e}", skill.name));
+                        results.push(json!({
+                            "name": skill.name,
+                            "status": "failed",
+                            "reason": e.to_string(),
+                        }));
+                        continue;
+                    }
                 }
                 ConflictAction::Skip => {
                     ui::log_info(ctx, format!("skipped {}", skill.name))?;
@@ -129,6 +152,9 @@ pub fn run(args: AddArgs, ctx: &Context) -> Result<()> {
                     continue;
                 }
                 ConflictAction::Abort => {
+                    // Save what's been installed up to this point before
+                    // bailing — we may already have pushed entries for
+                    // earlier-loop-iteration skills.
                     project_config::save(&cwd, &project_cfg)?;
                     ui::outro_cancel(ctx, "aborted")?;
                     results.push(json!({
@@ -142,35 +168,67 @@ pub fn run(args: AddArgs, ctx: &Context) -> Result<()> {
             }
         }
 
-        fs_util::copy_dir_all(&skill.path, &dest)?;
-        let source_path = skill
-            .path
-            .strip_prefix(&library_root)
-            .with_context(|| {
-                format!(
-                    "computing path of {} relative to library at {}",
-                    skill.path.display(),
-                    library_root.display()
-                )
-            })?
-            .to_path_buf();
-        let destination_rel = fs_util::relative_to_or_self(&dest, &cwd);
-        project_cfg.installed.push(InstalledSkill {
-            name: skill.name.clone(),
-            source_path,
-            source_sha: source_sha.clone(),
-            destination: destination_rel.clone(),
-            installed_at: installed_at.clone(),
-        });
-        ui::log_success(ctx, format!("{} → {}", skill.name, dest.display()))?;
-        results.push(json!({
-            "name": skill.name,
-            "status": "installed",
-            "path": destination_rel.display().to_string(),
-            "source_sha": source_sha,
-        }));
+        // Per-skill IIFE: a single skill's copy failure (symlink reject,
+        // hardlink reject, FIFO inside, oversize SKILL.md, ...) logs and
+        // continues with the next skill instead of aborting the batch and
+        // orphaning partial work. The final `project_config::save` below
+        // captures every successful entry, so a half-finished `--all` run
+        // still produces a usable `.skills.toml`.
+        let outcome: Result<InstalledSkill> = (|| {
+            fs_util::copy_dir_all(&skill.path, &dest)?;
+            let source_path = skill
+                .path
+                .strip_prefix(&library_root)
+                .with_context(|| {
+                    format!(
+                        "computing path of {} relative to library at {}",
+                        skill.path.display(),
+                        library_root.display()
+                    )
+                })?
+                .to_path_buf();
+            let destination_rel = fs_util::relative_to_or_self(&dest, &cwd);
+            Ok(InstalledSkill {
+                name: skill.name.clone(),
+                source_path,
+                source_sha: source_sha.clone(),
+                destination: destination_rel,
+                installed_at: installed_at.clone(),
+            })
+        })();
+        match outcome {
+            Ok(entry) => {
+                let dest_display = entry.destination.display().to_string();
+                let source_sha_for_json = entry.source_sha.clone();
+                project_cfg.installed.push(entry);
+                ui::log_success(ctx, format!("{} → {}", skill.name, dest.display()))?;
+                results.push(json!({
+                    "name": skill.name,
+                    "status": "installed",
+                    "path": dest_display,
+                    "source_sha": source_sha_for_json,
+                }));
+            }
+            Err(e) => {
+                // Best-effort cleanup of any partial copy left behind by
+                // copy_dir_all's tempdir-or-final-rename path. The atomic
+                // swap pattern means dst is either old or new content, so
+                // there's typically nothing to undo — but if the failure
+                // happened during the staging copy we may still have a
+                // `.skillctl-tmp.*` sibling around (it normally cleans
+                // itself up; this is a paranoid sweep).
+                let _ = ui::log_warning(ctx, format!("add failed for `{}`: {e}", skill.name));
+                results.push(json!({
+                    "name": skill.name,
+                    "status": "failed",
+                    "reason": e.to_string(),
+                }));
+            }
+        }
     }
 
+    // Always save: captures both fully-successful batches and partial
+    // successes after one or more per-skill failures.
     project_config::save(&cwd, &project_cfg)?;
 
     if !aborted {

src/commands/add.rs

@@ -41,7 +41,7 @@ pub fn run(args: DetectArgs, ctx: &Context) -> Result<()> {
     if !library_root.exists() {
         return Err(AppError::Config(format!(
             "library cache not found at {} — run `skillctl init{}` again",
-            library_root.display(),
+            fs_util::display_path(&library_root),
             library.url
         ))
         .into());
@@ -169,7 +169,16 @@ pub fn run(args: DetectArgs, ctx: &Context) -> Result<()> {
         format!("add skills: {}", names.join(", "))
     };
     let new_sha = git::commit(&library_root, &message).map_err(|e| AppError::Git(e.to_string()))?;
-    git::push(&library_root).map_err(|e| AppError::Git(e.to_string()))?;
+    // Symmetric with push.rs: roll back the orphaned commit on push failure.
+    if let Err(e) = git::push(&library_root) {
+        if let Err(rollback_err) = git::reset_hard_to_parent(&library_root) {
+            let _ = ui::log_warning(
+                ctx,
+                format!("could not roll back the local commit after push failure: {rollback_err}"),
+            );
+        }
+        return Err(AppError::Git(e.to_string()).into());
+    }
 
     let installed_at = OffsetDateTime::now_utc()
         .format(&Rfc3339)

src/commands/detect.rs

@@ -59,7 +59,7 @@ pub fn classify(
         None => {
             return Err(anyhow::anyhow!(
                 "library HEAD doesn't resolve in the cache at {}",
-                library_root.display()
+                crate::fs_util::display_path(library_root)
             ));
         }
     };