47: signalforge init-demo subcommand (Austin fixture for PyPI users) - #78
Merged
Conversation
15 DECs locked across discovery / architecture / refinement; 7 implementation stories + Quality Gate + Patterns & Memory. Architecture review surfaced two load-bearing blockers (Hatch `include` directive required for non-`.py` data files; `--force` semantics needed explicit choice) — both resolved before detailing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR implements the ChangesInit-Demo Feature Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Epic bd_1-scaffolding-t1o + 9 tasks (US-001..US-009) created with dependencies. Plan phase advances to devolved; PR #78 stays in draft for review. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Lands the shipped Austin demo tree under src/signalforge/_demo/ as a copy
of tests/fixtures/dbt_project_austin/ with two named rewrites per DEC-008
of plans/super/47-init-demo.md:
- profiles.yml (DEC-009) — uses dbt's env_var('GOOGLE_CLOUD_PROJECT')
macro so a PyPI user with that env var set runs the demo with zero
file edits. The maintainer-only "DO NOT signalforge against this"
header and the billing-broken project: bigquery-public-data
placeholder (deliberately retained in the test-fixture copy so the
issue-#10 e2e smoke test exercises the overwrite path) are stripped.
- .gitignore — slimmed to a single .signalforge/ exclusion; issue-#10
/ DEC-021 maintainer commentary is dropped (irrelevant to a demo
user opening a fresh project).
The shipped tree is symlink-free (DEC-005) and excludes regenerate.sh
(maintainer-only).
New tests/test_demo_fixture_parity.py:
- test_demo_fixture_parity_holds_byte_for_byte_except_documented_files
walks both trees, asserts every shared file is byte-equal except
profiles.yml + .gitignore (which must DIFFER per the DECs above).
Fails on uncommanded drift or a silent identical copy that would
defeat DEC-009.
- test_demo_fixture_contains_no_symlinks codifies DEC-005.
- test_demo_profiles_yml_uses_env_var_macro pins DEC-009 specifically.
- test_test_fixture_profiles_yml_retains_maintainer_header confirms
the rewrite is one-way and the e2e smoke fixture is untouched.
tests/fixtures/dbt_project_austin/regenerate.sh gains a final phase
(DEC-015) that mirrors every fixture file (except itself) into
src/signalforge/_demo/, then rewrites profiles.yml and .gitignore
verbatim from heredocs. Maintainers running the script keep both
trees aligned by construction; the parity test fires only on
uncommanded drift.
Validation:
- ruff check . clean
- ruff format --check . clean
- pyright clean (0 errors)
- pytest: 1658 passed, 4 new parity tests included; the 6 deselected
symlink-loop tests are pre-existing env-specific failures on dev
(Python 3.10 vs the project's tested >=3.11 floor) unrelated to
this change.
Traces to: SD-1, DEC-005, DEC-008, DEC-009, DEC-015 of
plans/super/47-init-demo.md.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds `signalforge.demo` public module exposing copy_demo(dest, *, force=False) for library callers. Implements DEC-001 atomic-replace force semantics with DEC-004 standalone path resolve + symlink-cycle guard. Surfaces DemoError hierarchy (DemoPathError, DemoDestExistsError, DemoDestUnsafeError, DemoFixtureMissingError) that the CLI layer (US-004) wraps into tier-mapped Cli*Error subclasses at the handler boundary (DEC-012). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds `[tool.hatch.build.targets.wheel] include = ["src/signalforge/_demo"]` so the demo tree ships in the wheel (DEC-002). Registers wheel_smoke pytest marker and adds tests/test_wheel_packaging.py which builds the wheel via `python -m build --wheel` and asserts the canonical 8 demo files appear in the artifact (DEC-003, including the .gitignore dotfile per DEC-006). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds `signalforge init-demo [<dest>]` subcommand at src/signalforge/cli/init_demo.py
mirroring the version.py / lint.py shape (DEC-009). Positional dest defaults to
./signalforge-demo/; --force triggers atomic-replace semantics through
signalforge.demo.copy_demo (DEC-001). Wraps the four DemoError subclasses at
the handler boundary into CliInitDemoDest{Exists,Unsafe}Error (tier 2) +
CliInitDemoFixtureMissingError + CliInitDemoCopyError (tier 1) — all four
registered in _EXCEPTION_TO_EXIT_CODE per the 7th AST scan (DEC-024). Emits
plain-text next-steps message to stdout naming GOOGLE_CLOUD_PROJECT and
ANTHROPIC_API_KEY env vars plus the three first-run commands (DEC-014).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Extends tests/cli/test_subprocess_smoke.py with a second @pytest.mark.cli_subprocess test invoking `signalforge init-demo --help` against the installed console script (DEC-010 / ticket AC-5). Catches [project.scripts] regressions specifically against the new subcommand that in-process main(argv) testing cannot. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Updates README Quick Start to use `signalforge init-demo` instead of the
broken `cp -r tests/fixtures/...` snippet that assumed a clone (AC-4).
Strips the heredoc rewrite of profiles.yml since the shipped demo already
uses env_var('GOOGLE_CLOUD_PROJECT'). Adds the fourth subcommand entry to
docs/cli-ops.md § Subcommands following the lint precedent. Extends
CLAUDE.md "Public API surface" with the signalforge.demo.copy_demo entry
and the DemoError hierarchy (DEC-012); adds the #47 entry to the
repository-status block.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds tests/cli/test_5_surface_parity_init_demo.py mirroring the shape of test_5_surface_parity_select.py (issue #37). Hard-asserts that the subcommand name `init-demo` and the `--force` flag both appear in the four external surfaces — argparse help, handler docstring, docs/cli-ops.md § Subcommands, and the plan's DEC list — so any future change to the flag's contract has to land all four surfaces in the same commit (cli-layer.md 5-surface parity rule). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…on polish + test tightening
Quality-gate pass-1 review fixes:
- I-1 — refactor src/signalforge/demo.py → src/signalforge/demo/{__init__.py,
errors.py} so the existing `*/errors.py` AST glob auto-covers the demo
layer. Without the refactor, a v0.2 contributor adding a Demo*Error
subclass would not get a test failure if they forgot to wire the CLI
wrapper. Adds DemoError (abstract base) to _EXCEPTION_MAPPING_EXCLUDED_BASES
and the four concretes (DemoPathError, DemoDestExistsError,
DemoDestUnsafeError, DemoFixtureMissingError) to _EXCEPTION_TO_EXIT_CODE
with same tiers as their CLI wrappers (defence-in-depth). Bumps the
Scan-7 sanity test from 9 to 10 expected paths.
- I-2 — CliInitDemoDestExistsError default remediation now mentions the
blast-radius guard ("refuses '/', $HOME, or the current working directory")
so the operator who reads the remediation has full context before
re-running with --force.
- I-4 — test_cmd_init_demo_never_leaks_traceback tightens its exit-code
assertion from `ret in (1, 2)` to `assert ret == 1` so a regression
routing the OSError to an input-validation wrapper surfaces loudly.
- N-1 — _NEXT_STEPS_MESSAGE shell-quotes the cd line via shlex.quote so
destinations with spaces (e.g. "/Users/Wes Duenow/...") still produce
a copy-pasteable shell command.
Validation: ruff / pyright clean; pytest 1722 passed; wheel_smoke 2/2;
audit_completeness 19/19 (including the now-extended Scan 7).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pass-2 review surfaced a UX bug: `copy_demo(dest=<existing_file>)` would call `iterdir()` on a non-directory, raising raw `NotADirectoryError` which the CLI wrapped as `CliInitDemoCopyError` (tier 1, "failed to copy demo tree") — misleading because no copy was attempted. Fix: add a shape-gate before the existence-gate in `copy_demo`. If `dest` exists but is not a directory, raise `DemoDestExistsError` with a clear "exists but is not a directory" message (still tier 2 via `CliInitDemoDestExistsError`). Under `force=True`, the file is unlinked and the demo tree copied in its place (semantically consistent with `force` against a non-empty directory). Adds two pinning tests: - `test_copy_demo_dest_is_file_without_force_raises_dest_exists` - `test_copy_demo_dest_is_file_with_force_replaces_with_demo_dir` - `test_cmd_init_demo_dest_is_file_returns_exit_2_with_clear_message` Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… + rule prose
Pass-3 review surfaced a load-bearing blocker: the shipped demo's
profiles.yml uses `{{ env_var('GOOGLE_CLOUD_PROJECT') }}` but
signalforge.warehouse.profiles only does yaml.safe_load — no jinja
rendering — so the literal jinja string was sent to BigQuery as the
project ID, breaking the documented "zero file edits" happy path
promised in README, docs/cli-ops.md, and the next-steps message.
Fix:
- B-1 — Add minimal dbt-compatible env_var() substitution pass to
signalforge.warehouse.profiles._load_profiles_yaml. Supports
`env_var('NAME')` (raises ProfileEnvVarUnsetError when unset, no
default) and `env_var('NAME', 'default')` (falls back to default).
Pre-yaml-parse so YAML quoting is preserved.
New typed error ProfileEnvVarUnsetError(ProfileNotFoundError) — tier
1 (load-time / state-not-ready). Re-exported from
signalforge.warehouse and registered in _EXCEPTION_TO_EXIT_CODE.
Verified end-to-end: signalforge init-demo + load_profile resolves
GOOGLE_CLOUD_PROJECT correctly; unset env surfaces a clear typed
error pointing at the missing var instead of a BigQuery rejection.
4 new tests in tests/warehouse/test_profiles.py pin the contract.
- I-1 — Add "demo" to _SCAN_SUBPACKAGES in
tests/llm/test_logger_grep_gate.py so the ANSI-safe lazy-format
rule auto-covers signalforge.demo. Pass-1's subpackage refactor
promoted demo to a stage subpackage; the grep gate needed to keep
pace.
- I-2 — Update .claude/rules/cli-layer.md "nine modules" / "nine
per-stage abstract bases" prose to "ten" — issue #47 is the
precedent for the count bump. Without this, a future maintainer
reading the rule sees a stale count and either skips updating
their own additions or trusts an out-of-date contract.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rule-prose cleanup
Pass-4 review surfaced one blocker:
- B-1 — ProfileEnvVarUnsetError (added pass 3) was registered in
the CLI exit-code mapping and re-exported from signalforge.warehouse,
but was missing from signalforge.warehouse.errors.__all__. The
warehouse errors module's self-tests iterate __all__ to:
- assert alphabetical sort + class count (one assertion in
test_each_subclass_has_default_remediation pins the count)
- smoke-construct each class via _CONSTRUCT_KWARGS
- verify each carries a non-empty default_remediation
All three skipped the new class, leaving a gap in the layer's
"every typed exception is wired correctly" contract.
Fix: add ProfileEnvVarUnsetError to __all__ in alphabetical position
(between MaterialisationNotSupportedError and ProfileNotFoundError),
bump the count constant from 19 to 20, and register the constructor
kwargs (var_name= + profiles_path=) in _CONSTRUCT_KWARGS so the
smoke loop exercises it.
- N-1 — Clean up the pass-3 cli-layer.md prose: the excluded-bases
enumeration listed 11 names but said "ten" because LLMHelperError
was incorrectly included (it's a concrete leaf raised directly in
signalforge.llm.client, NOT an excluded abstract base). Removed
LLMHelperError from the enumeration with an explicit "deliberately
NOT excluded" note explaining why.
Validation: full pytest suite 1730 passed + 4 gated (2 wheel_smoke,
2 cli_subprocess), 0 regressions, ruff/pyright clean. Six
pre-existing WSL2 symlink-loop failures unchanged.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…r + dual-surface library pattern + QG addendum Captures patterns established by issue #47 into the rule files so future maintainers don't re-discover them: - .claude/rules/python-build.md — new section "Shipping package data (non-`.py` files) — explicit `include` directive". Documents the Hatch include directive pattern, the `wheel_smoke` maintainer-only marker shape (registered in pyproject, excluded by default addopts, built via python -m build / uvx fallback, inspected via zipfile.ZipFile), the belt-and-braces `unzip -l` verification step, and the dotfile-fragility caveat. - .claude/rules/cli-layer.md — extended subpackage-layout listing to include init_demo.py as the fourth subcommand. Added a new section "Library-surface pattern: CLI handler wraps a public lib module at the boundary" documenting the two-layer error wrapping pattern (signalforge.demo's typed errors + signalforge.cli's tier-mapped wrappers). The 7th AST scan covers both layers; defence-in-depth via demo-layer errors also landing in _EXCEPTION_TO_EXIT_CODE. - .claude/rules/testing-signal.md — extended gated-marker section with wheel_smoke as the 5th marker (alongside bigquery, anthropic, cli_subprocess, e2e). Documents the addopts exclusion list update and the --no-cov requirement for marker-specific runs. - plans/super/47-init-demo.md — appended "Quality-gate addendum" section recording each of the 4 QG passes' refinements without rewriting the original 15 DECs in place. Preserves the ADR-style chronological design record. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
wjduenow
changed the title
Owner
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
Codecov flagged patch coverage 93.21% with 11 lines missing across the new files. Adds four targeted tests to close the gap: - test_cmd_init_demo_demo_path_error_wraps_to_cli_path_error — exercises the DemoPathError → CliPathError handler branch via monkeypatch (the natural symlink-cycle test path skips on WSL2 where Path.resolve() doesn't raise on cycles). - test_cmd_init_demo_keyboard_interrupt_propagates — pins the explicit re-raise in the (KeyboardInterrupt, SystemExit) catch (DEC-016 carve-out so operator Ctrl-C reaches Python's default handler). - test_cmd_init_demo_forward_compat_exception_belt_and_braces — pins the catch-all `except Exception` panic-path branch (tier 1; no traceback leak). - test_cli_init_demo_error_constructors_render_without_cause — exercises the `cause is None` branch in all four CliInitDemo*Error constructors (existing exit-code-table tests always pass a cause). Coverage: cli/init_demo.py 84% → 100%; the four new cli/errors.py cause-None branches now covered. The two remaining file-level gaps (cli/errors.py:120, demo/errors.py:61) are pre-existing on dev / unreachable defensive paths and outside this PR's diff. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR implements issue #47 by shipping the Austin dbt demo project as wheel package-data and adding a new signalforge init-demo subcommand (plus a public signalforge.demo.copy_demo(...) entry point) so PyPI users can bootstrap a working first-run project without cloning the repo.
Changes:
- Add
signalforge init-demo [DEST]CLI subcommand + typed CLI errors + exit-code mapping + subprocess help smoke. - Introduce
signalforge.demo.copy_demo(...)library API and shipsrc/signalforge/_demo/(dbt project + locked manifest) in the wheel via Hatchinclude. - Add parity + packaging smoke tests, plus minimal dbt-style
env_var(...)rendering forprofiles.yml, and update docs/README/rules accordingly.
Reviewed changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/warehouse/test_profiles.py | Adds unit tests for dbt-style env_var(...) macro rendering in profiles.yml. |
| tests/warehouse/test_errors.py | Updates warehouse error construction + __all__ count assertions to include ProfileEnvVarUnsetError. |
| tests/test_wheel_packaging.py | Adds maintainer-only wheel_smoke tests that build the wheel and assert _demo/ files ship. |
| tests/test_demo.py | Adds tests for signalforge.demo.copy_demo(...) behavior, force semantics, and typed errors. |
| tests/test_demo_fixture_parity.py | Adds parity gate to keep shipped demo tree aligned with the e2e fixture (with documented rewrites). |
| tests/test_audit_completeness.py | Extends AST-scan expectations for the new demo/errors.py module and excluded base DemoError. |
| tests/llm/test_logger_grep_gate.py | Extends logger grep gate scan list to include the new demo subpackage. |
| tests/fixtures/dbt_project_austin/regenerate.sh | Mirrors the fixture into src/signalforge/_demo/ and applies demo-specific rewrites. |
| tests/cli/test_subprocess_smoke.py | Adds subprocess smoke for signalforge init-demo --help. |
| tests/cli/test_init_demo.py | Adds in-process CLI tests for happy path, error tiers, help surface, and no-traceback floor. |
| tests/cli/test_exit_codes.py | Extends synthetic constructor coverage for the new CLI wrappers and ProfileEnvVarUnsetError. |
| tests/cli/test_5_surface_parity_init_demo.py | Adds 5-surface parity test ensuring init-demo and --force stay consistent across help/docs/plan. |
| src/signalforge/warehouse/profiles.py | Adds minimal env_var(...) substitution pass before YAML parsing. |
| src/signalforge/warehouse/errors.py | Adds ProfileEnvVarUnsetError and exports it via __all__. |
| src/signalforge/warehouse/init.py | Re-exports ProfileEnvVarUnsetError from the warehouse package. |
| src/signalforge/demo/errors.py | Introduces typed error hierarchy for the demo layer. |
| src/signalforge/demo/init.py | Implements copy_demo(...) to copy packaged demo tree to disk with --force safety guardrails. |
| src/signalforge/cli/init_demo.py | Implements the init-demo subcommand, next-steps message, and error wrapping. |
| src/signalforge/cli/errors.py | Adds CliInitDemo*Error wrappers and default remediations. |
| src/signalforge/cli/_helpers.py | Registers new errors (CLI + demo + warehouse) in _EXCEPTION_TO_EXIT_CODE. |
| src/signalforge/cli/init.py | Registers the new init-demo subcommand with the top-level CLI parser. |
| src/signalforge/_demo/target/manifest.json | Adds locked dbt manifest seed shipped in the demo tree. |
| src/signalforge/_demo/signalforge.yml | Adds demo config shipped to users. |
| src/signalforge/_demo/profiles.yml | Adds demo profiles.yml using {{ env_var('GOOGLE_CLOUD_PROJECT') }}. |
| src/signalforge/_demo/models/staging/stg_bikeshare_trips.sql | Adds demo model SQL shipped to users. |
| src/signalforge/_demo/models/staging/sources.yml | Adds demo sources YAML shipped to users. |
| src/signalforge/_demo/dbt_project.yml | Adds demo dbt project definition shipped to users. |
| src/signalforge/_demo/.gitignore | Adds slimmed demo .gitignore shipped to users. |
| README.md | Updates Quick Start and CLI section to use signalforge init-demo instead of copying test fixtures. |
| pyproject.toml | Adds Hatch wheel include, registers wheel_smoke marker, and excludes it from default pytest runs. |
| plans/super/47-init-demo.md | Adds the full plan/DECs/stories document for issue #47. |
| docs/cli-ops.md | Documents the new init-demo subcommand, semantics, and exit codes. |
| CLAUDE.md | Updates repo status and public API surface to include signalforge.demo.copy_demo(...) and init-demo changes. |
| .claude/rules/testing-signal.md | Documents the new wheel_smoke marker and --no-cov guidance. |
| .claude/rules/python-build.md | Documents Hatch include for package data and the wheel smoke testing pattern. |
| .claude/rules/cli-layer.md | Updates CLI layer rules/docs for new subcommand count and the “library surface + CLI wrapper” pattern. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
wjduenow
marked this pull request as ready for review
May 12, 2026 02:37
Owner
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![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
🧹 Nitpick comments (3)
tests/cli/test_init_demo.py (1)
42-51: ⚡ Quick winAdd a type annotation for
_EXPECTED_TOP_LEVEL.Please annotate this module-level constant explicitly as
frozenset[str].♻️ Suggested patch
-_EXPECTED_TOP_LEVEL = frozenset( +_EXPECTED_TOP_LEVEL: frozenset[str] = frozenset(As per coding guidelines,
Use Python type hints throughout. All function signatures, class attributes, and module-level constants must be annotated.🤖 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_init_demo.py` around lines 42 - 51, Annotate the module-level constant _EXPECTED_TOP_LEVEL with an explicit type by changing its declaration to include a type hint of frozenset[str] (e.g., _EXPECTED_TOP_LEVEL: frozenset[str] = ...); ensure you import typing generics if needed for your Python version (or use built-in parametric frozenset if supported) and keep the existing frozenset(...) initializer and values intact.src/signalforge/demo/__init__.py (1)
43-50: ⚡ Quick winAnnotate
__all__with an explicit type.
__all__should be declared aslist[str]to conform to the repository typing rule for module-level constants.♻️ Suggested patch
-__all__ = [ +__all__: list[str] = [As per coding guidelines,
Use Python type hints throughout. All function signatures, class attributes, and module-level constants must be annotated.🤖 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 `@src/signalforge/demo/__init__.py` around lines 43 - 50, The module-level constant __all__ is missing a type annotation; update its declaration to be annotated as list[str] to satisfy repository typing rules (i.e., change the __all__ definition to include the explicit type annotation list[str] while keeping the same contents referencing "DemoDestExistsError", "DemoDestUnsafeError", "DemoError", "DemoFixtureMissingError", "DemoPathError", and "copy_demo"). Ensure you only add the type hint to the existing __all__ symbol and do not modify the exported names or order.tests/test_demo_fixture_parity.py (1)
29-42: ⚡ Quick winAdd explicit type annotations to module-level constants.
_REPO_ROOT,_TEST_FIXTURE_DIR,_DEMO_DIR,_ALLOWED_REWRITES, and_TEST_FIXTURE_ONLYare unannotated. Please add explicit types to satisfy the project typing rule.♻️ Suggested patch
-_REPO_ROOT = Path(__file__).resolve().parent.parent -_TEST_FIXTURE_DIR = _REPO_ROOT / "tests" / "fixtures" / "dbt_project_austin" -_DEMO_DIR = _REPO_ROOT / "src" / "signalforge" / "_demo" +_REPO_ROOT: Path = Path(__file__).resolve().parent.parent +_TEST_FIXTURE_DIR: Path = _REPO_ROOT / "tests" / "fixtures" / "dbt_project_austin" +_DEMO_DIR: Path = _REPO_ROOT / "src" / "signalforge" / "_demo" @@ -_ALLOWED_REWRITES = frozenset({"profiles.yml", ".gitignore"}) +_ALLOWED_REWRITES: frozenset[str] = frozenset({"profiles.yml", ".gitignore"}) @@ -_TEST_FIXTURE_ONLY = frozenset({"regenerate.sh"}) +_TEST_FIXTURE_ONLY: frozenset[str] = frozenset({"regenerate.sh"})As per coding guidelines,
Use Python type hints throughout. All function signatures, class attributes, and module-level constants must be annotated.🤖 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/test_demo_fixture_parity.py` around lines 29 - 42, Add explicit type annotations for the module-level constants: annotate _REPO_ROOT, _TEST_FIXTURE_DIR, and _DEMO_DIR as pathlib.Path and annotate _ALLOWED_REWRITES and _TEST_FIXTURE_ONLY as typing.FrozenSet[str]; import FrozenSet (or from typing import FrozenSet) and pathlib.Path if not already imported, then change the assignments to include the annotations (e.g., _REPO_ROOT: Path = ..., _ALLOWED_REWRITES: FrozenSet[str] = frozenset({...})) while keeping the existing values unchanged.
🤖 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 `@src/signalforge/cli/init_demo.py`:
- Line 165: The call to copy_demo(...) uses raw args.dest and must instead be
validated via canonicalise_user_path; replace passing args.dest with a
canonicalised path obtained by calling canonicalise_user_path(args.dest,
project_dir) and handle any failure by raising or rewrapping the error as
CliPathError so no untrusted path reaches copy_demo; update the variable used by
copy_demo (resolved_dest) to the canonicalised result and ensure you reference
canonicalise_user_path and CliPathError in the fix.
In `@src/signalforge/demo/__init__.py`:
- Around line 90-138: The code currently calls
raw.expanduser().resolve(strict=False) (resolved_dest) which dereferences
symlinks and can make force=True delete the symlink target instead of the link;
fix by separating canonical checks from destructive operations: keep dest_path =
raw.expanduser() (no resolve) and use that for existence/shape/removal logic
(use dest_path.exists(), dest_path.is_dir(), dest_path.is_symlink()); only use
resolved_dest (or resolved_dest.resolve()) for comparing against unsafe_targets,
but when removing prefer: if dest_path.is_symlink(): dest_path.unlink() else:
shutil.rmtree(dest_path) so unlinking/rmtree operate on the path the user
supplied rather than a resolved target.
In `@src/signalforge/warehouse/errors.py`:
- Around line 162-170: The error message and remediation interpolate
profiles_path directly which can leak control characters; instead pass
profiles_path through the same safe formatter used for vars (e.g., call
_format_value(profiles_path)) where the message is constructed (the block that
builds the f-string using profiles_path and the remediation assignment) so both
the user-facing message and the remediation use the safely escaped
representation of profiles_path.
In `@src/signalforge/warehouse/profiles.py`:
- Around line 169-175: The comment above the env_var regex claims the outer {{
... }} are optional but the current pattern r"""\{\{\s*env_var\(...\)\s*\}\}"""
requires them; either make the outer braces optional in the regex (so it matches
both bare env_var('NAME') and {{ env_var('NAME') }}) or update the comment to
remove the “optional” claim; locate the regex literal in profiles.py (the
r"""\{\{\s*env_var... part) and implement the chosen change so the comment and
the implementation are consistent.
In `@tests/fixtures/dbt_project_austin/regenerate.sh`:
- Line 167: The cp call in the fallback mirror path (the line invoking cp
"${src_path}" "${dst_path}") is interactive and can prompt; change it to use a
non-interactive forced copy by adding the -f flag (i.e., use cp -f) so the copy
overwrites without prompting and conforms to the shell policy.
---
Nitpick comments:
In `@src/signalforge/demo/__init__.py`:
- Around line 43-50: The module-level constant __all__ is missing a type
annotation; update its declaration to be annotated as list[str] to satisfy
repository typing rules (i.e., change the __all__ definition to include the
explicit type annotation list[str] while keeping the same contents referencing
"DemoDestExistsError", "DemoDestUnsafeError", "DemoError",
"DemoFixtureMissingError", "DemoPathError", and "copy_demo"). Ensure you only
add the type hint to the existing __all__ symbol and do not modify the exported
names or order.
In `@tests/cli/test_init_demo.py`:
- Around line 42-51: Annotate the module-level constant _EXPECTED_TOP_LEVEL with
an explicit type by changing its declaration to include a type hint of
frozenset[str] (e.g., _EXPECTED_TOP_LEVEL: frozenset[str] = ...); ensure you
import typing generics if needed for your Python version (or use built-in
parametric frozenset if supported) and keep the existing frozenset(...)
initializer and values intact.
In `@tests/test_demo_fixture_parity.py`:
- Around line 29-42: Add explicit type annotations for the module-level
constants: annotate _REPO_ROOT, _TEST_FIXTURE_DIR, and _DEMO_DIR as pathlib.Path
and annotate _ALLOWED_REWRITES and _TEST_FIXTURE_ONLY as typing.FrozenSet[str];
import FrozenSet (or from typing import FrozenSet) and pathlib.Path if not
already imported, then change the assignments to include the annotations (e.g.,
_REPO_ROOT: Path = ..., _ALLOWED_REWRITES: FrozenSet[str] = frozenset({...}))
while keeping the existing values unchanged.
🪄 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: 73292f5a-2f54-46ef-88bc-c6668f7d6323
📒 Files selected for processing (36)
.claude/rules/cli-layer.md.claude/rules/python-build.md.claude/rules/testing-signal.mdCLAUDE.mdREADME.mddocs/cli-ops.mdplans/super/47-init-demo.mdpyproject.tomlsrc/signalforge/_demo/.gitignoresrc/signalforge/_demo/dbt_project.ymlsrc/signalforge/_demo/models/staging/sources.ymlsrc/signalforge/_demo/models/staging/stg_bikeshare_trips.sqlsrc/signalforge/_demo/profiles.ymlsrc/signalforge/_demo/signalforge.ymlsrc/signalforge/_demo/target/manifest.jsonsrc/signalforge/cli/__init__.pysrc/signalforge/cli/_helpers.pysrc/signalforge/cli/errors.pysrc/signalforge/cli/init_demo.pysrc/signalforge/demo/__init__.pysrc/signalforge/demo/errors.pysrc/signalforge/warehouse/__init__.pysrc/signalforge/warehouse/errors.pysrc/signalforge/warehouse/profiles.pytests/cli/test_5_surface_parity_init_demo.pytests/cli/test_exit_codes.pytests/cli/test_init_demo.pytests/cli/test_subprocess_smoke.pytests/fixtures/dbt_project_austin/regenerate.shtests/llm/test_logger_grep_gate.pytests/test_audit_completeness.pytests/test_demo.pytests/test_demo_fixture_parity.pytests/test_wheel_packaging.pytests/warehouse/test_errors.pytests/warehouse/test_profiles.py
Fixed (6):
- [CRITICAL] CodeRabbit (demo/__init__.py:138): `--force` against a
symlink dest would `rmtree()` the link's RESOLVED target rather than
the link itself, potentially clobbering an unintended external
location. Added pre-resolve symlink check that raises
DemoDestUnsafeError before any destructive operation when
`dest.is_symlink() and force=True`. No-force behaviour preserved
(the existing test_copy_demo_with_symlink_dest_resolves_target
contract — copy follows the link — still holds). Pinned by new
test_copy_demo_force_against_symlink_dest_raises_dest_unsafe.
- CodeRabbit (warehouse/errors.py:170): ProfileEnvVarUnsetError
interpolated `profiles_path` raw into message + remediation. Routed
through `_format_value(str(profiles_path))` to match the rest of
the warehouse error layer's safe-rendering convention (DEC-022 of
warehouse-adapters.md).
- Copilot + CodeRabbit (profiles.py:172): `_ENV_VAR_RE` comment
claimed bare `env_var('NAME')` form is accepted but the regex
requires the outer `{{ }}` brackets. Updated the comment to match
reality — the bracket-required form matches dbt's own jinja-render
semantics; a bare form in YAML is just a string in dbt.
- Copilot (test_wheel_packaging.py:148): both wheel_smoke tests
called `_build_wheel(tmp_path)` and rebuilt the wheel twice per
run. Refactored to a module-scoped `_built_wheel_members` fixture
that builds once and returns the artifact's namelist; both tests
assert separate invariants over the shared member set. Run time
drops from ~5-6s to ~3s.
- Copilot (regenerate.sh:167): fallback mirror used `cp` without
`-f`. Added the flag per shell-non-interactive policy.
- Copilot (plans/super/47-init-demo.md:16): PR description said
"Phase: detailing (awaiting approval)" but the plan now says
"Phase: devolved" + QG addendum + all-9-stories-shipped. PR body
updated in a follow-up REST PATCH (recorded in the resolve-summary
comment).
False positives (2 — replied + resolved on the PR):
- CodeRabbit (init_demo.py:213): claimed init_demo needs a
fail-closed audit JSONL write. Cites a fabricated guideline —
init-demo is a copy operation, not a content-generating
orchestrator that interacts with LLM/warehouse. Plan US-007
explicitly says "no new audit-event class, no new fail-closed
writer".
- CodeRabbit (init_demo.py:165): claimed dest must route through
canonicalise_user_path. DEC-004 deliberately exempts init-demo
(no project_dir at create-time; init-demo is the one entry point
that *creates* a project, so the containment gate doesn't apply).
Documented in the public copy_demo docstring.
Validation: ruff/pyright clean; pytest 1735 passed (was 1734);
wheel_smoke 2/2 pass in 3.12s.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
PR Review SummaryRound-2 review (Copilot + CodeRabbit) — addressed in commit Fixed (6 items)
False Positives (2 items)
Validation post-fix
All 9 review threads resolved. 🤖 Generated with Claude Code |
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
Ships #47 —
signalforge init-demosubcommand that copies the bundled Austin demo out of the installed wheel for PyPI users. Replaces the brokencp -r tests/fixtures/...README snippet that assumed a clone but appeared under the PyPI quickstart.Phase: complete (epic + 9 stories + Quality Gate + Patterns & Memory all closed; QG ran 4 review passes inline + addressed external PR review comments).
Stories: 9 implementation + Quality Gate + Patterns & Memory.
Decisions: 15 DECs locked at plan-time + 6 QG-pass addenda recorded in
plans/super/47-init-demo.md§ "Quality-gate addendum".Highlights
src/signalforge/_demo/ships in the wheel;tests/fixtures/dbt_project_austin/stays the e2e fixture. Parity test asserts byte-equality except for two documented rewrites (profiles.yml,.gitignore).regenerate.shupdates both in lockstep.includedirective (DEC-002) +wheel_smokemaintainer gate (DEC-003): explicitinclude = ["src/signalforge/_demo"]inpyproject.toml;@pytest.mark.wheel_smoketest builds the wheel and inspects the artifact (run viapytest -m wheel_smoke --no-cov).signalforge.demo.copy_demo(...)(DEC-012): library entry point with its ownDemoErrorhierarchy; CLI wraps at the handler boundary into four tier-mappedCliInitDemo*Errorwrappers.--forcesemantics (DEC-001): atomicrmtree + copytree, refuses ifdestresolves to/,~,cwd, OR is a symlink (the symlink case is a QG follow-up — a symlink dest under--forcewouldrmtree()the link's resolved target, an arbitrary external location).env_var('GOOGLE_CLOUD_PROJECT')in shipped profile (DEC-009 + QG pass 3): added a minimal dbt-compatible jinja substitution tosignalforge.warehouse.profilesso the demo'sprofiles.ymlrenders the operator's env var at load time. New typed errorProfileEnvVarUnsetError(ProfileNotFoundError).Changes by surface
src/signalforge/_demo/(7 files, 22 KB),src/signalforge/demo/{__init__.py, errors.py},src/signalforge/cli/init_demo.py, 4 newCliInitDemo*Errorclasses + 1 newProfileEnvVarUnsetError, dbt-styleenv_var(...)rendering insignalforge.warehouse.profiles.tests/test_demo.py(24 cases),tests/cli/test_init_demo.py(22 cases),tests/test_demo_fixture_parity.py(4),tests/test_wheel_packaging.py(2, gated),tests/cli/test_5_surface_parity_init_demo.py(6), plus subprocessinit-demo --helpsmoke. Net +56 tests.README.md(Quick Start swapscp -rforinit-demo),docs/cli-ops.md§ Subcommands (newinit-demoentry),CLAUDE.md(public API surface + cli: signalforge init-demo subcommand (Austin fixture for PyPI users) #47 repo-status entry). Rule files updated in US-009:python-build.md,cli-layer.md,testing-signal.md.Testing
ruff check . && ruff format --check . && pyright && pytest→ all clean (1735 passed, 1 skipped, 6 pre-existing WSL2 failures that pass in GitHub Actions Linux runners).pytest -m wheel_smoke --no-cov→ 2 passed (3.12s after pass-2-of-PR-review fixture refactor).pytest -m cli_subprocess --no-cov→ 2 passed.signalforge init-demo /tmp/sf-demo && GOOGLE_CLOUD_PROJECT=foo /home/wesd/Projects/SignalForge/.venv/bin/python -c "from signalforge.warehouse.profiles import load_profile; print(load_profile(Path('/tmp/sf-demo')).project)"→'foo'.Compounding update
.claude/rules/python-build.md— new section on Hatchincludedirective for non-.pypackage data +wheel_smokemaintainer-gate pattern..claude/rules/cli-layer.md— extended subpackage layout forinit_demo.py; new section "Library-surface pattern: CLI handler wraps a public lib module at the boundary" documenting the two-layer error wrapping pattern..claude/rules/testing-signal.md—wheel_smokeas the 5th gated marker.plans/super/47-init-demo.md— appended "Quality-gate addendum" recording each of the 4 inline QG passes + the post-publish PR-review pass.🤖 Generated with Claude Code