From 28cf256bfb96a801801f302caf519652eb79b28b Mon Sep 17 00:00:00 2001 From: Wes Duenow Date: Sat, 30 May 2026 17:36:57 -0400 Subject: [PATCH 1/2] #141: SignalForge skill + install-skill (#166) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * #141: SignalForge skill + install-skill (plan) Phase 4 detailing complete. 24 decisions locked across: - skill source path (`src/signalforge/skills/signalforge/` package-data tree; `src/signalforge/skill/` Python lib) - destination policy (always overwrite SKILL.md, preserve siblings, no --force) - symlink/cycle defence (mirrors copy_demo verbatim) - error hierarchy (SkillError base + 3 concretes; spans tiers 1+2 → excluded base) - wheel packaging + wheel_smoke gate - AST scan #7 bump (12→13) - SKILL ↔ CLI parity gate (new test scans live argparse + key demo commands) - 5-surface parity for install-skill - self-grade ops (pre-release manual; pinned in eval.json + README badge) - e2e demo paths (zero-cred default + opt-in live) - skill-parity.md rule + cli-layer.md update (orchestrator-only) 11 stories laid out: US-001…US-009 implementation + US-010 Quality Gate + US-011 Patterns & Memory. Co-Authored-By: Claude Opus 4.7 (1M context) * #141: mark plan phase=published, link PR #166 Co-Authored-By: Claude Opus 4.7 (1M context) * docs(#141): add skill-parity rule documenting CLI/skill parity gate Co-Authored-By: Claude Opus 4.7 (1M context) * #141: devolved to beads (epic bd_1-scaffolding-ezn, 11 tasks) Approved + devolved. US-001 is at the front of the ready queue. Co-Authored-By: Claude Opus 4.7 (1M context) * bd_1-scaffolding-ezn.1: US-001 bootstrap skills tree + wheel packaging Lays down src/signalforge/skills/signalforge/{SKILL.md,assets/SKILL.eval.json} as structural placeholders. Wires [tool.hatch.build.targets.wheel].include for the skills tree. Extends wheel_smoke with _EXPECTED_SKILL_FILES (positive) and a negative assertion that no .claude/skills/* paths appear in the wheel. Plan: plans/super/141-claude-skill-install.md US-001 / DEC-001, 010, 011, 022. Co-Authored-By: Claude Opus 4.7 (1M context) * bd_1-scaffolding-ezn.2: US-002 signalforge.skill lib + typed errors + AST scan #7 bump Adds signalforge.skill subpackage with install_skill(dest) -> Path and the three-class typed-error hierarchy (SkillDestPathError tier 1, SkillDestUnsafeError tier 2, SkillPackageDataMissingError tier 1). Mirrors signalforge.demo.copy_demo verbatim for symlink-cycle defence and importlib.resources lookup; never rmtree. Registers the three lib concretes in _EXCEPTION_TO_EXIT_CODE; adds SkillError to _EXCEPTION_MAPPING_EXCLUDED_BASES; bumps AST scan #7 count 12 -> 13. Plan: plans/super/141-claude-skill-install.md US-002 / DEC-002, 003, 005, 006, 007, 008, 009. Co-Authored-By: Claude Opus 4.7 (1M context) * bd_1-scaffolding-ezn.3: US-003 CLI install-skill subcommand + handler + exit-code mapping + subprocess smoke Adds 'signalforge install-skill []' wired via add_parser/cmd_install_skill mirroring init_demo shape. Three CliInstallSkill*Error wrappers (Path/DestUnsafe/ PackageDataMissing) registered in _EXCEPTION_TO_EXIT_CODE (tier 1/2/1). Stdout success line + (replaced existing SKILL.md) on overwrite per DEC-017. Subprocess --help smoke under cli_subprocess marker. Per-class construction branches added to test_exit_codes.py for the three new wrappers. Plan: plans/super/141-claude-skill-install.md US-003 / DEC-002, 003, 004, 008, 009, 017, 024. Co-Authored-By: Claude Opus 4.7 (1M context) * bd_1-scaffolding-ezn.7: US-007 author SKILL.md prose (frontmatter + 7 body sections) Replaces the US-001 placeholder with the real user-facing workflow per DEC-020 (frontmatter) and DEC-021 (seven sections). Body teaches the full pipeline: point-at-project, zero-cred demo, real draft+prune, prune-existing, diff-reading, gated live e2e, troubleshooting. Contains every canonical token the US-004 parity gate scans for (subcommand names + demo command lines + install-skill bootstrap). Plan: plans/super/141-claude-skill-install.md US-007 / DEC-012, 013, 020, 021. Co-Authored-By: Claude Opus 4.7 (1M context) * bd_1-scaffolding-ezn.4: US-004 SKILL ↔ CLI parity gate NEW tests/cli/test_skill_cli_parity.py scans src/signalforge/skills/signalforge/ SKILL.md for three token categories per DEC-015: 1. Every subcommand name from the live argparse parser (auto-grows) 2. Four canonical demo command lines (signalforge init-demo / generate --write / prune-existing --schema / install-skill) 3. The install-skill bootstrap line (covered by category 2's fourth entry) Plain substring match; no normalisation. Planted-violation self-check proves the gate can fail loud — per testing-signal.md AST-source-scan-gate philosophy. The gate runs inside the canonical VALIDATE_CMD (uv run pytest) so /ralph-run keeps the skill current automatically without relying on the model remembering. Plan: plans/super/141-claude-skill-install.md US-004 / DEC-015, 016, 019. Co-Authored-By: Claude Opus 4.7 (1M context) * bd_1-scaffolding-ezn.9: US-009 skill-parity.md + cli-layer.md update Replaces the anticipatory skill-parity.md with the post-implementation contract: names the actual shipped artefacts (src/signalforge/skill/, src/signalforge/skills/signalforge/SKILL.md, signalforge install-skill, tests/cli/test_skill_cli_parity.py), documents the two-name convention (skills/ plural for package-data vs skill/ singular for the Python lib), the planted-violation self-check, and the wheel exclusion defence for maintainer-only .claude/skills/. cli-layer.md § Multi-surface parity gains a paragraph naming the bundled skill as the 6th parity surface, cross-linking to skill-parity.md and the test_skill_cli_parity.py gate. ORCHESTRATOR-ONLY commit per ralph-worker-claude-dir-perms memory — workers cannot Write under .claude/ in worktrees. Plan: plans/super/141-claude-skill-install.md US-009 / DEC-018, 019. Co-Authored-By: Claude Opus 4.7 (1M context) * bd_1-scaffolding-ezn.6: US-006 docs (skills.md + mkdocs nav + cli-ops + README pointer) NEW docs/skills.md catalog page describing the bundled SignalForge skill, the install-skill subcommand, the two demo paths (zero-cred default + opt-in live e2e), the parity gate, and the maintainer-only-skill exclusion. mkdocs.yml nav gains 'Claude Code Skill' entry. docs/cli-ops.md gains the install-skill subcommand entry with stderr shapes + exit codes. README Quick start gains a one-sentence pointer after pip install. clauditor badge intentionally NOT added here — US-008 owns that surface. Plan: plans/super/141-claude-skill-install.md US-006 / DEC-021, 023. Co-Authored-By: Claude Opus 4.7 (1M context) * bd_1-scaffolding-ezn.5: US-005 5-surface parity test for install-skill Mirrors tests/cli/test_5_surface_parity_init_demo.py for the install-skill subcommand. v0.1 canonical token: 'install-skill' (no flags). Pins the token across argparse help, handler docstring, docs/cli-ops.md, plan document, and the test docstring itself. Orthogonal to test_skill_cli_parity.py (US-004): that scans the FULL CLI surface against ONE skill body; this pins ONE subcommand across FIVE surfaces. Plan: plans/super/141-claude-skill-install.md US-005 / DEC-024. Co-Authored-By: Claude Opus 4.7 (1M context) * bd_1-scaffolding-ezn.8: US-008 clauditor self-grade + README badge Adds clauditor-eval to [dependency-groups].dev (PyPI dist name; provides the 'clauditor' CLI entry point — the upstream LLM-as-judge framework SignalForge's own grading layer shares its methodology with). Grade pending — clauditor-eval installs cleanly and 'uv run clauditor grade' runs, but a meaningful score requires a maintainer-crafted EvalSpec (SignalForge-specific assertions + grading criteria); the auto-scaffolded 'clauditor init' template is generic boilerplate that would grade noise-against-noise. Refined assets/SKILL.eval.json to a 'pending-first-grade' shape that pins the current signalforge.__version__, names the grader and regen command, and explains why the maintainer must hand-tune an EvalSpec before the first real grade. README shields.io badge surfaces the pending state ('clauditor: pending', lightgrey). New 'Self-grade' section in docs/skills.md documents the regen flow for the maintainer's pre-release workflow per DEC-014. VALIDATE_CMD green: 2745 passed, all four checks. wheel_smoke + cli_subprocess gated markers also green. Plan: plans/super/141-claude-skill-install.md US-008 / DEC-014. Co-Authored-By: Claude Opus 4.7 (1M context) * bd_1-scaffolding-ezn.10: Quality Gate — fix bugs from code review Four code-review passes surfaced 8 real findings; this commit addresses them. CORRECTNESS - src/signalforge/__init__.py: revert __version__ from 0.4.0.dev0 back to 0.5.0.dev0 — the dev branch is the 0.5.0 development line; the previous value was a stale-rebase artifact (Review 1). - src/signalforge/skill/__init__.py: extend symlink defence (DEC-005) to cover every install-tree ancestor (.claude/, .claude/skills/, .claude/skills/signalforge/) — not just SKILL.md itself. A symlinked ancestor dir would otherwise smuggle writes through copytree (Review 1). - tests/skill/test_install.py: new test_install_skill_refuses_when_install_dir_ancestor_is_symlink pins the ancestor-symlink defence with a concrete attacker-elsewhere repro. SKILL.md PROSE (drift between skill and live CLI surface) - Remove the "--force" example (DEC-003 explicitly forbids the flag; re-running install-skill overwrites SKILL.md by default and preserves siblings). - Frontmatter signalforge-version: "0.X.Y" → "0.5.0.dev0" (matches __version__; was a literal placeholder shipped to operators). - Section 2: "no warehouse, no API keys, no dbt profile" was misleading — the drafter always calls Anthropic. Reword to "no dbt profile / no warehouse credentials of your own" + name the ANTHROPIC_API_KEY requirement. - LLMCacheTooLargeError mis-labelled tier-3 → corrected to tier-2 (the cache-too-large check is a pre-LLM-call input-validation gate per cli-layer.md's four-tier taxonomy). docs/skills.md DRIFT - Canonical-commands list said "signalforge --version" — DEC-015's hardcoded list is init-demo / generate --write / prune-existing --schema / install-skill (no --version variant; the parity gate scans for `version` subcommand separately via category 1). - "signalforge --version is the first thing it runs" → "signalforge version (the subcommand)" — flag vs subcommand mismatch. PARITY GATE EXTENSION (the gate would have caught the --force bug) - tests/cli/test_skill_cli_parity.py: add fourth category that scans SKILL.md for `signalforge --` patterns and asserts each flag exists on the live subparser. Pinned with the same planted-violation philosophy (verified manually: reinstating --force fails the gate loud). Closes the "skill prose teaches a flag that doesn't exist" failure mode the original gate could not catch. Validation: - uv run ruff check . — clean - uv run ruff format --check . — clean - uv run pyright — 0 errors - uv run pytest — 2747 passed (was 2745 baseline; +2 new tests) - uv run pytest -m wheel_smoke --no-cov — 5/5 - uv run pytest -m cli_subprocess --no-cov — 8/8 Plan: plans/super/141-claude-skill-install.md US-010. Co-Authored-By: Claude Opus 4.7 (1M context) * #141: plan phase=implemented; record Ralph run completion + QG findings Co-Authored-By: Claude Opus 4.7 (1M context) * #141: close codecov patch-coverage gaps (5 lines, 3 files → 100%) codecov flagged 5 missed lines in the PR diff: - src/signalforge/cli/install_skill.py:177-178 — except OSError around the existed_before probe (silently downgrades on probe failure; the install should still proceed) - src/signalforge/skill/__init__.py:150 — resolve(strict=False) fallback for a dest that doesn't exist yet (the common fresh-project case) - src/signalforge/skill/__init__.py:157 — non-ELOOP OSError raw re-raise (narrow ELOOP-only routing is load-bearing — a PermissionError must NOT be mis-attributed as a symlink cycle) - src/signalforge/skill/errors.py:66 — SkillError.__str__'s no-footer branch (when neither remediation kwarg nor default_remediation is set) Per the qg-pass-3-defer-defensive-tests-fails-codecov memory, codecov holds patch coverage to project standard regardless of "is this a real bug today" — defensive branches need test coverage even when they're fallbacks. Adds 4 tests: - test_skill_error_str_omits_footer_when_remediation_is_none (errors.py:66) - test_install_skill_propagates_non_eloop_oserror_unchanged (skill:157) - test_install_skill_resolves_nonexistent_dest_via_strict_false_fallback (skill:150) - test_install_skill_handles_oserror_in_existed_before_probe (cli:177-178) Coverage: install_skill.py 95% → 100%, skill/__init__.py 95% → 100%, skill/errors.py 95% → 100%. Full pytest: 2751 passed (was 2747). Co-Authored-By: Claude Opus 4.7 (1M context) * #141: address CodeRabbit + Copilot PR review (13 threads) Trivial: - assets/SKILL.eval.json: version 0.4.0.dev0 → 0.5.0.dev0 (matches the __version__ revert in the previous QG commit; flagged by both reviewers). - SKILL.md fenced ASCII pipeline diagram: add `text` language tag (MD040). - plans/super/141: drop the leftover duplicate "_Pending Phase 3/4._" placeholders at the end of the doc. Substantive — symlink defence broadened to EVERY bundled path: - src/signalforge/skill/__init__.py: the symlink defence used to cover only `/.claude/skills/signalforge/SKILL.md`. A symlinked `assets/SKILL.eval.json` (or symlinked `assets/` directory) would smuggle writes through copytree. Now enumerates every relative path under the bundled source tree via `rglob` and refuses to overwrite any of them through a symlink. Pinned by `test_install_skill_refuses_when_assets_dir_is_symlink`. - Also wrap `mkdir(parents=True)` in try/except NotADirectoryError → raise SkillDestUnsafeError so a non-dir component along the install chain (e.g. `/.claude` is a regular file) yields a typed remediation-bearing message instead of a raw OSError. Pinned by `test_install_skill_wraps_notadirectoryerror_from_mkdir_chain`. Substantive — CLI existed_before probe: - src/signalforge/cli/install_skill.py: probe was `.exists()` which follows symlinks AND returns False for broken symlinks. The DEC-017 contract says "True for files and symlinks (both shapes are replaced from the operator's POV)" — a broken symlink is a third shape that the probe silently downgrades. Probe now OR's `.is_symlink()` to catch the broken-symlink case (semantics stay honest even though the lib seam then raises SkillDestUnsafeError on the same path). Substantive — SKILL.md ADC contradiction: - The frontmatter `compatibility:` field and Section 2 body claimed "zero-credential demo (no warehouse needed)" while simultaneously describing the demo as sampling a public BigQuery dataset under ADC (which requires `gcloud auth application-default login` + GOOGLE_CLOUD_PROJECT). Both surfaces rewritten to honestly describe the demo's real posture: removes the dbt-project setup cost, but needs ANTHROPIC_API_KEY + ADC + GOOGLE_CLOUD_PROJECT. Section 2 also surfaces `signalforge lint --model ` as the truly-offline fallback (manifest-only, no LLM, no warehouse). Substantive — dbt parse invocation: - SKILL.md Section 1 suggested running `dbt parse` but the `allowed-tools` frontmatter does NOT include `Bash(dbt *)`. Reworded to "ask the user to run dbt parse themselves" rather than implying the skill runs it — preserves the narrow tool grant. Substantive — parity gate (`test_skill_cli_parity.py`): - Broaden `_SKILL_FLAG_USAGE_RE` to match `signalforge [ ...] --` so canonical shapes like `generate --write` and `prune-existing --schema ` are no longer skipped (CodeRabbit + Copilot). Constrain to same-line `[ \t]` (not `\s`) so the match cannot span newlines — without this, prose like "signalforge installed (pip install ...)" plus "signalforge lint --model" two paragraphs later yields a spurious `installed --model` capture (caught during validation). - Unknown-subcommand branch now FAILS instead of skipping (Copilot). A typo like `signalforge instal-skill --force` would previously skip silently; now it surfaces in the assertion message. - Verified via planted-violation: `signalforge generate --xyzbogus` injection trips the gate; restoring SKILL.md returns the gate to green. Validation: - uv run ruff check . — clean - uv run ruff format --check . — clean - uv run pyright — 0 errors - uv run pytest — 2753 passed (was 2751; +2 new tests) - uv run pytest -m wheel_smoke --no-cov — 5/5 - Patch coverage: install_skill.py 100%, skill/__init__.py 100%, skill/errors.py 100% — codecov-clean. Co-Authored-By: Claude Opus 4.7 (1M context) --------- Co-authored-by: Claude Opus 4.7 (1M context) --- .claude/rules/cli-layer.md | 2 + .claude/rules/skill-parity.md | 52 ++ README.md | 6 +- docs/cli-ops.md | 101 ++- docs/skills.md | 191 +++++ mkdocs.yml | 1 + plans/super/141-claude-skill-install.md | 753 ++++++++++++++++++ pyproject.toml | 24 +- src/signalforge/cli/__init__.py | 2 + src/signalforge/cli/_helpers.py | 37 + src/signalforge/cli/errors.py | 139 ++++ src/signalforge/cli/install_skill.py | 220 +++++ src/signalforge/skill/__init__.py | 261 ++++++ src/signalforge/skill/errors.py | 115 +++ src/signalforge/skills/signalforge/SKILL.md | 191 +++++ .../skills/signalforge/assets/SKILL.eval.json | 10 + .../test_5_surface_parity_install_skill.py | 229 ++++++ tests/cli/test_exit_codes.py | 8 + tests/cli/test_install_skill.py | 438 ++++++++++ tests/cli/test_skill_cli_parity.py | 346 ++++++++ tests/cli/test_subprocess_smoke.py | 32 + tests/skill/test_install.py | 356 +++++++++ tests/test_audit_completeness.py | 26 +- tests/test_wheel_packaging.py | 58 ++ uv.lock | 15 + 25 files changed, 3598 insertions(+), 15 deletions(-) create mode 100644 .claude/rules/skill-parity.md create mode 100644 docs/skills.md create mode 100644 plans/super/141-claude-skill-install.md create mode 100644 src/signalforge/cli/install_skill.py create mode 100644 src/signalforge/skill/__init__.py create mode 100644 src/signalforge/skill/errors.py create mode 100644 src/signalforge/skills/signalforge/SKILL.md create mode 100644 src/signalforge/skills/signalforge/assets/SKILL.eval.json create mode 100644 tests/cli/test_5_surface_parity_install_skill.py create mode 100644 tests/cli/test_install_skill.py create mode 100644 tests/cli/test_skill_cli_parity.py create mode 100644 tests/skill/test_install.py diff --git a/.claude/rules/cli-layer.md b/.claude/rules/cli-layer.md index cd40517e..70e4d7e4 100644 --- a/.claude/rules/cli-layer.md +++ b/.claude/rules/cli-layer.md @@ -124,6 +124,8 @@ A behaviour change in the CLI touches **five surfaces**, all updated in the same When introducing a new flag, write surfaces 1–3 first, then the test against those, then back-fill the DEC. +**The bundled Claude Code skill is a sixth parity surface** (#141, see `skill-parity.md`). `src/signalforge/skills/signalforge/SKILL.md` teaches the CLI surface to operators driving Claude Code; a subcommand / flag / demo-command change updates the skill body in the same commit. Enforcement is a gate, not a prompt: `tests/cli/test_skill_cli_parity.py` parses the live argparse subparser registry plus the locked demo-command list and asserts every token appears in `SKILL.md`. The gate runs inside the canonical `VALIDATE_CMD` (`uv run pytest`), so a `/ralph-run` bead that drifts the CLI from the skill fails validation until the skill is fixed. The gate is mechanical (subcommand / flag / demo-command presence); semantic freshness is the clauditor self-grade plus reviewer attention. + ## API alignment with adjacent stages `add_parser(subparsers) -> None` and `cmd_(args) -> int` for every subcommand; `main(argv: list[str] | None = None) -> int` at the top. No top-level `try/except` in `main()` — typed errors flow up; `cmd_` does the explicit catch and returns the right exit code. **One layer's exception → one CLI handler → one exit code.** diff --git a/.claude/rules/skill-parity.md b/.claude/rules/skill-parity.md new file mode 100644 index 00000000..b630165e --- /dev/null +++ b/.claude/rules/skill-parity.md @@ -0,0 +1,52 @@ +# Skill parity (the bundled Claude Code skill is a CLI-parity surface) + +Established by [#141](https://github.com/wjduenow/SignalForge/issues/141) (ship a SignalForge Claude Code skill + install command). The shipped artefacts: + +- `src/signalforge/skills/signalforge/SKILL.md` — the user-facing skill that teaches Claude to drive `signalforge generate` / `lint` / `prune-existing` / `init-demo` / `install-skill` / `version` against a user's dbt project, including the zero-credential demo and the gated live e2e flow. +- `signalforge install-skill []` — the CLI subcommand that copies the bundled skill out of the wheel into `/.claude/skills/signalforge/`. Lib seam at `signalforge.skill.install_skill(...)`; CLI handler at `signalforge.cli.install_skill`. +- `tests/cli/test_skill_cli_parity.py` — the parity gate that closes the loop. + +Because SKILL.md documents the CLI surface, it is a **parity surface**: it must stay in lockstep with the actual CLI. + +## The skill is the 6th parity surface (extends `cli-layer.md`) + +A behaviour change to the CLI subcommand/flag surface — adding, renaming, or removing a subcommand; changing the flags or demo commands the skill names — updates `src/signalforge/skills/signalforge/SKILL.md` in the **same change**. The bundled skill is one more entry in `cli-layer.md`'s "a behaviour change touches N surfaces" rule; treat it exactly like the help string / ops doc / test surfaces already listed there. + +## Enforcement is a gate, not a prompt (load-bearing) + +Do **not** rely on the model remembering to update the skill during a `/ralph-run` (or any) session. `tests/cli/test_skill_cli_parity.py` is the **parity gate** — it parses the live CLI (`signalforge.cli._build_parser()` → walks the `_SubParsersAction.choices` mapping) plus the locked `_CANONICAL_DEMO_COMMANDS` tuple and asserts every token appears verbatim in `SKILL.md`. Three categories scanned (per #141 DEC-015): + +1. **Every subcommand name** from the live argparse parser. Auto-grows when a new subcommand lands — the gate iterates the parser, never a hardcoded set, so adding `signalforge foo` and forgetting `SKILL.md` fails the gate without anyone editing the test. +2. **Four canonical demo command lines** (hardcoded in the test, mirrors the demo flow taught in SKILL.md): `signalforge init-demo`, `signalforge generate --write`, `signalforge prune-existing --schema `, `signalforge install-skill`. Plain substring match; no whitespace / case normalisation (mirrors the envelope-breach guard pattern from `business-rule-tests.md`). +3. **The install-skill bootstrap line** — covered by category 2's fourth entry but documented as a separate concern in the test docstring. + +The third test in the file (`test_parity_gate_catches_missing_subcommand_planted_violation`) is the **planted-violation self-check** required by `testing-signal.md` § "AST source-scan gates": it writes a synthetic SKILL.md missing one subcommand to `tmp_path`, drives the same factored-out helper the real gate uses, and asserts an `AssertionError` is raised. Without it, a refactor that broke the scan visitor would silently disable the gate at the exact moment a real violation needed catching. + +Because `/ralph-run` runs `VALIDATE_CMD` on every bead, a change that drifts the CLI from the skill **fails validation until SKILL.md is updated** — the skill stays current automatically. The gate also encodes "**when appropriate**": it fires only on a relevant surface change, never on unrelated work. This is the same gate-over-prompt philosophy as the AST scans, drift detectors, and grep gates in `testing-signal.md`. + +## Worker-writability — keep the skill in `src/`, never `.claude/` + +Ralph workers **cannot write to `.claude/` in worktrees** (orchestrator-only — see the user memory `ralph-worker-claude-dir-perms`). The shipped skill therefore lives in `src/signalforge/skills/` and the parity gate in `tests/` — **both worker-writable** — so a worker that trips the gate fixes `SKILL.md` in `src/` itself. The maintainer-only skills (`release-manager`, `review-agentskills-spec`) stay under repo-root `.claude/skills/` and are excluded from the wheel + the install command: + +- They live at repo-root `.claude/skills/`, outside `src/`, so the Hatch `include = ["src/signalforge/skills"]` cannot reach them by construction (DEC-022 of #141). +- `signalforge install-skill` enumerates from `importlib.resources.files("signalforge").joinpath("skills")` — the package-data tree only — so there's no code path that could install them. +- A defensive **negative assertion** in `tests/test_wheel_packaging.py` (`test_wheel_excludes_maintainer_only_claude_skills`) documents this intent: no `.claude/skills/*` paths appear in the built wheel. + +Never move the shipped skill under `.claude/`: that would make it un-updatable by workers and defeat this rule. + +## The two-name convention (load-bearing) + +Two paths, one each side of the seam — easy to confuse, deliberately distinct: + +- **Package-data tree:** `src/signalforge/skills/signalforge/SKILL.md` — plural `skills/` parent matches the install destination shape (`.claude/skills/signalforge/SKILL.md`) and allows future sibling skills (e.g. `skills/signalforge-grade/`) without restructuring. NOT a Python package — no `__init__.py` under `skills/` or `skills/signalforge/`. Mirrors `src/signalforge/_demo/`'s posture (package-data, not a Python package). +- **Python lib package:** `src/signalforge/skill/` — singular `skill/`, a real Python package with `__init__.py` + `errors.py`. Owns `install_skill(...)` and the `SkillError` hierarchy. Mirrors `signalforge.demo` exactly. + +When adding a v0.2 sibling skill, add `src/signalforge/skills//SKILL.md` (recursive Hatch include picks it up); the parity gate auto-grows for ``'s subcommands; the Python lib stays a single `signalforge.skill` module. + +## What the gate cannot catch + +The gate enforces the **mechanical** surface (subcommands / flags / demo commands present). It cannot judge whether the skill's **prose** is still accurate after a behaviour change. Back that with the optional clauditor self-grade (`clauditor grade src/signalforge/skills/signalforge/SKILL.md`, see #141 DEC-014 + US-008 — pre-release manual run, pinned in `assets/SKILL.eval.json`, surfaced via shields.io README badge) and reviewer attention — the gate is necessary, not sufficient. + +## Reference + +`#141` — the skill, the `install-skill` command, and the parity gate. `plans/super/141-claude-skill-install.md` — the full plan (24 DECs). `cli-layer.md` § "Multi-surface parity for behaviour changes" — the N-surface parity rule the skill extends + the exit-code taxonomy the install command follows. `python-build.md` — wheel packaging of the skill (`include` + `wheel_smoke`). `testing-signal.md` — the gate-over-prompt philosophy + planted-violation self-check requirement. `tests/cli/test_skill_cli_parity.py` — the gate. `tests/cli/test_5_surface_parity_*.py` — the per-subcommand parity-test precedent (orthogonal to this gate — that one pins ONE subcommand across five surfaces; this one scans the FULL CLI surface against ONE skill body). diff --git a/README.md b/README.md index f5cdea37..192e70e1 100644 --- a/README.md +++ b/README.md @@ -1,4 +1,4 @@ -[![codecov](https://codecov.io/gh/wjduenow/SignalForge/branch/dev/graph/badge.svg)](https://codecov.io/gh/wjduenow/SignalForge) [![docs](https://img.shields.io/badge/docs-signalforge-blue?logo=materialformkdocs)](https://wjduenow.github.io/SignalForge/) +[![codecov](https://codecov.io/gh/wjduenow/SignalForge/branch/dev/graph/badge.svg)](https://codecov.io/gh/wjduenow/SignalForge) [![docs](https://img.shields.io/badge/docs-signalforge-blue?logo=materialformkdocs)](https://wjduenow.github.io/SignalForge/) [![clauditor-graded](https://img.shields.io/badge/clauditor-pending-lightgrey)](src/signalforge/skills/signalforge/assets/SKILL.eval.json) # SignalForge @@ -113,6 +113,10 @@ without adding it to a project environment. **Working from a clone (contributing)?** Install the dev toolchain with `uv sync --dev` — see [CONTRIBUTING.md](CONTRIBUTING.md) for the full workflow. +Run `signalforge install-skill` to drop the [Claude Code skill](docs/skills.md) +into your project's `.claude/skills/signalforge/` and let Claude drive +SignalForge end-to-end. + ### 2. Authenticate to BigQuery and your LLM provider ```bash diff --git a/docs/cli-ops.md b/docs/cli-ops.md index 5d56dbea..cd81be17 100644 --- a/docs/cli-ops.md +++ b/docs/cli-ops.md @@ -57,10 +57,10 @@ After install, the `signalforge` console script is registered via ## Subcommands -The CLI exposes five subcommands: `generate`, `init-demo`, `lint`, -`prune-existing`, `version`. `signalforge --help` prints the -top-level help; each subcommand has its own `--help` page (e.g. -`signalforge generate --help`). +The CLI exposes six subcommands: `generate`, `init-demo`, +`install-skill`, `lint`, `prune-existing`, `version`. `signalforge +--help` prints the top-level help; each subcommand has its own +`--help` page (e.g. `signalforge generate --help`). ### `signalforge generate ` @@ -335,6 +335,99 @@ signalforge lint signalforge generate models/staging/stg_bikeshare_trips.sql --dry-run ``` +### `signalforge install-skill []` + +Copy the bundled SignalForge Claude Code skill into +`/.claude/skills/signalforge/`. With the skill installed, a +Claude Code session in `` recognises requests like "draft tests +for `dim_customers`" or "prune my existing `schema.yml`," picks the +right `signalforge` subcommand and flags, and explains the resulting +kept / dropped / flagged diff back to the user. See +[docs/skills.md](skills.md) for the skill catalog entry and the body +sections it covers (DEC-021 of +[`plans/super/141-claude-skill-install.md`](../plans/super/141-claude-skill-install.md)). + +Wraps the public library entry point +`signalforge.skill.install_skill(dest) -> Path`; the CLI re-raises +the lower-level `SkillError` subclasses as `CliInstallSkill*Error` +wrappers at the handler boundary so the four-tier exit-code taxonomy +stays homogeneous (DEC-008). + +Positional argument: + +- `` — Destination directory. Optional; default `.` (the + current working directory), so the common invocation from a dbt + project root is just `signalforge install-skill`. Relative paths + resolve against the current working directory; `~` expands. + Symlink-cycle defence applies (resolves via `.resolve(strict=True)`, + falling back to `.resolve(strict=False)` on + `FileNotFoundError` / `NotADirectoryError`) and raises + `CliInstallSkillPathError` on a cycle on every supported Python + version (gh-108958). **No `--project-dir` containment gate applies** + — `install-skill` is the second subcommand that *creates* a project + context rather than operating *inside* one (the first is + `init-demo`), so the `canonicalise_user_path(...)` containment + helper used by every other CLI flag is deliberately bypassed + (DEC-006). + +Flags: none. There is **no `--force` flag** (DEC-003): the library +seam always overwrites every file SignalForge ships and preserves +every other file in the destination tree, so the +`--force`-against-symlink-dest hazard `init-demo --force` defends +against does not apply here. + +Install path: `/.claude/skills/signalforge/SKILL.md`. The +companion `assets/` subtree (also part of the bundled skill) lands +alongside it. + +Exit codes (four-tier taxonomy; see § Four-tier exit-code taxonomy +for the full table): + +- `0` — install succeeded; INFO line printed to stdout. +- `1` — `CliInstallSkillPathError` (symlink cycle on ``) or + `CliInstallSkillPackageDataMissingError` (broken wheel install: + the bundled skill tree could not be located via + `importlib.resources` — practically unreachable on a clean + `pip install signalforge-dbt` run). +- `2` — `CliInstallSkillDestUnsafeError`: `` exists as a + regular file (not a directory), OR the existing `SKILL.md` is a + symlink (writing would follow the link and clobber an arbitrary + destination). +- `3` — n/a. `install-skill` makes no network, warehouse, or LLM + call. + +Stdout shapes: + +- New install (no existing `SKILL.md` at the target): + ```text + Installed SignalForge skill to + ``` +- Upgrade-in-place (existing `SKILL.md` was overwritten — detected + via `Path.exists()` BEFORE the copy, DEC-017): + ```text + Installed SignalForge skill to (replaced existing SKILL.md) + ``` + + The `(replaced existing SKILL.md)` suffix surfaces the lib seam's + upgrade-in-place overwrite policy so operators know their + hand-edited `SKILL.md` was replaced. The operator can `git diff` if + they had the file under version control. + +Stderr shapes: standard `ERROR: ` + optional +`↳ Remediation: ` per tier (see § Stderr message shape per +tier); no multi-violation header / bullet form fires from this +subcommand. + +Example: + +```bash +cd /repo/dbt/analytics +signalforge install-skill +# stdout: Installed SignalForge skill to /repo/dbt/analytics/.claude/skills/signalforge/SKILL.md +echo $? +# 0 +``` + ### `signalforge lint` Validate the five existing `signalforge.yml` config blocks (`safety:`, diff --git a/docs/skills.md b/docs/skills.md new file mode 100644 index 00000000..ae6ebb3a --- /dev/null +++ b/docs/skills.md @@ -0,0 +1,191 @@ +# Claude Code Skill + +SignalForge ships a [Claude Code skill](https://docs.claude.com/en/docs/claude-code/skills) +that teaches Claude to drive the `signalforge` CLI end-to-end against a dbt +project. With the skill installed, a Claude Code session in the project root +recognises requests like "draft tests for `dim_customers`," "prune my existing +`schema.yml`," or "run the demo," picks the right `signalforge` subcommand and +flags, and explains the kept / kept-uncertain / dropped / flagged diff back to +the user. + +The skill is bundled inside the `signalforge-dbt` wheel under +`src/signalforge/skills/signalforge/` and installed into your project with one +command (issue [#141](https://github.com/wjduenow/SignalForge/issues/141)). + +## Install + +```bash +signalforge install-skill [] +``` + +Drops the bundled skill into `/.claude/skills/signalforge/SKILL.md`. +`` defaults to the current working directory, so the common invocation +from a dbt project root is just `signalforge install-skill`. + +| Aspect | Behaviour | +| --- | --- | +| Default `` | current working directory (`.`) | +| Install path | `/.claude/skills/signalforge/SKILL.md` | +| Overwrite policy | Always replaces every file SignalForge ships (no `--force` flag). **Preserves** every other file in the destination tree — your hand-edited `.claude/` siblings are untouched. | +| Overwrite signal | On success, stdout prints `Installed SignalForge skill to `; appends `(replaced existing SKILL.md)` when an existing file was overwritten. | + +The CLI handler wraps the public `signalforge.skill.install_skill(dest)` +library entry point at the `cmd_install_skill` boundary and re-raises the +three `SkillError` subclasses as `CliInstallSkill*Error` wrappers so the +four-tier exit-code taxonomy stays homogeneous. Exit codes: + +| Tier | Exit | Causes | +| --- | --- | --- | +| Load | `1` | `CliInstallSkillPathError` (symlink cycle on ``); `CliInstallSkillPackageDataMissingError` (broken wheel install — the bundled skill tree could not be located via `importlib.resources`). | +| Input | `2` | `CliInstallSkillDestUnsafeError` — `` exists as a regular file, OR the existing `SKILL.md` is a symlink (writing would follow the link). | +| API | `3` | n/a — install-skill makes no network / warehouse / LLM call. | + +Pointer to [docs/cli-ops.md § `signalforge install-skill`](cli-ops.md#signalforge-install-skill-dest) +for the full flag table and stderr shapes. + +## What the skill teaches + +The SKILL.md body is a numbered workflow that walks Claude through the full +SignalForge surface (DEC-021 of +[`plans/super/141-claude-skill-install.md`](../plans/super/141-claude-skill-install.md)): + +1. **Point at a dbt project** — verify `target/manifest.json` exists, name a + model to work on. +2. **Zero-credential demo** — `signalforge init-demo` followed by + `signalforge generate --write` against the bundled Austin + bikeshare fixture. No warehouse needed; runs entirely from the wheel. +3. **Real project: draft + prune** — `signalforge generate --write` + with the safety posture (schema-only default; `--mode sample` is opt-in; + document the cost) and `--estimate` for a pre-flight cost preview. +4. **Grade tests you already have** — `signalforge prune-existing + --schema ` runs the prune step (no LLM call) over an externally + authored `schema.yml`, so the warehouse tells you which existing tests + add signal. +5. **Reading the diff** — kept / kept-uncertain / dropped / flagged tiers + and the per-artifact "why" cascade (rationale → evidence → fallback). +6. **Optional: live e2e demonstration** — gated behind explicit user + confirmation, env-var checks, and a cost warning. Runs the maintainer + `pytest -m e2e --no-cov` flow against the public BigQuery dataset. +7. **Troubleshooting** — common errors (`ModelNotFoundError`, + `WarehouseAuthError`, `LLMCacheTooLargeError`, etc.) with one-line fixes + and a pointer to [docs/cli-ops.md](cli-ops.md). + +The skill always activates against the live `signalforge` CLI on the user's +PATH — `signalforge version` (the subcommand) is the first thing it runs to +confirm the install resolved. + +## Two demo paths + +The skill body offers two ways to demonstrate SignalForge to a user. Pick +based on whether the user has warehouse credentials ready. + +### Zero-credential demo (default) + +`signalforge init-demo` copies the bundled Austin bikeshare demo project out +of the wheel into a writable directory; `signalforge generate +--write` then runs the full draft + prune + grade + diff pipeline against +that fixture. + +**No warehouse access required.** The drafter still calls Anthropic (so the +demo needs `ANTHROPIC_API_KEY`), but the prune step works against the local +fixture rather than a live warehouse. This is the default path the skill +recommends — fastest time-to-signal, zero cloud setup. See +[docs/cli-ops.md § `signalforge init-demo`](cli-ops.md#signalforge-init-demo-dest) +for the dest-policy and overwrite story. + +### Live e2e (opt-in, gated) + +The full end-to-end smoke runs `uv run pytest -m e2e --no-cov` against the +public `bigquery-public-data.austin_bikeshare.bikeshare_trips` dataset. The +skill body **forces an explicit user confirmation** before triggering this +path: it checks that `SF_RUN_BQ=1`, `GOOGLE_CLOUD_PROJECT`, and +`ANTHROPIC_API_KEY` are all set, warns about the LLM + warehouse cost (a +single run typically lands well under \$0.15 of Anthropic spend plus +~200–500 MB of BigQuery scan), and only then invokes the gated test. See +[docs/e2e-smoke-test.md](e2e-smoke-test.md) for the maintainer-facing +walkthrough of the same flow. + +## Parity gate + +A pytest gate at `tests/cli/test_skill_cli_parity.py` parses the live CLI +(every registered subcommand from the `argparse` subparser registry) and +asserts that each subcommand AND the four canonical demo commands +(`signalforge init-demo`, `signalforge generate --write`, +`signalforge prune-existing --schema `, +`signalforge install-skill`) appear in `SKILL.md`. The gate runs inside the +canonical `VALIDATE_CMD` +(`uv run pytest`), so a CLI change that drifts the surface from the skill +fails validation until `SKILL.md` is updated in the same change. + +This is the gate-over-prompt enforcement described in +[`.claude/rules/skill-parity.md`](https://github.com/wjduenow/SignalForge/blob/dev/.claude/rules/skill-parity.md) +— the contributor never has to remember to update SKILL.md; the test +suite makes drift impossible. The gate is mechanical only (subcommand +names + demo command tokens present); reviewer attention still backs prose +accuracy. + +## Self-grade + +The skill prose is graded with [clauditor](https://github.com/wjduenow/clauditor) +(PyPI distribution `clauditor-eval`) — the LLM-as-judge harness +SignalForge's own grading layer (`signalforge.grade`) shares its +methodology with. The pinned score lives at +`src/signalforge/skills/signalforge/assets/SKILL.eval.json` and is surfaced +as the `clauditor-graded` shields.io badge on the +[project README](https://github.com/wjduenow/SignalForge#readme). + +**Operating model — pre-release manual, no CI integration.** Per +[DEC-014 of `plans/super/141-claude-skill-install.md`](https://github.com/wjduenow/SignalForge/blob/dev/plans/super/141-claude-skill-install.md), +the maintainer regrades before tagging a release; the same commit bumps +SKILL.md (if it changed), the pinned `assets/SKILL.eval.json`, and the +README badge. No Anthropic key lives in repo secrets; no per-PR cost. + +**Regenerate the grade:** + +```bash +uv run clauditor grade src/signalforge/skills/signalforge/SKILL.md +``` + +`clauditor-eval` is in `[dependency-groups].dev`, so `uv sync --dev` picks +it up. The command reads an `EvalSpec` (the SignalForge-specific +assertions + grading criteria, scaffolded via +`uv run clauditor init ` and then hand-tuned by the maintainer), +runs the configured grading model against the skill's output, and writes +per-iteration sidecars under `.clauditor/iteration-N/`. The maintainer +then transcribes the resulting `score`, the current +`signalforge.__version__`, and an ISO-8601 UTC `graded_at` timestamp into +`assets/SKILL.eval.json` so the README badge reflects the latest pinned +score. + +Until the first real grade lands, `assets/SKILL.eval.json` carries a +`status: "pending-first-grade"` placeholder and the README badge reads +`clauditor: pending`. + +## Maintainer-only skills (excluded) + +Two skills live at repo-root `.claude/skills/` rather than under `src/`: +`release-manager` (drives the PyPI release flow) and +`review-agentskills-spec` (the maintainer's reference for the +agentskills-spec project). Both are **outside the wheel by construction** +— Hatch's `tool.hatch.build.targets.wheel.packages = ["src/signalforge"]` +declaration only ships the `src/` tree, so a `pip install signalforge-dbt` +user never sees them and `signalforge install-skill` cannot install them. + +This intent is documented by a negative assertion in +`tests/test_wheel_packaging.py::test_wheel_excludes_maintainer_only_claude_skills`, +which builds the wheel and asserts neither maintainer-only skill name +appears in the artefact's file list. + +## Reference + +- [docs/cli-ops.md § `signalforge install-skill`](cli-ops.md#signalforge-install-skill-dest) + — full flag table, exit-code mapping, stderr shapes for the + `install-skill` subcommand. +- [docs/e2e-smoke-test.md](e2e-smoke-test.md) — operator walkthrough of + the live e2e flow the skill's gated demo path triggers. +- [`.claude/rules/skill-parity.md`](https://github.com/wjduenow/SignalForge/blob/dev/.claude/rules/skill-parity.md) + — contributor rule that documents the SKILL ↔ CLI parity gate. +- [`plans/super/141-claude-skill-install.md`](https://github.com/wjduenow/SignalForge/blob/dev/plans/super/141-claude-skill-install.md) + — design record (DEC-001 … DEC-024), including the seven SKILL.md body + sections (DEC-021), the bundled-skill-vs-maintainer-skill split + (DEC-022), and this docs entry (DEC-023). diff --git a/mkdocs.yml b/mkdocs.yml index 356d9404..e9126beb 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -46,6 +46,7 @@ markdown_extensions: nav: - Home: index.md - CLI Reference: cli-ops.md + - Claude Code Skill: skills.md - Pipeline Stages: - Manifest Loader: manifest-loader-ops.md - Warehouse Adapter: warehouse-adapter-ops.md diff --git a/plans/super/141-claude-skill-install.md b/plans/super/141-claude-skill-install.md new file mode 100644 index 00000000..223b664d --- /dev/null +++ b/plans/super/141-claude-skill-install.md @@ -0,0 +1,753 @@ +# 141: SignalForge Claude Code skill + `install-skill` command + +## Meta + +- **Ticket:** [GH #141](https://github.com/wjduenow/SignalForge/issues/141) +- **Branch:** `feature/141-claude-skill-install` +- **Worktree:** `../worktrees/SignalForge/141-claude-skill-install` +- **Phase:** implemented +- **PR:** [#166](https://github.com/wjduenow/SignalForge/pull/166) +- **Epic:** `bd_1-scaffolding-ezn` +- **Sessions:** + - 2026-05-29 — Phase 1 discovery (parallel research, 4 scoping decisions locked) + - 2026-05-29 — Phase 2 architecture review (no blockers, 2 concerns surfaced) + - 2026-05-29 — Phase 3 refinement (24 DECs locked), Phase 4 detailing (11 stories), Phase 5 published as draft PR #166 + - 2026-05-30 — Phase 6 approved, Phase 7 devolved to beads (epic `bd_1-scaffolding-ezn`, 11 tasks) + - 2026-05-30 — Ralph run end-to-end: all 11 stories landed (US-001 → US-011); QG (US-010) fixed 8 review findings including a stale-rebase __version__ downgrade, a SKILL.md `--force` line that didn't exist on the CLI (caught by Reviews 3+4 independently), and a symlinked-ancestor-dir bypass; gate extended with a flag-validity scan; PR #166 marked ready for review + +## Beads manifest + +- **Epic:** `bd_1-scaffolding-ezn` — "141: SignalForge skill + install-skill" +- **Tasks:** + - `bd_1-scaffolding-ezn.1` — US-001 — Bootstrap skills tree + wheel packaging (no deps; READY) + - `bd_1-scaffolding-ezn.2` — US-002 — Public `signalforge.skill` lib + errors (deps: .1) + - `bd_1-scaffolding-ezn.3` — US-003 — CLI `install-skill` subcommand (deps: .2) + - `bd_1-scaffolding-ezn.4` — US-004 — SKILL ↔ CLI parity gate (deps: .1, .3, .7) + - `bd_1-scaffolding-ezn.5` — US-005 — 5-surface parity for install-skill (deps: .3, .6) + - `bd_1-scaffolding-ezn.6` — US-006 — Docs (skills.md + nav + cli-ops + README) (deps: .3) + - `bd_1-scaffolding-ezn.7` — US-007 — Author SKILL.md prose (deps: .1, .3) + - `bd_1-scaffolding-ezn.8` — US-008 — Clauditor self-grade + README badge (deps: .7) + - `bd_1-scaffolding-ezn.9` — US-009 — skill-parity.md + cli-layer.md update [ORCHESTRATOR-ONLY] (deps: .3, .4) + - `bd_1-scaffolding-ezn.10` — Quality Gate — code-review ×4 + CodeRabbit (deps: .1…9) + - `bd_1-scaffolding-ezn.11` — Patterns & Memory (deps: .10) +- **Worktree:** `/home/wesd/Projects/worktrees/SignalForge/141-claude-skill-install` +- **Branch:** `feature/141-claude-skill-install` + +## Ticket summary + +Ship a user-facing **Claude Code skill** for SignalForge, bundled in the wheel, plus an +**install command** that drops it into a target project's `.claude/skills/` — mirroring +clauditor's `src/clauditor/skills/` + `clauditor setup` pattern. The skill teaches Claude +how to drive the `signalforge` CLI against a user's dbt project so adoption is "install the +package, run the skill," not "read the docs and assemble commands by hand." + +**AC-1** `pip install signalforge-dbt` then `signalforge install-skill` drops a working +`SKILL.md` into `.claude/skills/signalforge/`, and a fresh Claude Code session activates it +on a relevant prompt. + +**AC-2** `wheel_smoke` asserts the skill ships in the wheel. + +**AC-3** Install command honours the four-tier exit codes + no-traceback floor; registered +in the exit-code AST scan. + +**AC-4** README + docs link the skill; `mkdocs build` stays clean. + +**AC-5** On request, the skill runs the zero-credential `init-demo` → `generate` demo +end-to-end; the live `pytest -m e2e` path is gated behind explicit user confirmation + +env-var checks (clean skip when unset, cost warning before running). + +**AC-6** The skill's CLI surface is enforced by a parity gate running inside the canonical +`VALIDATE_CMD` (`uv run pytest`): adding/changing a subcommand or demo command without +updating `SKILL.md` fails the test — so `/ralph-run` keeps the skill current automatically, +without relying on the model remembering. + +## Discovery + +### Codebase findings (key seams) + +- **CLI subcommand template — `init-demo` is the closest precedent.** Both `add_parser` and + `cmd_init_demo` live in `src/signalforge/cli/init_demo.py`; registered from + `src/signalforge/cli/__init__.py:84`. The new module is `src/signalforge/cli/install_skill.py` + following the same shape verbatim. +- **Library-surface wrap pattern.** `signalforge.demo.copy_demo(dest, *, force=False) -> Path` + is the public lib; `Demo*Error` hierarchy lives in `signalforge.demo.errors`. The CLI handler + wraps lib errors into `CliInitDemo*Error` (`src/signalforge/cli/errors.py`). New + `signalforge.skill` subpackage mirrors this: `install_skill(dest, *, force) -> Path` + a + `Skill*Error` hierarchy + CLI-side `CliInstallSkill*Error` wrappers. +- **Exit-code registry.** `_EXCEPTION_TO_EXIT_CODE` in `src/signalforge/cli/_helpers.py` is + the single source of truth. `init-demo`'s registrations (DemoPathError → 1, + DemoDestExistsError → 2, CliInitDemoFixtureMissingError → 1, etc.) are the template. +- **Wheel packaging.** `[tool.hatch.build.targets.wheel]` in `pyproject.toml` carries + `packages = ["src/signalforge"]` + `include = ["src/signalforge/_demo"]`. Add a sibling + `include` entry for the skills tree. (See decision on path name below.) +- **`wheel_smoke` precedent.** `tests/test_wheel_packaging.py::_EXPECTED_DEMO_FILES` is a + 7-file tuple asserted present in the built `.whl`. The skills equivalent asserts + `SKILL.md` + `SKILL.eval.json` (if grading) + any `assets/` files appear under the + expected wheel path. +- **AST audit-completeness scan #7.** Already a depth-1∪depth-2 glob over + `src/signalforge/*/errors.py`; new `signalforge/skill/errors.py` lands automatically. + Update the count test (`test_scan_7_discovers_every_per_stage_errors_module` — bump + 12 → 13) AND add `SkillError` to `_EXCEPTION_MAPPING_EXCLUDED_BASES` if the hierarchy + spans tiers 1+2 (mirrors `DemoError`/`IngestError`). +- **5-surface parity test precedent.** `tests/cli/test_5_surface_parity_init_demo.py` + pins canonical tokens (subcommand name, key flags) across argparse help / handler docstring + / `docs/cli-ops.md` / plan / test name. The skill ticket needs the same shape for the + `install-skill` flags. +- **Skill ↔ CLI parity gate (NEW, separate test).** Distinct from the 5-surface parity gate: + this one parses the live argparse subparser registry from `signalforge.cli._build_parser()` + and asserts every registered subcommand name + the demo-flow commands appear verbatim in + `src/signalforge/skills/signalforge/SKILL.md`. Mirrors the mechanical surface-scan idea. +- **Subprocess smoke pattern.** `tests/cli/test_subprocess_smoke.py` runs `signalforge + install-skill --help` under `@pytest.mark.cli_subprocess` (default-deselected); asserts + `returncode == 0`, subcommand-unique tokens in stdout, no traceback on stderr. +- **MkDocs nav.** `mkdocs.yml` has a flat `nav:` with a "CLI Reference: cli-ops.md" + a + "Pipeline Stages" subsection. A "Claude Code Skill: skills.md" entry at the top level + (after CLI Reference) fits naturally. +- **README quick-start.** `README.md:78-100` has `## Quick start` → `Install` subsection. + The skill pointer fits as a follow-up sentence after `pip install`. + +### Agent-skills spec (what the SKILL.md must contain) + +From `.claude/skills/review-agentskills-spec/SKILL.md` + the `release-manager` example: + +- **Frontmatter:** `name` (matches parent dir), `description` (activation triggers — "use + when X, Y, or Z"), `compatibility` (hard requirements: dbt project, manifest.json present, + optional warehouse profile + API keys), `disable-model-invocation` (omit; this skill + reasons about the diff), `allowed-tools` (scoped Bash patterns). +- **`allowed-tools` scope** (zero-credential default; live e2e gated behind confirmation): + - `Bash(signalforge *)` — every CLI invocation + - `Bash(uv run signalforge *)` — uv-run variant + - `Bash(cat *)`, `Bash(ls *)`, `Bash(grep *)` — inspecting fixtures + diff output + - `Read`, `Write`, `Edit` — for the user's dbt project files only + - (Conditional, behind explicit user opt-in) `Bash(uv run pytest -m e2e*)` for the live + smoke; the skill body forces a confirmation gate before invoking +- **Body shape:** `# /signalforge — drafts and prunes dbt tests with an LLM` + numbered + workflow sections covering (1) point at a dbt project; (2) zero-cred demo via + `init-demo` → `generate`; (3) `prune-existing` for tests the user already has; (4) + reading the kept/kept-uncertain/dropped/flagged diff + per-artifact "why"; (5) safety + posture (schema-only default; sample is opt-in); (6) optional live e2e (gated). + +### Convention/rule constraints (filtered) + +Most-load-bearing per `.claude/rules/`: + +- **`cli-layer.md`:** `add_parser`/`cmd_install_skill`; four-tier exit codes (0/1/2/3); + library-surface wrap pattern (lib seam + thin CLI handler); typed-error registration in + `_EXCEPTION_TO_EXIT_CODE`; no-traceback floor; single-boundary `try/except Exception`; + path canonicalisation at orchestrator via `canonicalise_user_path(raw, project_dir)` (with + caveat: install-skill has NO project_dir requirement — the user runs it before they have + one configured, like `init-demo`); subprocess `--help` smoke under `cli_subprocess`; + 5-surface parity for new flags. +- **`python-build.md`:** Explicit `include = ["src/signalforge/skills"]`; `wheel_smoke` test + pins the skill file set (dotfile-inclusion fragility noted — SKILL.md is not a dotfile so + this is straightforward, but `assets/` recursion needs verification in the smoke test). +- **`docs-publishing.md`:** New `docs/skills.md` requires a `nav:` entry in `mkdocs.yml` in + the same commit. No new docs deps. +- **`testing-signal.md`:** No `assert True`-shaped tests; strict markers (already set); AST + source-scan gates if any new "must-call X" gate is added; marker-gated subprocess pattern. +- **`manifest-readers.md`:** Three symlink/containment traps apply to install-skill's + destination-path validation. +- **`skill-parity.md` (anticipatory rule — file NOT YET on disk, lives in CLAUDE.md context + only).** Specifies the parity gate contract: skill lives at + `src/signalforge/skills/signalforge/SKILL.md` (worker-writable, NEVER `.claude/`); gate + parses live CLI subparser registry; runs inside `VALIDATE_CMD`. The rule file is part of + this ticket — Deliverable 7 ("parity-surface rule entry") ships it. +- **`safety-layer.md`:** NOT APPLICABLE. install-skill is a deterministic file copy with no + LLM/warehouse/audit seam. + +## Scoping decisions (Phase 1 close) + +- **S-1 Destination policy:** Always overwrite SKILL.md (+ the files we ship); preserve any + sibling files the user has added under `.claude/skills/signalforge/`. **No `--force` flag** + in v0.1. Friendlier for upgrade-in-place ("re-run install-skill, get the new SKILL.md"). + We still refuse if SKILL.md *itself* is a symlink — writing follows the link, which is + the same defence init-demo's `copy_demo` already implements for `--force`-against-symlink. +- **S-2 e2e demo paths:** Both. Zero-credential `init-demo` → `generate` is the always-on + default. Live `pytest -m e2e` is opt-in — the skill body forces an explicit user + confirmation, checks `SF_RUN_BQ` / `GOOGLE_CLOUD_PROJECT` / `ANTHROPIC_API_KEY`, and warns + about warehouse + LLM cost before invoking. `allowed-tools` scopes the live path + conditionally. +- **S-3 Self-grade badge:** Include in v0.1. Run `clauditor grade` against the SKILL.md, + pin the score in `assets/SKILL.eval.json` (sibling of SKILL.md), and surface a shields.io + badge from the README. (The CI-vs-local-pinning question is Phase 3 refinement.) +- **S-4 Skill src path:** `src/signalforge/skills/signalforge/SKILL.md` — plural `skills/` + parent allows a future sibling skill (e.g., `skills/signalforge-grade/`) without + restructuring; matches the install destination shape exactly; matches the anticipatory + `skill-parity.md` rule verbatim. + +## Architecture review + +| Area | Rating | Findings | +|------|--------|----------| +| **Security** | pass | Mirror `signalforge.demo.copy_demo`'s symlink-cycle trap (`resolve(strict=True)` first, fall back to `strict=False` on `FileNotFoundError`/`NotADirectoryError`, catch both `RuntimeError` (≤3.12) and `OSError(errno.ELOOP)` (≥3.13)). Per S-1 we never `rmtree`, so the `--force`-against-symlink-dest hazard collapses; we still refuse to overwrite if `/.claude/skills/signalforge/SKILL.md` is a symlink (writing follows the link). Path canonicalisation rolled inline like `copy_demo` (NOT via `canonicalise_user_path`, which requires a project_dir — install-skill is the second "creates the project context" entry point alongside `init-demo`, and its module docstring will document this verbatim, citing the `copy_demo` precedent). | +| **API design** | concern | Default `` is `.` (CWD), so the install path becomes `/.claude/skills/signalforge/SKILL.md` — operator runs the command from the dbt project root. Mirrors `init-demo`'s `./signalforge-demo/` ergonomics. Lib seam: `install_skill(dest: Path \| str = ".") -> Path` returns the absolute SKILL.md path. **Concern:** if `/.claude/skills/signalforge/` exists with an unmodelled file alongside SKILL.md, do we report what we preserved? Lock the answer in Phase 3. | +| **Packaging / wheel_smoke** | pass | `include = ["src/signalforge/skills"]` ships the tree recursively (confirmed by the `_demo` precedent — every nested file lands in the wheel without additional globs). `wheel_smoke` extends with a sibling `_EXPECTED_SKILL_FILES` tuple naming SKILL.md + SKILL.eval.json (+ any v0.1 assets). Dotfile-fragility note in `python-build.md` doesn't apply (SKILL.md is not a dotfile). | +| **Observability** | pass | One INFO log line at success: `{"installed": "", "preserved_siblings": [...]}` (lazy-format JSON; raw paths are user-owned, no PII concerns). No DEBUG/WARNING/audit JSONL — install-skill is a deterministic file copy. | +| **Testing strategy** | pass | (1) lib seam unit tests (`tests/skill/test_install.py`) — happy / overwrite-existing / preserve-siblings / SkillDestUnsafeError / SkillPackageDataMissingError. (2) CLI handler tests (`tests/cli/test_install_skill.py`) — main(argv) paths exercising each exit code. (3) Subprocess `--help` smoke under `cli_subprocess` marker. (4) `wheel_smoke` extension. (5) Skill ↔ CLI parity gate (NEW — scope locked in Phase 3). (6) 5-surface parity for `install-skill` itself (no flags in v0.1, so canonical tokens reduce to the subcommand name). | +| **Docs** | pass | New `docs/skills.md` catalog + `mkdocs.yml` nav entry (one line under "CLI Reference"). README "Quick start" gains one sentence after `pip install signalforge-dbt` pointing at `signalforge install-skill`. The shields.io self-grade badge surfaces at the README top per `clauditor`'s precedent. | +| **AST scan #7 (typed-error registry)** | pass | New `signalforge/skill/errors.py` is the **13th** per-stage `errors.py` (current count: 12). Bump `test_scan_7_discovers_every_per_stage_errors_module` count + add `SkillError` to `_EXCEPTION_MAPPING_EXCLUDED_BASES` (its concretes will span tier 1 + tier 2, mirroring `DemoError`/`IngestError`). | +| **Worker-writability** | pass | All shipped artefacts land under worker-writable paths: SKILL.md + assets under `src/signalforge/skills/`; parity gate under `tests/`; rule file under `.claude/rules/skill-parity.md` (orchestrator-only edit). Per `ralph-worker-claude-dir-perms.md` memory, the orchestrator (not a worker) makes the one `.claude/rules/` edit. | +| **Worktree / branch** | pass | Worktree created at `/home/wesd/Projects/worktrees/SignalForge/141-claude-skill-install` on `feature/141-claude-skill-install` off `dev`. | + +No blockers. Two concerns surface as Phase 3 refinement questions: (1) clauditor self-grade +operating model (CI vs pinned-at-release), (2) Skill ↔ CLI parity gate token scope. + +## Refinement log + +### Decisions + +- **DEC-001 — Skill source path.** Package-data tree at + `src/signalforge/skills/signalforge/SKILL.md` (plural `skills/` parent allows future + sibling skills; matches `.claude/skills//SKILL.md` install destination shape; + matches the anticipatory `skill-parity.md` rule verbatim). NO `__init__.py` under + `skills/` or `skills/signalforge/` — the directory is package-data, NOT a Python + package. Mirrors `src/signalforge/_demo/` exactly. + +- **DEC-002 — Python lib subpackage name.** The runtime code lives at + `src/signalforge/skill/` (singular) — a real Python package with `__init__.py`, + `errors.py`, and the public `install_skill(...)` function. Singular name mirrors + `signalforge.demo`; the package-data tree's plural name is the install destination + convention, not the lib name. Two distinct paths, one each side of the seam. + +- **DEC-003 — Destination policy.** `install_skill(dest, *, ...)` always overwrites + every file SignalForge ships (SKILL.md + SKILL.eval.json + any `assets/*` we + enumerate from the bundled tree); never touches any other file in the dest dir. No + `--force` flag in v0.1. Friendlier for upgrade-in-place; eliminates the + `--force`-against-symlink-dest hazard that `copy_demo` defends against because we + never `rmtree`. + +- **DEC-004 — Default destination.** Positional `` defaults to `"."` (CWD). The + effective install path is `/.claude/skills/signalforge/SKILL.md`. Mirrors + `init-demo`'s default-to-CWD ergonomics. The operator runs from the dbt project root. + +- **DEC-005 — Symlink defence (mirror `copy_demo` verbatim).** `install_skill` resolves + `` via `.resolve(strict=True)` first; falls back to `.resolve(strict=False)` on + `FileNotFoundError` / `NotADirectoryError` (common — dest dir may not exist yet); + catches `RuntimeError` (Python ≤3.12) AND `OSError(errno.ELOOP)` (Python ≥3.13) on + cycle detection. Wraps cycle failures as `SkillDestPathError` (tier 1). Additionally: + if `/.claude/skills/signalforge/SKILL.md` exists AND is a symlink, raise + `SkillDestUnsafeError` (tier 2) — writing would follow the link to an arbitrary + destination. + +- **DEC-006 — Path canonicalisation lives in the lib, not via `canonicalise_user_path`.** + `canonicalise_user_path` enforces a `project_dir` containment boundary. `install-skill` + is the second "creates the project context" entry point (alongside `init-demo`) where + no project_dir applies. The lib seam rolls its own resolution mirroring + `signalforge.demo.copy_demo`; the module docstring documents the precedent verbatim. + +- **DEC-007 — Package-data lookup.** Mirror `copy_demo` verbatim: + `files("signalforge").joinpath("skills").joinpath("signalforge")` wrapped in + `as_file(...)` for zipapp/zipimport safety. Failure to find the bundled tree raises + `SkillPackageDataMissingError` (tier 1) — signals a corrupted install. + +- **DEC-008 — Error hierarchy.** + - Lib (`signalforge.skill.errors`): + - `SkillError(Exception)` — abstract base; `extra="forbid"` is N/A (not a Pydantic + model); `__str__` renders `message` + optional `↳ Remediation:` line per + `manifest-readers.md` § "Errors carry remediation." + - `SkillDestPathError(SkillError)` — tier 1; symlink cycle / containment failure. + - `SkillDestUnsafeError(SkillError)` — tier 2; dest is a file (not dir), SKILL.md is + a symlink, dest permission denied at write time. + - `SkillPackageDataMissingError(SkillError)` — tier 1; bundled SKILL.md absent. + - CLI (`signalforge.cli.errors`): + - `CliInstallSkillPathError(CliError)` — tier 1; wraps `SkillDestPathError`. + - `CliInstallSkillDestUnsafeError(CliError)` — tier 2; wraps `SkillDestUnsafeError`. + - `CliInstallSkillPackageDataMissingError(CliError)` — tier 1; wraps + `SkillPackageDataMissingError`. + - **Concretes span tiers 1 + 2**, so `SkillError` joins `DemoError` / `IngestError` + pattern: register only in `_EXCEPTION_MAPPING_EXCLUDED_BASES`, never in the + `_EXCEPTION_TO_EXIT_CODE` table. + +- **DEC-009 — AST scan #7.** Bump + `test_scan_7_discovers_every_per_stage_errors_module` count 12 → 13 in lockstep + with `signalforge/skill/errors.py` landing. Add `SkillError` to + `_EXCEPTION_MAPPING_EXCLUDED_BASES` (frozenset). Register every concrete CLI wrapper + (`CliInstallSkillPathError` / `CliInstallSkillDestUnsafeError` / + `CliInstallSkillPackageDataMissingError`) AND every lib concrete (`SkillDestPathError` + / `SkillDestUnsafeError` / `SkillPackageDataMissingError`) in + `_EXCEPTION_TO_EXIT_CODE` (defence-in-depth: both layers in the table even though MRO + walk would resolve the lib raise via the CLI wrapper). + +- **DEC-010 — Wheel packaging.** Extend `[tool.hatch.build.targets.wheel].include` to + `["src/signalforge/_demo", "src/signalforge/skills"]`. The directory-level include + is transitive — every nested file (SKILL.md, SKILL.eval.json, assets/*) ships + recursively. Confirmed by the `_demo` precedent (recursively ships nested + `models/staging/*.sql`, `target/*.json`). + +- **DEC-011 — `wheel_smoke` extension.** Add `_EXPECTED_SKILL_FILES` tuple alongside + `_EXPECTED_DEMO_FILES` in `tests/test_wheel_packaging.py`. v0.1 set: + `("signalforge/skills/signalforge/SKILL.md", + "signalforge/skills/signalforge/assets/SKILL.eval.json")`. Run via + `uv run pytest -m wheel_smoke --no-cov`. Also add a NEGATIVE assertion: no + `.claude/skills/*` paths appear in the built wheel (defence against accidentally + including maintainer-only `release-manager` / `review-agentskills-spec` — they live + at repo-root `.claude/skills/`, outside `src/`, so they're already excluded, but + the negative assertion documents intent). + +- **DEC-012 — e2e demo paths (both, with live gated).** Zero-credential default: + `signalforge init-demo /tmp/signalforge-demo` → `signalforge generate + models/staging/stg_bikeshare_trips.sql --write` (schema-only mode by default) → + walk through the kept / kept-uncertain / dropped / flagged diff. Live e2e (opt-in): + the skill body forces an explicit user confirmation ("This will run paid LLM + + warehouse queries — proceed?"), checks `SF_RUN_BQ`, `GOOGLE_CLOUD_PROJECT`, + `ANTHROPIC_API_KEY` (clean skip-with-reason when absent), then invokes + `uv run pytest -m e2e --no-cov`. Cost warning before invocation. + +- **DEC-013 — `allowed-tools` scope.** Comma-separated: + `Bash(signalforge *), Bash(uv run signalforge *), Bash(uv run pytest -m e2e*), + Bash(cat *), Bash(ls *), Bash(grep *), Bash(head *), Bash(tail *), + Read, Write, Edit`. The `pytest -m e2e*` scope is required for the live-gated path + per DEC-012; the skill body's confirmation gate is the user-facing defence. + +- **DEC-014 — Self-grade operating model.** Pre-release manual run. + Maintainer runs `clauditor grade src/signalforge/skills/signalforge/SKILL.md` locally + before tagging a release; captures the score; pins it in + `src/signalforge/skills/signalforge/assets/SKILL.eval.json`. README badge surfaces the + pinned score via shields.io. Same commit updates SKILL.md + eval.json + README + badge. No CI integration, no Anthropic key in repo secrets, no per-PR cost. Adds + `clauditor` to `[dependency-groups].dev` if not already present. + +- **DEC-015 — Parity gate scope.** New test + `tests/cli/test_skill_cli_parity.py` scans for three categories of tokens, all of which + must appear verbatim in `src/signalforge/skills/signalforge/SKILL.md`: + 1. Every subcommand name from the live argparse parser (auto-grows). Source: + `signalforge.cli._build_parser()` → walk `parser._subparsers._group_actions[0].choices`. + Current v0.2 set: `generate`, `lint`, `prune-existing`, `init-demo`, `install-skill`, + `version`. + 2. The four canonical demo command lines: `signalforge init-demo`, + `signalforge generate --write`, `signalforge prune-existing --schema + `, `signalforge install-skill`. Plain substring match — no regex, no whitespace + normalisation (mirrors envelope-breach guard pattern from `business-rule-tests.md`). + 3. The install-skill bootstrap line itself (`signalforge install-skill`). + The gate is mechanical, not semantic — semantic freshness lives in the clauditor self-grade. + +- **DEC-016 — Parity gate is a NEW test, not an extension of 5-surface parity.** The + 5-surface parity tests in `tests/cli/test_5_surface_parity_*.py` pin canonical tokens for + ONE subcommand across five surfaces (help/docstring/ops/plan/test). The skill parity + gate scans the FULL CLI surface against ONE skill body. Different shape, different + failure modes; keeping them as separate tests preserves the locality of each gate's + failure message. + +- **DEC-017 — Overwrite UX.** Single INFO line on success: + `Installed SignalForge skill to `. When an existing SKILL.md was overwritten, + append `(replaced existing SKILL.md)`. No diff, no backup file. The operator can + `git diff` if they had the file under version control. Lazy-format JSON; not via + `_LOGGER` (the CLI writes to stdout for success messages, stderr for errors). + +- **DEC-018 — `cli-layer.md` parity-surface entry.** Add a paragraph under the + "Multi-surface parity for behaviour changes" section noting that the bundled skill is + the Nth parity surface — a change to the CLI subcommand/flag surface updates + `src/signalforge/skills/signalforge/SKILL.md` in the same commit, and the + `tests/cli/test_skill_cli_parity.py` gate enforces it. Adds a "6th surface" entry to + the list (currently: help/docstring/ops/test/DEC). + +- **DEC-019 — `skill-parity.md` rule file.** The orchestrator (NOT a worker) writes + `.claude/rules/skill-parity.md` in this PR per the + `ralph-worker-claude-dir-perms.md` memory — workers cannot Write under `.claude/` in + worktrees. The content is the contract written verbatim in DEC-013…DEC-018 above plus + a pointer back to this plan + cli-layer.md. + +- **DEC-020 — SKILL.md frontmatter.** + ```yaml + --- + name: signalforge + description: Use when the user wants to draft, prune, or grade dbt tests / docs with an LLM, has a dbt project (manifest.json + sql models), or asks about SignalForge. Drives the `signalforge` CLI end-to-end: drafts candidate tests, runs them against warehouse samples, drops the noise, and explains every kept/dropped artifact. + compatibility: "Requires: signalforge installed (pip install signalforge-dbt). For the zero-credential demo: no warehouse needed. For real dbt projects: dbt-core + a populated manifest.json. For live e2e: a configured warehouse profile (BigQuery v0.1) + ANTHROPIC_API_KEY." + metadata: + signalforge-version: "0.X.Y" + allowed-tools: Bash(signalforge *), Bash(uv run signalforge *), Bash(uv run pytest -m e2e*), Bash(cat *), Bash(ls *), Bash(grep *), Bash(head *), Bash(tail *), Read, Write, Edit + --- + ``` + No `disable-model-invocation` — the skill reasons about the per-artifact "why" output + to help the operator interpret the diff. `signalforge-version` is updated by the + release-manager skill in lockstep with the wheel version. + +- **DEC-021 — SKILL.md body sections.** Numbered workflow: + 1. **Point at a dbt project** — verify `manifest.json` exists, name a model. + 2. **Zero-credential demo** — `init-demo` → `generate --write` walkthrough. + 3. **Real project: draft + prune** — `generate --write` with the safety + posture (schema-only default; `--mode sample` is opt-in; document the cost). + 4. **Grade tests you already have** — `prune-existing --schema `. + 5. **Reading the diff** — kept / kept-uncertain / dropped / flagged tiers + the + per-artifact "why" cascade. + 6. **Optional: live e2e demonstration** — gated behind explicit user confirmation, + env-var checks, cost warning. + 7. **Troubleshooting** — common errors (`ModelNotFoundError`, `WarehouseAuthError`, + `LLMCacheTooLargeError`) with one-line fixes; pointer to `docs/cli-ops.md`. + +- **DEC-022 — Maintainer-only skill exclusion.** `release-manager` and + `review-agentskills-spec` live at repo-root `.claude/skills/`, which is outside `src/` + — they're never in the wheel by construction. install-skill enumerates from + `files("signalforge").joinpath("skills")` (the package-data tree only), so there's + no code path that could install them. The wheel_smoke negative assertion (DEC-011) + documents this intent. + +- **DEC-023 — Docs entry.** New `docs/skills.md` page describing the bundled skill + + install command + the two demo paths (zero-cred and live-gated). `mkdocs.yml` `nav:` + gains `- Claude Code Skill: skills.md` under "CLI Reference". README "Quick start" + gets a one-sentence pointer after the `pip install` block. The README self-grade + badge surfaces the clauditor score (DEC-014). + +- **DEC-024 — 5-surface parity for the `install-skill` subcommand itself.** Canonical + tokens (v0.1, no flags): `"install-skill"`. The test mirrors + `test_5_surface_parity_init_demo.py` shape across (1) argparse help, (2) handler + docstring, (3) `docs/cli-ops.md`, (4) this plan, (5) test docstring. The + SKILL ↔ CLI parity gate (DEC-015) is orthogonal — that one scans the *full* CLI + surface against SKILL.md; this one pins one subcommand across five surfaces. + +### Session notes + +- 2026-05-29 — Phase 1 discovery: parallel research locked the four scoping decisions + (dest policy, e2e paths, self-grade inclusion, src path); architecture review pass + surfaced two refinement concerns (self-grade ops, parity gate scope, overwrite UX); + Phase 3 closed all 24 decisions. Plan now at `detailing` phase, ready for story + generation. + +## Detailed breakdown + +The 11 stories below follow the natural architecture order: package-data + wheel +packaging → public lib seam → CLI handler → enforcement gates → docs/grade → rules +ledger → quality gate → memory. + +**Acceptance check repeated for every story:** +`uv sync --dev && uv run ruff check . && uv run ruff format --check . && uv run pyright && uv run pytest` +(the canonical `VALIDATE_CMD`). + +--- + +### US-001 — Bootstrap `src/signalforge/skills/signalforge/` tree + wheel packaging + +Lay down the package-data skeleton (empty-but-shaped SKILL.md + SKILL.eval.json +placeholder under `assets/`), wire wheel packaging, and extend `wheel_smoke` to gate +the file set. Content of SKILL.md stays a placeholder (`# SignalForge skill — draft`) +until US-007 fills it in; this story owns the *shape*. + +**Traces to:** DEC-001, DEC-010, DEC-011, DEC-022. + +**Files:** +- `src/signalforge/skills/signalforge/SKILL.md` — placeholder body; full content lands + in US-007. +- `src/signalforge/skills/signalforge/assets/SKILL.eval.json` — placeholder JSON + (`{"score": null, "version": "0.0.0", "graded_at": null}`); pinned in US-008. +- `pyproject.toml` — extend `[tool.hatch.build.targets.wheel].include` to + `["src/signalforge/_demo", "src/signalforge/skills"]`. +- `tests/test_wheel_packaging.py` — add `_EXPECTED_SKILL_FILES` tuple + assertion; + add negative assertion that no `.claude/skills/*` paths appear in the wheel. + +**Done when:** `uv build && unzip -l dist/*.whl | grep signalforge/skills/` shows +SKILL.md + assets/SKILL.eval.json; `uv run pytest -m wheel_smoke --no-cov` passes +including the negative `.claude/skills/*` assertion; full `VALIDATE_CMD` passes. + +**TDD:** Not pure TDD — the wheel_smoke test IS the test for this story. Write the +expected file tuple + negative assertion FIRST (red), then update pyproject.toml +include + create the placeholder files (green). + +**Depends on:** none. + +--- + +### US-002 — Public `signalforge.skill` lib module + typed errors + +Create the `signalforge.skill` Python package with `install_skill(dest) -> Path` and +the four-class typed-error hierarchy. Mirror `copy_demo`'s symlink/cycle defence +verbatim; mirror its `importlib.resources` lookup; never `rmtree`. AST scan #7 picks +up the new `errors.py` automatically (depth-1 glob). + +**Traces to:** DEC-002, DEC-003, DEC-005, DEC-006, DEC-007, DEC-008, DEC-009. + +**Files:** +- `src/signalforge/skill/__init__.py` — exports `install_skill`, the three lib errors, + and `SkillError` base. `__all__` is the public contract. +- `src/signalforge/skill/errors.py` — `SkillError` base + three concretes. +- `tests/skill/test_install.py` — unit tests (see TDD below). +- `tests/test_audit_completeness.py` — bump `test_scan_7_discovers_every_per_stage_errors_module` + count 12 → 13; add `SkillError` to `_EXCEPTION_MAPPING_EXCLUDED_BASES`. + +**Done when:** `install_skill(tmp_path)` returns the absolute SKILL.md path under +`/.claude/skills/signalforge/`; preserves any sibling files; symlink-cycle +dest raises `SkillDestPathError`; symlinked-SKILL.md dest raises +`SkillDestUnsafeError`; patched-away source raises `SkillPackageDataMissingError`; +AST scan #7 passes; full `VALIDATE_CMD` passes. + +**TDD:** Write these tests FIRST: +1. `test_install_skill_to_fresh_dir_writes_skill_md` — happy path; assert returned + path is absolute and exists. +2. `test_install_skill_overwrites_existing_skill_md_unchanged_otherwise` — pre-create + `.claude/skills/signalforge/SKILL.md` with `"OLD"` + a sibling `notes.txt`; + `install_skill` returns; assert SKILL.md content changed AND notes.txt untouched. +3. `test_install_skill_refuses_when_skill_md_is_symlink` — pre-create the dest + tree with SKILL.md as a symlink; assert `SkillDestUnsafeError`. +4. `test_install_skill_with_cyclic_symlink_dest_raises_dest_path_error` — create + a symlink cycle as dest; assert `SkillDestPathError`. +5. `test_install_skill_missing_package_data_raises` — monkeypatch + `importlib.resources.files` to return a non-dir; assert + `SkillPackageDataMissingError`. +6. `test_install_skill_dest_is_file_raises_unsafe` — pass an existing regular file + as dest; assert `SkillDestUnsafeError`. + +**Depends on:** US-001 (placeholder SKILL.md must exist in the source tree). + +--- + +### US-003 — CLI `install-skill` subcommand + handler + exit-code mapping + subprocess smoke + +Wire the subcommand into the argparse registry; add the three `CliInstallSkill*Error` +wrappers; register every typed error in `_EXCEPTION_TO_EXIT_CODE`; ship the +subprocess `--help` smoke under `cli_subprocess`. + +**Traces to:** DEC-002, DEC-003, DEC-004, DEC-008, DEC-009, DEC-017, DEC-024. + +**Files:** +- `src/signalforge/cli/install_skill.py` — `add_parser(subparsers)` + `cmd_install_skill(args) -> int`. +- `src/signalforge/cli/__init__.py` — register via + `install_skill_cmd.add_parser(subparsers)` in `_build_parser()`. +- `src/signalforge/cli/errors.py` — three `CliInstallSkill*Error` wrapper classes. +- `src/signalforge/cli/_helpers.py` — register six new entries in + `_EXCEPTION_TO_EXIT_CODE` (three lib + three CLI wrappers per DEC-009). +- `tests/cli/test_install_skill.py` — main([…]) tests for each exit-code path; assert + no traceback on stderr. +- `tests/cli/test_subprocess_smoke.py` — add `test_signalforge_install_skill_help_via_subprocess` + under `@pytest.mark.cli_subprocess`. + +**Done when:** +- `signalforge install-skill ` returns 0, writes file, INFO line on stdout per + DEC-017. +- `signalforge install-skill ` returns 2, prints + `ERROR: ` + remediation, no traceback. +- `signalforge install-skill ` returns 1, no traceback. +- `uv run pytest -m cli_subprocess --no-cov` passes the new `--help` smoke. +- Full `VALIDATE_CMD` passes. + +**TDD:** Write these tests FIRST: +1. `test_install_skill_success_returns_zero_writes_file_prints_info` — happy path. +2. `test_install_skill_overwrite_appends_replaced_notice` — pre-create old SKILL.md; + assert stdout contains `(replaced existing SKILL.md)` per DEC-017. +3. `test_install_skill_dest_is_file_returns_two_no_traceback` — tier 2. +4. `test_install_skill_dest_with_symlink_cycle_returns_one_no_traceback` — tier 1. +5. `test_install_skill_missing_package_data_returns_one_no_traceback` — + monkeypatched. +6. `test_install_skill_default_dest_is_cwd` — `chdir(tmp_path)`, run + `main(["install-skill"])`, assert file lands at `tmp_path/.claude/skills/signalforge/SKILL.md`. + +**Depends on:** US-002. + +--- + +### US-004 — SKILL ↔ CLI parity gate + +The mechanical enforcement test that closes the +"forgot-to-update-SKILL.md-when-changing-the-CLI" loop. Lives under `tests/` so +workers can update it. + +**Traces to:** DEC-015, DEC-016, DEC-019. + +**Files:** +- `tests/cli/test_skill_cli_parity.py` — NEW test file. + +**Done when:** +- Test reads `src/signalforge/skills/signalforge/SKILL.md` once. +- Walks `signalforge.cli._build_parser()._subparsers._group_actions[0].choices` to + enumerate every registered subcommand; asserts each name appears as a substring of + the SKILL.md body. +- Asserts the four canonical demo command lines (per DEC-015) appear verbatim. +- Asserts the install-skill bootstrap line (`signalforge install-skill`) appears. +- Failure prints which subcommand / demo command / bootstrap line was missing. +- Planted-violation self-check: a separate test inside the same file edits a copy of + SKILL.md in `tmp_path` to remove `"generate"`, asserts the gate raises + `AssertionError` — proves the gate can fail. + +**TDD:** Write the planted-violation self-check FIRST (it's a red test for a gate +that doesn't exist yet → write the gate to make it green). + +**Depends on:** US-001 (SKILL.md placeholder), US-003 (install-skill subcommand +registered). The SKILL.md placeholder from US-001 needs to be expanded enough to +contain the canonical tokens this test scans for — coordinated with US-007 which +writes the prose; US-004 may temporarily fail until US-007 lands. Sequence US-004 to +either land AFTER US-007 or to be merged together; document dependency. + +--- + +### US-005 — 5-surface parity test for `install-skill` + +Mirror `test_5_surface_parity_init_demo.py` for the new subcommand. v0.1 canonical +tokens: `"install-skill"` (no flags yet, so the surface is minimal). + +**Traces to:** DEC-024. + +**Files:** +- `tests/cli/test_5_surface_parity_install_skill.py` — NEW test mirroring the + `init_demo` precedent. + +**Done when:** test asserts `"install-skill"` appears in all five surfaces: (1) +argparse help (rendered from `add_parser`), (2) `cmd_install_skill` docstring, (3) +`docs/cli-ops.md` § Subcommands, (4) this plan +(`plans/super/141-claude-skill-install.md`), (5) the test docstring itself. Failure +names which surface lacks the token. + +**Depends on:** US-003 (subcommand exists), US-007 (`docs/cli-ops.md` updated). Mirror +US-004's coordination — may need to land alongside US-007. + +--- + +### US-006 — Docs: `docs/skills.md`, `mkdocs.yml` nav, `docs/cli-ops.md`, README pointer + +Single docs story covering all four surfaces. Authoritative content for the skill +catalog page; updates README quick-start with the one-line pointer; extends +`docs/cli-ops.md` with the `install-skill` subcommand entry (Flag reference / Exit +codes / Stderr shapes). + +**Traces to:** DEC-021, DEC-023. + +**Files:** +- `docs/skills.md` — NEW. Describes the bundled skill, what it teaches, the install + command, and both demo paths (zero-cred + live-gated). Pointer to clauditor + self-grade. +- `mkdocs.yml` — add `- Claude Code Skill: skills.md` under "CLI Reference". +- `docs/cli-ops.md` — add `install-skill` entry to Subcommands section; map to exit + codes; show stderr shapes for each tier-2/1 error. +- `README.md` — one-sentence pointer after `pip install signalforge-dbt`: + `Run \`signalforge install-skill\` to drop the Claude Code skill into your project.` + +**Done when:** `uv run --only-group docs mkdocs build` is clean; new nav entry +renders; README quick-start shows the pointer; `docs/cli-ops.md` § install-skill +matches the actual handler help text. + +**Depends on:** US-003 (subcommand exists so help text + cli-ops entry can be +generated against the real handler). + +--- + +### US-007 — Author the SKILL.md prose (the actual user-facing workflow) + +Fill in the placeholder from US-001 with the real workflow per DEC-020 (frontmatter) ++ DEC-021 (body sections). This is the prose-heavy story; expect iteration with the +clauditor self-grade in US-008. + +**Traces to:** DEC-012, DEC-013, DEC-020, DEC-021. + +**Files:** +- `src/signalforge/skills/signalforge/SKILL.md` — replace placeholder with full body. + +**Done when:** +- Frontmatter matches DEC-020 verbatim. +- All seven body sections from DEC-021 present. +- Both demo paths (zero-cred + live-gated) include the exact CLI invocations. +- Live-gated section enforces the user confirmation + env-var check + cost warning. +- SKILL ↔ CLI parity gate (US-004) passes against the new content. +- 5-surface parity (US-005) passes. + +**Depends on:** US-001 (placeholder exists), US-003 (install-skill subcommand +registered so SKILL.md can reference it accurately). + +--- + +### US-008 — Clauditor self-grade + README badge + +Add `clauditor` to dev-deps if absent; run grading; pin the score in +`assets/SKILL.eval.json`; surface the shields.io badge on the README. + +**Traces to:** DEC-014. + +**Files:** +- `pyproject.toml` — add `clauditor` to `[dependency-groups].dev` if not present. +- `src/signalforge/skills/signalforge/assets/SKILL.eval.json` — replace placeholder + with real graded JSON. +- `README.md` — add shields.io badge near the top (alongside any existing + badges). +- `docs/skills.md` — add a "Self-grade" subsection pointing at the pinned score and + the regeneration command. + +**Done when:** `clauditor grade src/signalforge/skills/signalforge/SKILL.md` runs +clean against the SKILL.md from US-007; the JSON has a numeric `score`, a non-null +`graded_at` ISO-8601 UTC timestamp, and a `signalforge-version` matching +`signalforge.__version__`; README badge URL points at the pinned score; full +`VALIDATE_CMD` passes. + +**Depends on:** US-007 (SKILL.md prose stable). Run AFTER US-007 lands so the score +reflects the real content. + +--- + +### US-009 — Skill-parity rule file + cli-layer.md update (orchestrator) + +The rule files under `.claude/rules/` are orthogonal to worker-writable code per +`ralph-worker-claude-dir-perms.md` memory — the orchestrator (this conversation OR +the maintainer in a closing PR commit) writes them, not a Ralph worker. Worker +implementations of US-001…US-008 reference these rules; this story lands them +durably. + +**Traces to:** DEC-018, DEC-019. + +**Files:** +- `.claude/rules/skill-parity.md` — NEW; written by the orchestrator. +- `.claude/rules/cli-layer.md` — add a paragraph under "Multi-surface parity for + behaviour changes" naming the bundled skill as a parity surface; cross-link to + skill-parity.md. + +**Done when:** both files present, lint-clean, cross-referenced; ralph workers can +read them. No test gates this directly (rule files are read by humans + the model); +absence is caught at code-review time. + +**Depends on:** US-003, US-004 (the contracts these rules document must exist). + +--- + +### US-010 — Quality Gate + +Run `code-review` x4 across the full diff; address each pass's findings; run +CodeRabbit if available; ensure `VALIDATE_CMD` is green; gated marker runs +(`wheel_smoke`, `cli_subprocess`) clean. + +**Traces to:** ALL prior decisions. + +**Done when:** four code-review passes complete with all real findings resolved; +CodeRabbit review posted + addressed; `VALIDATE_CMD` green; `uv run pytest -m +wheel_smoke --no-cov` green; `uv run pytest -m cli_subprocess --no-cov` green. + +**Depends on:** US-001 … US-009. + +--- + +### US-011 — Patterns & Memory + +Capture durable lessons from this work. Likely additions: +- "Skill-shaped lib seam mirrors init-demo verbatim" — pattern for any future "ship a + user-facing artifact into the user's project" subcommand. +- "Two-name convention: `skills/` (plural) for the package-data tree matching the + install destination; `skill/` (singular) for the Python lib module matching + `signalforge.demo`." +- "Parity gate over prompt — the model can't be relied on to update SKILL.md from + context; the pytest gate is the durable enforcement." +- Memory file under `~/.claude/projects/-home-wesd-Projects-SignalForge/memory/` + + MEMORY.md pointer per the harness memory protocol. + +**Traces to:** Lessons learned from US-001 … US-010. + +**Done when:** new memory files written; MEMORY.md updated with one-line pointers; +`.claude/rules/` changes (if any) reviewed. + +**Depends on:** US-010. + +## Risks & non-goals + +**Non-goals:** +- No CI integration for clauditor grading (manual pre-release per DEC-014). +- No multi-skill install (v0.1 ships exactly one skill; the `skills/` plural parent + anticipates v0.2+). +- No `--force` flag (per DEC-003). +- No `.bak` file on overwrite (per DEC-017). +- No diff-on-overwrite output (per DEC-017). + +**Risks:** +- **R-1: SKILL.md prose churn drives badge churn.** Every SKILL.md edit triggers a + new clauditor grade + eval.json + README badge update (3-file commit). Mitigation: + group SKILL.md edits into PRs where possible; document the regen command in + `docs/skills.md`. +- **R-2: SKILL ↔ CLI parity gate false negatives.** A subcommand could be added with + a name that's also a common English word (e.g. if someone adds a `signalforge run`) + — the substring scan would pass even if the SKILL.md doesn't actually teach the + command. Acceptable for v0.1; the clauditor self-grade catches semantic gaps. +- **R-3: Anticipatory rule file (skill-parity.md) drift.** The rule file references + contracts that other rules also reference. If we update one and forget the other, + the rules drift. Mitigation: keep skill-parity.md short and link out to + cli-layer.md / python-build.md rather than restating their contracts. diff --git a/pyproject.toml b/pyproject.toml index 1d2f6e3a..eff368bc 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -72,6 +72,14 @@ dev = [ "tiktoken>=0.7,<1.0", "google-genai>=0.5,<1", "sqlglot>=30,<31", + # clauditor-eval powers the pre-release SKILL.md self-grade run per + # DEC-014 of plans/super/141-claude-skill-install.md. PyPI dist name is + # `clauditor-eval`; it provides the `clauditor` CLI entry point. The + # maintainer runs `uv run clauditor grade + # src/signalforge/skills/signalforge/SKILL.md` and pins the score in + # `src/signalforge/skills/signalforge/assets/SKILL.eval.json`. No CI + # integration — manual pre-release only. + "clauditor-eval>=0.1,<1", {include-group = "docs"}, ] @@ -87,13 +95,21 @@ path = "src/signalforge/__init__.py" [tool.hatch.build.targets.wheel] packages = ["src/signalforge"] -# `include` is defence-in-depth for the demo tree under `src/signalforge/_demo/` +# `include` is defence-in-depth for non-`.py` data trees under `src/signalforge/` # — Hatchling's default `packages` glob picks `.py` reliably but its behaviour # on non-`.py` data files (and dotfiles like `.gitignore` per DEC-006 of # `plans/super/47-init-demo.md`) is not contractually guaranteed across releases. -# The wheel_smoke marker (`tests/test_wheel_packaging.py`) gates the demo file -# set in the built artifact; this directive is the production-side guarantee. -include = ["src/signalforge/_demo"] +# The wheel_smoke marker (`tests/test_wheel_packaging.py`) gates the on-disk +# file set in the built artifact for both trees; this directive is the +# production-side guarantee. +# - `_demo/` — bundled demo project (DEC-002 of plans/super/47-init-demo.md). +# - `skills/` — bundled SignalForge Claude Code skill that the `install-skill` +# CLI copies into `~/.claude/skills/` (DEC-010 of +# plans/super/141-claude-skill-install.md). Maintainer-only +# skills under repo-root `.claude/skills/` (release-manager, +# review-agentskills-spec) are deliberately NOT listed here — +# see DEC-022 + the negative-assertion test in test_wheel_packaging.py. +include = ["src/signalforge/_demo", "src/signalforge/skills"] [tool.ruff] line-length = 100 diff --git a/src/signalforge/cli/__init__.py b/src/signalforge/cli/__init__.py index 10db8070..748fc8a5 100644 --- a/src/signalforge/cli/__init__.py +++ b/src/signalforge/cli/__init__.py @@ -20,6 +20,7 @@ import signalforge from signalforge.cli import generate as generate_cmd from signalforge.cli import init_demo as init_demo_cmd +from signalforge.cli import install_skill as install_skill_cmd from signalforge.cli import lint as lint_cmd from signalforge.cli import prune_existing as prune_existing_cmd from signalforge.cli import version as version_cmd @@ -82,6 +83,7 @@ def _build_parser() -> argparse.ArgumentParser: lint_cmd.add_parser(subparsers) generate_cmd.add_parser(subparsers) init_demo_cmd.add_parser(subparsers) + install_skill_cmd.add_parser(subparsers) prune_existing_cmd.add_parser(subparsers) return parser diff --git a/src/signalforge/cli/_helpers.py b/src/signalforge/cli/_helpers.py index e28cacc7..4da25fc2 100644 --- a/src/signalforge/cli/_helpers.py +++ b/src/signalforge/cli/_helpers.py @@ -47,6 +47,9 @@ CliInitDemoDestUnsafeError, CliInitDemoFixtureMissingError, CliInputError, + CliInstallSkillDestUnsafeError, + CliInstallSkillPackageDataMissingError, + CliInstallSkillPathError, CliPathError, CliSelectorNoMatchError, CliSelectorParseError, @@ -160,6 +163,11 @@ SafetyError, UnknownConfigKeyError, ) +from signalforge.skill import ( + SkillDestPathError, + SkillDestUnsafeError, + SkillPackageDataMissingError, +) from signalforge.warehouse import ( BytesBilledExceededError, ColumnNotFoundError, @@ -283,6 +291,24 @@ # walk in :func:`map_exception_to_exit_code`. DemoPathError: 1, DemoFixtureMissingError: 1, + # signalforge.skill typed errors (issue #141 / DEC-008). The CLI + # wrappers will land in US-003 and re-raise these into + # ``CliInstallSkill*Error`` at the handler boundary; the lib + # concretes still appear here as defence-in-depth so the 7th AST + # scan finds them and so an escaping raise gets a sensible exit code + # via the MRO walk in :func:`map_exception_to_exit_code`. Like + # ``DemoError`` and ``IngestError``, the concretes span tiers 1 and + # 2, so ``SkillError`` itself has no single-tier fallback entry — + # it lives only in ``_EXCEPTION_MAPPING_EXCLUDED_BASES``. + SkillDestPathError: 1, + SkillPackageDataMissingError: 1, + # CLI wrappers for the skill-install handler boundary (issue #141 / + # US-003 / DEC-008). Tier 1 for the two load-time failures: a + # symlink-cycle resolve failure on ```` and a broken-install + # case where the bundled skill tree is missing. Mirrors the tiering + # of the underlying ``Skill*Error`` lib concretes above. + CliInstallSkillPathError: 1, + CliInstallSkillPackageDataMissingError: 1, # Ingest layer (issue #104 / DEC-001 / US-001). The reader parses an # external dbt schema.yml into a CandidateSchema. These three are # load-tier: the schema file is missing, unparseable, or exceeds the @@ -376,6 +402,17 @@ # above for the defence-in-depth rationale. DemoDestExistsError: 2, DemoDestUnsafeError: 2, + # signalforge.skill input-validation concrete (issue #141 / DEC-008). + # Fires when ``dest`` is a regular file or when the existing + # ``SKILL.md`` is a symlink — both operator-supplied input states + # that conflict with the install contract; mirrors + # ``DemoDestUnsafeError``'s tier. + SkillDestUnsafeError: 2, + # CLI wrapper for the skill-install dest-unsafe boundary (issue #141 + # / US-003 / DEC-008). Tier 2 (input-validation — the operator + # supplied a destination state we refuse to write under); mirrors + # ``CliInitDemoDestUnsafeError``'s tier. + CliInstallSkillDestUnsafeError: 2, # Ingest layer (issue #104 / DEC-002 of US-001). Both fire on # operator-supplied input that conflicts with the manifest/schema: # the named model is absent from the schema.yml (mirrors diff --git a/src/signalforge/cli/errors.py b/src/signalforge/cli/errors.py index 8395ad1c..c40b7352 100644 --- a/src/signalforge/cli/errors.py +++ b/src/signalforge/cli/errors.py @@ -348,3 +348,142 @@ def __init__( ) self.dest = dest self.cause = cause + + +# --------------------------------------------------------------------------- +# install-skill wrappers (issue #141 — US-003, DEC-008 / DEC-009) +# --------------------------------------------------------------------------- +# +# The CLI subcommand ``signalforge install-skill`` calls into the public +# :func:`signalforge.skill.install_skill` helper. The helper raises three typed +# :class:`signalforge.skill.SkillError` subclasses; the CLI handler wraps each +# at the boundary into one of the three ``CliInstallSkill*Error`` classes below +# so the four-tier exit-code taxonomy stays homogeneous (DEC-008). DEC-008 also +# locks the tier assignment: path-resolution (symlink cycle) and broken-install +# (bundled tree missing) land at tier 1 (load); dest-unsafe (regular file or +# symlinked SKILL.md) lands at tier 2 (input-validation — the operator chose a +# destination state we refuse to write under). +# +# Each class carries a ``default_remediation`` so the layer-base ``__str__`` +# renders the canonical ``ERROR: \n ↳ Remediation: `` shape +# without subclasses having to redefine rendering. + + +_CLI_INSTALL_SKILL_PATH_DEFAULT_REMEDIATION: str = ( + "Remove the symlink cycle at the destination or pick a different path." +) + +_CLI_INSTALL_SKILL_DEST_UNSAFE_DEFAULT_REMEDIATION: str = ( + "Pick an existing directory as the destination, or remove the symlinked SKILL.md first." +) + +_CLI_INSTALL_SKILL_PACKAGE_DATA_MISSING_DEFAULT_REMEDIATION: str = ( + "Reinstall signalforge-dbt — the bundled Claude Code skill tree is missing from your install." +) + + +class CliInstallSkillPathError(CliError): + """Raised by ``cmd_install_skill`` when the destination path cannot + be canonicalised (symlink cycle). + + Wraps :class:`signalforge.skill.SkillDestPathError`. Tier 1 (load — + the filesystem state cannot be resolved into a coherent shape + before work begins). Mirrors the precedent set by + :class:`CliPathError` (every CLI-originated path-resolution failure + is tier 1). + """ + + def __init__( + self, + *, + dest: str, + cause: Exception | None = None, + remediation: str | None = None, + ) -> None: + if cause is None: + message = f"failed to resolve install destination {dest!r}" + else: + message = f"failed to resolve install destination {dest!r}: {cause}" + super().__init__( + message, + remediation=( + remediation + if remediation is not None + else _CLI_INSTALL_SKILL_PATH_DEFAULT_REMEDIATION + ), + ) + self.dest = dest + self.cause = cause + + +class CliInstallSkillDestUnsafeError(CliInputError): + """Raised by ``cmd_install_skill`` when ```` is in a shape the + install seam refuses to write under. + + Wraps :class:`signalforge.skill.SkillDestUnsafeError`. Two surfaces + fire this: ```` exists as a regular file (not a directory), + OR the existing ``SKILL.md`` is a symlink (writing would follow the + link and clobber an arbitrary destination). Tier 2 (input + validation — the operator chose a destination state we cannot + safely write into). + """ + + def __init__( + self, + *, + dest: str, + cause: Exception | None = None, + remediation: str | None = None, + ) -> None: + if cause is None: + message = f"refusing to install skill to unsafe destination {dest!r}" + else: + message = f"refusing to install skill to unsafe destination {dest!r}: {cause}" + super().__init__( + message, + remediation=( + remediation + if remediation is not None + else _CLI_INSTALL_SKILL_DEST_UNSAFE_DEFAULT_REMEDIATION + ), + ) + self.dest = dest + self.cause = cause + + +class CliInstallSkillPackageDataMissingError(CliError): + """Raised by ``cmd_install_skill`` when the bundled + ``signalforge/skills/signalforge/`` tree cannot be located via + :mod:`importlib.resources`. + + Wraps :class:`signalforge.skill.SkillPackageDataMissingError`. Tier + 1 (load — the wheel install is broken and there is no work that + can proceed). The wheel-packaging convention in + ``.claude/rules/python-build.md`` makes this practically + unreachable on a clean ``pip install signalforge-dbt`` run, but a + corrupted install (partial wheel extract, hand-edited + site-packages) would surface here. + """ + + def __init__( + self, + *, + cause: Exception | None = None, + remediation: str | None = None, + ) -> None: + if cause is None: + message = "bundled SignalForge skill tree is missing from the signalforge-dbt install" + else: + message = ( + "bundled SignalForge skill tree is missing from the " + f"signalforge-dbt install: {cause}" + ) + super().__init__( + message, + remediation=( + remediation + if remediation is not None + else _CLI_INSTALL_SKILL_PACKAGE_DATA_MISSING_DEFAULT_REMEDIATION + ), + ) + self.cause = cause diff --git a/src/signalforge/cli/install_skill.py b/src/signalforge/cli/install_skill.py new file mode 100644 index 00000000..29348f4e --- /dev/null +++ b/src/signalforge/cli/install_skill.py @@ -0,0 +1,220 @@ +"""``signalforge install-skill`` subcommand (US-003 — issue #141). + +Drops the bundled SignalForge Claude Code skill (the +``src/signalforge/skills/signalforge/`` tree) into +``/.claude/skills/signalforge/`` so a user can pair their Claude +Code session with SignalForge in one command. Wraps the +:func:`signalforge.skill.install_skill` library entry point (US-002) and +re-raises the three :class:`signalforge.skill.SkillError` subclasses at +the handler boundary as ``CliInstallSkill*Error`` wrappers so the CLI's +four-tier exit-code taxonomy stays homogeneous (DEC-008). + +Path-handling note +================== + +``install-skill`` is the second CLI subcommand that *creates* the +project context rather than operating *inside* one (the first is +``init-demo``), so it deliberately does **not** route ``dest`` through +:func:`signalforge.cli._helpers.canonicalise_user_path` — that helper +enforces a ``project_dir`` containment boundary appropriate for paths +the CLI consumes inside an existing project (DEC-006 of +``plans/super/141-claude-skill-install.md``). Symlink-cycle defence +still applies: :func:`signalforge.skill.install_skill` resolves ``dest`` +via ``.resolve(strict=True)`` first (falling back to ``strict=False`` +on ``FileNotFoundError`` / ``NotADirectoryError``) and raises +:class:`signalforge.skill.SkillDestPathError` on a cycle on every +supported Python version (gh-108958). + +Default-dest is CWD +=================== + +The positional ```` defaults to ``"."`` (current working +directory) per DEC-004. An operator running from the dbt project root +gets ``/.claude/skills/signalforge/SKILL.md`` with no flag tuning +needed. Mirrors :mod:`signalforge.cli.init_demo`'s +default-to-CWD-friendly ergonomics. + +Overwrite UX (DEC-017) +====================== + +On success the handler prints a single INFO line to stdout: + + ``Installed SignalForge skill to `` + +If a SKILL.md already existed at the install path (detected BEFORE the +copy via :func:`Path.exists`), the line appends +``(replaced existing SKILL.md)``. The lib seam's overwrite policy is +upgrade-in-place friendly (DEC-003 — overwrites every file SignalForge +ships; preserves every other file in the destination tree); the CLI +surfaces just this one delta so operators know their hand-edited +SKILL.md was replaced. No ``--force`` flag, no ``.bak`` file, no diff +output — the operator can ``git diff`` if they had the file under +version control. +""" + +from __future__ import annotations + +import argparse +from pathlib import Path + +from signalforge.cli._helpers import ( + format_error_to_stderr, + map_exception_to_exit_code, + print_stderr, +) +from signalforge.cli.errors import ( + CliInstallSkillDestUnsafeError, + CliInstallSkillPackageDataMissingError, + CliInstallSkillPathError, +) +from signalforge.skill import ( + SkillDestPathError, + SkillDestUnsafeError, + SkillPackageDataMissingError, + install_skill, +) + +__all__ = ["add_parser", "cmd_install_skill"] + + +# Path components for the SKILL.md install location relative to +# ````. Mirrors ``signalforge.skill``'s private constants — kept +# here for the pre-write existence probe that drives the DEC-017 +# ``(replaced existing SKILL.md)`` suffix decision. +_INSTALLED_SKILL_REL: Path = Path(".claude") / "skills" / "signalforge" / "SKILL.md" + + +def add_parser(subparsers: argparse._SubParsersAction) -> None: # type: ignore[type-arg] + """Register the ``install-skill`` subcommand on the top-level parser. + + Mirrors the registration shape of :mod:`signalforge.cli.init_demo` + (DEC-009 of ``.claude/rules/cli-layer.md`` — one flat module per + subcommand). One surface: + + * Positional ``dest`` — optional (``nargs="?"``) with a string + default of ``"."`` (current working directory) per DEC-004. String + (not :class:`pathlib.Path`) so argparse's default stringification + is predictable across Python versions and platforms; + :func:`signalforge.skill.install_skill` itself runs ``Path(dest)`` + so callers can pass either form. + + Per DEC-003 there is no ``--force`` flag in v0.1 — the lib seam + always overwrites the bundled-skill files in place and never + touches any other file in the destination tree, so the + ``--force``-against-symlink-dest hazard ``copy_demo`` defends + against does not apply here. + """ + parser = subparsers.add_parser( + "install-skill", + help=( + "Install the bundled SignalForge Claude Code skill into " + "/.claude/skills/signalforge/." + ), + description=( + "Drop the bundled SignalForge Claude Code skill (SKILL.md " + "+ assets) into /.claude/skills/signalforge/ so a " + "Claude Code session in picks up the skill. Default " + " is the current working directory. Overwrites the " + "files SignalForge ships; preserves every other file in " + "the destination tree (no --force flag, no backup file)." + ), + ) + parser.add_argument( + "dest", + nargs="?", + default=".", + metavar="DEST", + help=( + "Destination directory. Default: current working " + "directory. The skill lands at " + "/.claude/skills/signalforge/SKILL.md." + ), + ) + parser.set_defaults(func=cmd_install_skill) + + +def cmd_install_skill(args: argparse.Namespace) -> int: + """Install the bundled SignalForge skill under ``args.dest`` and + print the DEC-017 INFO line. + + Returns the integer exit code per the four-tier CLI taxonomy + (DEC-008 of ``.claude/rules/cli-layer.md``): + + * ``0`` — install succeeded; INFO line printed to stdout. + * ``1`` — broken install + (:class:`CliInstallSkillPackageDataMissingError`), symlink cycle + (:class:`CliInstallSkillPathError`), or an unexpected + forward-compat exception caught at the + ``except Exception`` belt-and-braces boundary. + * ``2`` — operator-side dest mistakes + (:class:`CliInstallSkillDestUnsafeError`): ```` is a + regular file, or the existing ``SKILL.md`` is a symlink (writing + would follow the link). + + The single ``try / except Exception`` boundary matches DEC-016 (no + traceback ever leaks); failures route through + :func:`format_error_to_stderr` so the canonical + ``ERROR: `` + ``↳ Remediation: `` shape applies + uniformly with the rest of the CLI. + + DEC-017 — the success path prints a single INFO line to stdout + naming the absolute install path. When a SKILL.md already existed + at the install target (detected BEFORE the copy), the line appends + ``(replaced existing SKILL.md)`` so the operator knows the lib's + upgrade-in-place overwrite policy fired. + """ + raw_dest = args.dest + # Pre-probe for an existing SKILL.md so the DEC-017 suffix is + # accurate. ``Path(...).expanduser()`` is enough — we do not need + # full canonicalisation here; the lib seam does that. ``exists()`` + # ``.exists()`` returns True for regular files AND working symlinks + # (it follows the link); ``.is_symlink()`` returns True for symlinks + # regardless of whether the target is broken. We OR both so the + # probe reports "replaced" for every shape an operator would call + # an existing SKILL.md — including a broken symlink, which the lib + # seam refuses with ``SkillDestUnsafeError`` (the suffix is moot for + # that path but the semantics stay honest). If the parent dir is + # unreadable the probe silently returns False and the suffix is + # omitted — the lib seam's own failure surfaces in the except + # ladder below. + try: + target_skill_md = Path(raw_dest).expanduser() / _INSTALLED_SKILL_REL + existed_before = target_skill_md.exists() or target_skill_md.is_symlink() + except OSError: + existed_before = False + + try: + installed_path = install_skill(raw_dest) + except SkillDestPathError as exc: + wrapped: Exception = CliInstallSkillPathError(dest=str(raw_dest), cause=exc) + print_stderr(format_error_to_stderr(wrapped)) + return map_exception_to_exit_code(wrapped) + except SkillDestUnsafeError as exc: + wrapped = CliInstallSkillDestUnsafeError(dest=str(raw_dest), cause=exc) + print_stderr(format_error_to_stderr(wrapped)) + return map_exception_to_exit_code(wrapped) + except SkillPackageDataMissingError as exc: + wrapped = CliInstallSkillPackageDataMissingError(cause=exc) + print_stderr(format_error_to_stderr(wrapped)) + return map_exception_to_exit_code(wrapped) + except (KeyboardInterrupt, SystemExit): + # Preserve Python's default semantics for operator Ctrl-C and + # any clean SystemExit raised from within ``install_skill`` + # (none today, but defensive parity with the rest of the CLI). + raise + except Exception as exc: # noqa: BLE001 — uniform CLI boundary catch (DEC-016) + # Belt-and-braces — any forward-compat exception added to the + # install helper's raise surface routes through the canonical + # formatter + mapper rather than leaking a traceback. + print_stderr(format_error_to_stderr(exc)) + return map_exception_to_exit_code(exc) + + # DEC-017 — single INFO line, names the absolute install path. + # ``installed_path`` is already an absolute :class:`Path` from the + # lib seam (``target_skill_md.resolve()``); ``str(...)`` is what + # operators copy-paste. + line = f"Installed SignalForge skill to {installed_path}" + if existed_before: + line += " (replaced existing SKILL.md)" + print(line) + return 0 diff --git a/src/signalforge/skill/__init__.py b/src/signalforge/skill/__init__.py new file mode 100644 index 00000000..09ca5c78 --- /dev/null +++ b/src/signalforge/skill/__init__.py @@ -0,0 +1,261 @@ +"""Public ``signalforge.skill`` subpackage — programmatic install of the +bundled SignalForge Claude Code skill. + +Library callers (notebooks, scripts, CI bootstrap) can drop the bundled +``signalforge/skills/signalforge/`` tree into a target project's +``.claude/skills/signalforge/`` via :func:`install_skill`. The CLI +subcommand ``signalforge install-skill`` (issue #141, US-003) will wrap +this function and re-raise the lower-level :class:`SkillError` +subclasses into ``CliInstallSkill*Error`` wrappers so the CLI exit-code +taxonomy stays homogeneous (DEC-008). + +Two-name convention +=================== + +The runtime code lives at ``signalforge.skill`` (singular) — this +module — mirroring the existing ``signalforge.demo``. The package-data +tree lives at ``src/signalforge/skills/signalforge/`` (plural +``skills/``), matching the install destination shape +``.claude/skills//`` and anticipating future sibling skills +(DEC-001, DEC-002). + +Path-handling note +================== + +:func:`install_skill` does **not** route ``dest`` through the +project-wide ``canonicalise_user_path`` helper — that helper enforces a +``project_dir`` containment boundary appropriate for paths the CLI +consumes *inside* an existing project. ``install-skill`` is the second +entry point in the toolchain (alongside ``init-demo``) that operates +*before* a project context exists, so the containment gate doesn't +apply (DEC-006). + +The function still defends against symlink cycles +(``.resolve(strict=True)`` raises ``RuntimeError`` on cycles on Python +<= 3.12 and ``OSError(ELOOP)`` on >= 3.13 — gh-108958), refuses to +overwrite a symlinked SKILL.md (writing would follow the link), and +refuses a regular-file ``dest`` (DEC-005, DEC-008). + +Overwrite policy (DEC-003) +========================== + +:func:`install_skill` always overwrites every file SignalForge ships +(SKILL.md + bundled assets) and never touches any other file in the +destination tree. There is no ``--force`` flag in v0.1 — the policy +is upgrade-in-place friendly ("re-run install-skill, get the new +SKILL.md") and the no-``rmtree`` discipline eliminates the +``--force``-against-symlink-dest hazard ``copy_demo`` has to defend +against. + +See ``plans/super/141-claude-skill-install.md`` § US-002 + DEC-002, +DEC-003, DEC-005, DEC-006, DEC-007, DEC-008, DEC-009 for the full +contract. +""" + +from __future__ import annotations + +import errno +import shutil +from importlib.resources import as_file, files +from pathlib import Path + +from signalforge.skill.errors import ( + SkillDestPathError, + SkillDestUnsafeError, + SkillError, + SkillPackageDataMissingError, +) + +__all__ = [ + "SkillDestPathError", + "SkillDestUnsafeError", + "SkillError", + "SkillPackageDataMissingError", + "install_skill", +] + + +# Path components for the destination tree under ````. Mirrors +# the install destination shape ``.claude/skills//`` that +# the Claude Code skill loader scans. +_CLAUDE_DIR = ".claude" +_SKILLS_DIR = "skills" +_SKILL_NAME = "signalforge" +_SKILL_MD = "SKILL.md" + + +def install_skill(dest: Path | str) -> Path: + """Install the bundled SignalForge Claude Code skill under ``dest``. + + Drops the bundled ``signalforge/skills/signalforge/`` tree into + ``/.claude/skills/signalforge/``. Overwrites every file + SignalForge ships (SKILL.md + bundled assets); preserves every other + file already in the destination tree (DEC-003 — friendly to + upgrade-in-place workflows). + + Parameters + ---------- + dest: + Destination directory. Resolved via + ``Path(dest).expanduser().resolve(strict=True)``, falling back to + ``resolve(strict=False)`` only when the destination does not exist + yet (``FileNotFoundError`` / ``NotADirectoryError``) — a relative + path resolves against the current working directory; ``~`` + expands; symlinks are followed; cycles raise + :class:`SkillDestPathError`. Mirrors :func:`signalforge.demo.copy_demo` + verbatim (DEC-005, DEC-006). + + Returns + ------- + Path + The absolute path to the installed + ``/.claude/skills/signalforge/SKILL.md`` file. Library + callers and the CLI's next-steps message both consume this for + downstream messaging. + + Raises + ------ + SkillDestPathError + Symlink cycle at ``dest``. + SkillDestUnsafeError + ``dest`` exists as a regular file, OR the existing + ``/.claude/skills/signalforge/SKILL.md`` is a symlink (we + would otherwise follow the link and write into the link target). + SkillPackageDataMissingError + The bundled ``signalforge/skills/signalforge/`` tree is missing + from the installed package (broken install). + """ + + raw = Path(dest) + expanded_dest = raw.expanduser() + # Resolve strict=True first so a symlink cycle surfaces on every + # supported Python: <= 3.12 raises RuntimeError, >= 3.13 raises + # OSError(ELOOP) (gh-108958). A genuinely missing destination (the + # common case — the dest dir need not exist yet) raises + # FileNotFoundError / NotADirectoryError, where we fall back to + # strict=False. (Under 3.13, strict=False stops at the loop silently + # and the cycle guard would never fire.) Mirrors + # ``signalforge.demo.copy_demo`` verbatim per DEC-005. + try: + resolved_dest = expanded_dest.resolve(strict=True) + except RuntimeError as exc: # pragma: no cover - <=3.12 cycle signal + raise SkillDestPathError( + f"failed to resolve destination path {str(raw)!r}: {exc}", + cause=exc, + ) from exc + except (FileNotFoundError, NotADirectoryError): + # Destination does not exist yet — fall back to best-effort + # resolution. Narrow to these two so a PermissionError / other + # OSError surfaces instead of being silently downgraded. + resolved_dest = expanded_dest.resolve(strict=False) + except OSError as exc: + if exc.errno == errno.ELOOP: # Python >= 3.13 symlink cycle (gh-108958) + raise SkillDestPathError( + f"failed to resolve destination path {str(raw)!r}: {exc}", + cause=exc, + ) from exc + raise + + # Shape gate — ``dest`` must be a directory (or not exist yet, in + # which case we create the chain). A regular file (or symlink to a + # file, etc.) cannot serve as the project root we install under. + # Without this, the ``mkdir(parents=True)`` below would raise + # ``NotADirectoryError`` which surfaces through the CLI as a less + # informative wrap. + if resolved_dest.exists() and not resolved_dest.is_dir(): + raise SkillDestUnsafeError( + f"destination {str(resolved_dest)!r} exists but is not a directory" + ) + + # Symlinked-target defence (DEC-005). ``copytree`` with + # ``dirs_exist_ok=True`` will faithfully overwrite a regular file at + # the same path, but on a symlink it would follow the link and write + # into the link target — a destination the operator did not consent + # to. The check covers: + # (a) every install-tree ancestor under ```` back to + # ``.claude/`` (so a symlinked ``.claude/skills/signalforge/`` + # dir cannot smuggle writes through), AND + # (b) every bundled file path we will overwrite — enumerated from + # the source tree below — so a symlinked + # ``assets/SKILL.eval.json`` (or symlinked ``assets/`` dir) is + # refused, not just SKILL.md. + # Refuse loudly before any source materialisation. + target_skill_dir = resolved_dest / _CLAUDE_DIR / _SKILLS_DIR / _SKILL_NAME + for ancestor in ( + resolved_dest / _CLAUDE_DIR, + resolved_dest / _CLAUDE_DIR / _SKILLS_DIR, + target_skill_dir, + ): + if ancestor.is_symlink(): + raise SkillDestUnsafeError( + f"refusing to install through symlinked ancestor {str(ancestor)!r}: " + "would follow the link and write into the resolved target. Remove the " + "symlink first or pick a different destination." + ) + + # Source lookup via importlib.resources — handles editable installs, + # wheel installs, and zipapp/zipimport cases. ``as_file`` + # materialises zip-extracted resources to a real Path; for + # filesystem installs it's an effective no-op. All file I/O is + # performed inside the ``with`` block so the materialised path is + # valid for the duration of the copy (DEC-007 — mirrors + # ``copy_demo`` verbatim). + source_ref = files("signalforge").joinpath(_SKILLS_DIR).joinpath(_SKILL_NAME) + if not source_ref.is_dir(): + raise SkillPackageDataMissingError( + "bundled signalforge/skills/signalforge/ tree not found in the installed package" + ) + + # Per-bundled-path symlink defence. We enumerate every path the + # bundled source ships and refuse to overwrite any of them through + # a symlink (file OR dir). This generalises the SKILL.md-only check + # the original DEC-005 implementation carried — a symlinked + # ``assets/SKILL.eval.json`` or symlinked ``assets/`` dir would + # otherwise let copytree write into an arbitrary target. + with as_file(source_ref) as _src_for_enumeration: + bundled_rel_paths = tuple( + sorted(p.relative_to(_src_for_enumeration) for p in _src_for_enumeration.rglob("*")) + ) + for rel in bundled_rel_paths: + target = target_skill_dir / rel + if target.is_symlink(): + raise SkillDestUnsafeError( + f"refusing to overwrite symlinked bundled path at {str(target)!r}: " + "would follow the link and clobber the resolved target. Remove the " + "symlink first or pick a different destination." + ) + + # Ensure the destination chain exists. ``exist_ok=True`` so an + # already-present skill dir (the upgrade-in-place case) is fine. + # A non-dir component along the chain (e.g. ``/.claude`` is a + # regular file) raises ``NotADirectoryError`` from ``mkdir``; wrap + # to :class:`SkillDestUnsafeError` so the operator sees a typed, + # remediation-bearing message instead of a raw OSError. + try: + target_skill_dir.mkdir(parents=True, exist_ok=True) + except NotADirectoryError as exc: + raise SkillDestUnsafeError( + f"cannot create install chain under {str(resolved_dest)!r}: a non-directory " + "component blocks ``.claude/skills/signalforge/``. Remove the offending file " + "or pick a different destination." + ) from exc + + with as_file(source_ref) as source_path: + # ``dirs_exist_ok=True`` enables the overwrite-files / + # preserve-siblings policy (DEC-003): copytree walks the source + # tree and overwrites every matching file in the destination + # tree; any file in the destination tree without a counterpart + # in the source tree is left untouched. ``symlinks=False`` + # follows source symlinks (the shipped tree carries none — the + # wheel-smoke negative-assertion + the parity test pin that). + shutil.copytree( + source_path, + target_skill_dir, + symlinks=False, + dirs_exist_ok=True, + ) + + # Return the canonical resolved path to the installed SKILL.md so + # callers (and the CLI's next-steps message) get an absolute path + # they can hand to the user. + return (target_skill_dir / _SKILL_MD).resolve() diff --git a/src/signalforge/skill/errors.py b/src/signalforge/skill/errors.py new file mode 100644 index 00000000..5c51cf6c --- /dev/null +++ b/src/signalforge/skill/errors.py @@ -0,0 +1,115 @@ +"""Typed error hierarchy for ``signalforge.skill``. + +Mirrors the layer-base pattern in every other ``signalforge.*.errors`` +module (manifest, warehouse, safety, llm, draft, prune, grade, diff, +cli, demo, ingest, llm.cost — twelve before this module landed). The +:class:`SkillError` base carries an optional ``remediation`` field; +``__str__`` renders ``message`` plus a ``↳ Remediation: `` line +when remediation is set. Subclasses define a ``default_remediation`` +class attribute used when no explicit ``remediation`` is provided. + +The CLI subcommand ``signalforge install-skill`` (issue #141 / US-003) +will catch each concrete subclass and re-raise it as the matching +``CliInstallSkill*Error`` so the CLI exit-code taxonomy stays +homogeneous (DEC-008 of ``plans/super/141-claude-skill-install.md``). +The 7th AST scan in ``tests/test_audit_completeness.py`` walks every +``errors.py`` under ``src/signalforge/*/`` (including this one — the +13th per-stage ``errors.py``) and gates that every concrete leaf +appears in ``signalforge.cli._helpers._EXCEPTION_TO_EXIT_CODE``. The +three concretes below are mapped there at the same tiers as their CLI +wrappers (defence-in-depth — a forward-compat ``Skill*Error`` subclass +that escapes the CLI's try/except ladder still gets a sensible exit +code via ``map_exception_to_exit_code``'s MRO walk). + +Like :class:`signalforge.demo.errors.DemoError`, the three concretes +span tiers 1 and 2, so :class:`SkillError` itself is listed only in +``_EXCEPTION_MAPPING_EXCLUDED_BASES`` — there is no single fallback tier +that fits both classes (DEC-009). +""" + +from __future__ import annotations + +__all__ = [ + "SkillDestPathError", + "SkillDestUnsafeError", + "SkillError", + "SkillPackageDataMissingError", +] + + +class SkillError(Exception): + """Abstract base for ``signalforge.skill`` errors. + + Listed in ``_EXCEPTION_MAPPING_EXCLUDED_BASES`` — every concrete + leaf below must appear in the exit-code mapping, but the base is + excluded (the MRO walk in ``map_exception_to_exit_code`` resolves + forward-compat subclasses to their parent's tier; the bases span + tiers 1 and 2 so no single fallback tier fits — see DEC-009). + """ + + default_remediation: str | None = None + + def __init__( + self, + message: str, + *, + remediation: str | None = None, + cause: Exception | None = None, + ) -> None: + super().__init__(message) + self.message = message + self.remediation = remediation if remediation is not None else self.default_remediation + self.cause = cause + + def __str__(self) -> str: + if self.remediation is None: + return self.message + return f"{self.message}\n ↳ Remediation: {self.remediation}" + + +class SkillDestPathError(SkillError): + """Raised when the destination path cannot be canonicalised. + + Currently fires on symlink-cycle detection. The triggering error + rides on the ``cause`` kwarg: ``RuntimeError`` on Python <= 3.12, + ``OSError(errno.ELOOP)`` on >= 3.13 (gh-108958 changed + ``Path.resolve()``'s cycle signal). The CLI wraps this as + ``CliInstallSkillPathError`` (tier 1). + """ + + default_remediation = "Remove the symlink cycle at the destination or pick a different path." + + +class SkillDestUnsafeError(SkillError): + """Raised when ``dest`` is in a shape we refuse to write under. + + Two surfaces fire this: + + * ``dest`` exists and is a regular file (not a directory) — we + cannot create ``/.claude/skills/...`` underneath it. + * ``/.claude/skills/signalforge/SKILL.md`` exists and is a + symlink — writing would follow the link and clobber an arbitrary + destination, mirroring ``copy_demo``'s symlink-dest refusal. + + The CLI wraps this as ``CliInstallSkillDestUnsafeError`` (tier 2). + """ + + default_remediation = ( + "Pick an existing directory as the destination, or remove the symlinked SKILL.md first." + ) + + +class SkillPackageDataMissingError(SkillError): + """Raised when ``importlib.resources`` cannot locate the bundled + ``skills/signalforge/`` tree. + + Indicates a broken install — the wheel target packaging should + always ship ``src/signalforge/skills/`` (``python-build.md`` + DEC-011 + plan DEC-010 of #141). The CLI wraps this as + ``CliInstallSkillPackageDataMissingError`` (tier 1). + """ + + default_remediation = ( + "Reinstall signalforge-dbt — the bundled Claude Code skill tree is missing " + "from your install." + ) diff --git a/src/signalforge/skills/signalforge/SKILL.md b/src/signalforge/skills/signalforge/SKILL.md new file mode 100644 index 00000000..7a662f6d --- /dev/null +++ b/src/signalforge/skills/signalforge/SKILL.md @@ -0,0 +1,191 @@ +--- +name: signalforge +description: Use when the user wants to draft, prune, or grade dbt tests / docs with an LLM, has a dbt project (manifest.json + sql models), or asks about SignalForge. Drives the `signalforge` CLI end-to-end: drafts candidate tests, runs them against warehouse samples, drops the noise, and explains every kept/dropped artifact. +compatibility: "Requires: signalforge installed (pip install signalforge-dbt) + ANTHROPIC_API_KEY (the drafter always calls Anthropic). The `signalforge lint` and `signalforge install-skill` paths are fully offline. The bundled demo (`init-demo` + `generate`) reads a public BigQuery dataset, so it needs ADC (`gcloud auth application-default login`) + `GOOGLE_CLOUD_PROJECT` for billing — no proprietary warehouse setup of your own, but not credential-free. For real dbt projects: dbt-core + a populated manifest.json + your warehouse profile. For live e2e: BigQuery v0.1." +metadata: + signalforge-version: "0.5.0.dev0" +allowed-tools: Bash(signalforge *), Bash(uv run signalforge *), Bash(uv run pytest -m e2e*), Bash(cat *), Bash(ls *), Bash(grep *), Bash(head *), Bash(tail *), Read, Write, Edit +--- + +# SignalForge — draft, prune, and grade dbt tests with an LLM + +You help the user drive SignalForge against a dbt project. SignalForge's differentiator vs. dbt Copilot / dbt-codegen / DinoAI is the **prune step**: competitors generate; SignalForge generates *and grades*. A candidate test that always passes on warehouse samples is **dropped, not shipped** — always-pass is noise that consumes reviewer attention. + +The pipeline is four stages, each explainable: + +```text +model.sql + manifest + project ctx + -> LLM drafts candidate artifacts (draft) + -> run candidates against warehouse samples (prune) + -> drop always-pass tests; drop tests that fail on known-clean data + -> grade kept artifacts against a rubric (grade) + -> emit graded YAML + diff with per-artifact "why" (diff) +``` + +Every kept/dropped/flagged artifact ships with a one-line "why." Read the diff before writing it. + +## Bootstrap: installing this skill into another project + +If the user is asking how to get this skill into a fresh dbt project, the answer is: + +```bash +pip install signalforge-dbt +signalforge install-skill +``` + +That drops `SKILL.md` into `/.claude/skills/signalforge/`. A fresh Claude Code session activates the skill on the next relevant prompt. Re-running `signalforge install-skill` overwrites `SKILL.md` (and any other files SignalForge ships) while preserving everything else under that directory — stdout reports `(replaced existing SKILL.md)` so you know when the swap fired. There is no `--force` flag. + +`signalforge version` confirms the install resolved. + +--- + +## 1. Point at a dbt project + +Before any pipeline command, verify the dbt project is in a state SignalForge can read. + +```bash +ls target/manifest.json +``` + +If the file is missing, the project has not been parsed yet. Ask the user to run `dbt parse` themselves — this skill's `allowed-tools` deliberately does NOT include `Bash(dbt *)` so it cannot run the parse for them. `dbt parse` requires a configured dbt profile (`~/.dbt/profiles.yml`) and does NOT hit the warehouse, but it does need the profile to exist. If the user has no profile yet, jump to Section 2 (bundled demo) so they can try SignalForge before standing up a real warehouse. + +Once `target/manifest.json` is present, identify the model to work on. SignalForge accepts: + +- A file path: `models/staging/stg_orders.sql` +- A unique_id: `model..` +- A bare name (via `lint` only): `stg_orders` + +Sanity-check a single model's manifest entry — useful when SignalForge later raises `ModelNotFoundError`: + +```bash +signalforge lint --model stg_orders +``` + +`lint` reads the manifest and surfaces obvious manifest-shape issues (missing model, hidden by `enabled: false`, ambiguous bare name) without making any LLM or warehouse calls. + +## 2. Bundled demo + +The fastest way to see the full pipeline end-to-end is the bundled Austin bikeshare demo. It removes the dbt-project setup cost (the demo ships a frozen `manifest.json` and a ready-to-go `signalforge.yml`) but it is **not credential-free** — the demo's `generate` step actually queries the public BigQuery dataset `bigquery-public-data.austin_bikeshare`, billed to your `GOOGLE_CLOUD_PROJECT`. Before running it, confirm: + +- `ANTHROPIC_API_KEY` is set (the drafter always calls Anthropic; free tier covers the demo). +- `GOOGLE_CLOUD_PROJECT` is exported to your own GCP billing project. +- ADC are configured: `gcloud auth application-default login`. + +If any of those are missing, fall back to the truly offline path: `signalforge lint --model ` reads only the manifest (no LLM, no warehouse) and surfaces shape issues. Use it to demonstrate manifest-shape diagnostics without spend. + +```bash +signalforge init-demo +``` + +That writes a self-contained dbt project into `./signalforge-demo/` by default. Pass a path to override (`signalforge init-demo /tmp/sf-demo`). Then: + +```bash +cd signalforge-demo +signalforge generate models/staging/stg_bikeshare_trips.sql --write +``` + +`--write` materialises the proposed `schema.yml` + any singular `tests/*.sql` files into the project. Without `--write`, `generate` prints the diff to stdout and exits — read-only is the safe default. + +SignalForge defaults to `safety: schema-only` — only column **names** and **types** leave the warehouse / fixture. No row values, no aggregates. The demo runs under that posture; nothing sensitive can leak. + +Read the printed diff. You should see: + +- A **kept** column-test table (every test SignalForge believes adds signal). +- A **kept-uncertain** column (tests SignalForge couldn't positively evaluate — shipped under the conservative-bias contract). +- A **dropped** column with the always-pass reasons (`always-passes`, `failed-on-known-clean-data`, `requires-future-data`). +- A unified diff against the (initially empty) existing `schema.yml`. + +Every row in every column has a one-line "why" — read these before deciding whether to keep the run. + +## 3. Real project: draft + prune + +For a user's own dbt project, the command shape is the same: + +```bash +signalforge generate --write +``` + +Where `` is a file path or unique_id from Section 1. The safety posture matters: + +- **Default** is `safety: schema-only` — schema-only is the deployment-blocker safe default. The LLM sees column names + types only. +- **Opt-in** `--mode sample` ships a small warehouse sample to the LLM. This has both **cost** (warehouse query + larger LLM prompt) and **privacy** (real row values leave the warehouse) implications. Use it only when the LLM's schema-only output is missing context the operator needs. +- **Opt-in** `--mode aggregate-only` ships per-column aggregates (count, distinct, null-rate) without raw rows. Middle-ground. + +`signalforge version` verifies the install resolved before you spend warehouse / API budget. `signalforge generate --estimate` previews the LLM + warehouse byte cost without firing real calls. + +The `.signalforge/` directory under the project carries durable audit JSONLs for every stage (safety, llm_responses, prune, grade) plus per-run sidecars (`grade.json`, `diff.json`). These are append-only; they survive crashes mid-run. + +## 4. Grade tests you already have + +If the user has **existing dbt tests** authored by dbt-codegen, dbt Copilot, DinoAI, or a human, SignalForge can grade them without re-drafting. This path makes **no LLM call** — it runs the ingest → prune → diff pipeline against externally-authored `schema.yml`: + +```bash +signalforge prune-existing --schema +``` + +Where `` is the `schema.yml` file containing the existing tests. The command is **read-only** — there's no `--write` flag, because the source `schema.yml` is hand-authored and overwriting it would be surprising. The diff shows what to **remove** from the file (always-pass tests, failed-on-known-clean tests). Apply the diff by hand, or pipe it through your usual review process. + +The same scope / sample-strategy flags from `generate` apply (`--scope`, `--sample-strategy`). The `--mode` flag is inert here — `prune-existing` never builds an LLM payload, so the safety policy has nothing to shape. + +## 5. Reading the diff + +Every kept / kept-uncertain / dropped / flagged row carries a one-line "why." The four tiers: + +| Tier | Meaning | Ships in `schema.yml`? | +|---|---|---| +| **kept** | Test ran with positive evidence — caught a real failing row on warehouse samples, OR grader passed. | Yes | +| **kept-uncertain** | Test could not be positively evaluated (budget elapsed, identifier rejected, materialisation failed, prune disabled). Shipped because the conservative-bias contract says "drop only with positive evidence." | Yes | +| **dropped** | Test ran with positive evidence to drop — always-pass on the sample, OR failed against a `trusted_models` opt-in (test is wrong). | No | +| **flagged** | Test survived prune AND a grader was attached AND the grader scored below threshold. | Yes (but flagged for review) | + +The `why` cascade for kept tests: **drafter rationale** → **first non-empty grader evidence** → **fallback** (decision text for kept-uncertain rows, description for docs). One source per row; never concatenated. + +If the user is surprised by a kept-uncertain row, the `why` text names the specific cause ("total prune budget exceeded before evaluation," "identifier rejected by SQL safety check," etc.). Tune the relevant `prune.*` knob in `signalforge.yml` if the cause is recurring. + +For the full diff, sidecar, and per-run audit shapes, point at `docs/diff-ops.md` and `docs/cli-ops.md`. + +## 6. Optional: live e2e demonstration + +The repo ships a live end-to-end test that exercises the full pipeline against real BigQuery + real Anthropic. **It is gated for a reason** — it costs real money and real API quota. Before invoking it, you MUST: + +1. **Confirm with the user, verbatim:** + + > **This will run paid LLM + warehouse queries — proceed?** + + If the user does not say yes, STOP. Do not run the test. + +2. **Check the required env vars.** All three must be set: + + ```bash + echo "SF_RUN_BQ=${SF_RUN_BQ:-}" + echo "GOOGLE_CLOUD_PROJECT=${GOOGLE_CLOUD_PROJECT:-}" + echo "ANTHROPIC_API_KEY=${ANTHROPIC_API_KEY:+}" + ``` + + If any is unset, **clean-skip with a clear reason** — name the missing var, do NOT run the test. Example: "Skipping live e2e: `SF_RUN_BQ` is unset (need `SF_RUN_BQ=1`)." + +3. **Surface the cost expectation.** The current live e2e costs on the order of a few cents per run (small Austin bikeshare model + one Anthropic Sonnet draft + one grade pass). Cost detail and tuning notes live in `docs/e2e-smoke-test.md` — point the user at that file before running. + +4. **Only THEN run the test:** + + ```bash + uv run pytest -m e2e --no-cov + ``` + + `--no-cov` is required — coverage's `--cov-fail-under` would fail a marker-specific run that exercises only a fraction of the codebase. + +Surface the test output verbatim. The live e2e is the cleanest demonstration that SignalForge actually drops always-pass tests against real data, but it is **never** the default — Section 2's zero-credential demo is. + +## 7. Troubleshooting + +Common errors and their one-line fixes: + +- **`ModelNotFoundError`** — the model arg did not resolve. Verify with `signalforge lint --model ` (accepts bare names + disambiguates collisions across packages). For `generate` / `prune-existing`, use the file path (`models/staging/stg_orders.sql`) or unique_id (`model..`) form. Tier-2 exit code. +- **`WarehouseAuthError`** — adapter could not authenticate. Check `~/.dbt/profiles.yml` is configured for the target profile + that ambient credentials (gcloud ADC, service-account JSON) are valid. Tier-3 exit code (external dependency). +- **`LLMCacheTooLargeError`** — the cached prompt block (model under draft + its direct refs / depends_on neighbours) exceeded 8000 input tokens. Narrow the surface: either trim the model's manifest scope (smaller graph) or split the model into smaller models. Tier-2 exit code (input validation — pre-LLM-call payload-size check). +- **`PromptEnvelopeBreachError`** — drafted SQL or `meta.signalforge.business_rules` content contains the literal closing-tag fence (`` or ``). Remove or rephrase the offending content. Tier-2 exit code. +- **`SamplingRequiresPartitionFilterError`** — model is large (>= 100M rows) and no partition filter was supplied. Either configure `prune.partition_filter` in `signalforge.yml`, or scope to a smaller model. + +Exit codes follow the four-tier taxonomy: **0** success; **1** load / parse failure (manifest missing, config malformed); **2** input validation (bad model id, anchor-contract violation); **3** external dependency (warehouse, LLM, audit-write durability). No traceback ever leaks — every CLI handler wraps its pipeline in one boundary catch. + +For the full flag reference, exit-code table, and per-stage operational detail, point the user at `docs/cli-ops.md`. The per-stage ops docs (`docs/safety-ops.md`, `docs/draft-ops.md`, `docs/prune-ops.md`, `docs/grade-ops.md`, `docs/diff-ops.md`) carry the configuration surface for each layer's `signalforge.yml` block. diff --git a/src/signalforge/skills/signalforge/assets/SKILL.eval.json b/src/signalforge/skills/signalforge/assets/SKILL.eval.json new file mode 100644 index 00000000..b6eb80c3 --- /dev/null +++ b/src/signalforge/skills/signalforge/assets/SKILL.eval.json @@ -0,0 +1,10 @@ +{ + "score": null, + "version": "0.5.0.dev0", + "graded_at": null, + "status": "pending-first-grade", + "skill_path": "src/signalforge/skills/signalforge/SKILL.md", + "grader": "clauditor-eval", + "regen_command": "uv run clauditor grade src/signalforge/skills/signalforge/SKILL.md", + "notes": "Pre-release manual grade per DEC-014 of plans/super/141-claude-skill-install.md. Maintainer crafts an EvalSpec tailored to the SignalForge skill (the auto-generated init template is boilerplate and grades noise-against-noise), runs the grade command above, and replaces this file with the resulting score, version (signalforge.__version__), and ISO-8601 UTC graded_at timestamp." +} diff --git a/tests/cli/test_5_surface_parity_install_skill.py b/tests/cli/test_5_surface_parity_install_skill.py new file mode 100644 index 00000000..ee589480 --- /dev/null +++ b/tests/cli/test_5_surface_parity_install_skill.py @@ -0,0 +1,229 @@ +"""5-surface parity test for the issue #141 / US-005 ``install-skill`` subcommand. + +DEC-024 of ``plans/super/141-claude-skill-install.md`` plus the +``cli-layer.md`` 5-surface parity rule require that the +``install-skill`` subcommand name appears consistently across: + +1. **argparse help** — the ``install-skill`` subparser's ``--help`` + output. Source of truth lives in + :func:`signalforge.cli.install_skill.add_parser` (US-003 wired it). +2. **Handler docstring** — :mod:`signalforge.cli.install_skill`'s module + docstring plus :func:`signalforge.cli.install_skill.cmd_install_skill`'s + docstring. Both reference the subcommand by name. +3. **docs/cli-ops.md § Subcommands** — the ``signalforge install-skill`` + subsection ships with US-006. +4. **plans/super/141-claude-skill-install.md** — DEC-024 names the + canonical token; the user-story section also names the subcommand. +5. **The test file itself** — implicitly satisfied (this file). + +The test reads bytes from each external surface at runtime and asserts +the canonical token (``"install-skill"``) appears in each. Bespoke per +``cli-layer.md`` 5-surface parity rule — future flags get their own +parity test (or extend this one). + +This test is **orthogonal** to +:mod:`tests.cli.test_skill_cli_parity` (US-004): that gate scans the +*full* CLI subparser registry against the bundled ``SKILL.md`` body; +this gate pins *one* subcommand across *five* surfaces. +""" + +from __future__ import annotations + +import argparse +from pathlib import Path + +import pytest + +import signalforge.cli.install_skill as install_skill_module +from signalforge.cli import main +from signalforge.cli.install_skill import add_parser, cmd_install_skill + +# --------------------------------------------------------------------------- +# Surface locations +# --------------------------------------------------------------------------- + +# The plan + ops doc live at the repository root; ``__file__`` is at +# ``tests/cli/test_5_surface_parity_install_skill.py``. +_REPO_ROOT = Path(__file__).resolve().parent.parent.parent +_PLAN_FILE = _REPO_ROOT / "plans" / "super" / "141-claude-skill-install.md" +_OPS_DOC = _REPO_ROOT / "docs" / "cli-ops.md" + +# Canonical tokens the four external surfaces must all carry. Sourced +# from DEC-024. v0.1 has no flags on ``install-skill`` (DEC-003 — no +# ``--force``), so the subcommand name is the only token; a future flag +# extends this tuple in the same commit that adds the flag. +_CANONICAL_TOKENS = ("install-skill",) + + +def _install_skill_help_text() -> str: + """Render the full ``signalforge install-skill --help`` output. + + Builds a fresh top-level parser and registers the subcommand via + :func:`signalforge.cli.install_skill.add_parser`, then asks the + subparser for its formatted help — mirrors + :func:`tests.cli.test_5_surface_parity_init_demo._init_demo_help_text` + so a reviewer sees the same shape across both parity tests. + """ + parser = argparse.ArgumentParser(prog="signalforge") + subparsers = parser.add_subparsers(dest="command") + add_parser(subparsers) + sub = subparsers.choices["install-skill"] + return sub.format_help() + + +# --------------------------------------------------------------------------- +# Surface 1: argparse help +# --------------------------------------------------------------------------- + + +def test_install_skill_in_argparse_help() -> None: + """Each canonical token appears in the rendered ``install-skill --help`` + output (surface 1 of 5). + """ + help_text = _install_skill_help_text() + for token in _CANONICAL_TOKENS: + assert token in help_text, ( + f"install-skill --help missing canonical token {token!r}; got:\n{help_text}" + ) + + +def test_install_skill_help_via_main_entrypoint( + capsys: pytest.CaptureFixture[str], +) -> None: + """End-to-end variant of surface 1: drive + ``main(["install-skill", "--help"])`` so argparse's ``--help`` action + prints to stdout. + + Belt-and-braces against a refactor that moves the subparser + registration out of :func:`add_parser` — only the full ``main`` + dispatch path catches that drift. argparse's ``--help`` action + raises :class:`SystemExit(0)`; :func:`signalforge.cli.main` catches + it and returns ``0`` so the ``-> int`` contract holds (see + ``.claude/rules/cli-layer.md`` § "No traceback ever leaks"). + """ + rc = main(["install-skill", "--help"]) + # argparse --help exits 0; main() returns it as an int. + assert rc == 0 + captured = capsys.readouterr() + for token in _CANONICAL_TOKENS: + assert token in captured.out, ( + f"main(['install-skill', '--help']) stdout missing token " + f"{token!r}; got:\n{captured.out}" + ) + + +# --------------------------------------------------------------------------- +# Surface 2: handler docstring +# --------------------------------------------------------------------------- + + +def test_install_skill_in_handler_docstring() -> None: + """Each canonical token appears in either the module docstring or the + handler docstring (surface 2 of 5). + + The handler ships three docstring surfaces — the module-level one + (describing what ``install-skill`` does and how it differs from + ``init-demo``'s path handling), the per-function one on + :func:`cmd_install_skill`, and the registration docstring on + :func:`add_parser`. The parity check accepts a hit in any of them so + a future refactor that consolidates the prose into one surface + doesn't break the contract. + """ + module_doc = install_skill_module.__doc__ or "" + handler_doc = cmd_install_skill.__doc__ or "" + add_parser_doc = add_parser.__doc__ or "" + combined = "\n".join((module_doc, handler_doc, add_parser_doc)) + for token in _CANONICAL_TOKENS: + assert token in combined, ( + f"signalforge.cli.install_skill docstrings missing canonical " + f"token {token!r}; got module:\n{module_doc}\n\n" + f"handler:\n{handler_doc}\n\nadd_parser:\n{add_parser_doc}" + ) + + +# --------------------------------------------------------------------------- +# Surface 3: docs/cli-ops.md § Subcommands +# --------------------------------------------------------------------------- + + +def test_install_skill_in_cli_ops_doc() -> None: + """Each canonical token appears in ``docs/cli-ops.md`` (surface 3 of 5). + + The check is intentionally whole-file rather than scoped to the + ``install-skill`` subsection — restricting to the subsection would + couple the test to the doc's heading structure (brittle on a + refactor that splits or merges sections). + """ + assert _OPS_DOC.exists(), f"docs/cli-ops.md not found at {_OPS_DOC}" + ops_text = _OPS_DOC.read_text(encoding="utf-8") + for token in _CANONICAL_TOKENS: + assert token in ops_text, ( + f"docs/cli-ops.md missing canonical token {token!r} — " + "5-surface parity break (US-006 ships this surface)" + ) + + +# --------------------------------------------------------------------------- +# Surface 4: plans/super/141-claude-skill-install.md DEC list +# --------------------------------------------------------------------------- + + +def test_install_skill_in_plan_dec_list() -> None: + """Each canonical token appears in + ``plans/super/141-claude-skill-install.md`` (surface 4 of 5). + + The plan's DEC-024 names the canonical token; the user-story + section names the subcommand. Whole-file check rather than + DEC-scoped for the same reason as surface 3 — the contract is + "the tokens appear somewhere in the plan," not "in a specific + section." + """ + assert _PLAN_FILE.exists(), f"plan file not found at {_PLAN_FILE}" + plan_text = _PLAN_FILE.read_text(encoding="utf-8") + for token in _CANONICAL_TOKENS: + assert token in plan_text, ( + f"plans/super/141-claude-skill-install.md missing canonical " + f"token {token!r} — 5-surface parity break" + ) + + +# --------------------------------------------------------------------------- +# Aggregate parity summary +# --------------------------------------------------------------------------- + + +def test_install_skill_consistent_across_surfaces( + capsys: pytest.CaptureFixture[str], +) -> None: + """Aggregate check: every canonical token appears in every external + surface (1, 2, 3, 4 — the 5th is this test file). + + This is the single test a reviewer reads first to verify the + contract; the per-surface tests above pinpoint exactly which + surface drifted on failure. + """ + surfaces: dict[str, str] = { + "argparse_help": _install_skill_help_text(), + "handler_docstring": "\n".join( + ( + install_skill_module.__doc__ or "", + cmd_install_skill.__doc__ or "", + add_parser.__doc__ or "", + ) + ), + "cli_ops_doc": _OPS_DOC.read_text(encoding="utf-8"), + "plan_dec_list": _PLAN_FILE.read_text(encoding="utf-8"), + } + missing: list[tuple[str, str]] = [] + for surface_name, surface_text in surfaces.items(): + for token in _CANONICAL_TOKENS: + if token not in surface_text: + missing.append((surface_name, token)) + assert not missing, ( + f"5-surface parity break — canonical tokens missing from one or more surfaces: {missing!r}" + ) + # Drain any stdout the help-rendering helpers produced (argparse's + # ``--help`` action prints when exercised through ``main(...)``; here + # we used ``format_help`` so no stdout, but capsys is part of the + # signature for parity with the surface-1 test). + capsys.readouterr() diff --git a/tests/cli/test_exit_codes.py b/tests/cli/test_exit_codes.py index 8119ae48..b2a14176 100644 --- a/tests/cli/test_exit_codes.py +++ b/tests/cli/test_exit_codes.py @@ -269,6 +269,14 @@ class _Probe(BaseModel): if name == "CliInitDemoCopyError": return cls(dest="/tmp/synthetic", cause=_SENTINEL_CAUSE) + # install-skill CLI wrappers (issue #141 / DEC-008 — US-003). Path + # and dest-unsafe variants take ``dest=`` + optional ``cause=`` kwargs; + # the package-data-missing wrapper takes ``cause=`` only. + if name in {"CliInstallSkillPathError", "CliInstallSkillDestUnsafeError"}: + return cls(dest="/tmp/synthetic", cause=_SENTINEL_CAUSE) + if name == "CliInstallSkillPackageDataMissingError": + return cls(cause=_SENTINEL_CAUSE) + # Warehouse profile env_var failure (issue #47 — supports init-demo's # bundled `{{ env_var('GOOGLE_CLOUD_PROJECT') }}` profile). Requires # (var_name, profiles_path) as positional args. diff --git a/tests/cli/test_install_skill.py b/tests/cli/test_install_skill.py new file mode 100644 index 00000000..7c5d4a5d --- /dev/null +++ b/tests/cli/test_install_skill.py @@ -0,0 +1,438 @@ +"""Tests for ``signalforge install-skill`` (US-003 — issue #141). + +In-process e2e via :func:`signalforge.cli.main` + ``capsys``. Covers the +US-003 acceptance criteria from +``plans/super/141-claude-skill-install.md``: + +* happy path (fresh dest) → exit 0, INFO line on stdout per DEC-017 +* pre-existing SKILL.md → exit 0, stdout appends ``(replaced existing + SKILL.md)`` per DEC-017 +* ```` is a regular file → exit 2 (input-validation — + :class:`CliInstallSkillDestUnsafeError`), no traceback +* ```` resolves through a symlink cycle → exit 1 (load — + :class:`CliInstallSkillPathError`), no traceback +* bundled package data missing (broken install) → exit 1 + (:class:`CliInstallSkillPackageDataMissingError`), no traceback +* no positional ```` → default to current working directory per + DEC-004; file lands at ``$CWD/.claude/skills/signalforge/SKILL.md`` + +Every test asserts the DEC-016 no-traceback floor on stderr (the +``cli-layer.md`` § "No traceback ever leaks" contract). +""" + +from __future__ import annotations + +import errno +import os +from pathlib import Path + +import pytest + +from signalforge.cli import main + + +def _capture(capsys: pytest.CaptureFixture[str]) -> tuple[str, str]: + captured = capsys.readouterr() + return captured.out, captured.err + + +_INSTALLED_REL = Path(".claude") / "skills" / "signalforge" / "SKILL.md" + + +# --------------------------------------------------------------------------- +# Happy path +# --------------------------------------------------------------------------- + + +def test_install_skill_success_returns_zero_writes_file_prints_info( + tmp_path: Path, + capsys: pytest.CaptureFixture[str], +) -> None: + """Fresh dest → exit 0; SKILL.md materialises under + ``/.claude/skills/signalforge/`` and the DEC-017 stdout INFO + line names the absolute install path. + """ + ret = main(["install-skill", str(tmp_path)]) + out, err = _capture(capsys) + assert ret == 0, f"stdout: {out}\nstderr: {err}" + + installed = tmp_path / _INSTALLED_REL + assert installed.is_file(), f"expected {installed} to exist" + + # DEC-017 — single INFO line, names the absolute path. + assert out.startswith("Installed SignalForge skill to ") + assert str(installed.resolve()) in out + # Fresh dest → no ``(replaced existing SKILL.md)`` suffix. + assert "(replaced existing SKILL.md)" not in out + # No-traceback floor. + assert "Traceback" not in err + + +# --------------------------------------------------------------------------- +# Overwrite path +# --------------------------------------------------------------------------- + + +def test_install_skill_overwrite_appends_replaced_notice( + tmp_path: Path, + capsys: pytest.CaptureFixture[str], +) -> None: + """Pre-existing SKILL.md → stdout appends + ``(replaced existing SKILL.md)`` per DEC-017. + """ + skill_dir = tmp_path / _INSTALLED_REL.parent + skill_dir.mkdir(parents=True) + pre = skill_dir / "SKILL.md" + pre.write_text("# stale prior content\n") + + ret = main(["install-skill", str(tmp_path)]) + out, err = _capture(capsys) + assert ret == 0, f"stdout: {out}\nstderr: {err}" + + assert pre.is_file() + # The file was overwritten — its content no longer matches the seed. + assert pre.read_text() != "# stale prior content\n" + + # DEC-017 contract: the replaced-existing notice appended to the + # single INFO line. + assert out.startswith("Installed SignalForge skill to ") + assert "(replaced existing SKILL.md)" in out + assert "Traceback" not in err + + +# --------------------------------------------------------------------------- +# Dest-is-file → tier 2 +# --------------------------------------------------------------------------- + + +def test_install_skill_dest_is_file_returns_two_no_traceback( + tmp_path: Path, + capsys: pytest.CaptureFixture[str], +) -> None: + """```` is a regular file (not a directory) → exit 2 + (input-validation, :class:`CliInstallSkillDestUnsafeError`). + """ + dest = tmp_path / "not-a-dir" + dest.write_text("i am a regular file") + + ret = main(["install-skill", str(dest)]) + out, err = _capture(capsys) + assert ret == 2, f"expected tier 2; got {ret}\nstdout: {out}\nstderr: {err}" + assert err.startswith("ERROR: ") + # The lib's ``SkillDestUnsafeError`` message names "not a directory". + assert "not a directory" in err + # Remediation footer surfaces. + assert "↳ Remediation:" in err + # The original file is untouched. + assert dest.read_text() == "i am a regular file" + # No-traceback floor. + assert "Traceback" not in err + + +# --------------------------------------------------------------------------- +# Symlink-cycle → tier 1 +# --------------------------------------------------------------------------- + + +def test_install_skill_dest_with_symlink_cycle_returns_one_no_traceback( + tmp_path: Path, + capsys: pytest.CaptureFixture[str], + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A symlink cycle at ```` resolves to + :class:`CliInstallSkillPathError` (tier 1), no traceback. WSL2's + filesystem does not raise on cycles, and Python 3.13 changed the + ``Path.resolve()`` signal from ``RuntimeError`` to + ``OSError(ELOOP)`` (gh-108958), so we synthesise the error by + monkeypatching the lib seam — exercises the CLI handler branch + deterministically across every supported version. + """ + from signalforge.cli import install_skill as install_skill_mod + from signalforge.skill import SkillDestPathError + + def _raise(*_args: object, **_kwargs: object) -> Path: + raise SkillDestPathError( + "failed to resolve destination path 'fake': simulated cycle", + cause=OSError(errno.ELOOP, "Too many levels of symbolic links"), + ) + + monkeypatch.setattr(install_skill_mod, "install_skill", _raise) + ret = main(["install-skill", str(tmp_path)]) + out, err = _capture(capsys) + assert ret == 1, f"expected tier 1; got {ret}\nstdout: {out}\nstderr: {err}" + assert err.startswith("ERROR: ") + # The wrapper carries the resolve / symlink language through. + assert "resolve" in err.lower() or "symlink" in err.lower() + assert "Traceback" not in err + + +# --------------------------------------------------------------------------- +# Missing package data → tier 1 +# --------------------------------------------------------------------------- + + +def test_install_skill_missing_package_data_returns_one_no_traceback( + tmp_path: Path, + capsys: pytest.CaptureFixture[str], + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Simulated broken install (bundled skill tree missing) → exit 1 + with the reinstall-signalforge-dbt remediation. + """ + import signalforge.skill as skill_mod + + class _NotADir: + def joinpath(self, _name: str) -> _NotADir: + return self + + def is_dir(self) -> bool: + return False + + monkeypatch.setattr(skill_mod, "files", lambda _pkg: _NotADir()) + + ret = main(["install-skill", str(tmp_path)]) + out, err = _capture(capsys) + assert ret == 1, f"stdout: {out}\nstderr: {err}" + assert err.startswith("ERROR: ") + # The lib's ``SkillPackageDataMissingError`` default remediation + # points the operator at reinstalling the wheel. + assert "Reinstall" in err or "reinstall" in err.lower() + assert "Traceback" not in err + + +# --------------------------------------------------------------------------- +# Default-dest is CWD (DEC-004) +# --------------------------------------------------------------------------- + + +def test_install_skill_default_dest_is_cwd( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + capsys: pytest.CaptureFixture[str], +) -> None: + """No positional ```` → default to current working directory + per DEC-004; file materialises at + ``$CWD/.claude/skills/signalforge/SKILL.md``. + """ + monkeypatch.chdir(tmp_path) + ret = main(["install-skill"]) + out, err = _capture(capsys) + assert ret == 0, f"stdout: {out}\nstderr: {err}" + + installed = tmp_path / _INSTALLED_REL + assert installed.is_file(), f"expected {installed} to exist after default-dest run" + # The INFO line names the absolute resolved path, so it must contain + # the resolved tmp_path location. + assert str(installed.resolve()) in out + assert "Traceback" not in err + + +# --------------------------------------------------------------------------- +# Exit-code-table membership (paired with the 7th AST scan) +# --------------------------------------------------------------------------- + + +def test_cli_install_skill_wrappers_in_exit_code_table() -> None: + """The three ``CliInstallSkill*Error`` wrappers are registered in + :data:`_EXCEPTION_TO_EXIT_CODE` at the DEC-008 tiers. + """ + from signalforge.cli._helpers import _EXCEPTION_TO_EXIT_CODE + from signalforge.cli.errors import ( + CliInputError, + CliInstallSkillDestUnsafeError, + CliInstallSkillPackageDataMissingError, + CliInstallSkillPathError, + ) + + # Path failure → tier 1 (load). + assert CliInstallSkillPathError in _EXCEPTION_TO_EXIT_CODE + assert _EXCEPTION_TO_EXIT_CODE[CliInstallSkillPathError] == 1 + + # Dest-unsafe → tier 2 (input-validation); subclass of CliInputError + # so callers can pattern-match on the base. + assert CliInstallSkillDestUnsafeError in _EXCEPTION_TO_EXIT_CODE + assert _EXCEPTION_TO_EXIT_CODE[CliInstallSkillDestUnsafeError] == 2 + assert issubclass(CliInstallSkillDestUnsafeError, CliInputError) + + # Package-data-missing → tier 1 (load — broken install). + assert CliInstallSkillPackageDataMissingError in _EXCEPTION_TO_EXIT_CODE + assert _EXCEPTION_TO_EXIT_CODE[CliInstallSkillPackageDataMissingError] == 1 + + +# --------------------------------------------------------------------------- +# argparse help surface +# --------------------------------------------------------------------------- + + +def test_install_skill_help_lists_dest_positional( + capsys: pytest.CaptureFixture[str], +) -> None: + """``signalforge install-skill --help`` mentions the ``DEST`` + positional (metavar) so operators know it accepts a path argument. + """ + ret = main(["install-skill", "--help"]) + out, err = _capture(capsys) + assert ret == 0 + assert "install-skill" in out + assert "DEST" in out or "dest" in out.lower() + assert "Traceback" not in err + + +# --------------------------------------------------------------------------- +# Forward-compat belt-and-braces (DEC-016 panic-path coverage) +# --------------------------------------------------------------------------- + + +def test_install_skill_forward_compat_exception_belt_and_braces( + tmp_path: Path, + capsys: pytest.CaptureFixture[str], + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A forward-compat exception type raised inside ``install_skill`` + still routes through the canonical formatter + mapper without + leaking a traceback (the catch-all ``except Exception`` in + ``cmd_install_skill`` per DEC-016). + """ + from signalforge.cli import install_skill as install_skill_mod + + class _FutureSkillConcurrencyError(Exception): + """Hypothetical v0.x error type.""" + + def _raise(*_args: object, **_kwargs: object) -> Path: + raise _FutureSkillConcurrencyError("skill busy in another process") + + monkeypatch.setattr(install_skill_mod, "install_skill", _raise) + ret = main(["install-skill", str(tmp_path)]) + out, err = _capture(capsys) + # Unmapped → tier 1 (panic-path default). + assert ret == 1, f"stdout: {out}\nstderr: {err}" + assert err.startswith("ERROR: ") + assert "skill busy" in err + assert "Traceback" not in err + + +# --------------------------------------------------------------------------- +# KeyboardInterrupt propagation (DEC-016 carve-out) +# --------------------------------------------------------------------------- + + +def test_install_skill_keyboard_interrupt_propagates( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """``KeyboardInterrupt`` from inside ``install_skill`` propagates — + operator Ctrl-C must reach Python's default handler for sane shell + semantics. DEC-016 carve-out (same as ``init-demo``). + """ + from signalforge.cli import install_skill as install_skill_mod + + def _raise(*_args: object, **_kwargs: object) -> Path: + raise KeyboardInterrupt + + monkeypatch.setattr(install_skill_mod, "install_skill", _raise) + with pytest.raises(KeyboardInterrupt): + main(["install-skill", str(tmp_path)]) + + +# --------------------------------------------------------------------------- +# Wrapper-constructor branch coverage +# --------------------------------------------------------------------------- + + +def test_cli_install_skill_error_constructors_render_without_cause() -> None: + """Every ``CliInstallSkill*Error`` constructor accepts ``cause=None`` + and renders a sensible message + remediation. Exit-code-table + assertions always pass a ``cause``; this exercises the + ``cause is None`` branch in each wrapper. + """ + from signalforge.cli.errors import ( + CliInstallSkillDestUnsafeError, + CliInstallSkillPackageDataMissingError, + CliInstallSkillPathError, + ) + + e1 = CliInstallSkillPathError(dest="/tmp/x") + assert "resolve" in str(e1).lower() + assert e1.cause is None + assert "↳ Remediation:" in str(e1) + + e2 = CliInstallSkillDestUnsafeError(dest="/tmp/x") + assert "unsafe" in str(e2).lower() or "refus" in str(e2).lower() + assert e2.cause is None + assert "↳ Remediation:" in str(e2) + + e3 = CliInstallSkillPackageDataMissingError() + assert "missing" in str(e3).lower() or "skill" in str(e3).lower() + assert e3.cause is None + assert "↳ Remediation:" in str(e3) + + +# --------------------------------------------------------------------------- +# Symlink-cycle defensive path (don't follow the link) +# --------------------------------------------------------------------------- + + +def test_install_skill_with_symlinked_skill_md_returns_two( + tmp_path: Path, + capsys: pytest.CaptureFixture[str], +) -> None: + """Existing ``SKILL.md`` is a symlink → exit 2 per the lib's + symlink-dest refusal (writing would follow the link). + """ + skill_dir = tmp_path / _INSTALLED_REL.parent + skill_dir.mkdir(parents=True) + real_target = tmp_path / "elsewhere.md" + real_target.write_text("victim file") + skill_md = skill_dir / "SKILL.md" + try: + os.symlink(real_target, skill_md) + except (OSError, NotImplementedError): + pytest.skip("filesystem does not support symlinks") + + ret = main(["install-skill", str(tmp_path)]) + out, err = _capture(capsys) + assert ret == 2, f"expected tier 2; got {ret}\nstdout: {out}\nstderr: {err}" + assert err.startswith("ERROR: ") + # The lib's symlink message is operator-actionable. + assert "symlink" in err.lower() + # Victim file untouched. + assert real_target.read_text() == "victim file" + assert "Traceback" not in err + + +def test_install_skill_handles_oserror_in_existed_before_probe( + tmp_path: Path, + capsys: pytest.CaptureFixture[str], + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The pre-install probe for ``existed_before`` swallows ``OSError`` + silently — a permission-denied or stat-failure on the dest tree + must NOT abort the install (the lib seam's own failure surfaces + through the typed except ladder below the probe instead). + + Pinned per the QG coverage gap: the ``except OSError`` branch around + the probe was patch-coverage-dead even though it exists for a real + reason (an unreadable parent dir during pre-probe should not punish + the operator before the lib gets a chance to raise its own typed + error). + """ + import pathlib + + real_exists = pathlib.Path.exists + + def _exists_raises(self: pathlib.Path) -> bool: + # Raise OSError only when the probe is inspecting SKILL.md. + # Real-existence checks on tmp_path / other paths still work. + if self.name == "SKILL.md": + raise OSError(errno.EACCES, "Permission denied") + return real_exists(self) + + monkeypatch.setattr(pathlib.Path, "exists", _exists_raises) + + ret = main(["install-skill", str(tmp_path)]) + out, err = _capture(capsys) + # Probe-OSError silently downgrades → install proceeds → exit 0. + assert ret == 0, f"expected install to proceed; got {ret}\nstdout: {out}\nstderr: {err}" + # Probe failed → existed_before stays False → no `(replaced existing SKILL.md)` suffix. + assert "(replaced existing SKILL.md)" not in out + assert "Traceback" not in err diff --git a/tests/cli/test_skill_cli_parity.py b/tests/cli/test_skill_cli_parity.py new file mode 100644 index 00000000..43b7039e --- /dev/null +++ b/tests/cli/test_skill_cli_parity.py @@ -0,0 +1,346 @@ +"""Mechanical SKILL.md ↔ CLI parity gate (issue #141 / US-004 / DEC-015, 016, 019). + +The bundled Claude Code skill at ``src/signalforge/skills/signalforge/SKILL.md`` +teaches Claude to drive the ``signalforge`` CLI, which makes it a *parity +surface*: every behaviour change to the CLI subcommand surface (add / rename / +remove a subcommand, or shift the canonical demo commands the skill names) MUST +update ``SKILL.md`` in the same change. + +This test is the **gate** that enforces the parity (per ``.claude/rules/ +skill-parity.md`` § "Enforcement is a gate, not a prompt"). Because it runs +inside the canonical ``VALIDATE_CMD`` (``uv run pytest``), a change that drifts +the CLI from the skill fails validation until ``SKILL.md`` is updated — the +skill stays current automatically without relying on the model remembering +during a ``/ralph-run`` session. + +Three token categories must appear verbatim in ``SKILL.md`` (plain substring +match — no regex, no whitespace / case normalisation; matches the +"boring substring match" defence philosophy used by every other prompt / +content gate in the project): + +1. **Every subcommand name from the LIVE argparse parser** — sourced by + walking ``signalforge.cli._build_parser()``'s sole + :class:`argparse._SubParsersAction` and reading ``.choices.keys()``. Today's + v0.2 set is ``version``, ``lint``, ``generate``, ``init-demo``, + ``install-skill``, ``prune-existing``; the iteration auto-grows as new + subcommands land. +2. **Four canonical demo command lines** (DEC-015 hardcoded list): + ``signalforge init-demo``, ``signalforge generate --write``, + ``signalforge prune-existing --schema ``, + ``signalforge install-skill``. Pinned as :data:`_CANONICAL_DEMO_COMMANDS`. +3. **The ``signalforge install-skill`` bootstrap line** — already covered by + category 2's fourth entry, but documented separately per DEC-015 so a + future refactor that drops the install-skill demo from category 2 still + surfaces the missing-bootstrap intent. + +The third test in this module plants a synthetic SKILL.md with one subcommand +missing and asserts the scan helper raises :class:`AssertionError`. Per +``.claude/rules/testing-signal.md`` § "AST source-scan gates must catch all +three bypass patterns", the planted-violation self-check is mandatory — without +it, a refactor that broke the scan visitor would silently disable the gate at +the precise moment a real violation needed catching. The check here is +substring rather than AST, but the philosophy is identical. + +This test is NOT an extension of ``test_5_surface_parity_init_demo.py`` (per +US-004 plan): that file covers ONE subcommand across five surfaces; this file +covers the WHOLE CLI surface against one external file. Different shape, +different responsibilities. + +The test file lives under ``tests/`` (not ``.claude/``) so Ralph workers can +update it — see :mod:`signalforge.skills` module docs and the ``ralph-worker- +claude-dir-perms`` memory: workers cannot write to ``.claude/`` in worktrees, +so the gate and the gated artefact both live in worker-writable trees. +""" + +from __future__ import annotations + +import argparse +import re +from pathlib import Path + +import pytest + +from signalforge.cli import _build_parser + +# --------------------------------------------------------------------------- +# Surface locations +# --------------------------------------------------------------------------- + +# The shipped SKILL.md lives under ``src/signalforge/skills/signalforge/``; +# ``__file__`` is at ``tests/cli/test_skill_cli_parity.py``. +_REPO_ROOT = Path(__file__).resolve().parent.parent.parent +_SKILL_MD = _REPO_ROOT / "src" / "signalforge" / "skills" / "signalforge" / "SKILL.md" + +# Hardcoded per DEC-015. Substring-matched verbatim — do NOT normalise +# whitespace, case, or angle brackets here or in the helper below. +_CANONICAL_DEMO_COMMANDS: tuple[str, ...] = ( + "signalforge init-demo", + "signalforge generate --write", + "signalforge prune-existing --schema ", + "signalforge install-skill", +) + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +def _live_subcommand_names() -> tuple[str, ...]: + """Return every subcommand registered on the live argparse parser. + + Walks :func:`signalforge.cli._build_parser`'s sole + :class:`argparse._SubParsersAction` rather than hardcoding the set — + so a new subcommand landing in ``signalforge.cli`` automatically grows + the parity contract. + """ + parser = _build_parser() + subparser_actions = [a for a in parser._actions if isinstance(a, argparse._SubParsersAction)] + assert len(subparser_actions) == 1, ( + f"expected exactly one subparser action on the top-level parser; got " + f"{len(subparser_actions)} — has the CLI grown a second subparsers " + "tree?" + ) + return tuple(subparser_actions[0].choices.keys()) + + +def _assert_skill_md_names_every_subcommand(skill_md_path: Path) -> None: + """Assert every live subcommand name appears as a substring of + ``skill_md_path``. + + Factored out of :func:`test_skill_md_names_every_registered_subcommand` + so :func:`test_parity_gate_catches_missing_subcommand_planted_violation` + can drive the same check against a synthetic SKILL.md under + :class:`pytest.TempPathFactory`'s ``tmp_path``. + + Raises :class:`AssertionError` naming the missing subcommand on + failure — the message MUST surface the missing token verbatim so the + operator can fix the drift without re-reading the test. + """ + assert skill_md_path.exists(), f"SKILL.md not found at {skill_md_path}" + body = skill_md_path.read_text(encoding="utf-8") + subcommand_names = _live_subcommand_names() + missing = [name for name in subcommand_names if name not in body] + assert not missing, ( + f"SKILL.md at {skill_md_path} is missing registered CLI subcommand(s): " + f"{missing!r}. The bundled Claude Code skill must name every " + "subcommand the live argparse parser exposes — see " + ".claude/rules/skill-parity.md." + ) + + +# --------------------------------------------------------------------------- +# Category 1: every live subcommand name appears in SKILL.md +# --------------------------------------------------------------------------- + + +def test_skill_md_names_every_registered_subcommand() -> None: + """Every subcommand registered on the live CLI argparse parser appears + verbatim in ``SKILL.md`` (category 1 of 3 per DEC-015). + + Iterates :func:`_live_subcommand_names` so a new subcommand grown into + ``signalforge.cli._build_parser`` automatically extends the contract. + On failure, the assertion message names exactly which subcommand(s) + drifted. + """ + _assert_skill_md_names_every_subcommand(_SKILL_MD) + + +# --------------------------------------------------------------------------- +# Category 2: the four canonical demo command lines appear in SKILL.md +# --------------------------------------------------------------------------- + + +def test_skill_md_contains_canonical_demo_command_lines() -> None: + """Each canonical demo command line from DEC-015 appears verbatim in + ``SKILL.md`` (category 2 of 3). + + The four demo commands are the operator-facing flow the skill teaches + Claude to walk through: ``init-demo`` → ``generate --write`` + → ``prune-existing --schema `` → ``install-skill``. A + drift here means the skill is teaching a flow that no longer matches + the CLI surface — exactly the failure mode the gate exists to + prevent. + + Substring-matched (not regex / case-folded) so angle-bracket + placeholders like ```` and ```` survive verbatim. + """ + assert _SKILL_MD.exists(), f"SKILL.md not found at {_SKILL_MD}" + body = _SKILL_MD.read_text(encoding="utf-8") + missing = [cmd for cmd in _CANONICAL_DEMO_COMMANDS if cmd not in body] + assert not missing, ( + f"SKILL.md is missing canonical demo command line(s): {missing!r}. " + "These are the four operator-facing commands the bundled Claude " + "Code skill teaches — see plans/super/141-claude-skill-install.md " + "DEC-015." + ) + + +# --------------------------------------------------------------------------- +# Category 3 (planted violation): the gate can fail loud +# --------------------------------------------------------------------------- + + +def test_parity_gate_catches_missing_subcommand_planted_violation( + tmp_path: Path, +) -> None: + """Planted-violation self-check: a synthetic SKILL.md missing one + subcommand MUST trip the gate (DEC-016 + ``testing-signal.md`` § "AST + source-scan gates"). + + Without this test, a refactor that broke + :func:`_assert_skill_md_names_every_subcommand` (e.g. a typo flipping + the ``not in`` check, or an inadvertent ``return`` short-circuit) + would silently disable the gate at the precise moment a real + violation needed catching. The check here is substring rather than + AST, but the philosophy from ``testing-signal.md`` applies verbatim. + + We craft a synthetic SKILL.md that names every subcommand EXCEPT one, + point the helper at it, and assert :class:`AssertionError` raises + with the missing subcommand named in the message. The synthetic body + avoids touching the real shipped SKILL.md so the planted violation + cannot accidentally leak into the gated artefact. + """ + subcommand_names = _live_subcommand_names() + # Choose the subcommand to omit deterministically — the first one in + # the live order. The gate doesn't care which we drop; the contract + # is "any missing subcommand trips the assert". + omitted = subcommand_names[0] + kept = tuple(n for n in subcommand_names if n != omitted) + synthetic_body = ( + "# Synthetic SKILL.md for planted-violation self-check.\n\n" + "This file names every CLI subcommand EXCEPT one, so the parity " + "gate must raise AssertionError naming the omitted subcommand:\n\n" + + "\n".join(f"- `signalforge {name}`" for name in kept) + + "\n" + ) + # Sanity: confirm the synthetic body actually omits the chosen + # subcommand verbatim (defends against a future change to the + # synthetic body that accidentally includes the omitted name as a + # substring of something else). + assert omitted not in synthetic_body, ( + f"synthetic SKILL.md inadvertently contains the omitted " + f"subcommand {omitted!r}; rewrite the synthetic body so the " + "planted violation is real" + ) + + synthetic_skill_md = tmp_path / "SKILL.md" + synthetic_skill_md.write_text(synthetic_body, encoding="utf-8") + + with pytest.raises(AssertionError) as exc_info: + _assert_skill_md_names_every_subcommand(synthetic_skill_md) + + # The assertion message must surface the omitted subcommand verbatim + # so an operator running the gate locally fixes the right token. + assert omitted in str(exc_info.value), ( + f"AssertionError message did not name the missing subcommand " + f"{omitted!r}; got: {exc_info.value!r}" + ) + + +# --------------------------------------------------------------------------- +# Category 4 (QG extension): flags the skill claims must exist on the live CLI +# --------------------------------------------------------------------------- + + +# Match ``signalforge [ ...] --`` occurrences +# inside SKILL.md so a typo / stale-rebase that teaches a non-existent flag +# (the US-008-era ``signalforge install-skill --force`` finding) trips the +# gate. Captures the subcommand AND the flag separately so we can dispatch +# the validity check to the right subparser. +# +# The intermediate ``(?:[ \t]+\S+)*?`` (non-greedy, **same-line only**) +# absorbs zero or more positional arguments between the subcommand and +# the flag — without it canonical shapes like ``signalforge generate +# --write`` and ``signalforge prune-existing --schema +# `` slip past the gate (CodeRabbit + Copilot QG finding). We +# constrain to ``[ \t]`` (NOT ``\s``) so the match cannot span newlines: +# a paragraph that mentions ``signalforge installed (pip install ...)`` +# in one sentence and ``signalforge lint --model`` two paragraphs later +# would otherwise yield a spurious ``installed --model`` capture (a +# false positive caught during the QG fix). +# +# Plain ASCII flag chars only — the regex deliberately does NOT match +# exotic shells (`/`, `=`, etc.) because skill prose only ever teaches +# the conventional long-flag form. +_SKILL_FLAG_USAGE_RE = re.compile( + r"signalforge ([a-z][a-z0-9-]*)(?:[ \t]+\S+)*?[ \t]+(--[a-z][a-z0-9-]*)" +) + + +def _subparser_flags(subcommand: str) -> frozenset[str]: + """Return every long-form flag (``--foo``) registered on ``subcommand``. + + Walks the live argparse subparser for the named subcommand and harvests + every ``option_string`` that starts with ``--`` from every action. + Returns an empty frozenset if the subcommand isn't registered (caller + is responsible for distinguishing that case). + """ + parser = _build_parser() + subparser_actions = [a for a in parser._actions if isinstance(a, argparse._SubParsersAction)] + assert len(subparser_actions) == 1 + choices = subparser_actions[0].choices + if subcommand not in choices: + return frozenset() + sub = choices[subcommand] + flags: set[str] = set() + for action in sub._actions: + for opt in action.option_strings: + if opt.startswith("--"): + flags.add(opt) + return frozenset(flags) + + +def test_skill_md_only_teaches_flags_that_exist_on_the_live_cli() -> None: + """Every ``signalforge --`` occurrence in SKILL.md + names a flag that actually exists on the live subparser (QG-extended + category 4 of 3, added after the US-008 review found SKILL.md teaching + ``signalforge install-skill --force`` — a flag DEC-003 explicitly + forbids). + + The substring match in category 1/2 catches missing subcommands and + drifted demo commands but NOT a typo flag the skill claims to exist. + This test closes that gap by scanning SKILL.md for any + ``signalforge X --flag`` pattern and asserting ``--flag`` is in the + live subparser's option strings. + + The skill-parity rule explicitly acknowledges the gate is "necessary + not sufficient" (per ``.claude/rules/skill-parity.md``) — semantic + prose freshness still rides reviewer attention + the clauditor self- + grade. This category 4 narrows that gap by promoting one specific + "prose lies about the CLI surface" failure mode into a mechanical + gate. + """ + assert _SKILL_MD.exists(), f"SKILL.md not found at {_SKILL_MD}" + body = _SKILL_MD.read_text(encoding="utf-8") + + live_subcommands = frozenset(_live_subcommand_names()) + bogus: list[tuple[str, str]] = [] + unknown_subcmd: list[tuple[str, str]] = [] + for match in _SKILL_FLAG_USAGE_RE.finditer(body): + subcommand, flag = match.group(1), match.group(2) + if subcommand not in live_subcommands: + # A typo like ``signalforge instal-skill --force`` would + # previously slip past this gate because the subcommand is + # unknown. Fail loud — a misspelled subcommand example in + # SKILL.md is exactly the drift this gate exists to catch + # (Copilot QG finding). + unknown_subcmd.append((subcommand, flag)) + continue + registered = _subparser_flags(subcommand) + if flag not in registered: + bogus.append((subcommand, flag)) + + assert not unknown_subcmd, ( + f"SKILL.md teaches a ``signalforge --`` example " + f"naming an unknown subcommand: {unknown_subcmd!r}. The subcommand " + "does not appear on the live argparse parser — likely a typo. Run " + "``signalforge --help`` for the real subcommand list." + ) + assert not bogus, ( + f"SKILL.md teaches flag(s) that do not exist on the live CLI: " + f"{bogus!r}. Either the flag was renamed / removed (update SKILL.md) " + "or the skill prose is wrong (e.g. teaching ``signalforge install-skill " + "--force`` when DEC-003 explicitly forbids ``--force``). Run " + "``signalforge --help`` to see the real flag set." + ) diff --git a/tests/cli/test_subprocess_smoke.py b/tests/cli/test_subprocess_smoke.py index 9273a6df..d12bc7f1 100644 --- a/tests/cli/test_subprocess_smoke.py +++ b/tests/cli/test_subprocess_smoke.py @@ -165,6 +165,38 @@ def test_signalforge_init_demo_help_via_subprocess() -> None: assert "Traceback" not in result.stderr +@pytest.mark.cli_subprocess +def test_signalforge_install_skill_help_via_subprocess() -> None: + """``signalforge install-skill --help`` exits 0 with the subcommand's help. + + US-003 of ``plans/super/141-claude-skill-install.md`` (#141 / DEC-009 + / DEC-024) — extends the subprocess-gated smoke to the new + ``install-skill`` subcommand so a ``[project.scripts]`` regression + specific to its argparse wiring (subparser deletion, ``add_parser`` + typo, console-script wrapper losing the dispatch entry) is caught by + ``pytest -m cli_subprocess``. The in-process ``main(argv)`` smoke + tests in ``tests/cli/`` cannot catch this class of regression — they + bypass the ``[project.scripts]`` table entirely. + """ + result = subprocess.run( + ["signalforge", "install-skill", "--help"], + capture_output=True, + text=True, + timeout=10, + ) + + assert result.returncode == 0 + # The presence of the subcommand name plus the ``DEST`` positional + # metavar jointly guarantees argparse is rendering the new + # subcommand's help, not the top-level usage. (``install-skill`` has + # no flags of its own in v0.1 per DEC-003, so the subcommand name + + # the rendered positional are the unique discriminators here.) + assert "install-skill" in result.stdout + assert "DEST" in result.stdout + # No-traceback floor — see the ``--version`` test above. + assert "Traceback" not in result.stderr + + @pytest.mark.cli_subprocess def test_signalforge_prune_existing_help_via_subprocess() -> None: """``signalforge prune-existing --help`` exits 0 with the subcommand's help. diff --git a/tests/skill/test_install.py b/tests/skill/test_install.py new file mode 100644 index 00000000..4afdda62 --- /dev/null +++ b/tests/skill/test_install.py @@ -0,0 +1,356 @@ +"""Unit tests for the public :mod:`signalforge.skill` module (US-002 of +issue #141 — ``plans/super/141-claude-skill-install.md``). + +These tests pin the six load-bearing contracts from US-002's TDD list: + +1. happy path — fresh dir, returns absolute SKILL.md path; +2. overwrite SKILL.md but preserve sibling user files (DEC-003); +3. refuse-with-typed-error when SKILL.md is a pre-existing symlink (DEC-005); +4. symlink-cycle dest raises ``SkillDestPathError`` (DEC-005, mirrors + ``copy_demo``'s 3.12/3.13 cycle signals); +5. monkeypatched package-data lookup raises + ``SkillPackageDataMissingError`` (DEC-007); +6. existing regular file at ``dest`` raises ``SkillDestUnsafeError`` (DEC-008). + +See ``.claude/rules/testing-signal.md`` — every test has at least one +real assertion that can fail; no ``tests/skill/__init__.py`` (pytest +src-layout convention). +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from signalforge.skill import ( + SkillDestPathError, + SkillDestUnsafeError, + SkillError, + SkillPackageDataMissingError, + install_skill, +) + +# --------------------------------------------------------------------------- +# Happy path +# --------------------------------------------------------------------------- + + +def test_install_skill_to_fresh_dir_writes_skill_md(tmp_path: Path) -> None: + """Happy path: ``install_skill`` writes SKILL.md under + ``/.claude/skills/signalforge/`` and returns its absolute path.""" + + result = install_skill(tmp_path) + + assert isinstance(result, Path) + assert result.is_absolute() + assert result == (tmp_path / ".claude" / "skills" / "signalforge" / "SKILL.md").resolve() + assert result.is_file() + assert result.stat().st_size > 0 + + +# --------------------------------------------------------------------------- +# Overwrite policy (DEC-003) — SKILL.md replaced, siblings preserved +# --------------------------------------------------------------------------- + + +def test_install_skill_overwrites_existing_skill_md_unchanged_otherwise( + tmp_path: Path, +) -> None: + """DEC-003: ``install_skill`` always overwrites the files SignalForge + ships (SKILL.md + bundled assets); preserves any sibling files the + operator added under ``.claude/skills/signalforge/``.""" + + skill_dir = tmp_path / ".claude" / "skills" / "signalforge" + skill_dir.mkdir(parents=True) + old_skill_md = skill_dir / "SKILL.md" + old_skill_md.write_text("OLD", encoding="utf-8") + sibling = skill_dir / "notes.txt" + sibling.write_text("keepme", encoding="utf-8") + + install_skill(tmp_path) + + assert old_skill_md.read_text(encoding="utf-8") != "OLD" + assert old_skill_md.stat().st_size > 0 + # User-authored sibling is untouched. + assert sibling.read_text(encoding="utf-8") == "keepme" + + +# --------------------------------------------------------------------------- +# Symlinked SKILL.md → SkillDestUnsafeError (DEC-005) +# --------------------------------------------------------------------------- + + +def test_install_skill_refuses_when_skill_md_is_symlink(tmp_path: Path) -> None: + """DEC-005: if ``/.claude/skills/signalforge/SKILL.md`` exists + AND is a symlink, the install refuses — writing would follow the + link and clobber an arbitrary destination.""" + + skill_dir = tmp_path / ".claude" / "skills" / "signalforge" + skill_dir.mkdir(parents=True) + elsewhere = tmp_path / "elsewhere.md" + elsewhere.write_text("not ours", encoding="utf-8") + skill_md = skill_dir / "SKILL.md" + skill_md.symlink_to(elsewhere) + + with pytest.raises(SkillDestUnsafeError): + install_skill(tmp_path) + + # Defence: the link target is untouched. + assert elsewhere.read_text(encoding="utf-8") == "not ours" + + +def test_install_skill_refuses_when_install_dir_ancestor_is_symlink( + tmp_path: Path, +) -> None: + """DEC-005 (QG-extended): if ANY ancestor under back to + ``.claude/`` is a symlinked directory, the install refuses — writing + through a symlinked ancestor would land in the resolved target + (e.g. ``.claude/skills/signalforge/`` repointed to /tmp/attacker) + without operator consent. + + Without this gate, the SKILL.md-only ``is_symlink()`` check would + pass (SKILL.md inside the linked dir is not itself a symlink) and + ``shutil.copytree`` would write straight through. Pinned per the + QG review finding. + """ + + elsewhere = tmp_path / "elsewhere" + elsewhere.mkdir() + (elsewhere / "preexisting.txt").write_text("attacker's data", encoding="utf-8") + + skills_parent = tmp_path / ".claude" / "skills" + skills_parent.mkdir(parents=True) + # ``.claude/skills/signalforge/`` -> /tmp/.../elsewhere/ (symlinked ancestor) + (skills_parent / "signalforge").symlink_to(elsewhere, target_is_directory=True) + + with pytest.raises(SkillDestUnsafeError): + install_skill(tmp_path) + + # Defence: the link target's preexisting file is untouched. + assert (elsewhere / "preexisting.txt").read_text(encoding="utf-8") == "attacker's data" + # No SKILL.md materialised under the symlinked path. + assert not (elsewhere / "SKILL.md").exists() + + +def test_install_skill_refuses_when_assets_dir_is_symlink(tmp_path: Path) -> None: + """DEC-005 (Copilot QG-extended): the symlink defence covers EVERY + bundled path (``SKILL.md`` AND ``assets/SKILL.eval.json`` AND any + future bundled asset), not just SKILL.md. + + If a destination's ``assets/`` is a symlinked directory pointing + elsewhere, ``shutil.copytree(..., dirs_exist_ok=True)`` would follow + the link and overwrite ``assets/SKILL.eval.json`` at the resolved + target. The per-bundled-path enumeration refuses this BEFORE any + file is materialised. + """ + elsewhere = tmp_path / "attacker-assets" + elsewhere.mkdir() + (elsewhere / "SKILL.eval.json").write_text("victim eval data", encoding="utf-8") + + skill_dir = tmp_path / ".claude" / "skills" / "signalforge" + skill_dir.mkdir(parents=True) + (skill_dir / "assets").symlink_to(elsewhere, target_is_directory=True) + + with pytest.raises(SkillDestUnsafeError): + install_skill(tmp_path) + + # Defence: the link target's preexisting file is untouched. + assert (elsewhere / "SKILL.eval.json").read_text(encoding="utf-8") == "victim eval data" + + +def test_install_skill_wraps_notadirectoryerror_from_mkdir_chain( + tmp_path: Path, +) -> None: + """If a non-dir component blocks the install-chain creation + (``/.claude`` is a regular file rather than a directory), + the resulting ``NotADirectoryError`` from ``mkdir(parents=True)`` + is wrapped as :class:`SkillDestUnsafeError` with a typed + remediation, NOT propagated as a raw OSError (CodeRabbit QG + finding on ``__init__.py:151``). + """ + blocker = tmp_path / ".claude" + blocker.write_text("not a dir", encoding="utf-8") + + with pytest.raises(SkillDestUnsafeError) as excinfo: + install_skill(tmp_path) + + assert "non-directory" in str(excinfo.value) + # Blocker file untouched. + assert blocker.read_text(encoding="utf-8") == "not a dir" + + +# --------------------------------------------------------------------------- +# Symlink-cycle dest → SkillDestPathError (DEC-005, mirrors copy_demo) +# --------------------------------------------------------------------------- + + +def test_install_skill_with_cyclic_symlink_dest_raises_dest_path_error( + tmp_path: Path, +) -> None: + """DEC-005: a symlink cycle at ``dest`` surfaces as + :class:`SkillDestPathError` on both Python <=3.12 (``RuntimeError``) + and >=3.13 (``OSError(ELOOP)``, gh-108958).""" + + link_a = tmp_path / "loop_a" + link_b = tmp_path / "loop_b" + link_a.symlink_to(link_b) + link_b.symlink_to(link_a) + + # Probe the same way ``install_skill`` does — skip cleanly on + # filesystems where ``resolve(strict=True)`` does not enforce the + # cycle guard (matches the precedent in tests/test_demo.py). + try: + link_a.resolve(strict=True) + except (RuntimeError, OSError): + pass + else: + pytest.skip( + "filesystem does not raise on symlink cycles; " + "SkillDestPathError path is verified on the CI Linux runner" + ) + + with pytest.raises(SkillDestPathError) as excinfo: + install_skill(link_a) + assert isinstance(excinfo.value.cause, RuntimeError | OSError) + + +# --------------------------------------------------------------------------- +# Package-data missing → SkillPackageDataMissingError (DEC-007) +# --------------------------------------------------------------------------- + + +def test_install_skill_missing_package_data_raises( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """DEC-007: if ``importlib.resources`` cannot locate the bundled + ``skills/signalforge/`` tree, raise + :class:`SkillPackageDataMissingError`. + + Simulated by monkeypatching the ``files`` lookup to return a + non-directory traversable (matches the precedent in + ``tests/test_demo.py::test_copy_demo_fixture_missing_raises``). + """ + + import signalforge.skill as skill_mod + + class _NotADir: + def joinpath(self, name: str) -> _NotADir: + return self + + def is_dir(self) -> bool: + return False + + monkeypatch.setattr(skill_mod, "files", lambda pkg: _NotADir()) + + with pytest.raises(SkillPackageDataMissingError) as excinfo: + install_skill(tmp_path) + assert "bundled" in str(excinfo.value) or "missing" in str(excinfo.value).lower() + + +# --------------------------------------------------------------------------- +# Regular-file dest → SkillDestUnsafeError (DEC-008) +# --------------------------------------------------------------------------- + + +def test_install_skill_dest_is_file_raises_unsafe(tmp_path: Path) -> None: + """DEC-008: passing an existing regular file as ``dest`` raises + :class:`SkillDestUnsafeError` — we cannot create + ``/.claude/skills/...`` underneath it.""" + + file_dest = tmp_path / "not_a_dir" + file_dest.write_text("hello", encoding="utf-8") + + with pytest.raises(SkillDestUnsafeError): + install_skill(file_dest) + + # File contents unchanged — defence-in-depth. + assert file_dest.read_text(encoding="utf-8") == "hello" + + +# --------------------------------------------------------------------------- +# Error-class shape (DEC-008) +# --------------------------------------------------------------------------- + + +def test_skill_errors_share_base_class() -> None: + """All three concretes subclass :class:`SkillError`.""" + assert issubclass(SkillDestPathError, SkillError) + assert issubclass(SkillDestUnsafeError, SkillError) + assert issubclass(SkillPackageDataMissingError, SkillError) + + +def test_skill_error_str_renders_remediation_footer() -> None: + """``__str__`` renders ``message`` + ``↳ Remediation:`` line when + a ``default_remediation`` is set (mirrors :class:`DemoError`).""" + err = SkillDestUnsafeError("destination 'x' is not a directory") + rendered = str(err) + assert "destination 'x' is not a directory" in rendered + assert "↳ Remediation:" in rendered + + +def test_skill_error_str_omits_footer_when_remediation_is_none() -> None: + """``__str__`` returns just ``message`` (no footer) when neither an + explicit ``remediation`` kwarg NOR a class-level ``default_remediation`` + is set. + + The :class:`SkillError` base declares ``default_remediation: str | None + = None`` so constructing it directly hits the no-footer branch. Without + this test the line is patch-coverage-dead — codecov flags it on the PR + even though the branch is a real fallback contract (forward-compat + subclasses without their own default_remediation render plainly). + """ + err = SkillError("bare message, no remediation") + assert str(err) == "bare message, no remediation" + assert "↳" not in str(err) + + +def test_install_skill_propagates_non_eloop_oserror_unchanged( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """An ``OSError`` whose ``errno`` is NOT ``ELOOP`` propagates raw + out of :func:`install_skill` — it is NOT wrapped as + :class:`SkillDestPathError`. + + The narrow ELOOP-only routing is load-bearing: a ``PermissionError`` + (an OSError subclass) on the canonicalise path is a different class + of failure that the CLI's panic catch surfaces as tier 1 with a + generic message. Conflating the two would mis-attribute permission + errors as symlink-cycle errors and waste the operator's time. + """ + import errno + import pathlib + + eacces_err = OSError(errno.EACCES, "Permission denied") + + def _raise_eacces(self: pathlib.Path, *, strict: bool = False) -> pathlib.Path: + raise eacces_err + + monkeypatch.setattr(pathlib.Path, "resolve", _raise_eacces) + + with pytest.raises(OSError) as excinfo: + install_skill(tmp_path) + # Raw OSError — NOT SkillDestPathError. + assert not isinstance(excinfo.value, SkillError) + assert excinfo.value.errno == errno.EACCES + + +def test_install_skill_resolves_nonexistent_dest_via_strict_false_fallback( + tmp_path: Path, +) -> None: + """A ``dest`` that does not exist yet falls through ``resolve(strict= + True)`` (raises ``FileNotFoundError``) to the ``resolve(strict=False)`` + fallback. Covers the common case where the operator runs + ``signalforge install-skill /tmp/new-project`` against a path that + only exists after the install creates it. + + Without this test the ``strict=False`` fallback line is patch- + coverage-dead even though every fresh-project install hits it. + """ + new_dest = tmp_path / "fresh-project-that-does-not-exist-yet" + assert not new_dest.exists() + + installed = install_skill(new_dest) + # Install proceeded — the fallback resolved the path successfully. + assert installed.exists() + assert installed.is_relative_to(new_dest.resolve()) diff --git a/tests/test_audit_completeness.py b/tests/test_audit_completeness.py index 1aa1537d..65fee0eb 100644 --- a/tests/test_audit_completeness.py +++ b/tests/test_audit_completeness.py @@ -700,6 +700,18 @@ def test_grade_event_construction_in_grade_audit_module_is_present() -> None: # require it to be mapped (the table entry is the safety net, not # the contract). "CostError", + # ``SkillError`` (issue #141 / DEC-008, DEC-009) — abstract base + # of the ``signalforge.skill`` typed-error hierarchy (the 13th + # per-stage ``errors.py``). Its three concrete subclasses are + # individually mapped in ``_EXCEPTION_TO_EXIT_CODE`` + # (``SkillDestPathError`` / ``SkillPackageDataMissingError`` → + # tier 1; ``SkillDestUnsafeError`` → tier 2). Like ``DemoError`` + # and ``IngestError``, the concretes span tiers 1 and 2, so the + # base gets NO single fallback-tier entry — it lives only here + # in the excluded set; a forgotten concrete falls through to + # tier 1 and the AST scan catches the missing per-class entry + # at test time. + "SkillError", } ) @@ -825,13 +837,14 @@ def test_scan_7_discovers_every_per_stage_errors_module() -> None: """Sanity: ``_enumerate_error_module_paths`` finds every per-stage ``errors.py`` in the project. If a future stage forgets to ship ``errors.py`` the scan would still pass (because there'd be nothing - to walk for that stage); this test pins the expected set of twelve - modules. + to walk for that stage); this test pins the expected set of + thirteen modules. Issue #157 / DEC-002 of US-001 added the first sub-stage ``errors.py`` (``llm/cost/errors.py`` — the cost-rollup layer); the glob was extended to depth-2 in lockstep so the expected count - bumped 11 → 12. + bumped 11 → 12. Issue #141 / US-002 / DEC-009 added the + ``signalforge.skill`` package and bumped 12 → 13. """ paths = _enumerate_error_module_paths() rel_names = sorted(p.relative_to(_SIGNALFORGE_DIR).as_posix() for p in paths) @@ -847,12 +860,13 @@ def test_scan_7_discovers_every_per_stage_errors_module() -> None: "manifest/errors.py", "prune/errors.py", "safety/errors.py", + "skill/errors.py", "warehouse/errors.py", ], ( - "Expected exactly twelve per-stage errors.py modules (one per " + "Expected exactly thirteen per-stage errors.py modules (one per " "stage; demo added in #47, ingest in #104, llm/cost added in " - f"#157); got: {rel_names}. If this changes, update Scan 7's " - "expected set." + f"#157, skill added in #141); got: {rel_names}. If this " + "changes, update Scan 7's expected set." ) diff --git a/tests/test_wheel_packaging.py b/tests/test_wheel_packaging.py index 295ea160..c8743c92 100644 --- a/tests/test_wheel_packaging.py +++ b/tests/test_wheel_packaging.py @@ -59,6 +59,19 @@ "signalforge/_demo/target/manifest.json", ) +# Canonical bundled-skill file set under ``signalforge/skills/`` inside the +# built wheel. Established by US-001 of ``plans/super/141-claude-skill-install.md`` +# (DEC-001 — the shipped SignalForge skill lives in ``src/signalforge/skills/`` +# so Ralph workers can update it from worktrees; DEC-010 — wheel packaging via +# ``[tool.hatch.build.targets.wheel].include``; DEC-011 — wheel_smoke gates the +# file set so a drop fails loud at packaging time; DEC-022 — maintainer-only +# skills under repo-root ``.claude/skills/`` MUST stay excluded). The placeholder +# eval-sidecar lives under ``assets/`` to mirror the SKILL Spec convention. +_EXPECTED_SKILL_FILES: tuple[str, ...] = ( + "signalforge/skills/signalforge/SKILL.md", + "signalforge/skills/signalforge/assets/SKILL.eval.json", +) + def _build_command(outdir: Path) -> list[str]: """Pick the wheel-build invocation available in the current environment. @@ -166,6 +179,51 @@ def test_wheel_excludes_scripts_directory(_built_wheel_members: set[str]) -> Non ) +@pytest.mark.wheel_smoke +def test_wheel_includes_all_bundled_skill_files(_built_wheel_members: set[str]) -> None: + """Every file in ``src/signalforge/skills/`` ships in the built wheel. + + Gates DEC-010 (``include = [..., "src/signalforge/skills"]``) at + packaging time. The bundled SignalForge skill (US-007) plus its + placeholder eval sidecar (US-001) must reach an installed wheel so + the ``install-skill`` CLI (US-002) can copy them into + ``~/.claude/skills/signalforge/``. Without the directive Hatchling's + default ``packages`` glob is not guaranteed to pick up non-``.py`` + skill data (mirrors the demo-tree precedent, ``DEC-002`` of + ``plans/super/47-init-demo.md``). + """ + missing = [name for name in _EXPECTED_SKILL_FILES if name not in _built_wheel_members] + assert not missing, ( + f"wheel is missing bundled skill files: {missing}. " + f"Check `[tool.hatch.build.targets.wheel] include` in pyproject.toml." + ) + + +@pytest.mark.wheel_smoke +def test_wheel_excludes_maintainer_only_claude_skills(_built_wheel_members: set[str]) -> None: + """No ``.claude/skills/*`` entry may ship in the built wheel. + + DEC-022 of ``plans/super/141-claude-skill-install.md`` — maintainer-only + skills (``release-manager``, ``review-agentskills-spec``) live at + repo-root ``.claude/skills/`` and MUST stay out of the distributed + wheel. They are orchestrator-only conventions and a Ralph worker + cannot edit them from a worktree (see the ``ralph-worker-claude-dir-perms`` + memory), so accidentally bundling them would both bloat the wheel and + surface internal tooling to end-users. The shipped, user-facing skill + lives under ``src/signalforge/skills/`` (see the positive assertion + above); this negative gate catches a future contributor who mirrors + the repo-root ``.claude/skills/`` tree into the wheel by mistake. + """ + leaked = sorted(name for name in _built_wheel_members if ".claude/skills/" in name) + assert not leaked, ( + "wheel unexpectedly ships entries under `.claude/skills/`: " + f"{leaked}. Maintainer-only skills (release-manager, " + "review-agentskills-spec) must stay at repo-root `.claude/skills/` " + "and out of the wheel (DEC-022 of plans/super/141-claude-skill-install.md). " + "The shipped user-facing skill lives under `src/signalforge/skills/`." + ) + + @pytest.mark.wheel_smoke def test_wheel_includes_demo_gitignore_dotfile(_built_wheel_members: set[str]) -> None: """``signalforge/_demo/.gitignore`` ships in the wheel (DEC-006). diff --git a/uv.lock b/uv.lock index ce41b279..bafa8bd9 100644 --- a/uv.lock +++ b/uv.lock @@ -300,6 +300,19 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/db/8f/61959034484a4a7c527811f4721e75d02d653a35afb0b6054474d8185d4c/charset_normalizer-3.4.7-py3-none-any.whl", hash = "sha256:3dce51d0f5e7951f8bb4900c257dad282f49190fdbebecd4ba99bcc41fef404d", size = 61958, upload-time = "2026-04-02T09:28:37.794Z" }, ] +[[package]] +name = "clauditor-eval" +version = "0.1.3" +source = { registry = "https://pypi.org/simple" } +dependencies = [ + { name = "anthropic" }, + { name = "openai" }, +] +sdist = { url = "https://files.pythonhosted.org/packages/18/a5/604d0226ccc176f352bcc91dead024b6a6146ad23f26b52ad685dfe4d764/clauditor_eval-0.1.3.tar.gz", hash = "sha256:0acd40063ea6aba9ae458f4bb73baa5bdc4cf47d6331946164c2a91f9d0e3231", size = 821353, upload-time = "2026-05-26T16:27:05.624Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/af/a0/8d98eba2726c47539b7b4f9b879aaa9e768ef6e1cb657fb04b185c940776/clauditor_eval-0.1.3-py3-none-any.whl", hash = "sha256:8fd87b4896a80411c367f3de7f5bc21c0147674bf80909b6a90749de1e4cc403", size = 370827, upload-time = "2026-05-26T16:27:03.827Z" }, +] + [[package]] name = "click" version = "8.3.3" @@ -2252,6 +2265,7 @@ snowflake = [ [package.dev-dependencies] dev = [ { name = "build" }, + { name = "clauditor-eval" }, { name = "dbt-core" }, { name = "fakesnow" }, { name = "google-genai" }, @@ -2303,6 +2317,7 @@ provides-extras = ["dev", "gemini", "openai", "snowflake"] [package.metadata.requires-dev] dev = [ { name = "build", specifier = ">=1.2,<2" }, + { name = "clauditor-eval", specifier = ">=0.1,<1" }, { name = "dbt-core", specifier = ">=1.8,<2" }, { name = "fakesnow", specifier = ">=0.9" }, { name = "google-genai", specifier = ">=0.5,<1" }, From bfdb40d0a5bd3e127b48aa4a109c34e8117c493a Mon Sep 17 00:00:00 2001 From: wjduenow Date: Sat, 30 May 2026 15:50:11 -0700 Subject: [PATCH 2/2] chore: release 0.5.0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cuts v0.5.0 to PyPI. Bumps version 0.4.0 → 0.5.0 and adds a new [0.5.0] CHANGELOG section honestly scoped to #141 (the SignalForge Claude Code skill + install-skill subcommand). Also bumps the SKILL.md frontmatter and assets/SKILL.eval.json version strings 0.5.0.dev0 → 0.5.0 so the shipped skill metadata matches the release. Branch shape: release/0.5.0 is based on main + cherry-pick of 709f147 (#141's squash-merge commit on dev). Main's content-equivalence to dev means the cherry-pick reproduces dev's tree modulo this commit; no intervening dev work is lost. Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 16 +++++++++++++++- src/signalforge/__init__.py | 2 +- src/signalforge/skills/signalforge/SKILL.md | 2 +- .../skills/signalforge/assets/SKILL.eval.json | 2 +- 4 files changed, 18 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9f6c0e5a..1a7f4170 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,19 @@ All notable changes to SignalForge are documented here. The format is loosely ba _Nothing yet — entries land here on `dev` and get promoted to a dated section at release time._ +## [0.5.0] — 2026-05-30 + +### Added + +- **SignalForge skill for Claude Code + `install-skill` subcommand (#141).** Ships a bundled Claude Code skill at `src/signalforge/skills/signalforge/SKILL.md` that teaches Claude to drive the SignalForge CLI against a user's dbt project — pointing at a project, running the zero-cred demo, drafting + pruning real models, reading diffs, and the gated live e2e flow. New `signalforge install-skill []` subcommand copies the bundled skill out of the wheel into `/.claude/skills/signalforge/`; library seam at `signalforge.skill.install_skill(...)` with a three-class typed-error hierarchy (`SkillDestPathError` tier 1, `SkillDestUnsafeError` tier 2, `SkillPackageDataMissingError` tier 1) mirroring `signalforge.demo.copy_demo` verbatim for symlink-cycle defence. Symlink protection covers every bundled path (SKILL.md AND every `assets/` file), not just the top-level destination — a symlinked ancestor directory or sibling asset cannot smuggle writes through copytree. +- **SKILL ↔ CLI parity gate (`tests/cli/test_skill_cli_parity.py`).** The bundled skill is the project's **6th parity surface** (`.claude/rules/skill-parity.md`). The gate parses the live argparse subparser registry, four canonical demo command lines, and `signalforge --` patterns from the SKILL body, then asserts every token appears verbatim in `SKILL.md`. Runs inside the canonical `uv run pytest` so a CLI change that drifts from the skill fails validation until the skill is updated in the same commit — gate-over-prompt, not a reviewer-discretion item. Planted-violation self-check per `.claude/rules/testing-signal.md` § AST source-scan gates. +- **`docs/skills.md` documentation + README skill pointer.** New MkDocs page documenting the bundled skill, the `install-skill` subcommand, the two demo paths (zero-cred default + opt-in live e2e), the parity gate, and the maintainer-only-skill wheel exclusion. README Quick-start gains a one-sentence pointer. + +### Fixed + +- **`install-skill` symlink defence extended to every bundled path (#141 CodeRabbit/Copilot).** The pre-review version protected only `/.claude/skills/signalforge/SKILL.md`; a symlinked `assets/SKILL.eval.json` (or symlinked `assets/` directory) would have smuggled writes through copytree. The seam now enumerates every relative path under the bundled source tree via `rglob` and refuses to overwrite any of them through a symlink, with `mkdir(parents=True)` wrapped to raise `SkillDestUnsafeError` (not raw `OSError`) when a non-directory component sits along the install chain (e.g. `/.claude` is a regular file). +- **`install-skill` `existed_before` probe now catches broken symlinks (#141 CodeRabbit).** The DEC-017 stdout contract is "True for files and symlinks (both shapes are replaced from the operator's POV)." `.exists()` alone follows symlinks AND returns False for broken symlinks; probe now ORs `.is_symlink()` so a broken-symlink destination is honestly reported as "replaced" (even though the lib seam then refuses to write through it). + ## [0.4.0] — 2026-05-30 ### Added @@ -90,7 +103,8 @@ signalforge --version - OSS-first, Core-friendly — no dbt Cloud dependency; runs against any dbt-core project, locally or in CI. - Explainable diffs — every kept/dropped/flagged artifact ships with a one-line "why"; every run produces a sidecar JSON with reproducibility hashes. -[Unreleased]: https://github.com/wjduenow/SignalForge/compare/v0.4.0...HEAD +[Unreleased]: https://github.com/wjduenow/SignalForge/compare/v0.5.0...HEAD +[0.5.0]: https://github.com/wjduenow/SignalForge/releases/tag/v0.5.0 [0.4.0]: https://github.com/wjduenow/SignalForge/releases/tag/v0.4.0 [0.3.0]: https://github.com/wjduenow/SignalForge/releases/tag/v0.3.0 [0.2.0]: https://github.com/wjduenow/SignalForge/releases/tag/v0.2.0 diff --git a/src/signalforge/__init__.py b/src/signalforge/__init__.py index b7f609a5..66b0e5a1 100644 --- a/src/signalforge/__init__.py +++ b/src/signalforge/__init__.py @@ -1,3 +1,3 @@ """SignalForge: LLM-drafted, warehouse-pruned dbt artifacts.""" -__version__ = "0.4.0" +__version__ = "0.5.0" diff --git a/src/signalforge/skills/signalforge/SKILL.md b/src/signalforge/skills/signalforge/SKILL.md index 7a662f6d..83600d60 100644 --- a/src/signalforge/skills/signalforge/SKILL.md +++ b/src/signalforge/skills/signalforge/SKILL.md @@ -3,7 +3,7 @@ name: signalforge description: Use when the user wants to draft, prune, or grade dbt tests / docs with an LLM, has a dbt project (manifest.json + sql models), or asks about SignalForge. Drives the `signalforge` CLI end-to-end: drafts candidate tests, runs them against warehouse samples, drops the noise, and explains every kept/dropped artifact. compatibility: "Requires: signalforge installed (pip install signalforge-dbt) + ANTHROPIC_API_KEY (the drafter always calls Anthropic). The `signalforge lint` and `signalforge install-skill` paths are fully offline. The bundled demo (`init-demo` + `generate`) reads a public BigQuery dataset, so it needs ADC (`gcloud auth application-default login`) + `GOOGLE_CLOUD_PROJECT` for billing — no proprietary warehouse setup of your own, but not credential-free. For real dbt projects: dbt-core + a populated manifest.json + your warehouse profile. For live e2e: BigQuery v0.1." metadata: - signalforge-version: "0.5.0.dev0" + signalforge-version: "0.5.0" allowed-tools: Bash(signalforge *), Bash(uv run signalforge *), Bash(uv run pytest -m e2e*), Bash(cat *), Bash(ls *), Bash(grep *), Bash(head *), Bash(tail *), Read, Write, Edit --- diff --git a/src/signalforge/skills/signalforge/assets/SKILL.eval.json b/src/signalforge/skills/signalforge/assets/SKILL.eval.json index b6eb80c3..0fbf1b11 100644 --- a/src/signalforge/skills/signalforge/assets/SKILL.eval.json +++ b/src/signalforge/skills/signalforge/assets/SKILL.eval.json @@ -1,6 +1,6 @@ { "score": null, - "version": "0.5.0.dev0", + "version": "0.5.0", "graded_at": null, "status": "pending-first-grade", "skill_path": "src/signalforge/skills/signalforge/SKILL.md",