From e89e821945e6e7b35707d44c6977c9fc2e2f8323 Mon Sep 17 00:00:00 2001 From: Icaro0310 Date: Tue, 27 Oct 2026 16:27:51 +0000 Subject: [PATCH 1/2] feat(skill-catalog): import-bundle --clean and lint --slop import-bundle --clean replaces the catalog content for the bundle's scope instead of merging: pre-clean state (store copies + registry entries) is snapshotted to skill-catalog/import-snapshots/ first, each store copy is staged and swapped atomically, and a mid-run failure rolls back -- fail-closed, never a half-replaced catalog. import-bundle --restore SNAPSHOT_DIR recovers a snapshot manually (plan-first). Items still land quarantined; retired items are skipped and items absent from the bundle are untouched. Adds a same-state Registry.replace so re-importing an already-quarantined item stays auditable. lint --slop adds a deterministic anti-slop pass over item markdown with five regex rules -- slop-fingerprint, slop-emdash, slop-sycophancy, slop-not-just-but, slop-filler -- each with a documented rationale and a shared exceptions mechanism (slop-allow / slop-allow: line markers, slop-allow-file). Fenced code blocks are skipped. Findings carry rule id + line refs and exit 1; --warn-only reports WARN and exits 0. Generated with [Devin](https://devin.ai) & QA Engineer Icaro Galvao Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- packages/skill-catalog/CHANGELOG.md | 21 + packages/skill-catalog/README.md | 52 +- packages/skill-catalog/STATUS.md | 19 +- packages/skill-catalog/llms.txt | 3 +- .../src/devin_skill_catalog/bundle.py | 295 ++++++++++- .../src/devin_skill_catalog/cli.py | 174 ++++++- .../src/devin_skill_catalog/registry.py | 32 ++ .../src/devin_skill_catalog/slop.py | 222 +++++++++ .../skill-catalog/tests/test_bundle_clean.py | 463 ++++++++++++++++++ packages/skill-catalog/tests/test_slop.py | 266 ++++++++++ 10 files changed, 1529 insertions(+), 18 deletions(-) create mode 100644 packages/skill-catalog/src/devin_skill_catalog/slop.py create mode 100644 packages/skill-catalog/tests/test_bundle_clean.py create mode 100644 packages/skill-catalog/tests/test_slop.py diff --git a/packages/skill-catalog/CHANGELOG.md b/packages/skill-catalog/CHANGELOG.md index c488fdb..80262ba 100644 --- a/packages/skill-catalog/CHANGELOG.md +++ b/packages/skill-catalog/CHANGELOG.md @@ -28,6 +28,27 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- `import-bundle --clean`: replace the catalog content for the bundle's + scope instead of merging. The pre-clean state (store copies + + registry entries) is snapshotted to + `.devin-ecosystem/skill-catalog/import-snapshots/` first, each store + copy is staged and swapped atomically (`os.rename`), and a mid-run + failure rolls the touched items back — fail-closed, never a + half-replaced catalog. `import-bundle --restore SNAPSHOT_DIR` + recovers a snapshot manually (plan-first like every mutation); + snapshots are kept until pruned. Items still land `quarantined`: + `approved`/`active` items are demoted, already-quarantined items get + a same-state `Registry.replace` history entry, `retired` items are + skipped, and items absent from the bundle are untouched. +- `lint --slop`: a deterministic content pass over each item's markdown + (SKILL.md, rule files, bundled `.md`s) with five regex rules — + `slop-fingerprint` (stock LLM phrases), `slop-emdash` (dash clause + separators; ranges like `3.10–3.14` exempt), `slop-sycophancy` + (assistant openers/closers), `slop-not-just-but` and `slop-filler`. + Fenced code blocks are skipped; `slop-allow` / `slop-allow:` + line markers and `slop-allow-file` suppress findings so legitimate + content is never blocked by a bare denylist. Findings carry rule id + + line refs and exit 1; `--warn-only` reports WARN and exits 0. - `g3 ITEM --judge --evidence PATH [--out PATH]` — produce a `g3-report/0.1` document by asking poordjaevin (the optional `judge` extra) three concrete yes/no questions about the promotion evidence. diff --git a/packages/skill-catalog/README.md b/packages/skill-catalog/README.md index 389bcae..1ac28b7 100644 --- a/packages/skill-catalog/README.md +++ b/packages/skill-catalog/README.md @@ -53,7 +53,9 @@ content. `.devin/rules/*.md`, hashes each item (sha256), annotates it with its registry state. - **Lint** — structural checks: frontmatter presence, `name` matching - the directory, a real `description`, `# ` titles on rules. + the directory, a real `description`, `# ` titles on rules. `lint + --slop` adds a deterministic content pass: five regex heuristics for + LLM-flavored prose, each with a per-line/per-file allowlist escape. - **Gates** — two offline evidence gates (G1/G2, below) plus a G3 promotion gate driven by `g3-report` verdicts. G1/G2 are heuristics, **not proof**. @@ -83,6 +85,7 @@ PyPI install produce the same CLI. ```bash devin-skill-catalog scan [PATH ...] # inventory (read-only) devin-skill-catalog lint [PATH ...] # structural findings + [--slop] [--warn-only] # deterministic anti-slop rules devin-skill-catalog diff A B # inventory diff by sha256 devin-skill-catalog gate g1 PATH # static hygiene gate devin-skill-catalog gate g2 PATH [--packs-dir D] # grounding gate @@ -96,6 +99,8 @@ devin-skill-catalog activate ITEM # approved → active devin-skill-catalog retire ITEM # any state → retired devin-skill-catalog export-bundle --out DIR # pack approved items devin-skill-catalog import-bundle PATH # land items quarantined + [--clean] # replace item copies the bundle carries + [--restore SNAP] # recover a pre-clean snapshot ``` `ITEM` is `kind:name` (`skill:foo`, `rule:bar`) or a bare name when it @@ -124,6 +129,44 @@ The only write targets are the registry file and quarantine store under the Devin config dir, plus whatever you point `--out` at. Quarantine snapshots the item; removing the original from `.devin/` is your call. +**Clean imports.** `import-bundle` merges by default: an item that is +already registered keeps its stored copy and the import skips it. +`import-bundle --clean` *replaces* the catalog content for the bundle's +scope instead — the stored copy of every item the bundle carries is +swapped for the bundle's version. Safely, in order: + +1. the pre-clean state (store copies + registry entries) is snapshotted + to `.devin-ecosystem/skill-catalog/import-snapshots//` first; +2. each new copy is staged in a sibling dir and swapped in with + `os.rename` — no half-written stores; +3. a mid-run failure rolls the touched items back, and + `import-bundle --restore --apply` recovers the same state + manually. Snapshots are kept until you prune them. + +Clean still lands everything `quarantined` (`approved`/`active` items +are demoted; `retired` items are skipped — a clean import does not +resurrect a deliberate end-state), and items absent from the bundle are +untouched. + +**Slop lint.** `lint --slop PATH` runs the deterministic anti-slop +rules over each item's markdown (SKILL.md, rule files, bundled `.md`s): + +| rule | what it flags | +|------|---------------| +| `slop-fingerprint` | stock LLM phrases — `delve into`, `seamless`, `robust`, `comprehensive`, `it's worth noting`, `in summary`/`in conclusion`, sentence-initial `Additionally,`/`Moreover,`/`Furthermore,` | +| `slop-emdash` | `—` (and spaced `–`) used as a clause separator — not en-dashes in ranges like `3.10–3.14` | +| `slop-sycophancy` | assistant-style openers/closers — `Great question`, `Happy to help`, `Hope this helps`, `Let me know`, `You're absolutely right` | +| `slop-not-just-but` | the `not just X but Y` escalation (also `not only/merely/simply`) | +| `slop-filler` | hype/throat-clearing — `in today's fast-paced world`, `cutting-edge`, `game-changer`, `a testament to`, `plays a vital role`, `at the end of the day`, `first and foremost`, `when it comes to`, `in the world of` | + +Lines inside fenced code blocks are skipped (examples are not prose), +and every rule honors the same allowlist escapes — a finding is +suppressed when its line carries `slop-allow`, `slop-allow:`, +or the file carries `slop-allow-file` (put the marker in an HTML +comment: ``). Findings print with the +rule id and line number and exit 1; `--warn-only` reports them as WARN +and exits 0. + ## Lifecycle ``` @@ -271,9 +314,10 @@ Rules of the road: quarantine on the receiving side. - On upstream changes, re-vendor by **replacing** the staged item dir (delete it, then copy) and re-quarantining from scratch — copying over - the old tree leaves files upstream deleted, and the quarantine store - merges rather than replaces, so stale files would survive the - re-review. + the old tree leaves files upstream deleted, and the plain + `import-bundle` merges rather than replaces, so stale files would + survive the re-review (`import-bundle --clean` is the replacing + variant). Worked example: 25 skills from `addyosmani/agent-skills` (MIT), 6 from `DietrichGebert/ponytail` (MIT) and a local `graphify` install were diff --git a/packages/skill-catalog/STATUS.md b/packages/skill-catalog/STATUS.md index d20efcb..5e2cbbd 100644 --- a/packages/skill-catalog/STATUS.md +++ b/packages/skill-catalog/STATUS.md @@ -16,6 +16,10 @@ block scalars), degrades gracefully. - `lint.py` — structural findings: frontmatter block, `name` vs dir, real `description`, `# ` title on rules. +- `slop.py` — deterministic anti-slop rules (`slop-fingerprint`, + `slop-emdash`, `slop-sycophancy`, `slop-not-just-but`, `slop-filler`) + over item markdown; fenced code skipped; `slop-allow[:]` / + `slop-allow-file` allowlist markers. - `gates.py` — G1 (lint + injection/exfil/`curl|sh`/destructive-shell phrasing + secret-shaped strings, values suppressed) and G2 (declared files/commands grounded, body references WARN, `--packs-dir` verifies @@ -36,23 +40,28 @@ - `bundle.py` — export `approved` items to dir/`.tar[.gz]` with `manifest.json` (items, per-file sha256, source profile); import verifies checksums and lands items `quarantined`, never active. -- `cli.py` — `scan`, `lint`, `diff`, `gate g1|g2`, `g3 --judge`, + `--clean` replaces the store copies the bundle carries (pre-clean + snapshot to `import-snapshots/`, atomic `os.rename` swap, rollback on + failure); `--restore` recovers a snapshot. +- `cli.py` — `scan`, `lint` (structural + `--slop` anti-slop mode), + `diff`, `gate g1|g2`, `g3 --judge`, `quarantine`, `promote` (runs G1 on the stored copy, `--force` overrides G1; enforces the G3 policy — always-on rules require a `g3-report` with `improves`/`no-detectable-effect`, `regresses` hard-blocks every kind even under `--force`, `inconclusive` skills need `--g3-inconclusive-reason`), `activate`, `retire`, `export-bundle`, - `import-bundle`. Mutations are plan-first: no writes without + `import-bundle` (`--clean` safe replacement, `--restore` recovery). + Mutations are plan-first: no writes without `--apply`. -- Tests: 123 (+1 when the `judge` extra is installed), all on +- Tests: 216 (+1 when the `judge` extra is installed), all on synthetic `.devin/` fixture trees; judge tests use scripted clients/backends — never a real model. ## Verified -- `python -m pytest packages/skill-catalog` — 124 passed (Linux / - Python 3.14, poordjaevin installed), 123 + 1 skip without it. +- `python -m pytest packages/skill-catalog` — 217 passed (Linux / + Python 3.14, poordjaevin installed), 216 + 1 skip without it. - Manual end-to-end: scan → quarantine --apply → promote --apply (G1 pass) → export-bundle → import-bundle --apply lands quarantined. - Manual `g3 --judge --evidence` run through a real poordjaevin diff --git a/packages/skill-catalog/llms.txt b/packages/skill-catalog/llms.txt index b83d163..59149a7 100644 --- a/packages/skill-catalog/llms.txt +++ b/packages/skill-catalog/llms.txt @@ -11,10 +11,11 @@ ## Key commands - `devin-skill-catalog scan [PATH ...]`: inventory skills/rules (read-only) - `devin-skill-catalog lint|gate g1|gate g2 PATH`: findings, exit 1 on FAIL +- `devin-skill-catalog lint --slop PATH`: deterministic anti-slop rules (`slop-fingerprint|emdash|sycophancy|not-just-but|filler`; `slop-allow[:]`/`slop-allow-file` markers suppress; `--warn-only` exits 0) - `devin-skill-catalog g3 ITEM --judge --evidence FILE [--out R.json]`: produce a g3-report via poordjaevin (optional `judge` extra; `POORDJAEVIN_BACKEND` selects the backend, default keyless local NLI) - `devin-skill-catalog quarantine|promote|activate|retire ITEM --apply`: lifecycle mutations (plan printed without `--apply`) - `devin-skill-catalog diff A B`: inventory diff by content hash -- `devin-skill-catalog export-bundle --out DIR` / `import-bundle PATH --apply`: move approved items, import lands quarantined +- `devin-skill-catalog export-bundle --out DIR` / `import-bundle PATH --apply`: move approved items, import lands quarantined (`--clean` replaces the copies the bundle carries — pre-clean snapshot + atomic swap + rollback; `--restore SNAP` recovers) - `pipx install "devin-skill-catalog @ git+https://github.com/Icaro0310/devin-devkit.git#subdirectory=packages/skill-catalog"`: install the CLI - `python -m pytest`: run the test suite (synthetic fixtures only) diff --git a/packages/skill-catalog/src/devin_skill_catalog/bundle.py b/packages/skill-catalog/src/devin_skill_catalog/bundle.py index b12725d..c765eaf 100644 --- a/packages/skill-catalog/src/devin_skill_catalog/bundle.py +++ b/packages/skill-catalog/src/devin_skill_catalog/bundle.py @@ -16,6 +16,13 @@ ``active``. Promotion still requires the gates. Every file's sha256 is verified against the manifest before it lands in the store. +``import-bundle --clean`` *replaces* the catalog content for the +bundle's scope instead of merging: the pre-clean state is snapshotted +to ``import-snapshots/`` first, each store copy is swapped atomically, +and a mid-run failure rolls back — ``--restore`` recovers a snapshot +manually. ``retired`` items are skipped and items absent from the +bundle are untouched. + **Incremental export** (``--incremental``) is a manifest-hash diff: a shared ``devin_internals.cursor.Cursor`` named ``bundle-export`` stores the per-item sha256 map of the last committed export, and items whose @@ -33,6 +40,7 @@ from __future__ import annotations +import copy import hashlib import io import json @@ -48,16 +56,20 @@ from devin_skill_catalog.model import ( EXPORTABLE_STATES, KINDS, + STATE_PROPOSED, STATE_QUARANTINED, + STATE_RETIRED, Item, item_key, ) -from devin_skill_catalog.paths import quarantine_item_dir +from devin_skill_catalog.paths import quarantine_item_dir, store_dir from devin_skill_catalog.registry import Registry, utc_now_iso BUNDLE_VERSION = 1 MANIFEST_NAME = "manifest.json" ITEMS_DIRNAME = "items" +SNAPSHOT_MANIFEST_NAME = "snapshot.json" +IMPORT_SNAPSHOTS_DIRNAME = "import-snapshots" _TAR_SUFFIXES = (".tar", ".tar.gz", ".tgz") @@ -584,3 +596,284 @@ def apply_import( ) results.append({"key": plan["key"], "action": "quarantined"}) return results + + +# -------------------------------------------------------------------------- +# clean import — replace, recoverably +# -------------------------------------------------------------------------- +# +# ``import-bundle --clean`` replaces the catalog content for the bundle's +# scope (the items the bundle carries) instead of merging. The safety +# envelope is fail-closed: +# +# 1. ``snapshot_clean_scope`` copies every pre-existing store copy and +# registry entry the bundle touches into +# ``skill-catalog/import-snapshots/-/`` BEFORE anything is +# mutated. +# 2. Each item's new content is staged in a sibling dir and swapped in +# with ``os.rename`` — a crash never leaves a half-written store copy. +# 3. Any failure rolls the touched items back from the snapshot; if the +# rollback itself fails, ``import-bundle --restore `` +# recovers the same state. Snapshots are kept until manually pruned — +# recovery data is never deleted automatically. +# +# Items still land ``quarantined``: new/proposed, approved and active +# items transition; an already-quarantined item keeps its state +# (``Registry.replace`` records the swap in history). ``retired`` items +# are skipped — a deliberate end-state a clean import does not +# resurrect. Items absent from the bundle are untouched. + + +def _new_snapshot_dir(config_dir: Path) -> Path: + """A fresh ``import-snapshots/-[-n]`` path — never reused.""" + base = store_dir(config_dir) / IMPORT_SNAPSHOTS_DIRNAME + stamp = utc_now_iso().replace("-", "").replace(":", "") + cand = base / f"{stamp}-{os.getpid()}" + n = 0 + while cand.exists(): + n += 1 + cand = base / f"{stamp}-{os.getpid()}-{n}" + return cand + + +def _entry_store_dir( + entry: dict | None, config_dir: Path, kind: str, name: str +) -> Path | None: + """The dir holding the item's current stored copy — the recorded + ``store`` path wins; else the canonical quarantine dir when it holds + files.""" + if entry and entry.get("store"): + p = Path(entry["store"]) + if p.is_dir(): + return p + p = quarantine_item_dir(config_dir, kind, name) + return p if p.is_dir() else None + + +def snapshot_clean_scope( + config_dir: Path, registry: Registry, plans: list[dict] +) -> Path: + """Snapshot the pre-clean state of every item the plans touch — + the store copy (recorded ``store`` path or canonical quarantine dir) + plus a deep copy of each registry entry — into + ``import-snapshots/-/``. Entries that did not exist + pre-clean are recorded as ``null`` so restore removes them.""" + snap = _new_snapshot_dir(config_dir) + entries: dict[str, dict | None] = {} + for plan in plans: + kind, name = plan["kind"], plan["name"] + entry = registry.get(kind, name) + entries[plan["key"]] = copy.deepcopy(entry) if entry else None + src = _entry_store_dir(entry, config_dir, kind, name) + if src is not None: + shutil.copytree(src, snap / "items" / kind / name) + snap.mkdir(parents=True, exist_ok=True) + _atomic_write_bytes( + snap / SNAPSHOT_MANIFEST_NAME, + ( + json.dumps( + { + "version": 1, + "created_at": utc_now_iso(), + "tool": "devin-skill-catalog", + "tool_version": __version__, + "entries": entries, + }, + indent=2, + sort_keys=True, + ) + + "\n" + ).encode("utf-8"), + ) + return snap + + +def load_clean_snapshot(snapshot_dir: Path) -> dict: + """Read a snapshot's manifest — raises ``BundleError`` when the dir + is not a readable pre-clean snapshot.""" + snap = Path(snapshot_dir) + try: + raw = (snap / SNAPSHOT_MANIFEST_NAME).read_bytes() + except OSError as exc: + raise BundleError( + f"{snap} is not a readable pre-clean snapshot" + ) from exc + try: + meta = json.loads(raw.decode("utf-8")) + except (UnicodeDecodeError, json.JSONDecodeError) as exc: + raise BundleError( + f"snapshot manifest in {snap} is not valid JSON" + ) from exc + if not isinstance(meta, dict) or not isinstance( + meta.get("entries"), dict + ): + raise BundleError( + f"{snap} is not a pre-clean snapshot (no entries map)" + ) + return meta + + +def restore_clean_snapshot( + snapshot_dir: Path, registry: Registry, config_dir: Path +) -> list[str]: + """Restore the pre-clean state captured by + :func:`snapshot_clean_scope`: the current store copy of every + snapshotted item is removed and its pre-clean copy put back (items + with no prior copy keep none), and registry entries return to their + pre-clean content with a ``restored`` history note appended. + Returns the restored keys.""" + meta = load_clean_snapshot(snapshot_dir) + snap = Path(snapshot_dir) + restored: list[str] = [] + for key in sorted(meta["entries"]): + entry = meta["entries"][key] + kind, _, name = key.partition(":") + if kind not in KINDS or not name or ( + entry is not None and not isinstance(entry, dict) + ): + raise BundleError(f"snapshot entry {key!r} is malformed") + canon = quarantine_item_dir(config_dir, kind, name) + snap_copy = snap / "items" / kind / name + try: + if canon.is_dir(): + shutil.rmtree(canon) + if snap_copy.is_dir(): + dest = ( + Path(entry["store"]) + if entry and entry.get("store") + else canon + ) + if dest.exists(): + shutil.rmtree(dest) + dest.parent.mkdir(parents=True, exist_ok=True) + shutil.copytree(snap_copy, dest) + except OSError as exc: + raise BundleError( + f"restore of {key} failed — snapshot {snap} is intact, " + f"re-run --restore: {exc}" + ) from exc + if entry is None: + registry.entries().pop(key, None) + else: + prev_state = (registry.get(kind, name) or {}).get("state") + restored_entry = copy.deepcopy(entry) + restored_entry.setdefault("history", []).append( + { + "at": utc_now_iso(), + "from": prev_state or entry.get("state"), + "to": entry.get("state"), + "note": f"restored from pre-clean snapshot {snap}", + } + ) + registry.entries()[key] = restored_entry + restored.append(key) + return restored + + +def _swap_store_copy( + reader: BundleReader, config_dir: Path, plan: dict +) -> Path: + """Stage the item's bundle files in a sibling dir and atomically + replace its quarantine-store copy. Returns the store path.""" + kind, name = plan["kind"], plan["name"] + store = quarantine_item_dir(config_dir, kind, name) + stage = store.with_name(f"{store.name}.new-{os.getpid()}") + prev = store.with_name(f"{store.name}.prev-{os.getpid()}") + store.parent.mkdir(parents=True, exist_ok=True) + try: + if stage.exists(): # stale leftover from a crashed run + shutil.rmtree(stage) + stage.mkdir(parents=True) # an empty plan swaps to an empty dir + for fspec in plan["files"]: + data = reader.read_bytes( + f"{ITEMS_DIRNAME}/{kind}/{name}/{fspec['path']}" + ) + dest = stage / fspec["path"] + dest.parent.mkdir(parents=True, exist_ok=True) + dest.write_bytes(data) + except BaseException: + shutil.rmtree(stage, ignore_errors=True) + raise + had_prev = store.exists() + if had_prev: + if prev.exists(): # stale leftover from a crashed run + shutil.rmtree(prev) + os.rename(store, prev) + try: + os.rename(stage, store) + except OSError: + if had_prev: + os.rename(prev, store) # put the old copy back + raise + if had_prev: + shutil.rmtree(prev, ignore_errors=True) + return store + + +def apply_clean_import( + reader: BundleReader, + registry: Registry, + config_dir: Path, + plans: list[dict], + *, + note: str = "", +) -> tuple[list[dict], Path | None]: + """The ``--clean`` import: REPLACE the stored copy of each item the + bundle carries instead of merging. Returns ``(results, snapshot)`` — + the same per-item result dicts as :func:`apply_import` plus the + pre-clean snapshot dir (``None`` for an empty plan; kept for + ``--restore`` after success). + + Fail-closed: a raised error means the touched items were rolled back + to the snapshot (or the error text carries the snapshot path for a + manual ``--restore``).""" + if not plans: + return [], None + snapshot = snapshot_clean_scope(config_dir, registry, plans) + results: list[dict] = [] + try: + for plan in plans: + kind, name = plan["kind"], plan["name"] + state = registry.state_of(kind, name) or STATE_PROPOSED + if state == STATE_RETIRED: + results.append( + { + "key": plan["key"], + "action": "skipped", + "reason": "retired — a clean import does not " + "resurrect it", + } + ) + continue + store = _swap_store_copy(reader, config_dir, plan) + hist_note = ( + note + or f"clean import — pre-clean snapshot {snapshot}" + ) + if state == STATE_QUARANTINED: + registry.replace( + kind, name, note=hist_note, store=str(store) + ) + else: + registry.transition( + kind, + name, + STATE_QUARANTINED, + note=hist_note, + store=str(store), + ) + results.append({"key": plan["key"], "action": "quarantined"}) + except BaseException as exc: # noqa: BLE001 — an interrupted clean + # must roll back too; re-raised below + try: + restore_clean_snapshot(snapshot, registry, config_dir) + tail = f"pre-clean state restored from {snapshot}" + except (OSError, BundleError) as rb_exc: + tail = ( + f"rollback failed ({rb_exc}) — recover with " + f"import-bundle --restore {snapshot}" + ) + raise BundleError( + f"clean import failed — {tail}: {exc}" + ) from exc + return results, snapshot diff --git a/packages/skill-catalog/src/devin_skill_catalog/cli.py b/packages/skill-catalog/src/devin_skill_catalog/cli.py index 19a4221..9041814 100644 --- a/packages/skill-catalog/src/devin_skill_catalog/cli.py +++ b/packages/skill-catalog/src/devin_skill_catalog/cli.py @@ -4,6 +4,8 @@ scan [PATH ...] inventory skills/rules (read-only) lint [PATH ...] structural lint findings + (--slop: deterministic anti-slop + content rules on item markdown) diff A B inventory diff between two dirs gate g1|g2 PATH offline gates; --apply records result g3 ITEM --judge --evidence F produce a g3-report/0.1 via poordjaevin @@ -15,6 +17,9 @@ (--incremental: manifest-hash diff via the bundle-export cursor) import-bundle PATH land items quarantined (never active) + (--clean: replace the catalog copies + the bundle carries — snapshot first, + atomic swap, --restore recovers) Lifecycle mutations are **plan-first**: without ``--apply`` the command prints what it would do and writes nothing. The registry lives at @@ -43,6 +48,7 @@ judge, lint, scan, + slop, ) from devin_skill_catalog.diffing import ( DiffRow, @@ -229,7 +235,17 @@ def cmd_scan(args) -> int: def cmd_lint(args) -> int: - return _emit(args, lint.lint_items(_scan(args))) + if args.warn_only and not args.slop: + print( + "error: --warn-only only applies to --slop mode", + file=sys.stderr, + ) + return 2 + if not args.slop: + return _emit(args, lint.lint_items(_scan(args))) + return _emit( + args, slop.slop_items(_scan(args), warn_only=args.warn_only) + ) def cmd_diff(args) -> int: @@ -664,6 +680,28 @@ def cmd_export(args) -> int: def cmd_import(args) -> int: + if args.restore is not None: + if args.clean: + print( + "error: --clean and --restore are mutually exclusive", + file=sys.stderr, + ) + return 2 + if args.bundle is not None: + print( + "error: --restore takes a snapshot dir — the bundle " + "path must be omitted", + file=sys.stderr, + ) + return 2 + return _cmd_restore(args) + if args.bundle is None: + print( + "error: a bundle path is required (or pass " + "--restore SNAPSHOT_DIR)", + file=sys.stderr, + ) + return 2 path = Path(args.bundle) try: with bundle.BundleReader(path) as reader: @@ -682,15 +720,43 @@ def cmd_import(args) -> int: "active/approved" ), ] - for p in plans: - cur = reg.state_of(p["kind"], p["name"]) or "new" + if args.clean: lines.append( - f"{p['key']} ({len(p['files'])} file(s), " - f"currently {cur}) → quarantined" + "--clean: the stored copy of every listed item is " + "REPLACED (pre-clean snapshot → " + "skill-catalog/import-snapshots/, atomic swap, " + "rollback on failure)" ) + for p in plans: + cur = reg.state_of(p["kind"], p["name"]) or "new" + if args.clean and cur == "retired": + lines.append( + f"{p['key']} ({len(p['files'])} file(s), " + f"currently {cur}) → skipped — retired (a " + f"clean import does not resurrect it)" + ) + elif args.clean and cur != "new": + lines.append( + f"{p['key']} ({len(p['files'])} file(s), " + f"currently {cur}) → replaced (pre-clean copy " + f"snapshotted) → quarantined" + ) + else: + lines.append( + f"{p['key']} ({len(p['files'])} file(s), " + f"currently {cur}) → quarantined" + ) if not _plan(lines, args.apply): return 0 - results = bundle.apply_import(reader, reg, config_dir, plans) + snapshot = None + if args.clean: + results, snapshot = bundle.apply_clean_import( + reader, reg, config_dir, plans, note=args.note + ) + else: + results = bundle.apply_import( + reader, reg, config_dir, plans + ) except bundle.BundleError as exc: print(f"error: {exc}", file=sys.stderr) return 1 @@ -700,6 +766,62 @@ def cmd_import(args) -> int: print(f"quarantined {r['key']}") else: print(f"skipped {r['key']} — {r['reason']}") + if snapshot is not None: + print(f"pre-clean snapshot: {snapshot}") + print( + f" recover with: import-bundle --restore {snapshot} " + f"--config-dir {config_dir} --apply" + ) + return 0 + + +def _cmd_restore(args) -> int: + """Restore a pre-clean snapshot written by ``--clean`` — plan-first + like every mutation.""" + reg, config_dir = _registry(args) + snap = Path(args.restore) + try: + meta = bundle.load_clean_snapshot(snap) + except bundle.BundleError as exc: + print(f"error: {exc}", file=sys.stderr) + return 1 + entries = meta["entries"] + lines = [ + f"restore pre-clean snapshot {snap}", + f"snapshot created at {meta.get('created_at', '?')}", + ] + for key in sorted(entries): + entry = entries[key] + kind, _, name = key.partition(":") + if kind not in KINDS or not name or ( + entry is not None and not isinstance(entry, dict) + ): + print( + f"error: snapshot entry {key!r} is malformed", + file=sys.stderr, + ) + return 1 + cur = reg.state_of(kind, name) or "unregistered" + if entry is None: + lines.append( + f"{key}: remove entry + store copy (unregistered " + f"pre-clean; currently {cur})" + ) + else: + lines.append( + f"{key}: restore entry + store copy (currently {cur} " + f"→ {entry.get('state')})" + ) + if not _plan(lines, args.apply): + return 0 + try: + restored = bundle.restore_clean_snapshot(snap, reg, config_dir) + except bundle.BundleError as exc: + print(f"error: {exc}", file=sys.stderr) + return 1 + reg.save() + for key in restored: + print(f"restored {key}") return 0 @@ -768,6 +890,21 @@ def build_parser() -> argparse.ArgumentParser: p = sub.add_parser("lint", help="structural lint findings per item") _add_scan_args(p) + p.add_argument( + "--slop", + action="store_true", + help="content-slop mode: run the deterministic anti-slop rules " + "(slop-fingerprint, slop-emdash, slop-sycophancy, " + "slop-not-just-but, slop-filler) over each item's markdown " + "instead of the structural checks; a 'slop-allow[:]' " + "comment suppresses a line, 'slop-allow-file' the whole file", + ) + p.add_argument( + "--warn-only", + action="store_true", + help="(--slop only) report findings as WARN and exit 0 even " + "when they exist", + ) p.set_defaults(func=cmd_lint) p = sub.add_parser( @@ -975,7 +1112,30 @@ def build_parser() -> argparse.ArgumentParser: "import-bundle", help="import a bundle — items land quarantined, never active", ) - p.add_argument("bundle", type=Path, help="bundle dir or .tar[.gz]") + p.add_argument( + "bundle", + type=Path, + nargs="?", + help="bundle dir or .tar[.gz] (omitted with --restore)", + ) + p.add_argument( + "--clean", + action="store_true", + help="replace the stored copy of every item the bundle carries " + "instead of merging: the pre-clean state is snapshotted to " + ".devin-ecosystem/skill-catalog/import-snapshots/ first, each " + "store copy is swapped atomically and a mid-run failure rolls " + "back — retired items are skipped, items absent from the bundle " + "are untouched", + ) + p.add_argument( + "--restore", + type=Path, + default=None, + metavar="SNAPSHOT_DIR", + help="restore a pre-clean snapshot written by --clean " + "(mutually exclusive with --clean and a bundle path)", + ) _add_mutation_args(p) p.set_defaults(func=cmd_import) diff --git a/packages/skill-catalog/src/devin_skill_catalog/registry.py b/packages/skill-catalog/src/devin_skill_catalog/registry.py index c3638bf..9815384 100644 --- a/packages/skill-catalog/src/devin_skill_catalog/registry.py +++ b/packages/skill-catalog/src/devin_skill_catalog/registry.py @@ -148,6 +148,38 @@ def transition( ) return entry + def replace( + self, + kind: str, + name: str, + *, + note: str = "", + sha256: str | None = None, + origin: str | None = None, + store: str | None = None, + ) -> dict: + """Rewrite an entry's recorded fields without changing its state + — a same-state content swap (``import-bundle --clean`` replacing + the stored copy of an already-quarantined item). History records + a ``from == to`` entry so the swap stays auditable.""" + entry = self._touch(kind, name) + current = entry["state"] + if sha256: + entry["sha256"] = sha256 + if origin: + entry["origin"] = origin + if store: + entry["store"] = store + entry["history"].append( + { + "at": utc_now_iso(), + "from": current, + "to": current, + "note": note, + } + ) + return entry + def record_gate(self, kind: str, name: str, gate: str, status: str) -> None: entry = self._touch(kind, name) entry.setdefault("gates", {})[gate] = { diff --git a/packages/skill-catalog/src/devin_skill_catalog/slop.py b/packages/skill-catalog/src/devin_skill_catalog/slop.py new file mode 100644 index 0000000..d903b86 --- /dev/null +++ b/packages/skill-catalog/src/devin_skill_catalog/slop.py @@ -0,0 +1,222 @@ +"""Deterministic anti-slop lint — regex heuristics for LLM-flavored prose. + +``lint --slop`` runs these rules over every ``.md`` file of an item +(``SKILL.md``, rule files and any markdown shipped inside a skill dir). +Every rule is a compiled pattern with a stable ``slop-*`` id, a +documented rationale and a finding message. Rules are deterministic — +same input, same findings — and fire per line. + +Two classes of content are never flagged: + +- lines inside fenced code blocks (````` ``` `` or ``~~~``) — quoted + examples are not prose; +- lines carrying an **allowlist marker**, so legitimate usage is never + blocked by a bare denylist: + + - ``slop-allow`` — suppress every rule on that line + - ``slop-allow:`` — suppress only that rule on that line + - ``slop-allow-file`` anywhere in the file — suppress the whole file + + Markers are meant to sit inside an HTML comment + (````) but match as plain text. +""" + +from __future__ import annotations + +import re +from dataclasses import dataclass +from pathlib import Path + +from devin_skill_catalog.model import Finding, Item, Status + +ALLOW_MARKER = "slop-allow" +ALLOW_FILE_MARKER = "slop-allow-file" + + +@dataclass(frozen=True) +class SlopRule: + """One deterministic slop check: ``pattern`` on a line → a finding.""" + + id: str + pattern: re.Pattern[str] + message: str # finding text — what was matched and why it is slop + rationale: str # why this pattern is a rule at all (docs/audits) + + +# A sentence start: line start or right after sentence-ending +# punctuation. Fingerprint connectives like "Additionally," only count +# there — mid-sentence "additionally" is ordinary prose. +_SENT = r"(?:^|(?<=[.!?;]\s))" + +SLOP_RULES: tuple[SlopRule, ...] = ( + SlopRule( + id="slop-fingerprint", + pattern=re.compile( + r"\bdelve\s+(?:into|deeper)\b" + r"|\bseamless(?:ly|ness)?\b" + r"|\brobust\b" + r"|\bcomprehensive\b" + r"|\bit(?:'s| is) worth noting\b" + r"|" + + _SENT + + r"(?:in summary|in conclusion|" + r"additionally|moreover|furthermore)\b", + re.IGNORECASE, + ), + message="stock LLM-fingerprint phrase", + rationale="Phrases LLMs emit far more often than human authors " + "('delve into', 'seamless', 'robust', 'comprehensive', 'it's " + "worth noting', sentence-initial 'in summary'/'in conclusion'/" + "'Additionally,/'Moreover,/'Furthermore,') — filler that pads " + "prose without adding information.", + ), + SlopRule( + id="slop-emdash", + pattern=re.compile( + r"—|(?:(?<=\s)|^)–(?=\s|$)", + ), + message="dash used as a clause separator", + rationale="Em-dash (and spaced en-dash) separators are a " + "recognizable LLM fingerprint; a comma, period or parentheses " + "reads like a person wrote it. Bare en-dashes inside ranges " + "('2020–2024') are not separators and do not fire.", + ), + SlopRule( + id="slop-sycophancy", + pattern=re.compile( + r"^\s*(?:great question|good question|sure thing|" + r"certainly|absolutely|of course|" + r"you(?:'re| are) (?:absolutely |completely )?right)\b[.!,]*" + r"|\b(?:hope (?:this|that) helps|let me know\b|" + r"happy to help|glad to help|" + r"feel free to (?:reach out|ask|let me know))", + re.IGNORECASE, + ), + message="sycophantic opener/closer", + rationale="Assistant-style address ('Great question', 'Happy " + "to help', 'Hope this helps', 'Let me know', 'You are " + "absolutely right') is chat filler — skill content instructs a " + "reader, it does not flatter them.", + ), + SlopRule( + id="slop-not-just-but", + pattern=re.compile( + r"\bnot\s+(?:just|only|merely|simply)\b.{0,160}?\bbut\b", + re.IGNORECASE, + ), + message="'not just X but Y' construction", + rationale="The 'not just X but Y' escalation is a stock LLM " + "rhetorical frame; a plain statement carries the same fact.", + ), + SlopRule( + id="slop-filler", + pattern=re.compile( + r"\bin today's\b.{0,40}?\b(?:world|landscape|age|era)\b" + r"|\bever-evolving\b" + r"|\bfast-paced\b" + r"|\bcutting-edge\b" + r"|\bstate-of-the-art\b" + r"|\bgame[- ]changer\b" + r"|\ba testament to\b" + r"|\bplays? a (?:vital|crucial|key|pivotal|central) role\b" + r"|\bat the end of the day\b" + r"|\bfirst and foremost\b" + r"|\bwhen it comes to\b" + r"|\bin the (?:world|realm) of\b", + re.IGNORECASE, + ), + message="generic filler phrase", + rationale="Hype and throat-clearing ('in today's fast-paced " + "world', 'cutting-edge', 'game-changer', 'a testament to', " + "'plays a vital role', 'at the end of the day', 'first and " + "foremost', 'when it comes to', 'in the world of') adds tone " + "where instructions need facts.", + ), +) + +_FENCE_RE = re.compile(r"^\s*(```|~~~)") +_ALLOW_RE = re.compile( + rf"{ALLOW_MARKER}(?::([a-z0-9-]+))?", re.IGNORECASE +) + + +def _line_allowances(line: str) -> tuple[bool, frozenset[str]]: + """``(allow_all, {rule ids})`` declared by markers on the line.""" + allow_all = False + ids: set[str] = set() + for m in _ALLOW_RE.finditer(line): + if m.group(1): + ids.add(m.group(1).lower()) + else: + allow_all = True + return allow_all, frozenset(ids) + + +def slop_text( + text: str, item_key: str, *, warn_only: bool = False +) -> list[Finding]: + """Run every slop rule over ``text`` — one finding per (rule, line).""" + if ALLOW_FILE_MARKER in text: + return [] + status = Status.WARN if warn_only else Status.FAIL + findings: list[Finding] = [] + in_fence = False + for lineno, line in enumerate(text.splitlines(), 1): + if _FENCE_RE.match(line): + in_fence = not in_fence + continue + if in_fence: + continue + allow_all, allowed = _line_allowances(line) + if allow_all: + continue + for rule in SLOP_RULES: + if rule.id in allowed: + continue + m = rule.pattern.search(line) + if m is None: + continue + findings.append( + Finding( + status=status, + check=rule.id, + item=item_key, + line=lineno, + message=f"{rule.message}: {m.group(0).strip()!r}", + ) + ) + return findings + + +def _item_markdown_files(item: Item) -> list[Path]: + """The item's prose surface — every ``.md`` it ships.""" + files = [f for f in item.files if f.suffix.lower() == ".md"] + if not files and item.path.suffix.lower() == ".md": + files = [item.path] + return sorted(files) + + +def slop_item(item: Item, *, warn_only: bool = False) -> list[Finding]: + findings: list[Finding] = [] + for path in _item_markdown_files(item): + try: + text = path.read_text(encoding="utf-8", errors="replace") + except OSError as exc: + findings.append( + Finding( + status=Status.WARN, + check="slop-read", + item=item.key, + message=f"{path.name} unreadable: {exc}", + ) + ) + continue + findings.extend(slop_text(text, item.key, warn_only=warn_only)) + return findings + + +def slop_items(items: list[Item], *, warn_only: bool = False) -> list[Finding]: + out: list[Finding] = [] + for item in items: + out.extend(slop_item(item, warn_only=warn_only)) + return out diff --git a/packages/skill-catalog/tests/test_bundle_clean.py b/packages/skill-catalog/tests/test_bundle_clean.py new file mode 100644 index 0000000..5f888b5 --- /dev/null +++ b/packages/skill-catalog/tests/test_bundle_clean.py @@ -0,0 +1,463 @@ +"""``import-bundle --clean`` — safe-replacement semantics. + +The default import MERGES: an already-registered item keeps its stored +copy. ``--clean`` REPLACES the catalog content for the bundle's scope — +the items the bundle carries — under a fail-closed envelope: the +pre-clean state is snapshotted to +``skill-catalog/import-snapshots/`` first, each store copy is swapped +atomically, and a mid-run failure rolls back (``--restore`` recovers a +snapshot manually). +""" + +from __future__ import annotations + +import hashlib +import json +from pathlib import Path + +import pytest +from devin_skill_catalog import bundle, cli +from devin_skill_catalog.model import ( + STATE_APPROVED, + STATE_QUARANTINED, + STATE_RETIRED, +) +from devin_skill_catalog.paths import ( + quarantine_item_dir, + registry_path, + store_dir, +) +from devin_skill_catalog.registry import Registry + +SKILL_V1 = { + "SKILL.md": ( + b"---\nname: s\ndescription: version one of the fixture\n---\n" + b"# v1\n" + ), + "helper.py": b"print('v1')\n", +} +SKILL_V2 = { + "SKILL.md": ( + b"---\nname: s\ndescription: version two of the fixture\n---\n" + b"# v2\n" + ), + "helper.py": b"print('v2')\n", + "extra.txt": b"new in v2\n", +} +RULE_V1 = {"r.md": b"# Rule v1\n\nOld body.\n"} +RULE_V2 = {"r.md": b"# Rule v2\n\nNew body.\n"} + + +def _reg(d: Path) -> Registry: + return Registry(registry_path(d)) + + +def _make_bundle(bdir: Path, items: dict[str, dict[str, bytes]]) -> Path: + """A minimal valid bundle: manifest.json + per-item files.""" + specs = [] + for key, files in items.items(): + kind, _, name = key.partition(":") + fspecs = [] + for rel, data in sorted(files.items()): + dest = bdir / "items" / kind / name / rel + dest.parent.mkdir(parents=True, exist_ok=True) + dest.write_bytes(data) + fspecs.append( + { + "path": rel, + "sha256": hashlib.sha256(data).hexdigest(), + } + ) + specs.append( + { + "key": key, + "kind": kind, + "name": name, + "state": "approved", + "files": fspecs, + } + ) + manifest = { + "version": 1, + "created_at": "2026-01-01T00:00:00Z", + "source": {"tool": "test"}, + "items": specs, + } + bdir.mkdir(parents=True, exist_ok=True) + (bdir / "manifest.json").write_text( + json.dumps(manifest), encoding="utf-8" + ) + return bdir + + +def _seed_item( + config_dir: Path, + kind: str, + name: str, + state: str, + files: dict[str, bytes] | None, +) -> Path: + """Register ``kind:name`` at ``state`` with ``files`` as its store + copy. Returns the store dir.""" + store = quarantine_item_dir(config_dir, kind, name) + if files is not None: + for rel, data in files.items(): + dest = store / rel + dest.parent.mkdir(parents=True, exist_ok=True) + dest.write_bytes(data) + reg = _reg(config_dir) + reg.transition(kind, name, STATE_QUARANTINED, store=str(store)) + if state == STATE_APPROVED: + reg.transition(kind, name, STATE_APPROVED) + elif state == STATE_RETIRED: + reg.transition(kind, name, STATE_RETIRED) + assert state in (STATE_QUARANTINED, STATE_APPROVED, STATE_RETIRED) + reg.save() + return store + + +def _store_files(store: Path) -> dict[str, bytes]: + return { + p.relative_to(store).as_posix(): p.read_bytes() + for p in store.rglob("*") + if p.is_file() + } + + +def _snapshots(config_dir: Path) -> list[Path]: + root = store_dir(config_dir) / "import-snapshots" + return sorted(p for p in root.iterdir() if p.is_dir()) if root.is_dir() else [] + + +# ---------------------------------------------------------------------- +# replaces, not merges +# ---------------------------------------------------------------------- + + +def test_merge_keeps_old_copy_clean_replaces( + config_dir: Path, tmp_path: Path +): + """Same setup, two modes: merge skips the already-quarantined item + (v1 survives); clean swaps in v2 and stays quarantined.""" + bdir = _make_bundle(tmp_path / "b", {"skill:s": SKILL_V2}) + _seed_item(config_dir, "skill", "s", STATE_QUARANTINED, SKILL_V1) + + assert ( + cli.main( + [ + "import-bundle", + str(bdir), + "--apply", + "--config-dir", + str(config_dir), + ] + ) + == 0 + ) + store = quarantine_item_dir(config_dir, "skill", "s") + assert (store / "helper.py").read_bytes() == SKILL_V1["helper.py"] + assert not (store / "extra.txt").exists() + + assert ( + cli.main( + [ + "import-bundle", + str(bdir), + "--clean", + "--apply", + "--config-dir", + str(config_dir), + ] + ) + == 0 + ) + assert _store_files(store) == SKILL_V2 + assert _reg(config_dir).state_of("skill", "s") == STATE_QUARANTINED + + +def test_clean_demotes_approved_and_records_history( + config_dir: Path, tmp_path: Path, capsys +): + """An approved item is demoted back to quarantined with the new + content; an already-quarantined one keeps its state and logs a + from==to history entry.""" + bdir = _make_bundle( + tmp_path / "b", {"skill:s": SKILL_V2, "rule:r": RULE_V2} + ) + _seed_item(config_dir, "skill", "s", STATE_APPROVED, SKILL_V1) + _seed_item(config_dir, "rule", "r", STATE_QUARANTINED, RULE_V1) + + assert ( + cli.main( + [ + "import-bundle", + str(bdir), + "--clean", + "--apply", + "--config-dir", + str(config_dir), + ] + ) + == 0 + ) + reg = _reg(config_dir) + assert reg.state_of("skill", "s") == STATE_QUARANTINED + assert _store_files(quarantine_item_dir(config_dir, "skill", "s")) == SKILL_V2 + hist = reg.get("rule", "r")["history"] + assert hist[-1]["from"] == STATE_QUARANTINED + assert hist[-1]["to"] == STATE_QUARANTINED # same-state replace + assert "snapshot" in hist[-1]["note"] + assert "pre-clean snapshot" in capsys.readouterr().out + + +def test_clean_skips_retired_items(config_dir: Path, tmp_path: Path, capsys): + """retired is a deliberate end-state — a clean import does not + resurrect it.""" + bdir = _make_bundle(tmp_path / "b", {"skill:s": SKILL_V2}) + store = _seed_item(config_dir, "skill", "s", STATE_RETIRED, SKILL_V1) + + assert ( + cli.main( + [ + "import-bundle", + str(bdir), + "--clean", + "--apply", + "--config-dir", + str(config_dir), + ] + ) + == 0 + ) + out = capsys.readouterr().out + assert "skipped" in out and "retired" in out + assert _reg(config_dir).state_of("skill", "s") == STATE_RETIRED + assert _store_files(store) == SKILL_V1 + + +# ---------------------------------------------------------------------- +# snapshot + restore +# ---------------------------------------------------------------------- + + +def test_preclean_state_recoverable_via_restore( + config_dir: Path, tmp_path: Path, capsys +): + """After a clean, ``--restore`` puts back the pre-clean store copy + and registry entry.""" + bdir = _make_bundle(tmp_path / "b", {"skill:s": SKILL_V2}) + store = _seed_item(config_dir, "skill", "s", STATE_QUARANTINED, SKILL_V1) + assert ( + cli.main( + [ + "import-bundle", + str(bdir), + "--clean", + "--apply", + "--config-dir", + str(config_dir), + ] + ) + == 0 + ) + snaps = _snapshots(config_dir) + assert len(snaps) == 1 + assert (snaps[0] / "snapshot.json").is_file() + assert (snaps[0] / "items" / "skill" / "s" / "SKILL.md").read_bytes() == SKILL_V1["SKILL.md"] + + # plan-first: no --apply → nothing restored + assert ( + cli.main( + [ + "import-bundle", + "--restore", + str(snaps[0]), + "--config-dir", + str(config_dir), + ] + ) + == 0 + ) + assert "nothing written" in capsys.readouterr().out + assert _store_files(store) == SKILL_V2 + + assert ( + cli.main( + [ + "import-bundle", + "--restore", + str(snaps[0]), + "--apply", + "--config-dir", + str(config_dir), + ] + ) + == 0 + ) + assert "restored skill:s" in capsys.readouterr().out + assert _store_files(store) == SKILL_V1 + entry = _reg(config_dir).get("skill", "s") + assert entry["state"] == STATE_QUARANTINED + assert "restored" in entry["history"][-1]["note"] + + +def test_restore_removes_items_absent_preclean( + config_dir: Path, tmp_path: Path +): + """An item that did not exist pre-clean gets its entry and store + copy removed on restore.""" + bdir = _make_bundle(tmp_path / "b", {"skill:new-s": SKILL_V2}) + assert ( + cli.main( + [ + "import-bundle", + str(bdir), + "--clean", + "--apply", + "--config-dir", + str(config_dir), + ] + ) + == 0 + ) + store = quarantine_item_dir(config_dir, "skill", "new-s") + assert store.is_dir() + snap = _snapshots(config_dir)[0] + reg = _reg(config_dir) + bundle.restore_clean_snapshot(snap, reg, config_dir) + assert reg.get("skill", "new-s") is None # in-memory + assert not store.exists() + reg.save() + assert _reg(config_dir).get("skill", "new-s") is None # persisted + + +# ---------------------------------------------------------------------- +# fail-closed +# ---------------------------------------------------------------------- + + +def test_interrupted_clean_rolls_back(config_dir: Path, tmp_path: Path): + """A bundle file that vanishes between plan and apply aborts the + clean: the already-swapped item is rolled back and the registry is + untouched.""" + bdir = _make_bundle( + tmp_path / "b", + {"skill:a": SKILL_V2, "skill:b": SKILL_V2}, + ) + store_a = _seed_item(config_dir, "skill", "a", STATE_QUARANTINED, SKILL_V1) + store_b = _seed_item(config_dir, "skill", "b", STATE_QUARANTINED, SKILL_V1) + + with bundle.BundleReader(bdir) as reader: + _manifest, plans = bundle.plan_import(reader) + # break the bundle AFTER planning — the second item's file is + # gone when apply reads it + (bdir / "items" / "skill" / "b" / "SKILL.md").unlink() + reg = _reg(config_dir) + with pytest.raises(bundle.BundleError, match="clean import failed"): + bundle.apply_clean_import(reader, reg, config_dir, plans) + reg.save() # even if saved, entries must be the pre-clean ones + + # the pre-clean state is intact: v1 copies + original entries + assert _store_files(store_a) == SKILL_V1 + assert _store_files(store_b) == SKILL_V1 + reg2 = _reg(config_dir) + assert reg2.state_of("skill", "a") == STATE_QUARANTINED + assert reg2.state_of("skill", "b") == STATE_QUARANTINED + # the snapshot survived for a manual --restore + assert _snapshots(config_dir) + + +def test_corrupt_bundle_with_clean_fails_before_any_write( + config_dir: Path, tmp_path: Path, capsys +): + """Checksum tampering is caught at plan time — before the snapshot + or any store write.""" + bdir = _make_bundle(tmp_path / "b", {"skill:s": SKILL_V2}) + (bdir / "items" / "skill" / "s" / "SKILL.md").write_bytes(b"tampered\n") + _seed_item(config_dir, "skill", "s", STATE_QUARANTINED, SKILL_V1) + rc = cli.main( + [ + "import-bundle", + str(bdir), + "--clean", + "--apply", + "--config-dir", + str(config_dir), + ] + ) + assert rc == 1 + assert "checksum" in capsys.readouterr().err + store = quarantine_item_dir(config_dir, "skill", "s") + assert _store_files(store) == SKILL_V1 + assert _snapshots(config_dir) == [] + + +def test_clean_plan_first_writes_nothing( + config_dir: Path, tmp_path: Path, capsys +): + """Without --apply the clean plan is printed but nothing is + snapshotted, swapped or registered.""" + bdir = _make_bundle(tmp_path / "b", {"skill:s": SKILL_V2}) + store = _seed_item(config_dir, "skill", "s", STATE_QUARANTINED, SKILL_V1) + assert ( + cli.main( + [ + "import-bundle", + str(bdir), + "--clean", + "--config-dir", + str(config_dir), + ] + ) + == 0 + ) + out = capsys.readouterr().out + assert "nothing written" in out + assert "replaced" in out + assert _store_files(store) == SKILL_V1 + assert _snapshots(config_dir) == [] + + +# ---------------------------------------------------------------------- +# flag validation +# ---------------------------------------------------------------------- + + +def test_incompatible_flag_combinations_rejected( + tmp_path: Path, capsys +): + bdir = _make_bundle(tmp_path / "b", {"skill:s": SKILL_V2}) + snap = tmp_path / "snap" + + # --clean and --restore are mutually exclusive + assert ( + cli.main( + [ + "import-bundle", + str(bdir), + "--clean", + "--restore", + str(snap), + ] + ) + == 2 + ) + # --restore takes no bundle path + assert cli.main( + ["import-bundle", str(bdir), "--restore", str(snap)] + ) == 2 + # neither a bundle nor --restore given + assert cli.main(["import-bundle"]) == 2 + # --restore on a dir that is not a snapshot → error, not a crash + assert ( + cli.main( + [ + "import-bundle", + "--restore", + str(bdir), + "--apply", + ] + ) + == 1 + ) + assert "snapshot" in capsys.readouterr().err diff --git a/packages/skill-catalog/tests/test_slop.py b/packages/skill-catalog/tests/test_slop.py new file mode 100644 index 0000000..ee998ec --- /dev/null +++ b/packages/skill-catalog/tests/test_slop.py @@ -0,0 +1,266 @@ +"""``lint --slop`` — deterministic anti-slop rules. + +Every rule is a compiled pattern with a stable ``slop-*`` id and a +documented rationale; every rule is suppressed by the same allowlist +markers (``slop-allow``, ``slop-allow:``, ``slop-allow-file``), +so legitimate content is never blocked by a bare denylist. Findings +carry the rule id and a line ref; they are FAIL by default (exit 1) and +WARN under ``--warn-only`` (exit 0). +""" + +from __future__ import annotations + +import textwrap +from pathlib import Path + +import pytest +from devin_skill_catalog import cli, scan, slop +from devin_skill_catalog.model import Status + +from conftest import GOOD_RULE, GOOD_SKILL, write + +REPO_ROOT = Path(__file__).resolve().parents[3] +OWN_SKILLS = [ + Path(__file__).parents[1] + / "adapters" + / "skills" + / "devin-skill-catalog" + / "SKILL.md", + REPO_ROOT / "packages" / "devkit" / "adapters" / "skills" + / "devin-devkit" / "SKILL.md", +] + + +def _ids(findings) -> set[str]: + return {f.check for f in findings} + + +# ---------------------------------------------------------------------- +# each rule fires on a known-bad fixture +# ---------------------------------------------------------------------- + + +def test_fingerprint_rule_fires(): + findings = slop.slop_text( + "Let us delve into this seamless, robust and comprehensive " + "solution.\nAdditionally, it is worth noting the tradeoff.\n", + "skill:x", + ) + assert _ids(findings) == {"slop-fingerprint"} + assert len(findings) == 2 # one per line + assert findings[0].line == 1 and findings[1].line == 2 + + +def test_fingerprint_sentence_connectives(): + """Sentence-initial connectives fire; mid-sentence ones do not — + the fingerprint is the opener position.""" + bad = slop.slop_text( + "The tool works. Moreover, it is cheap.\n", "skill:x" + ) + good = slop.slop_text( + "it is fast and additionally cheap moreover quiet.\n", "skill:x" + ) + assert _ids(bad) == {"slop-fingerprint"} + assert good == [] + + +def test_emdash_separator_rule(): + bad = slop.slop_text( + "The item lands quarantined — never active.\n" + "spaced en dash – same fingerprint\n", + "skill:x", + ) + assert _ids(bad) == {"slop-emdash"} + assert len(bad) == 2 + # en dashes inside ranges are not separators + assert slop.slop_text("Covers versions 3.10–3.14.\n", "skill:x") == [] + # a plain hyphen is not the fingerprint + assert slop.slop_text("a well-known fact - yes\n", "skill:x") == [] + + +def test_sycophancy_rule(): + text = textwrap.dedent( + """\ + Great question! The fix is below. + Hope this helps. + Let me know if anything is unclear. + """ + ) + findings = slop.slop_text(text, "skill:x") + assert _ids(findings) == {"slop-sycophancy"} + assert len(findings) == 3 + + +def test_not_just_but_rule(): + findings = slop.slop_text( + "It is not just a linter but a governance layer.\n", "skill:x" + ) + assert _ids(findings) == {"slop-not-just-but"} + # 'just' without the construction does not fire + assert slop.slop_text("adjust the knob\n", "skill:x") == [] + assert slop.slop_text("it is not just fast\n", "skill:x") == [] + + +def test_filler_rule(): + findings = slop.slop_text( + "In today's fast-paced world, you need cutting-edge tooling.\n" + "At the end of the day, it plays a vital role.\n", + "skill:x", + ) + assert _ids(findings) == {"slop-filler"} + assert len(findings) == 2 + + +# ---------------------------------------------------------------------- +# exceptions mechanism +# ---------------------------------------------------------------------- + + +def test_allow_marker_suppresses_line(): + text = ( + "A seamless solution. \n" + "Another seamless solution.\n" + ) + findings = slop.slop_text(text, "skill:x") + assert [f.line for f in findings] == [2] + + +def test_allow_marker_with_rule_id_is_selective(): + """``slop-allow:`` suppresses only that rule on the line — + other rules still fire.""" + text = ( + "A seamless solution — truly. \n" + ) + findings = slop.slop_text(text, "skill:x") + assert _ids(findings) == {"slop-emdash"} + + +def test_allow_file_marker_suppresses_file(tmp_path: Path): + f = write( + tmp_path / "f.md", + "\n" + "A seamless, robust delve into greatness — Great question!\n", + ) + findings = slop.slop_text(f.read_text(), "rule:f") + assert findings == [] + + +def test_fenced_code_is_not_prose(): + """Slop inside ``` fences is quoted example content — not prose.""" + text = ( + "Document the flag:\n\n" + "```text\n" + "Great question — delve into seamless robustness.\n" + "```\n\n" + "Clean prose stays clean.\n" + ) + assert slop.slop_text(text, "skill:x") == [] + + +# ---------------------------------------------------------------------- +# clean content passes +# ---------------------------------------------------------------------- + + +def test_clean_skill_and_rule_pass(tmp_path: Path): + d = tmp_path / ".devin" + write(d / "skills" / "s" / "SKILL.md", GOOD_SKILL) + write(d / "rules" / "r.md", GOOD_RULE) + items = scan.scan_devin_dir(d) + assert items and slop.slop_items(items) == [] + + +def test_cli_slop_clean_workspace(devin_dir: Path, capsys): + """The synthetic fixture tree has no slop → exit 0.""" + rc = cli.main(["lint", str(devin_dir), "--no-user", "--slop"]) + assert rc == 0 + assert "0 finding(s)" in capsys.readouterr().out + + +# ---------------------------------------------------------------------- +# findings shape, exit codes, --warn-only +# ---------------------------------------------------------------------- + + +def test_findings_carry_rule_id_item_and_line(devin_dir: Path): + write( + devin_dir / "rules" / "sloppy.md", + "# Sloppy\n\nA seamless delight — delve into it.\n", + ) + items = { + it.key: it for it in scan.scan_devin_dir(devin_dir) + } + findings = slop.slop_item(items["rule:sloppy"]) + assert {f.check for f in findings} == { + "slop-fingerprint", + "slop-emdash", + } + for f in findings: + assert f.item == "rule:sloppy" + assert f.line == 3 + assert f.status is Status.FAIL + + +def test_cli_slop_exit_codes(devin_dir: Path, capsys): + write( + devin_dir / "rules" / "sloppy.md", + "# Sloppy\n\nHope this helps.\n", + ) + assert cli.main(["lint", str(devin_dir), "--no-user", "--slop"]) == 1 + out = capsys.readouterr().out + assert "[slop-sycophancy]:3" in out + # --warn-only reports but exits 0 + assert ( + cli.main( + ["lint", str(devin_dir), "--no-user", "--slop", "--warn-only"] + ) + == 0 + ) + out = capsys.readouterr().out + assert "WARN rule:sloppy" in out + assert "FAIL rule:sloppy" not in out + + +def test_warn_only_rejected_without_slop(devin_dir: Path, capsys): + assert ( + cli.main(["lint", str(devin_dir), "--no-user", "--warn-only"]) + == 2 + ) + assert "--slop" in capsys.readouterr().err + + +def test_cli_slop_json(devin_dir: Path, capsys): + import json + + write( + devin_dir / "rules" / "sloppy.md", + "# Sloppy\n\nDelve into it.\n", + ) + rc = cli.main( + ["lint", str(devin_dir), "--no-user", "--slop", "--json"] + ) + assert rc == 1 + findings = json.loads(capsys.readouterr().out)["findings"] + assert findings[0]["check"] == "slop-fingerprint" + assert findings[0]["line"] == 3 + assert findings[0]["item"] == "rule:sloppy" + + +# ---------------------------------------------------------------------- +# measured false-positive check on the repo's own skill files +# ---------------------------------------------------------------------- + + +@pytest.mark.parametrize( + "skill_md", [p for p in OWN_SKILLS if p.is_file()] +) +def test_repo_own_skills_measured_fp(skill_md: Path): + """The repo's own SKILL.md files carry no slop-rule false + positives: the only findings are ``slop-emdash`` hits — intended + true positives, since the project convention itself bans dash + separators in this content. Every other rule must report zero.""" + items = scan.resolve_target(skill_md) + assert len(items) == 1 + findings = slop.slop_item(items[0]) + false_positives = [f for f in findings if f.check != "slop-emdash"] + assert false_positives == [] From 70f72e004f0f230f7eeb1495c5084d3b8a6f0b97 Mon Sep 17 00:00:00 2001 From: Icaro0310 Date: Wed, 28 Oct 2026 11:57:36 +0000 Subject: [PATCH 2/2] chore: re-dispatch review (queue-riding lane B)