From 5df00ee053d4489b7b8a6c7ec287a40ca308b5a1 Mon Sep 17 00:00:00 2001 From: Stephen Benjamin Date: Mon, 6 Jul 2026 19:29:16 -0400 Subject: [PATCH 1/4] Add git-aware diff linting: skillsaw lint --since REF MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `--since REF` reports only the violations introduced since the merge-base of HEAD and REF — no committed baseline file, no setup. Mechanism: an ephemeral git baseline. The merge-base is checked out into a temporary detached worktree, linted with the same config, and the result becomes an in-memory BaselineFile fed through the existing baseline subtraction. Entries are fingerprinted against the snapshot toplevel, then baseline.root_path is pointed at the real repo toplevel so current violations fingerprint to identical relative paths. Because fingerprints hash rule_id + relative path + stripped line content (never line numbers), pre-existing violations stay suppressed even when the change shifts every line around them, and ratchet rules compose: growing a file past its merge-base token count re-fires. Mirrors `skillsaw baseline` semantics (INFO dropped, baseline_modes captured). Config is deliberately pinned from the current working tree. --since takes precedence over .skillsaw-baseline.json and cannot be combined with --no-baseline. Shallow clones get a precise error suggesting fetch-depth: 0 / git fetch --unshallow. The GitHub Action gains a `since` input (default '', byte-compatible behavior when unset). Co-Authored-By: Claude Fable 5 --- README.md | 18 + action.yml | 8 + docs/baseline.md | 56 +++ docs/ci.md | 30 ++ docs/cli.md | 1 + src/skillsaw/cli/_lint.py | 65 ++- src/skillsaw/cli/_parser.py | 10 + src/skillsaw/git_baseline.py | 222 +++++++++++ .../lint-since/.claude/rules/architecture.md | 6 + tests/fixtures/lint-since/.skillsaw.yaml | 12 + tests/fixtures/lint-since/CLAUDE.md | 22 ++ tests/test_lint_since.py | 371 ++++++++++++++++++ 12 files changed, 808 insertions(+), 13 deletions(-) create mode 100644 src/skillsaw/git_baseline.py create mode 100644 tests/fixtures/lint-since/.claude/rules/architecture.md create mode 100644 tests/fixtures/lint-since/.skillsaw.yaml create mode 100644 tests/fixtures/lint-since/CLAUDE.md create mode 100644 tests/test_lint_since.py diff --git a/README.md b/README.md index 5357dbe8..da96a54b 100644 --- a/README.md +++ b/README.md @@ -154,6 +154,24 @@ drift and reports stale entries when old violations are fixed. See the [Baseline guide](https://skillsaw.org/baseline/) for matching behavior, ratchet behavior, and refresh workflow. +### Lint only your changes + +`--since` builds the baseline on the fly from git history — no committed +`.skillsaw-baseline.json` needed. It lints the merge-base of HEAD and the +given ref in a temporary git worktree and reports only the violations +introduced since: + +```bash +# Report only violations introduced on your branch +skillsaw lint --since origin/main +``` + +Pre-existing violations stay suppressed even when your change shifts the +lines around them, and ratchet rules (like `context-budget`) only fire when +your change makes the tracked value worse. See +[Lint only your changes](https://skillsaw.org/baseline/#lint-only-your-changes-since) +for the mechanism and caveats. + ## CI Integration ```yaml diff --git a/action.yml b/action.yml index 401d091b..08f81f19 100644 --- a/action.yml +++ b/action.yml @@ -33,6 +33,10 @@ inputs: description: 'Skip custom rules defined in .skillsaw.yaml (recommended for CI on untrusted PRs)' required: false default: 'true' + since: + description: 'Only report violations introduced since the merge-base of HEAD and this ref (e.g. origin/main). Requires git history: use actions/checkout with fetch-depth: 0 (or fetch the base branch).' + required: false + default: '' outputs: exit-code: @@ -84,12 +88,16 @@ runs: SKILLSAW_FAIL_ON: ${{ inputs.fail-on }} SKILLSAW_VERBOSE: ${{ inputs.verbose }} SKILLSAW_NO_CUSTOM_RULES: ${{ inputs.no-custom-rules }} + SKILLSAW_SINCE: ${{ inputs.since }} run: | set +e ARGS="" if [ "$SKILLSAW_STRICT" = "true" ]; then ARGS="$ARGS --strict" fi + if [ -n "$SKILLSAW_SINCE" ]; then + ARGS="$ARGS --since $SKILLSAW_SINCE" + fi if [ -n "$SKILLSAW_FAIL_ON" ]; then case "$SKILLSAW_FAIL_ON" in error|warning|info) ARGS="$ARGS --fail-on $SKILLSAW_FAIL_ON" ;; diff --git a/docs/baseline.md b/docs/baseline.md index 1bc1f7a1..82fb9b08 100644 --- a/docs/baseline.md +++ b/docs/baseline.md @@ -64,6 +64,62 @@ Run lint without baseline filtering: skillsaw lint --no-baseline ``` +## Lint Only Your Changes (`--since`) + +`--since REF` builds the baseline on the fly from git history — no +committed `.skillsaw-baseline.json`, no setup: + +```bash +# Report only violations introduced on your branch +skillsaw lint --since origin/main + +# Report only violations introduced by the last commit +skillsaw lint --since HEAD~1 +``` + +### How it works + +`--since` resolves the **merge-base** of HEAD and REF (the commit your +work diverged from), checks it out into a temporary git worktree, lints +that snapshot with the same configuration, and uses the result as an +ephemeral baseline. The temporary worktree is always removed afterwards. + +Because the ephemeral baseline uses the same content-hash fingerprints +as a committed baseline, everything above applies: + +- **Drift immunity** — pre-existing violations stay suppressed even when + your change inserts or removes lines around them. +- **Ratchet composition** — value-carrying rules (`context-budget`, + `content-instruction-budget`, `content-actionability-score`) re-fire + only when your change makes the tracked value worse. Growing a + SKILL.md past its merge-base token count reports the regression; + shrinking it stays quiet. +- Fixed violations are reported as + `Baseline: N violation(s) fixed since REF`. + +Merge-base semantics mean commits that landed on REF after you branched +are not counted against you — only your own changes are compared. + +The rule configuration (`.skillsaw.yaml`, `--rule`/`--skip-rule`, +`--no-custom-rules`, `--no-plugins`) is deliberately taken from the +*current* working tree for both lints: `--since` measures how the +repository changed, not how the rule configuration changed. + +`--since` takes precedence over a committed `.skillsaw-baseline.json` +and cannot be combined with `--no-baseline`. + +### Limitations + +- **Renames resurface old violations** — fingerprints include the + relative file path, so violations in a renamed file no longer match + their merge-base entries and are reported again. +- **Roughly doubles lint time** — the merge-base snapshot is linted in + addition to the working tree. +- **Requires git history** — the merge-base commit must be reachable. + In shallow CI clones, fetch history first (e.g. `fetch-depth: 0` with + `actions/checkout`, or `git fetch --unshallow`); skillsaw reports a + precise error when it cannot resolve the merge-base. + ## Stale Entries When you fix a baselined violation, its baseline entry becomes **stale**. diff --git a/docs/ci.md b/docs/ci.md index 26b46159..4925854f 100644 --- a/docs/ci.md +++ b/docs/ci.md @@ -100,6 +100,7 @@ jobs: | `verbose` | Include info-level violations | `false` | | `no-custom-rules` | Skip custom rules defined in `.skillsaw.yaml` | `true` | | `plugins` | Newline-separated list of plugin packages to install | `''` | +| `since` | Only report violations introduced since the merge-base of HEAD and this ref (e.g. `origin/main`) — see [Lint only your changes](baseline.md#lint-only-your-changes-since) | `''` | ### Outputs @@ -110,6 +111,35 @@ jobs: | `warnings` | Number of warnings found | | `report-file` | Path to JSON report file | +### Lint only the PR's changes + +Set `since` to fail CI only on violations the PR itself introduces — +pre-existing violations in the base branch are suppressed automatically, +with no committed baseline file. The merge-base commit must be available, +so check out with full history: + +```yaml +jobs: + skillsaw: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v5 + with: + fetch-depth: 0 # --since needs history to find the merge-base + persist-credentials: false + - uses: stbenjam/skillsaw@v0 + with: + strict: true + since: origin/${{ github.base_ref }} +``` + +(Instead of `fetch-depth: 0`, fetching just the base branch also works: +`git fetch origin "$BASE_REF"`.) + +`since` composes with the review action above: the uploaded report only +contains the PR's own regressions, so inline comments shrink to exactly +what the PR introduced. + ### Supply Chain Protection The examples above use `@v0` for brevity. For supply-chain protection, diff --git a/docs/cli.md b/docs/cli.md index 647bcba7..b2ed5417 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -23,6 +23,7 @@ Lint agent skills, plugins, and AI coding assistant context | `--rule` | Only run these rules (repeatable). Config still comes from .skillsaw.yaml. | | | `--skip-rule` | Skip these rules (repeatable). Cannot be combined with --rule. | | | `--no-baseline` | Ignore baseline file even if .skillsaw-baseline.json exists | | +| `--since` | Only report violations introduced since the merge-base of HEAD and REF (e.g. origin/main). Lints that commit in a temporary git worktree and uses the result as an ephemeral baseline. Takes precedence over .skillsaw-baseline.json; cannot be combined with --no-baseline. Requires git history for the merge-base. | | | `--no-custom-rules` | Skip custom rules defined in .skillsaw.yaml (recommended for CI on untrusted PRs) | | | `--no-plugins` | Skip rules from installed plugin packages (skillsaw.plugins entry points) | | | `--no-progress` | Disable the interactive per-rule progress indicator (auto-disabled when stderr is not a terminal) | | diff --git a/src/skillsaw/cli/_lint.py b/src/skillsaw/cli/_lint.py index 640a3e92..d6d82dff 100644 --- a/src/skillsaw/cli/_lint.py +++ b/src/skillsaw/cli/_lint.py @@ -43,6 +43,14 @@ def _run_lint(args): ) sys.exit(1) + if args.since and args.no_baseline: + print( + "Error: --since and --no-baseline cannot be combined " + "(--since builds its own ephemeral baseline from git)", + file=sys.stderr, + ) + sys.exit(1) + output_formats = {} for spec in args.outputs: try: @@ -109,8 +117,39 @@ def _run_lint(args): else: fail_level = config.effective_fail_level() + rule_ids = set(args.rule_ids) if args.rule_ids else None + skip_rule_ids = set(args.skip_rule_ids) if args.skip_rule_ids else None + if rule_ids and skip_rule_ids: + print("Error: --rule and --skip-rule cannot be combined", file=sys.stderr) + sys.exit(1) + baseline = None - if not args.no_baseline: + if args.since: + # --since builds an ephemeral baseline from the merge-base of HEAD + # and the given ref; it takes precedence over any committed + # .skillsaw-baseline.json. + from ..git_baseline import GitBaselineError, build_git_baseline, repo_toplevel + + try: + repo_root = repo_toplevel(paths[0]) + baseline, merge_base = build_git_baseline( + repo_root, + args.since, + config, + paths, + cli_version, + rule_ids=rule_ids, + skip_rule_ids=skip_rule_ids, + no_custom_rules=args.no_custom_rules, + no_plugins=args.no_plugins, + ) + except GitBaselineError as e: + print(f"Error: {e}", file=sys.stderr) + sys.exit(1) + + if args.fmt == "text": + print(f"Comparing against merge-base {merge_base[:12]} (--since {args.since})\n") + elif not args.no_baseline: from ..baseline import find_baseline, load_baseline baseline_path = find_baseline(config.config_dir or paths[0]) @@ -126,12 +165,6 @@ def _run_lint(args): except (ValueError, OSError) as e: print(f"Warning: Failed to load baseline: {e}", file=sys.stderr) - rule_ids = set(args.rule_ids) if args.rule_ids else None - skip_rule_ids = set(args.skip_rule_ids) if args.skip_rule_ids else None - if rule_ids and skip_rule_ids: - print("Error: --rule and --skip-rule cannot be combined", file=sys.stderr) - sys.exit(1) - lint_started = time.perf_counter() all_violations = [] all_rules = [] @@ -186,16 +219,22 @@ def _run_lint(args): if baseline and args.fmt == "text": stale = linter.stale_baseline_entries if stale: - print( - f"Baseline: {len(stale)} stale" - f" {'entry' if len(stale) == 1 else 'entries'}" - f" (violations resolved since baseline was set)" - ) + if args.since: + print(f"Baseline: {len(stale)} violation(s) fixed since {args.since}") + else: + print( + f"Baseline: {len(stale)} stale" + f" {'entry' if len(stale) == 1 else 'entries'}" + f" (violations resolved since baseline was set)" + ) if args.verbose: for entry in stale: location = f" [{entry.file_path}]" if entry.file_path else "" print(f" - {entry.rule_id}{location}: {entry.message}") - print(" Run `skillsaw baseline` to update.\n") + if args.since: + print() + else: + print(" Run `skillsaw baseline` to update.\n") merged_context = _build_merged_context(contexts) unique_rules = _dedup_rules(all_rules) diff --git a/src/skillsaw/cli/_parser.py b/src/skillsaw/cli/_parser.py index 485f0a9b..2f6e89a0 100644 --- a/src/skillsaw/cli/_parser.py +++ b/src/skillsaw/cli/_parser.py @@ -126,6 +126,16 @@ def _build_parser(): dest="no_baseline", help="Ignore baseline file even if .skillsaw-baseline.json exists", ) + lint_parser.add_argument( + "--since", + metavar="REF", + default=None, + help="Only report violations introduced since the merge-base of HEAD " + "and REF (e.g. origin/main). Lints that commit in a temporary git " + "worktree and uses the result as an ephemeral baseline. Takes " + "precedence over .skillsaw-baseline.json; cannot be combined with " + "--no-baseline. Requires git history for the merge-base.", + ) lint_parser.add_argument( "--no-custom-rules", action="store_true", diff --git a/src/skillsaw/git_baseline.py b/src/skillsaw/git_baseline.py new file mode 100644 index 00000000..b037c2e2 --- /dev/null +++ b/src/skillsaw/git_baseline.py @@ -0,0 +1,222 @@ +""" +Ephemeral git baseline for ``skillsaw lint --since REF``. + +``--since`` reports only the violations introduced since a git ref: it +checks out the merge-base of HEAD and REF into a temporary git worktree, +lints that snapshot with the same configuration, builds an in-memory +:class:`~skillsaw.baseline.BaselineFile` from the result, and hands it to +the normal baseline subtraction. No committed ``.skillsaw-baseline.json`` +is involved. + +Mechanics worth knowing: + +- Baseline fingerprints hash rule ID + relative file path + stripped + source-line content (never line numbers — see + :func:`skillsaw.baseline.fingerprint_violation`), so pre-existing + violations stay suppressed even when the change shifts every line + around them. +- Entries are fingerprinted against the *snapshot* toplevel, then the + returned baseline's ``root_path`` is pointed at the real repository + toplevel. Current violations relativize to identical paths, so the + existing subtraction in :class:`skillsaw.linter.Linter` — and everything + downstream (suppressed counts, stale entries, formatters, exit codes) — + works unchanged. +- The configuration is deliberately pinned from the CURRENT working + tree, not the snapshot: ``--since`` measures how the repository + changed, not how the rule configuration changed. Custom rule paths + therefore also resolve against the current tree. +- Mirroring ``skillsaw baseline`` semantics, INFO-severity violations are + excluded from the ephemeral baseline and ratchet rules contribute + their ``baseline_mode`` so value regressions (e.g. a SKILL.md growing + past its old token count) re-fire with the delta. +""" + +from __future__ import annotations + +import logging +import shutil +import subprocess +import tempfile +from contextlib import contextmanager +from pathlib import Path +from typing import Dict, Iterator, List, Optional, Sequence, Set, Tuple + +from .baseline import BaselineFile, build_baseline +from .config import LinterConfig +from .context import RepositoryContext +from .linter import Linter +from .rule import RuleViolation, Severity + +logger = logging.getLogger(__name__) + + +class GitBaselineError(Exception): + """Raised when the ephemeral ``--since`` baseline cannot be built.""" + + +def _git(cwd: Path, *argv: str) -> subprocess.CompletedProcess: + """Run a git command, capturing output. Raises GitBaselineError if git is missing.""" + try: + return subprocess.run( + ["git", *argv], + cwd=str(cwd), + capture_output=True, + text=True, + check=False, + ) + except FileNotFoundError as exc: + raise GitBaselineError( + "--since requires git, but the `git` executable was not found on PATH" + ) from exc + + +def repo_toplevel(path: Path) -> Path: + """Resolve the git working-tree toplevel containing *path*.""" + result = _git(path, "rev-parse", "--show-toplevel") + if result.returncode != 0: + raise GitBaselineError( + f"--since requires a git repository, but {path} is not inside one " + f"({result.stderr.strip() or 'git rev-parse failed'})" + ) + return Path(result.stdout.strip()).resolve() + + +def _is_shallow(repo_root: Path) -> bool: + result = _git(repo_root, "rev-parse", "--is-shallow-repository") + return result.returncode == 0 and result.stdout.strip() == "true" + + +def resolve_merge_base(repo_root: Path, ref: str) -> str: + """Resolve the merge-base of HEAD and *ref*, with a precise error on failure.""" + result = _git(repo_root, "merge-base", "HEAD", ref) + if result.returncode == 0: + return result.stdout.strip() + + detail = result.stderr.strip() or f"no merge-base found between HEAD and '{ref}'" + message = f"--since {ref}: cannot resolve merge-base: {detail}" + if _is_shallow(repo_root): + message += ( + " (this is a shallow clone — fetch more history first, e.g." + " `git fetch --unshallow` or use `fetch-depth: 0` with" + " actions/checkout)" + ) + raise GitBaselineError(message) + + +@contextmanager +def _snapshot_worktree(repo_root: Path, sha: str) -> Iterator[Path]: + """Check *sha* out into a temporary detached git worktree. + + The worktree (and its temp directory) is always removed on exit, even + when the body raises — a leftover registration would break later + ``git worktree`` operations in the user's repository. + """ + tmpdir = Path(tempfile.mkdtemp(prefix="skillsaw-since-")) + snapshot = tmpdir / "snapshot" + result = _git(repo_root, "worktree", "add", "--detach", str(snapshot), sha) + if result.returncode != 0: + shutil.rmtree(tmpdir, ignore_errors=True) + raise GitBaselineError( + f"--since: failed to create a temporary worktree for {sha[:12]}: " + f"{result.stderr.strip()}" + ) + try: + yield snapshot.resolve() + finally: + remove = _git(repo_root, "worktree", "remove", "--force", str(snapshot)) + if remove.returncode != 0: + # Fall back to deleting the checkout and pruning the stale + # registration so nothing lingers in .git/worktrees. + shutil.rmtree(snapshot, ignore_errors=True) + _git(repo_root, "worktree", "prune") + shutil.rmtree(tmpdir, ignore_errors=True) + + +def build_git_baseline( + repo_root: Path, + ref: str, + config: LinterConfig, + lint_paths: Sequence[Path], + version_string: str, + *, + rule_ids: Optional[Set[str]] = None, + skip_rule_ids: Optional[Set[str]] = None, + no_custom_rules: bool = False, + no_plugins: bool = False, +) -> Tuple[BaselineFile, str]: + """Lint the merge-base of HEAD and *ref*; return it as an in-memory baseline. + + Each lint path is mapped to its location inside the snapshot worktree + (paths that do not exist at the merge-base simply contribute no + baseline entries). The snapshot is linted with the caller's *config* + (from the current working tree) and rule selection flags, INFO + violations are dropped, and ratchet ``baseline_mode``\\ s are captured — + all mirroring ``skillsaw baseline`` semantics. + + Returns ``(baseline, merge_base_sha)`` where ``baseline.root_path`` is + already pointed at *repo_root* so current violations fingerprint to + the same relative paths as the snapshot entries. + """ + merge_base = resolve_merge_base(repo_root, ref) + + violations: List[RuleViolation] = [] + baseline_modes: Dict[str, str] = {} + + with _snapshot_worktree(repo_root, merge_base) as snapshot_root: + for lint_path in lint_paths: + try: + rel = lint_path.resolve().relative_to(repo_root) + except ValueError: + raise GitBaselineError( + f"--since requires every lint path to be inside the git " + f"repository at {repo_root}, but {lint_path} is not" + ) from None + + mapped = snapshot_root / rel + if not mapped.exists(): + # The path was added after the merge-base: the base + # contributes no violations for it. + logger.info( + "--since: %s does not exist at merge-base %s; " "no baseline entries for it", + rel, + merge_base[:12], + ) + continue + + context = RepositoryContext( + mapped, + exclude_patterns=config.exclude_patterns, + content_paths=config.content_paths, + ) + try: + linter = Linter( + context, + config, + rule_ids=rule_ids, + skip_rule_ids=skip_rule_ids, + no_custom_rules=no_custom_rules, + no_plugins=no_plugins, + ) + except ValueError as e: + raise GitBaselineError(f"--since: failed to lint base snapshot: {e}") from e + + violations.extend(v for v in linter.run() if v.severity != Severity.INFO) + for rule in linter.rules: + if rule.baseline_mode: + baseline_modes.setdefault(rule.rule_id, rule.baseline_mode) + + logger.info( + "--since: baselined %d violation(s) from merge-base %s", + len(violations), + merge_base[:12], + ) + # Fingerprints read source lines from disk, so the baseline must be + # built while the snapshot worktree still exists. + baseline = build_baseline(violations, snapshot_root, version_string, baseline_modes) + + # THE key trick: entries were fingerprinted with snapshot-relative + # paths; pointing root_path at the real repo toplevel makes current + # violations fingerprint to identical relative paths, so the existing + # baseline subtraction just works. + baseline.root_path = repo_root + return baseline, merge_base diff --git a/tests/fixtures/lint-since/.claude/rules/architecture.md b/tests/fixtures/lint-since/.claude/rules/architecture.md new file mode 100644 index 00000000..96ef484c --- /dev/null +++ b/tests/fixtures/lint-since/.claude/rules/architecture.md @@ -0,0 +1,6 @@ +The service is split into three layers: HTTP handlers in `api/`, +business logic in `core/`, and persistence in `db/`. Handlers never +touch the database directly — they call into `core/` services. + +Database migrations live in `db/migrations/` and run with Alembic. +Name migration files with a numeric prefix: `0042_add_index.py`. diff --git a/tests/fixtures/lint-since/.skillsaw.yaml b/tests/fixtures/lint-since/.skillsaw.yaml new file mode 100644 index 00000000..d9cde346 --- /dev/null +++ b/tests/fixtures/lint-since/.skillsaw.yaml @@ -0,0 +1,12 @@ +# skillsaw configuration for the lint-since fixture. +# The low `rule` token budget makes .claude/rules/architecture.md exceed +# its limit, giving the fixture a ratchet (value-carrying) violation. +version: "0.16.0" + +rules: + context-budget: + enabled: true + severity: warning + limits: + rule: + warn: 40 diff --git a/tests/fixtures/lint-since/CLAUDE.md b/tests/fixtures/lint-since/CLAUDE.md new file mode 100644 index 00000000..60eae518 --- /dev/null +++ b/tests/fixtures/lint-since/CLAUDE.md @@ -0,0 +1,22 @@ +# Pipeline Service Guidelines + +This service is a Flask API that manages deployment pipelines. Run +`make test` before every commit and `make lint` before pushing. + +## Code Standards + +- Try to keep functions under 50 lines. +- Be careful when editing the migration scripts in `db/migrations/`. +- Format code with `black --line-length 100` before committing. +- Type annotations are required on all public functions. + +## Deployment + +1. Run `make build` to produce the container image +2. Push the image with `make push` +3. Trigger the rollout with `scripts/deploy.sh production` + +## Dependencies + +- Add runtime dependencies to `pyproject.toml` under `[project.dependencies]` +- Pin major versions only: `requests>=2.28,<3` diff --git a/tests/test_lint_since.py b/tests/test_lint_since.py new file mode 100644 index 00000000..5222d127 --- /dev/null +++ b/tests/test_lint_since.py @@ -0,0 +1,371 @@ +""" +End-to-end tests for ``skillsaw lint --since REF``. + +Each test copies the static ``lint-since`` fixture into a temp directory, +turns it into a real git repository (fixtures cannot contain ``.git`` +directories, so history is created here), invokes ``python -m skillsaw +lint --since ...`` via subprocess, and asserts on exit codes and the +parsed JSON output. + +The fixture carries three deliberate legacy violations: + +- two ``content-weak-language`` warnings in ``CLAUDE.md`` (lines with + "Try to" / "Be careful") +- one ``context-budget`` ratchet warning on + ``.claude/rules/architecture.md`` (82 tokens against the 40-token + ``rule`` budget configured in ``.skillsaw.yaml``) +""" + +import json +import shutil +import subprocess +import sys +from pathlib import Path + +FIXTURES = Path(__file__).parent / "fixtures" + +SECRET_LINE = "\nUse the deploy token `ghp_abcdefghij0123456789abcdefghij012345` when pushing.\n" + + +# ── Helpers ────────────────────────────────────────────────────── + + +def git(cwd, *argv): + result = subprocess.run( + [ + "git", + "-c", + "user.email=skillsaw-tests@example.com", + "-c", + "user.name=skillsaw tests", + "-c", + "commit.gpgsign=false", + *argv, + ], + cwd=str(cwd), + capture_output=True, + text=True, + timeout=60, + ) + assert result.returncode == 0, f"git {' '.join(argv)} failed: {result.stderr}" + return result + + +def copy_fixture(tmp_path): + dst = tmp_path / "repo" + shutil.copytree(FIXTURES / "lint-since", dst) + return dst + + +def commit_all(repo, message): + git(repo, "add", "-A") + git(repo, "commit", "-q", "-m", message) + + +def git_repo(tmp_path): + """Copy the fixture and commit it as the initial state.""" + repo = copy_fixture(tmp_path) + git(repo, "init", "-q") + commit_all(repo, "Initial import") + return repo + + +def run_lint(path, *extra_args, fmt="json", verbose=False): + args = [sys.executable, "-m", "skillsaw", "lint"] + if fmt: + args.extend(["--format", fmt]) + if verbose: + args.append("-v") + args.append(str(path)) + args.extend(extra_args) + result = subprocess.run(args, capture_output=True, text=True, timeout=120) + output = None + if fmt == "json" and result.stdout.strip(): + output = json.loads(result.stdout) + return { + "rc": result.returncode, + "out": output, + "stdout": result.stdout, + "stderr": result.stderr, + } + + +def violations(r): + return r["out"]["violations"] if r["out"] else [] + + +def rule_ids(r): + return {v["rule_id"] for v in violations(r)} + + +def suppressed(r): + return r["out"]["summary"]["baseline_suppressed"] + + +def worktree_count(repo): + result = git(repo, "worktree", "list", "--porcelain") + return result.stdout.count("worktree ") + + +def append(path, text): + path.write_text(path.read_text() + text) + + +# ── Baseline subtraction ───────────────────────────────────────── + + +def test_clean_change_suppresses_all_legacy_violations(tmp_path): + repo = git_repo(tmp_path) + append(repo / "CLAUDE.md", "\n## Releases\n\nTag releases with `make release VERSION=x.y.z`.\n") + commit_all(repo, "Document the release process") + + r = run_lint(repo, "--since", "HEAD~1", "--strict") + + assert r["rc"] == 0, r["stderr"] + assert violations(r) == [] + assert suppressed(r) == 3 + + +def test_new_error_is_the_only_violation_reported(tmp_path): + repo = git_repo(tmp_path) + append(repo / "CLAUDE.md", SECRET_LINE) + commit_all(repo, "Add deploy instructions") + + r = run_lint(repo, "--since", "HEAD~1") + + assert r["rc"] == 1 + assert [v["rule_id"] for v in violations(r)] == ["content-embedded-secrets"] + assert suppressed(r) == 3 + + +def test_text_mode_prints_merge_base_header(tmp_path): + repo = git_repo(tmp_path) + merge_base = git(repo, "rev-parse", "HEAD").stdout.strip() + append(repo / "CLAUDE.md", "\nRun `make docs` to rebuild the API reference.\n") + commit_all(repo, "Mention docs build") + + r = run_lint(repo, "--since", "HEAD~1", fmt="text") + + assert r["rc"] == 0 + assert f"Comparing against merge-base {merge_base[:12]} (--since HEAD~1)" in r["stdout"] + assert "3 suppressed" in r["stdout"] + + +def test_fixed_violations_reported_without_baseline_hint(tmp_path): + repo = git_repo(tmp_path) + content = (repo / "CLAUDE.md").read_text() + content = content.replace("- Try to keep functions under 50 lines.\n", "") + (repo / "CLAUDE.md").write_text(content) + commit_all(repo, "Drop the function-length guideline") + + r = run_lint(repo, "--since", "HEAD~1", fmt="text") + + assert r["rc"] == 0 + assert "1 violation(s) fixed since HEAD~1" in r["stdout"] + # The committed-baseline refresh hint makes no sense for --since. + assert "skillsaw baseline" not in r["stdout"] + + +# ── Drift immunity ─────────────────────────────────────────────── + + +def test_line_drift_keeps_legacy_violations_suppressed(tmp_path): + repo = git_repo(tmp_path) + content = (repo / "CLAUDE.md").read_text() + header = ( + "# Pipeline Service Guidelines\n\n" + "## Environment\n\n" + "Local development requires Python 3.11 and Docker 24 or newer.\n" + "Copy `.env.example` to `.env` before the first run.\n" + ) + # Insert a new section right below the title so every legacy + # violation moves to a different line number. + (repo / "CLAUDE.md").write_text(content.replace("# Pipeline Service Guidelines\n", header, 1)) + commit_all(repo, "Document local environment setup") + + r = run_lint(repo, "--since", "HEAD~1", "--strict") + + assert r["rc"] == 0, r["stdout"] + assert violations(r) == [] + assert suppressed(r) == 3 + + +# ── Ratchet composition ────────────────────────────────────────── + + +def test_ratchet_refires_when_tracked_value_grows(tmp_path): + repo = git_repo(tmp_path) + append( + repo / ".claude/rules/architecture.md", + "\nBackground jobs run through Celery workers defined in `jobs/`.\n" + "Every job must be idempotent — retries are automatic on failure.\n", + ) + commit_all(repo, "Document background jobs") + + r = run_lint(repo, "--since", "HEAD~1", "--strict") + + assert r["rc"] == 1 + assert [v["rule_id"] for v in violations(r)] == ["context-budget"] + assert "exceeds rule warn limit" in violations(r)[0]["message"] + # The two weak-language violations stay suppressed. + assert suppressed(r) == 2 + + +def test_ratchet_improvement_stays_suppressed(tmp_path): + repo = git_repo(tmp_path) + content = (repo / ".claude/rules/architecture.md").read_text() + content = content.replace( + "Name migration files with a numeric prefix: `0042_add_index.py`.\n", "" + ) + (repo / ".claude/rules/architecture.md").write_text(content) + commit_all(repo, "Trim the migration naming rule") + + r = run_lint(repo, "--since", "HEAD~1", "--strict") + + # Still over the 40-token budget, but better than the merge-base + # value — the ratchet suppresses it. + assert r["rc"] == 0, r["stdout"] + assert violations(r) == [] + assert suppressed(r) == 3 + + +# ── Flag validation and error paths ────────────────────────────── + + +def test_since_with_no_baseline_is_an_error(tmp_path): + repo = git_repo(tmp_path) + + r = run_lint(repo, "--since", "HEAD~1", "--no-baseline") + + assert r["rc"] == 1 + assert "--since and --no-baseline cannot be combined" in r["stderr"] + + +def test_non_git_directory_is_a_clear_error(tmp_path): + repo = copy_fixture(tmp_path) + + r = run_lint(repo, "--since", "HEAD~1") + + assert r["rc"] == 1 + assert "--since requires a git repository" in r["stderr"] + assert "Traceback" not in r["stderr"] + + +def test_unknown_ref_is_a_clear_error(tmp_path): + repo = git_repo(tmp_path) + + r = run_lint(repo, "--since", "no-such-branch") + + assert r["rc"] == 1 + assert "cannot resolve merge-base" in r["stderr"] + assert "no-such-branch" in r["stderr"] + assert "Traceback" not in r["stderr"] + + +def test_shallow_clone_error_suggests_fetching_history(tmp_path): + origin = git_repo(tmp_path) + append(origin / "CLAUDE.md", "\nRun `make docs` to rebuild the API reference.\n") + commit_all(origin, "Mention docs build") + + clone = tmp_path / "shallow" + git(tmp_path, "clone", "-q", "--depth", "1", f"file://{origin}", str(clone)) + + r = run_lint(clone, "--since", "HEAD~1") + + assert r["rc"] == 1 + assert "cannot resolve merge-base" in r["stderr"] + assert "shallow" in r["stderr"] + assert "fetch" in r["stderr"] + assert "Traceback" not in r["stderr"] + + +# ── Precedence over a committed baseline ───────────────────────── + + +def test_since_takes_precedence_over_committed_baseline(tmp_path): + repo = git_repo(tmp_path) + append(repo / "CLAUDE.md", SECRET_LINE) + # Baseline the secret into a committed .skillsaw-baseline.json. + result = subprocess.run( + [sys.executable, "-m", "skillsaw", "baseline", str(repo)], + capture_output=True, + text=True, + timeout=120, + ) + assert result.returncode == 0, result.stderr + commit_all(repo, "Add deploy token and baseline it") + + # The committed baseline suppresses the secret... + r = run_lint(repo) + assert r["rc"] == 0 + assert violations(r) == [] + + # ...but --since compares against the merge-base instead, where the + # secret did not exist yet. + r = run_lint(repo, "--since", "HEAD~1") + assert r["rc"] == 1 + assert rule_ids(r) == {"content-embedded-secrets"} + + +# ── Worktree hygiene ───────────────────────────────────────────── + + +def test_worktree_removed_after_successful_run(tmp_path): + repo = git_repo(tmp_path) + append(repo / "CLAUDE.md", "\nRun `make docs` to rebuild the API reference.\n") + commit_all(repo, "Mention docs build") + + r = run_lint(repo, "--since", "HEAD~1") + + assert r["rc"] == 0 + assert worktree_count(repo) == 1 # only the main worktree + + +def test_worktree_removed_after_failing_run(tmp_path): + repo = git_repo(tmp_path) + append(repo / "CLAUDE.md", "\nRun `make docs` to rebuild the API reference.\n") + commit_all(repo, "Mention docs build") + + # The unknown rule id makes Linter construction fail while the + # snapshot worktree exists — cleanup must still run. + r = run_lint(repo, "--since", "HEAD~1", "--rule", "does-not-exist") + + assert r["rc"] == 1 + assert "does-not-exist" in r["stderr"] + assert worktree_count(repo) == 1 + + +# ── Working tree states ────────────────────────────────────────── + + +def test_uncommitted_new_violation_is_caught(tmp_path): + repo = git_repo(tmp_path) + append(repo / "CLAUDE.md", "\nRun `make docs` to rebuild the API reference.\n") + commit_all(repo, "Mention docs build") + # Dirty working tree: the secret is not committed anywhere. + append(repo / "CLAUDE.md", SECRET_LINE) + + r = run_lint(repo, "--since", "HEAD~1") + + assert r["rc"] == 1 + assert rule_ids(r) == {"content-embedded-secrets"} + + +def test_lint_path_missing_at_merge_base_contributes_no_baseline(tmp_path): + repo = git_repo(tmp_path) + nested = repo / "nested" + nested.mkdir() + (nested / "CLAUDE.md").write_text( + "# Nested Tool Guidelines\n\n" + "Run `make check` before committing changes to this tool.\n" + SECRET_LINE + ) + commit_all(repo, "Add nested tool") + + # nested/ does not exist at the merge-base: the base contributes no + # violations for it, so everything in it is new. + r = run_lint(nested, "--since", "HEAD~1") + + assert r["rc"] == 1 + assert "content-embedded-secrets" in rule_ids(r) + assert suppressed(r) == 0 + assert worktree_count(repo) == 1 From 226c1a54e8e418816aba388db8c3f297cd032a56 Mon Sep 17 00:00:00 2001 From: Stephen Benjamin Date: Mon, 6 Jul 2026 19:41:22 -0400 Subject: [PATCH 2/4] Add in-process tests for git_baseline and the --since CLI branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 16 integration tests in test_lint_since.py drive the CLI via subprocess, which coverage.py does not trace — Codecov reported the patch as uncovered even though it is exercised end-to-end. Add tests/test_git_baseline.py: 25 in-process tests that import and call the code directly (repo_toplevel, resolve_merge_base including the shallow-clone hint, _snapshot_worktree cleanup on success/failure/ bad sha/remove-failure fallback, build_git_baseline root repointing, INFO exclusion, ratchet modes, Linter suppression, and the lint CLI's --since, committed-baseline, and stale-hint branches via _run_lint). git_baseline.py is now at 100% line coverage in-process, and every line the PR adds to cli/_lint.py is covered. The subprocess tests are kept — they verify real exit codes and process-level behavior. Co-Authored-By: Claude Fable 5 --- tests/test_git_baseline.py | 404 +++++++++++++++++++++++++++++++++++++ 1 file changed, 404 insertions(+) create mode 100644 tests/test_git_baseline.py diff --git a/tests/test_git_baseline.py b/tests/test_git_baseline.py new file mode 100644 index 00000000..071f8ff8 --- /dev/null +++ b/tests/test_git_baseline.py @@ -0,0 +1,404 @@ +""" +In-process unit tests for ``skillsaw.git_baseline`` and the ``--since`` +branch of the lint CLI. + +The subprocess integration tests in ``test_lint_since.py`` exercise the +same code end-to-end; these tests import and call it directly so the +behavior is also visible to coverage. They reuse the ``lint-since`` +fixture and its git helpers: three legacy violations — two +``content-weak-language`` warnings in ``CLAUDE.md`` and one +``context-budget`` ratchet warning on ``.claude/rules/architecture.md``. +""" + +import json +import subprocess + +import pytest + +from skillsaw.baseline import BaselineFile +from skillsaw.config import LinterConfig +from skillsaw.context import RepositoryContext +from skillsaw.git_baseline import ( + GitBaselineError, + _snapshot_worktree, + build_git_baseline, + repo_toplevel, + resolve_merge_base, +) +from skillsaw.linter import Linter +from skillsaw.rule import Severity + +from .test_lint_since import ( + SECRET_LINE, + append, + commit_all, + copy_fixture, + git, + git_repo, + worktree_count, +) + + +def rev_parse(repo, ref): + return git(repo, "rev-parse", ref).stdout.strip() + + +def fixture_config(repo): + return LinterConfig.from_file(repo / ".skillsaw.yaml") + + +# ── repo_toplevel ──────────────────────────────────────────────── + + +class TestRepoToplevel: + def test_resolves_toplevel(self, tmp_path): + repo = git_repo(tmp_path) + assert repo_toplevel(repo) == repo.resolve() + + def test_resolves_toplevel_from_subdirectory(self, tmp_path): + repo = git_repo(tmp_path) + assert repo_toplevel(repo / ".claude" / "rules") == repo.resolve() + + def test_not_a_git_repo(self, tmp_path): + plain = copy_fixture(tmp_path) + with pytest.raises(GitBaselineError, match="requires a git repository"): + repo_toplevel(plain) + + def test_git_not_on_path(self, tmp_path, monkeypatch): + def raise_missing(*args, **kwargs): + raise FileNotFoundError("git") + + monkeypatch.setattr("skillsaw.git_baseline.subprocess.run", raise_missing) + with pytest.raises(GitBaselineError, match="was not found on PATH"): + repo_toplevel(tmp_path) + + +# ── resolve_merge_base ─────────────────────────────────────────── + + +class TestResolveMergeBase: + def test_resolves_merge_base_of_two_commits(self, tmp_path): + repo = git_repo(tmp_path) + append(repo / "CLAUDE.md", "\nRun `make docs` to rebuild the API reference.\n") + commit_all(repo, "Mention docs build") + + assert resolve_merge_base(repo, "HEAD~1") == rev_parse(repo, "HEAD~1") + + def test_unknown_ref(self, tmp_path): + repo = git_repo(tmp_path) + with pytest.raises(GitBaselineError, match="cannot resolve merge-base") as exc: + resolve_merge_base(repo, "no-such-branch") + assert "no-such-branch" in str(exc.value) + + def test_shallow_clone_mentions_fetching_history(self, tmp_path): + origin = git_repo(tmp_path) + append(origin / "CLAUDE.md", "\nRun `make docs` to rebuild the API reference.\n") + commit_all(origin, "Mention docs build") + + clone = tmp_path / "shallow" + git(tmp_path, "clone", "-q", "--depth", "1", f"file://{origin}", str(clone)) + + with pytest.raises(GitBaselineError, match="shallow") as exc: + resolve_merge_base(clone, "HEAD~1") + assert "fetch" in str(exc.value) + + +# ── _snapshot_worktree ─────────────────────────────────────────── + + +class TestSnapshotWorktree: + def test_yields_checkout_of_requested_sha(self, tmp_path): + repo = git_repo(tmp_path) + base_sha = rev_parse(repo, "HEAD") + append(repo / "CLAUDE.md", SECRET_LINE) + commit_all(repo, "Add deploy token") + + with _snapshot_worktree(repo, base_sha) as snapshot: + assert snapshot.is_dir() + assert rev_parse(snapshot, "HEAD") == base_sha + # The snapshot holds the base content, not the working tree's. + assert "ghp_" not in (snapshot / "CLAUDE.md").read_text() + + assert not snapshot.exists() + assert worktree_count(repo) == 1 + + def test_removed_when_body_raises(self, tmp_path): + repo = git_repo(tmp_path) + sha = rev_parse(repo, "HEAD") + + with pytest.raises(RuntimeError, match="boom"): + with _snapshot_worktree(repo, sha) as snapshot: + assert snapshot.is_dir() + raise RuntimeError("boom") + + assert not snapshot.exists() + assert worktree_count(repo) == 1 + + def test_invalid_sha_is_a_clear_error(self, tmp_path): + repo = git_repo(tmp_path) + + with pytest.raises(GitBaselineError, match="failed to create a temporary worktree"): + with _snapshot_worktree(repo, "0" * 40): + pass # pragma: no cover - the context manager raises first + + assert worktree_count(repo) == 1 + + def test_remove_failure_falls_back_to_prune(self, tmp_path, monkeypatch): + import skillsaw.git_baseline as git_baseline_module + + repo = git_repo(tmp_path) + sha = rev_parse(repo, "HEAD") + + real_git = git_baseline_module._git + + def failing_remove(cwd, *argv): + if argv[:2] == ("worktree", "remove"): + return subprocess.CompletedProcess(argv, 1, "", "simulated remove failure") + return real_git(cwd, *argv) + + monkeypatch.setattr(git_baseline_module, "_git", failing_remove) + + with _snapshot_worktree(repo, sha) as snapshot: + assert snapshot.is_dir() + + # The fallback deletes the checkout and prunes the registration. + assert not snapshot.exists() + assert worktree_count(repo) == 1 + + +# ── build_git_baseline ─────────────────────────────────────────── + + +class TestBuildGitBaseline: + def test_baselines_merge_base_violations(self, tmp_path): + repo = git_repo(tmp_path) + base_sha = rev_parse(repo, "HEAD") + append(repo / "CLAUDE.md", "\nRun `make docs` to rebuild the API reference.\n") + commit_all(repo, "Mention docs build") + + repo_root = repo.resolve() + baseline, merge_base = build_git_baseline( + repo_root, "HEAD~1", fixture_config(repo), [repo_root], "0.0.0-test" + ) + + assert merge_base == base_sha + assert isinstance(baseline, BaselineFile) + # root_path is repointed from the snapshot to the real toplevel so + # current violations fingerprint to the same relative paths. + assert baseline.root_path == repo_root + + by_rule = {} + for entry in baseline.violations: + by_rule.setdefault(entry.rule_id, []).append(entry) + assert len(by_rule["content-weak-language"]) == 2 + assert len(by_rule["context-budget"]) == 1 + + # Entry paths are repo-relative, never snapshot-absolute. + assert {e.file_path for e in baseline.violations} == { + "CLAUDE.md", + ".claude/rules/architecture.md", + } + + # INFO violations are excluded, mirroring `skillsaw baseline`. + assert all(e.severity != "info" for e in baseline.violations) + + # The ratchet entry carries its value and baseline mode. + ratchet = by_rule["context-budget"][0] + assert ratchet.baseline_mode == "ceiling" + assert ratchet.value is not None and ratchet.value > 40 + + assert worktree_count(repo) == 1 + + def test_baseline_suppresses_current_violations_via_linter(self, tmp_path): + repo = git_repo(tmp_path) + append(repo / "CLAUDE.md", SECRET_LINE) # uncommitted new violation + + repo_root = repo.resolve() + config = fixture_config(repo) + baseline, _ = build_git_baseline(repo_root, "HEAD", config, [repo_root], "0.0.0-test") + + context = RepositoryContext( + repo_root, + exclude_patterns=config.exclude_patterns, + content_paths=config.content_paths, + ) + linter = Linter(context, config, baseline=baseline) + kept = [v for v in linter.run() if v.severity != Severity.INFO] + + assert [v.rule_id for v in kept] == ["content-embedded-secrets"] + assert linter.baseline_suppressed_count == 3 + assert linter.stale_baseline_entries == [] + + def test_lint_path_missing_at_merge_base_contributes_nothing(self, tmp_path): + repo = git_repo(tmp_path) + nested = repo / "nested" + nested.mkdir() + (nested / "CLAUDE.md").write_text( + "# Nested Tool Guidelines\n\n" + "Run `make check` before committing changes to this tool.\n" + ) + + repo_root = repo.resolve() + baseline, _ = build_git_baseline( + repo_root, "HEAD", fixture_config(repo), [nested.resolve()], "0.0.0-test" + ) + + assert baseline.violations == [] + assert worktree_count(repo) == 1 + + def test_lint_path_outside_repository_is_an_error(self, tmp_path): + repo = git_repo(tmp_path) + outside = tmp_path / "elsewhere" + outside.mkdir() + + with pytest.raises(GitBaselineError, match="inside the git repository"): + build_git_baseline( + repo.resolve(), "HEAD", fixture_config(repo), [outside.resolve()], "0.0.0-test" + ) + assert worktree_count(repo) == 1 + + def test_bad_rule_selection_is_a_clear_error(self, tmp_path): + repo = git_repo(tmp_path) + repo_root = repo.resolve() + + with pytest.raises(GitBaselineError, match="failed to lint base snapshot"): + build_git_baseline( + repo_root, + "HEAD", + fixture_config(repo), + [repo_root], + "0.0.0-test", + rule_ids={"does-not-exist"}, + ) + assert worktree_count(repo) == 1 + + +# ── CLI --since branch (in-process) ────────────────────────────── + + +def run_lint_in_process(capsys, *argv): + """Drive _run_lint directly through the real parser so coverage sees it.""" + from skillsaw.cli._lint import _run_lint + from skillsaw.cli._parser import _build_parser + + args = _build_parser().parse_args(["lint", *argv]) + with pytest.raises(SystemExit) as exc: + _run_lint(args) + captured = capsys.readouterr() + return exc.value.code or 0, captured.out, captured.err + + +class TestLintSinceCli: + def test_since_with_no_baseline_conflict(self, tmp_path, capsys): + repo = git_repo(tmp_path) + + rc, _, err = run_lint_in_process(capsys, "--since", "HEAD", "--no-baseline", str(repo)) + + assert rc == 1 + assert "--since and --no-baseline cannot be combined" in err + + def test_rule_and_skip_rule_conflict(self, tmp_path, capsys): + repo = git_repo(tmp_path) + + rc, _, err = run_lint_in_process( + capsys, "--rule", "context-budget", "--skip-rule", "content-weak-language", str(repo) + ) + + assert rc == 1 + assert "--rule and --skip-rule cannot be combined" in err + + def test_since_git_error_exits_cleanly(self, tmp_path, capsys): + plain = copy_fixture(tmp_path) + + rc, _, err = run_lint_in_process(capsys, "--since", "HEAD~1", str(plain)) + + assert rc == 1 + assert "requires a git repository" in err + + def test_since_reports_only_new_violations(self, tmp_path, capsys): + repo = git_repo(tmp_path) + append(repo / "CLAUDE.md", SECRET_LINE) + commit_all(repo, "Add deploy token") + + rc, out, _ = run_lint_in_process(capsys, "--format", "json", "--since", "HEAD~1", str(repo)) + + assert rc == 1 + report = json.loads(out) + assert [v["rule_id"] for v in report["violations"]] == ["content-embedded-secrets"] + assert report["summary"]["baseline_suppressed"] == 3 + + def test_since_text_output_header_and_fixed_hint(self, tmp_path, capsys): + repo = git_repo(tmp_path) + merge_base = rev_parse(repo, "HEAD") + content = (repo / "CLAUDE.md").read_text() + content = content.replace("- Try to keep functions under 50 lines.\n", "") + (repo / "CLAUDE.md").write_text(content) + commit_all(repo, "Drop the function-length guideline") + + rc, out, _ = run_lint_in_process(capsys, "--since", "HEAD~1", str(repo)) + + assert rc == 0 + assert f"Comparing against merge-base {merge_base[:12]} (--since HEAD~1)" in out + assert "1 violation(s) fixed since HEAD~1" in out + assert "skillsaw baseline" not in out + + def test_since_verbose_lists_fixed_violations(self, tmp_path, capsys): + repo = git_repo(tmp_path) + content = (repo / "CLAUDE.md").read_text() + content = content.replace("- Try to keep functions under 50 lines.\n", "") + (repo / "CLAUDE.md").write_text(content) + commit_all(repo, "Drop the function-length guideline") + + rc, out, _ = run_lint_in_process(capsys, "-v", "--since", "HEAD~1", str(repo)) + + assert rc == 0 + assert "1 violation(s) fixed since HEAD~1" in out + assert "- content-weak-language [CLAUDE.md]:" in out + + +class TestCommittedBaselineCli: + """The committed-baseline branch that --since takes precedence over.""" + + def _write_committed_baseline(self, repo, capsys): + from skillsaw.cli._baseline import _run_baseline + from skillsaw.cli._parser import _build_parser + + args = _build_parser().parse_args(["baseline", str(repo)]) + with pytest.raises(SystemExit) as exc: + _run_baseline(args) + assert (exc.value.code or 0) == 0 + capsys.readouterr() # discard the "Baselined N violation(s)" line + + def test_committed_baseline_loaded_without_since(self, tmp_path, capsys): + repo = git_repo(tmp_path) + self._write_committed_baseline(repo, capsys) + + rc, out, _ = run_lint_in_process(capsys, "-v", str(repo)) + + assert rc == 0 + assert "Using baseline:" in out + assert "(3 entries)" in out + + def test_committed_baseline_stale_hint_suggests_refresh(self, tmp_path, capsys): + repo = git_repo(tmp_path) + self._write_committed_baseline(repo, capsys) + content = (repo / "CLAUDE.md").read_text() + content = content.replace("- Try to keep functions under 50 lines.\n", "") + (repo / "CLAUDE.md").write_text(content) + + rc, out, _ = run_lint_in_process(capsys, str(repo)) + + assert rc == 0 + assert "Baseline: 1 stale entry (violations resolved since baseline was set)" in out + assert "Run `skillsaw baseline` to update." in out + + def test_corrupt_committed_baseline_warns_and_continues(self, tmp_path, capsys): + repo = git_repo(tmp_path) + (repo / ".skillsaw-baseline.json").write_text("{not json") + + rc, _, err = run_lint_in_process(capsys, str(repo)) + + # The fixture only has warning-level violations, so the lint still + # passes; the unreadable baseline is a warning, not a crash. + assert rc == 0 + assert "Warning: Failed to load baseline:" in err From b869de02d8251034f427793f73d6b0fc3e10ec37 Mon Sep 17 00:00:00 2001 From: Stephen Benjamin Date: Mon, 6 Jul 2026 19:53:48 -0400 Subject: [PATCH 3/4] Address Gemini review: propagate --type to the base lint, harden refs - Propagate --type repo-type overrides to the merge-base snapshot lint (new repo_types parameter on build_git_baseline). Without it the base and current lints could run different rule sets, reporting spurious "new" violations for rules enabled only by the override. Regression tests verified to fail without the fix. - Reject --since refs that are empty or start with "-" before they reach git, where they would parse as command-line flags. - Make repo_toplevel robust to file paths (use the parent directory as git's cwd). Not reachable via the CLI (_resolve_lint_paths converts files to parent dirs first) but defensive for direct API callers. - Use Path.as_uri() instead of f"file://{path}" in the shallow-clone tests for Windows-safe URLs (a plain local path would make git silently ignore --depth, defeating the test). Co-Authored-By: Claude Fable 5 --- src/skillsaw/cli/_lint.py | 1 + src/skillsaw/git_baseline.py | 23 +++++++++++++++----- tests/test_git_baseline.py | 41 ++++++++++++++++++++++++++++++++++-- tests/test_lint_since.py | 32 +++++++++++++++++++++++++++- 4 files changed, 89 insertions(+), 8 deletions(-) diff --git a/src/skillsaw/cli/_lint.py b/src/skillsaw/cli/_lint.py index d6d82dff..e76c9884 100644 --- a/src/skillsaw/cli/_lint.py +++ b/src/skillsaw/cli/_lint.py @@ -138,6 +138,7 @@ def _run_lint(args): config, paths, cli_version, + repo_types=override_types, rule_ids=rule_ids, skip_rule_ids=skip_rule_ids, no_custom_rules=args.no_custom_rules, diff --git a/src/skillsaw/git_baseline.py b/src/skillsaw/git_baseline.py index b037c2e2..127aa26d 100644 --- a/src/skillsaw/git_baseline.py +++ b/src/skillsaw/git_baseline.py @@ -43,7 +43,7 @@ from .baseline import BaselineFile, build_baseline from .config import LinterConfig -from .context import RepositoryContext +from .context import RepositoryContext, RepositoryType from .linter import Linter from .rule import RuleViolation, Severity @@ -72,7 +72,10 @@ def _git(cwd: Path, *argv: str) -> subprocess.CompletedProcess: def repo_toplevel(path: Path) -> Path: """Resolve the git working-tree toplevel containing *path*.""" - result = _git(path, "rev-parse", "--show-toplevel") + # git needs a directory as cwd; with a file path subprocess would raise + # NotADirectoryError instead of producing a clean error. + cwd = path if path.is_dir() else path.parent + result = _git(cwd, "rev-parse", "--show-toplevel") if result.returncode != 0: raise GitBaselineError( f"--since requires a git repository, but {path} is not inside one " @@ -88,6 +91,12 @@ def _is_shallow(repo_root: Path) -> bool: def resolve_merge_base(repo_root: Path, ref: str) -> str: """Resolve the merge-base of HEAD and *ref*, with a precise error on failure.""" + # A ref starting with "-" would be parsed by git as a command-line flag + # rather than a revision — reject it before it reaches git. + if not ref or ref.startswith("-"): + raise GitBaselineError( + f"--since: invalid ref '{ref}' (refs must not be empty or start with '-')" + ) result = _git(repo_root, "merge-base", "HEAD", ref) if result.returncode == 0: return result.stdout.strip() @@ -139,6 +148,7 @@ def build_git_baseline( lint_paths: Sequence[Path], version_string: str, *, + repo_types: Optional[Set[RepositoryType]] = None, rule_ids: Optional[Set[str]] = None, skip_rule_ids: Optional[Set[str]] = None, no_custom_rules: bool = False, @@ -149,9 +159,11 @@ def build_git_baseline( Each lint path is mapped to its location inside the snapshot worktree (paths that do not exist at the merge-base simply contribute no baseline entries). The snapshot is linted with the caller's *config* - (from the current working tree) and rule selection flags, INFO - violations are dropped, and ratchet ``baseline_mode``\\ s are captured — - all mirroring ``skillsaw baseline`` semantics. + (from the current working tree), *repo_types* override (``--type``), + and rule selection flags, INFO violations are dropped, and ratchet + ``baseline_mode``\\ s are captured — all mirroring ``skillsaw baseline`` + semantics. The base and current lints must use the same rule set, or + rules enabled only on one side would report spurious differences. Returns ``(baseline, merge_base_sha)`` where ``baseline.root_path`` is already pointed at *repo_root* so current violations fingerprint to @@ -185,6 +197,7 @@ def build_git_baseline( context = RepositoryContext( mapped, + repo_types=repo_types, exclude_patterns=config.exclude_patterns, content_paths=config.content_paths, ) diff --git a/tests/test_git_baseline.py b/tests/test_git_baseline.py index 071f8ff8..371d712d 100644 --- a/tests/test_git_baseline.py +++ b/tests/test_git_baseline.py @@ -17,7 +17,7 @@ from skillsaw.baseline import BaselineFile from skillsaw.config import LinterConfig -from skillsaw.context import RepositoryContext +from skillsaw.context import RepositoryContext, RepositoryType from skillsaw.git_baseline import ( GitBaselineError, _snapshot_worktree, @@ -59,6 +59,12 @@ def test_resolves_toplevel_from_subdirectory(self, tmp_path): repo = git_repo(tmp_path) assert repo_toplevel(repo / ".claude" / "rules") == repo.resolve() + def test_resolves_toplevel_from_file_path(self, tmp_path): + # Robustness for direct API callers: a file path must not crash + # subprocess with NotADirectoryError. + repo = git_repo(tmp_path) + assert repo_toplevel(repo / "CLAUDE.md") == repo.resolve() + def test_not_a_git_repo(self, tmp_path): plain = copy_fixture(tmp_path) with pytest.raises(GitBaselineError, match="requires a git repository"): @@ -90,13 +96,26 @@ def test_unknown_ref(self, tmp_path): resolve_merge_base(repo, "no-such-branch") assert "no-such-branch" in str(exc.value) + def test_ref_starting_with_dash_rejected(self, tmp_path): + repo = git_repo(tmp_path) + # "-foo" would be parsed by git as a flag, not a revision. + with pytest.raises(GitBaselineError, match="invalid ref"): + resolve_merge_base(repo, "-foo") + + def test_empty_ref_rejected(self, tmp_path): + repo = git_repo(tmp_path) + with pytest.raises(GitBaselineError, match="invalid ref"): + resolve_merge_base(repo, "") + def test_shallow_clone_mentions_fetching_history(self, tmp_path): origin = git_repo(tmp_path) append(origin / "CLAUDE.md", "\nRun `make docs` to rebuild the API reference.\n") commit_all(origin, "Mention docs build") clone = tmp_path / "shallow" - git(tmp_path, "clone", "-q", "--depth", "1", f"file://{origin}", str(clone)) + # as_uri() builds a portable file:// URL (a plain local path would + # make git silently ignore --depth and produce a full clone). + git(tmp_path, "clone", "-q", "--depth", "1", origin.as_uri(), str(clone)) with pytest.raises(GitBaselineError, match="shallow") as exc: resolve_merge_base(clone, "HEAD~1") @@ -229,6 +248,24 @@ def test_baseline_suppresses_current_violations_via_linter(self, tmp_path): assert linter.baseline_suppressed_count == 3 assert linter.stale_baseline_entries == [] + def test_repo_types_override_applies_to_snapshot(self, tmp_path): + repo = git_repo(tmp_path) + repo_root = repo.resolve() + + baseline, _ = build_git_baseline( + repo_root, + "HEAD", + fixture_config(repo), + [repo_root], + "0.0.0-test", + repo_types={RepositoryType.SINGLE_PLUGIN}, + ) + + # plugin-json-required only fires under the single-plugin type; + # its presence proves the snapshot lint honored the override + # instead of auto-detecting dot-claude. + assert "plugin-json-required" in {e.rule_id for e in baseline.violations} + def test_lint_path_missing_at_merge_base_contributes_nothing(self, tmp_path): repo = git_repo(tmp_path) nested = repo / "nested" diff --git a/tests/test_lint_since.py b/tests/test_lint_since.py index 5222d127..9f0528b6 100644 --- a/tests/test_lint_since.py +++ b/tests/test_lint_since.py @@ -190,6 +190,23 @@ def test_line_drift_keeps_legacy_violations_suppressed(tmp_path): assert suppressed(r) == 3 +def test_type_override_applies_to_merge_base_lint(tmp_path): + repo = git_repo(tmp_path) + append(repo / "CLAUDE.md", "\nRun `make docs` to rebuild the API reference.\n") + commit_all(repo, "Mention docs build") + + # --type single-plugin enables rules (plugin-json-required, + # plugin-readme) that dot-claude auto-detection would not. The + # override must apply to the merge-base snapshot too, or those rules + # would fire only on the current tree and report spurious "new" + # violations for an unchanged repo. + r = run_lint(repo, "--since", "HEAD~1", "--type", "single-plugin", "--strict") + + assert r["rc"] == 0, r["stdout"] + assert violations(r) == [] + assert suppressed(r) > 0 + + # ── Ratchet composition ────────────────────────────────────────── @@ -262,13 +279,26 @@ def test_unknown_ref_is_a_clear_error(tmp_path): assert "Traceback" not in r["stderr"] +def test_ref_starting_with_dash_is_rejected(tmp_path): + repo = git_repo(tmp_path) + + # A ref like "-foo" must not reach git, where it would parse as a flag. + r = run_lint(repo, "--since=-foo") + + assert r["rc"] == 1 + assert "invalid ref" in r["stderr"] + assert "Traceback" not in r["stderr"] + + def test_shallow_clone_error_suggests_fetching_history(tmp_path): origin = git_repo(tmp_path) append(origin / "CLAUDE.md", "\nRun `make docs` to rebuild the API reference.\n") commit_all(origin, "Mention docs build") clone = tmp_path / "shallow" - git(tmp_path, "clone", "-q", "--depth", "1", f"file://{origin}", str(clone)) + # as_uri() builds a portable file:// URL (a plain local path would make + # git silently ignore --depth and produce a full, non-shallow clone). + git(tmp_path, "clone", "-q", "--depth", "1", origin.as_uri(), str(clone)) r = run_lint(clone, "--since", "HEAD~1") From e835d77d0de78e4c923ca616c9c6abbac5a317d4 Mon Sep 17 00:00:00 2001 From: Stephen Benjamin Date: Mon, 6 Jul 2026 19:56:33 -0400 Subject: [PATCH 4/4] Panel follow-ups: multi-path --since test, join split log literal Adds the review panel's suggested end-to-end test for --since with two sibling lint paths (one shared ephemeral baseline, suppression counted across both), and joins the accidentally split log-message literal in git_baseline.py. Co-Authored-By: Claude Fable 5 --- src/skillsaw/git_baseline.py | 2 +- tests/test_lint_since.py | 24 ++++++++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/src/skillsaw/git_baseline.py b/src/skillsaw/git_baseline.py index 127aa26d..295bdc69 100644 --- a/src/skillsaw/git_baseline.py +++ b/src/skillsaw/git_baseline.py @@ -189,7 +189,7 @@ def build_git_baseline( # The path was added after the merge-base: the base # contributes no violations for it. logger.info( - "--since: %s does not exist at merge-base %s; " "no baseline entries for it", + "--since: %s does not exist at merge-base %s; no baseline entries for it", rel, merge_base[:12], ) diff --git a/tests/test_lint_since.py b/tests/test_lint_since.py index 9f0528b6..e00b1c63 100644 --- a/tests/test_lint_since.py +++ b/tests/test_lint_since.py @@ -381,6 +381,30 @@ def test_uncommitted_new_violation_is_caught(tmp_path): assert rule_ids(r) == {"content-embedded-secrets"} +def test_multiple_lint_paths_share_one_ephemeral_baseline(tmp_path): + repo = git_repo(tmp_path) + alpha = repo / "tools" / "alpha" + beta = repo / "tools" / "beta" + for tool in (alpha, beta): + tool.mkdir(parents=True) + (tool / "CLAUDE.md").write_text( + f"# {tool.name} Guidelines\n\n" + "Run `make check` before committing changes to this tool.\n" + "Try to keep the command wrappers thin.\n" + ) + commit_all(repo, "Add tool guides") + append(alpha / "CLAUDE.md", SECRET_LINE) + commit_all(repo, "Add deploy token") + + r = run_lint(alpha, str(beta), "--since", "HEAD~1") + + assert r["rc"] == 1 + assert rule_ids(r) == {"content-embedded-secrets"} + # One legacy weak-language violation per sibling directory. + assert suppressed(r) == 2 + assert worktree_count(repo) == 1 + + def test_lint_path_missing_at_merge_base_contributes_no_baseline(tmp_path): repo = git_repo(tmp_path) nested = repo / "nested"