#141: SignalForge skill + install-skill - #166
Merged
Merged
Conversation
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) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughImplements GH ChangesClaude Code Skill Installation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Approved + devolved. US-001 is at the front of the ready queue. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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) <noreply@anthropic.com>
… 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) <noreply@anthropic.com>
…rors + AST scan #7 bump
… + exit-code mapping + subprocess smoke Adds 'signalforge install-skill [<dest>]' 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) <noreply@anthropic.com>
…andler + exit-code mapping + subprocess smoke
… 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) <noreply@anthropic.com>
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 <model> --write / prune-existing <model> --schema <path> / 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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
… + 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) <noreply@anthropic.com>
…+ README pointer)
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
…ure + pending badge
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 <model> --write / prune-existing <model> --schema <path> / 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 <subcommand> --<flag>` 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) <noreply@anthropic.com>
wjduenow
changed the title
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
wjduenow
marked this pull request as ready for review
May 30, 2026 18:26
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plans/super/141-claude-skill-install.md`:
- Around line 756-762: Remove the duplicate placeholder sections: delete the
second occurrences of the headings "## Refinement log" and "## Detailed
breakdown" that contain the "_Pending Phase 3._" and "_Pending Phase 4._" lines
(these are inconsistent with the document header "Phase: implemented" and with
the already-populated refinement log and detailed breakdown earlier in the
file); keep the populated sections (the earlier "Refinement log" and "Detailed
breakdown") and remove only the duplicate placeholder headers and their
placeholder text so the document reflects the implemented status.
In `@src/signalforge/cli/install_skill.py`:
- Around line 167-177: The pre-probe currently calls Path(raw_dest).expanduser()
/ _INSTALLED_SKILL_REL and uses .exists(), which follows symlinks; change the
probe so it checks the terminal path for a symlink first (use .is_symlink()) and
only calls .exists() when .is_symlink() is False, setting existed_before based
on either is_symlink() OR exists(); keep the existing try/except OSError around
the probe and assign to the same existed_before variable so the DEC-017 suffix
logic continues to work without dereferencing SKILL.md symlinks.
In `@src/signalforge/skill/__init__.py`:
- Around line 146-151: The handler that treats
FileNotFoundError/NotADirectoryError as “destination doesn’t exist yet” must
instead map cases where a non-directory ancestor exists to SkillDestUnsafeError;
update install_skill so after computing resolved_dest =
expanded_dest.resolve(strict=False) you iterate the ancestor chain (e.g. for p
in expanded_dest.parents or resolved_dest.parents) and if any ancestor.exists()
and not ancestor.is_dir() raise SkillDestUnsafeError with that ancestor/path
info, and keep the original fallback behavior only when no non-directory
ancestor is present; reference install_skill,
resolved_dest/expanded_dest.resolve, target_skill_dir.mkdir, and
SkillDestUnsafeError to locate and implement this change.
In `@src/signalforge/skills/signalforge/assets/SKILL.eval.json`:
- Line 3: The eval metadata in SKILL.eval.json still has "version": "0.4.0.dev0"
which should match the package's new version; update the "version" field in
SKILL.eval.json (the JSON key "version") to the package's current version (e.g.,
"0.5.0.dev0") so the eval metadata aligns with the released package version.
In `@src/signalforge/skills/signalforge/SKILL.md`:
- Line 16: The fenced code block that begins with the line "model.sql + manifest
+ project ctx" in SKILL.md is missing a language tag; update that fence to
include a language specifier (suggest "text") so the opening fence becomes
```text to satisfy MD040 (`fenced-code-language`) and prevent the lint warning
while preserving the block contents and formatting.
In `@tests/cli/test_skill_cli_parity.py`:
- Around line 246-253: The current _SKILL_FLAG_USAGE_RE only matches flags
immediately following the subcommand so forms like "signalforge generate <model>
--write" are missed; update the regex used by _SKILL_FLAG_USAGE_RE to allow
arbitrary (non-newline) tokens between the subcommand and the long flag while
still capturing the subcommand and the flag separately and preserving the
ASCII-only flag restriction—for example, change the pattern to match
"signalforge ([a-z][a-z0-9-]*)" then any chars up to the flag and capture
"(--[a-z][a-z0-9-]*)" (e.g. use a non-greedy [^\n]*? between the two capture
groups) so positional args are allowed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 784bb06c-62a2-4bd9-ad18-8f888df0017c
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
.claude/rules/cli-layer.md.claude/rules/skill-parity.mdREADME.mddocs/cli-ops.mddocs/skills.mdmkdocs.ymlplans/super/141-claude-skill-install.mdpyproject.tomlsrc/signalforge/__init__.pysrc/signalforge/cli/__init__.pysrc/signalforge/cli/_helpers.pysrc/signalforge/cli/errors.pysrc/signalforge/cli/install_skill.pysrc/signalforge/skill/__init__.pysrc/signalforge/skill/errors.pysrc/signalforge/skills/signalforge/SKILL.mdsrc/signalforge/skills/signalforge/assets/SKILL.eval.jsontests/cli/test_5_surface_parity_install_skill.pytests/cli/test_exit_codes.pytests/cli/test_install_skill.pytests/cli/test_skill_cli_parity.pytests/cli/test_subprocess_smoke.pytests/skill/test_install.pytests/test_audit_completeness.pytests/test_wheel_packaging.py
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) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR ships the SignalForge Claude Code skill requested by #141, bundles it in the wheel, and adds a signalforge install-skill path that mirrors the existing init-demo library/CLI/error-handling pattern.
Changes:
- Adds bundled skill package data plus wheel smoke gates and docs.
- Adds public
signalforge.skill.install_skill()with typed errors and a newinstall-skillCLI subcommand. - Adds parity/audit tests to keep the skill, CLI, docs, and packaging surfaces aligned.
Reviewed changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
uv.lock |
Locks new dev dependency for clauditor self-grading. |
pyproject.toml |
Adds skill package data to wheel include and clauditor dev dependency. |
src/signalforge/__init__.py |
Bumps package version to 0.5.0.dev0. |
src/signalforge/skill/__init__.py |
Implements public skill installer. |
src/signalforge/skill/errors.py |
Adds typed skill-install error hierarchy. |
src/signalforge/cli/install_skill.py |
Adds CLI handler and parser for install-skill. |
src/signalforge/cli/errors.py |
Adds CLI wrapper errors for install-skill failures. |
src/signalforge/cli/_helpers.py |
Registers new skill errors in exit-code mapping. |
src/signalforge/cli/__init__.py |
Registers the new subcommand. |
src/signalforge/skills/signalforge/SKILL.md |
Adds the shipped Claude Code skill body. |
src/signalforge/skills/signalforge/assets/SKILL.eval.json |
Adds placeholder self-grade sidecar. |
tests/skill/test_install.py |
Adds library installer tests. |
tests/cli/test_install_skill.py |
Adds CLI behavior and exit-code tests. |
tests/cli/test_subprocess_smoke.py |
Adds subprocess help smoke for install-skill. |
tests/cli/test_skill_cli_parity.py |
Adds SKILL ↔ CLI parity gate. |
tests/cli/test_5_surface_parity_install_skill.py |
Adds install-skill multi-surface parity test. |
tests/cli/test_exit_codes.py |
Extends synthetic constructor coverage for new CLI errors. |
tests/test_audit_completeness.py |
Extends error-module audit scan for signalforge.skill. |
tests/test_wheel_packaging.py |
Adds positive/negative wheel assertions for bundled skills. |
README.md |
Adds skill install pointer and self-grade badge. |
docs/skills.md |
Adds skill catalog and operating model docs. |
docs/cli-ops.md |
Documents install-skill behavior and exits. |
mkdocs.yml |
Adds skill docs to navigation. |
plans/super/141-claude-skill-install.md |
Adds implementation plan and decisions. |
.claude/rules/skill-parity.md |
Adds contributor rule for skill parity. |
.claude/rules/cli-layer.md |
Adds bundled skill as a sixth CLI parity surface. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/cli/test_install_skill.py (1)
432-438: ⚡ Quick winAssert the install still happened after the probe failure.
This regression test proves the
exists()OSErrorno longer aborts the command, but it would still pass if the handler returned0without actually callinginstall_skill. Add a direct assertion thattmp_path / _INSTALLED_RELexists (and ideally that stdout still contains the normal install message) to pin the intended behavior end to end.Suggested tightening
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}" + assert (tmp_path / _INSTALLED_REL).is_file() + assert out.startswith("Installed SignalForge skill to ") # 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🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/cli/test_install_skill.py` around lines 432 - 438, The test currently only asserts exit code and absence of error, but doesn't verify the install actually created files; update the test around the main(["install-skill", str(tmp_path)]) invocation to assert the installed artifact exists by checking (tmp_path / _INSTALLED_REL).exists() and also assert that stdout contains the normal install success message (e.g. the message produced by install_skill) to ensure install_skill was invoked; reference main, install-skill, tmp_path, _INSTALLED_REL and install_skill when adding these assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/cli/test_install_skill.py`:
- Around line 432-438: The test currently only asserts exit code and absence of
error, but doesn't verify the install actually created files; update the test
around the main(["install-skill", str(tmp_path)]) invocation to assert the
installed artifact exists by checking (tmp_path / _INSTALLED_REL).exists() and
also assert that stdout contains the normal install success message (e.g. the
message produced by install_skill) to ensure install_skill was invoked;
reference main, install-skill, tmp_path, _INSTALLED_REL and install_skill when
adding these assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1d0741cc-2826-47f1-ab92-4e069c558a1f
📒 Files selected for processing (2)
tests/cli/test_install_skill.pytests/skill/test_install.py
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 `<dest>/.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. `<dest>/.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 <name>` 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 <subcommand> [<positional> ...] --<flag>` so canonical shapes like `generate <model> --write` and `prune-existing <model> --schema <path>` 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 <model> --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) <noreply@anthropic.com>
Owner
Author
PR Review Summary (CodeRabbit + Copilot, round 1)All 13 unresolved threads addressed in commit Fixed (13 items)
Verification
False positivesNone — every finding was a real bug or a documented contradiction. 🤖 Resolved with Claude Code |
This was referenced May 30, 2026
Closed
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Super-plan for #141 — ship a SignalForge Claude Code skill (
src/signalforge/skills/signalforge/SKILL.md) bundled in the wheel, plus asignalforge install-skillCLI subcommand that drops it into a user's project at.claude/skills/signalforge/.Phase: detailing (awaiting approval)
Stories: 9 implementation + Quality Gate + Patterns & Memory = 11
Decisions: 24 captured (DEC-001 … DEC-024)
What's locked
init-demoverbatim — public lib seam atsignalforge.skill.install_skill(...)with three typed errors (SkillDestPathError/SkillDestUnsafeError/SkillPackageDataMissingError); thin CLI handler wraps toCliInstallSkill*per cli-layer.md's library-surface pattern..claude/skills/signalforge/. No--forceflag in v0.1.copy_demo's three-trap pattern (RuntimeError ≤3.12 + OSError(ELOOP) ≥3.13 +is_symlink()refuse-on-overwrite).init-demo→generate) is always-on; livepytest -m e2eis opt-in behind explicit confirmation + env-var check + cost warning.src/signalforge/skills/(plural, matches install destination); Python lib atsrc/signalforge/skill/(singular, matchessignalforge.demo).signalforge/skill/errors.pyautomatically (bump 12→13); NEWtests/cli/test_skill_cli_parity.pyscans the live argparse subparser registry + key demo commands + the install-skill bootstrap line against SKILL.md; 5-surface parity for the subcommand itself.clauditor graderun; pin score inassets/SKILL.eval.json; shields.io badge from pinned endpoint. No CI integration; no Anthropic key in repo secrets.Story map
src/signalforge/skills/signalforge/tree + wheel packaging + wheel_smoke extensionsignalforge.skilllib + typed errors + AST scan Quality grader: rubric scoring of surviving artifacts #7 bump (TDD)install-skillsubcommand + exit-code mapping + subprocess--helpsmoke (TDD)install-skilldocs/skills.md+ mkdocs.yml nav +docs/cli-ops.md+ README pointer).claude/rules/skill-parity.md+cli-layer.mdparity-surface update (orchestrator-only writes)Plan document
See
plans/super/141-claude-skill-install.mdfor the full 24-DEC plan with risks and non-goals.Next steps
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests
Chores