Add git-aware diff linting: skillsaw lint --since REF - #401
Conversation
`--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 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds ChangesLint --since feature
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the --since flag to skillsaw lint, enabling users to lint only changes introduced since a specific git reference by building an ephemeral baseline from the merge-base in a temporary worktree. The feedback highlights several critical improvements: resolving potential crashes when repo_toplevel is called with a file path, ensuring repository type overrides are correctly propagated to the merge-base snapshot linting process, securing git commands against argument injection using --, and fixing a Windows portability issue in the test suite.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| 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() |
There was a problem hiding this comment.
If path is a file (e.g., when linting a specific file like skillsaw lint my-skill/SKILL.md), _git will be called with a file path as the cwd. This causes subprocess.run to raise a NotADirectoryError (or OSError on Windows) and crash with a traceback.
To prevent this, resolve the path and use its parent directory if it is not a directory.
| 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 repo_toplevel(path: Path) -> Path: | |
| """Resolve the git working-tree toplevel containing *path*.""" | |
| resolved_path = path.resolve() | |
| cwd = resolved_path if resolved_path.is_dir() else resolved_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 " | |
| f"({result.stderr.strip() or 'git rev-parse failed'})" | |
| ) | |
| return Path(result.stdout.strip()).resolve() |
There was a problem hiding this comment.
Fixed in b869de0 — repo_toplevel now uses path if path.is_dir() else path.parent as git's cwd, with a direct unit test. Note this wasn't reachable via the CLI (_resolve_lint_paths converts file arguments to their parent directories before this code runs, verified with a live skillsaw lint CLAUDE.md --since HEAD~1 run), but the helper is now robust for direct API callers.
|
|
||
| from .baseline import BaselineFile, build_baseline | ||
| from .config import LinterConfig | ||
| from .context import RepositoryContext |
There was a problem hiding this comment.
Import RepositoryType so it can be used to type-annotate the repo_types parameter in build_git_baseline.
| from .context import RepositoryContext | |
| from .context import RepositoryContext, RepositoryType |
There was a problem hiding this comment.
Done in b869de0 — RepositoryType is imported and used to annotate the new repo_types parameter.
| 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]: |
There was a problem hiding this comment.
Add repo_types to the build_git_baseline signature so that repository type overrides (e.g., specified via --type) are correctly propagated when linting the merge-base snapshot.
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,
repo_types: Optional[Set[RepositoryType]] = None,
) -> Tuple[BaselineFile, str]:There was a problem hiding this comment.
Done in b869de0 — build_git_baseline now takes repo_types: Optional[Set[RepositoryType]] = None and documents that base and current lints must use the same rule set.
| context = RepositoryContext( | ||
| mapped, | ||
| exclude_patterns=config.exclude_patterns, | ||
| content_paths=config.content_paths, | ||
| ) |
There was a problem hiding this comment.
Pass repo_types to RepositoryContext to ensure the merge-base snapshot is analyzed with the same repository type overrides as the current working tree.
context = RepositoryContext(
mapped,
repo_types=repo_types,
exclude_patterns=config.exclude_patterns,
content_paths=config.content_paths,
)There was a problem hiding this comment.
Done in b869de0 — the snapshot RepositoryContext now receives repo_types. Added regression tests (subprocess and in-process) that fail without the fix: linting an unchanged repo with --type single-plugin --since HEAD~1 previously reported spurious new plugin-json-required/plugin-readme violations because the snapshot auto-detected dot-claude.
| 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, | ||
| ) |
There was a problem hiding this comment.
Pass override_types (which holds the --type overrides) to build_git_baseline so that the merge-base snapshot is linted with the correct repository types.
| 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, | |
| ) | |
| 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, | |
| repo_types=override_types, | |
| ) |
There was a problem hiding this comment.
Done in b869de0 — the call site now passes repo_types=override_types so --type applies to the merge-base snapshot lint too. Covered by test_type_override_applies_to_merge_base_lint, which fails without the fix.
|
|
||
| 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) |
There was a problem hiding this comment.
Use -- to separate git options from positional arguments (the ref). This prevents potential argument injection if a user-controlled ref starts with a hyphen (e.g., --help or other flags).
| result = _git(repo_root, "merge-base", "HEAD", ref) | |
| result = _git(repo_root, "merge-base", "HEAD", "--", ref) |
There was a problem hiding this comment.
Fixed in b869de0 — resolve_merge_base now rejects empty refs and refs starting with - before they reach git, with a clear invalid ref error. Tests cover --since=-foo end-to-end (clean exit 1, no traceback) and the helper directly.
| commit_all(origin, "Mention docs build") | ||
|
|
||
| clone = tmp_path / "shallow" | ||
| git(tmp_path, "clone", "-q", "--depth", "1", f"file://{origin}", str(clone)) |
There was a problem hiding this comment.
Using file://{origin} will fail on Windows because the path contains backslashes (e.g., C:\path\to\origin), resulting in an invalid URL. Passing str(origin) directly is fully portable across Windows, macOS, and Linux.
| git(tmp_path, "clone", "-q", "--depth", "1", f"file://{origin}", str(clone)) | |
| git(tmp_path, "clone", "-q", "--depth", "1", str(origin), str(clone)) |
There was a problem hiding this comment.
Fixed in b869de0 using Path.as_uri(), which produces a portable file:/// URL on all platforms. Note we can't pass the plain local path here: git silently ignores --depth for local-path clones ("--depth is ignored in local clones; use file:// instead"), which would produce a full clone and defeat the shallow-error test.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #401 +/- ##
==========================================
+ Coverage 93.28% 93.41% +0.13%
==========================================
Files 144 145 +1
Lines 11014 11114 +100
==========================================
+ Hits 10274 10382 +108
+ Misses 740 732 -8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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 <noreply@anthropic.com>
github-actions
Bot
removed
the
panel-review
Review Panel VerdictDisposition: APPROVE Specialist FindingsArchitecture Reviewer
Python Expert
Security & Supply Chain Reviewer
QA Engineer
Technical Writer
Ecosystem Reviewer
Panel SynthesisA well-engineered, well-tested, well-documented feature that reuses the existing baseline fingerprint/subtraction machinery instead of inventing a parallel path — squarely aligned with the panel's "existing patterns over novel ones" bias. Worktree lifecycle and error handling are careful, error messages are actionable and traceback-free, and the 41 new tests pass while covering the failure surface thoroughly. No BLOCKING findings. The one substantive item — Required Actions Before MergeNone. Approved. Optional Follow-ups
Generated by skillsaw-review-panel |
- 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_lint_since.py (1)
54-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
copy_fixturesignature diverges from documented convention.Guidelines describe a
copy_fixture(name, tmp_path)helper, but this definescopy_fixture(tmp_path)hardcoded to thelint-sincefixture. Since only one fixture is used in this suite it works, but it diverges from the documented shared helper contract and won't generalize if another fixture is added later.As per path instructions: "Use
copy_fixture(name, tmp_path)to copy fixture repos into a temporary directory before running tests."🤖 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_lint_since.py` around lines 54 - 57, The helper `copy_fixture` currently hardcodes the `lint-since` fixture and only accepts `tmp_path`, which diverges from the shared test convention. Update `copy_fixture` to accept both `name` and `tmp_path`, and use `name` to select the fixture directory when calling `shutil.copytree`, then adjust the `test_lint_since` callers to pass the fixture name so the helper matches the documented `copy_fixture(name, tmp_path)` contract.Source: Path instructions
🤖 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 `@docs/ci.md`:
- Around line 136-137: The fallback fetch example uses BASE_REF without defining
it first, so update the docs snippet to either inline the GitHub base-branch
expression directly in the git fetch command or introduce BASE_REF before the
fetch call. Keep the guidance centered on the ci.md example so readers can copy
it safely without relying on an unset variable.
In `@tests/test_git_baseline.py`:
- Around line 419-431: The test may be reusing cached file contents after
rewriting CLAUDE.md in the same process, which can make the re-lint use stale
data. Update the test around test_committed_baseline_stale_hint_suggests_refresh
and _write_committed_baseline so the file-read cache is invalidated after writes
or avoid reusing the same path by using a fresh file/path per scenario before
calling run_lint_in_process. Ensure the second lint is forced to read the
updated CLAUDE.md contents, not the pre-edit version.
---
Nitpick comments:
In `@tests/test_lint_since.py`:
- Around line 54-57: The helper `copy_fixture` currently hardcodes the
`lint-since` fixture and only accepts `tmp_path`, which diverges from the shared
test convention. Update `copy_fixture` to accept both `name` and `tmp_path`, and
use `name` to select the fixture directory when calling `shutil.copytree`, then
adjust the `test_lint_since` callers to pass the fixture name so the helper
matches the documented `copy_fixture(name, tmp_path)` contract.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 85ced3e0-06c1-48ae-8352-9c61e1729075
⛔ Files ignored due to path filters (3)
tests/fixtures/lint-since/.claude/rules/architecture.mdis excluded by!tests/fixtures/**tests/fixtures/lint-since/.skillsaw.yamlis excluded by!tests/fixtures/**tests/fixtures/lint-since/CLAUDE.mdis excluded by!tests/fixtures/**
📒 Files selected for processing (10)
README.mdaction.ymldocs/baseline.mddocs/ci.mddocs/cli.mdsrc/skillsaw/cli/_lint.pysrc/skillsaw/cli/_parser.pysrc/skillsaw/git_baseline.pytests/test_git_baseline.pytests/test_lint_since.py
| (Instead of `fetch-depth: 0`, fetching just the base branch also works: | ||
| `git fetch origin "$BASE_REF"`.) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Define BASE_REF before using it.
BASE_REF is never set in this snippet, so the fallback fetch command fails if someone copies it verbatim. Inline the GitHub expression or define the variable first.
💡 Proposed fix
-(Instead of `fetch-depth: 0`, fetching just the base branch also works:
-`git fetch origin "$BASE_REF"`.)
+(Instead of `fetch-depth: 0`, fetching just the base branch also works:
+`git fetch origin "${{ github.base_ref }}"`.)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (Instead of `fetch-depth: 0`, fetching just the base branch also works: | |
| `git fetch origin "$BASE_REF"`.) | |
| (Instead of `fetch-depth: 0`, fetching just the base branch also works: | |
| `git fetch origin "${{ github.base_ref }}"`.) |
🤖 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 `@docs/ci.md` around lines 136 - 137, The fallback fetch example uses BASE_REF
without defining it first, so update the docs snippet to either inline the
GitHub base-branch expression directly in the git fetch command or introduce
BASE_REF before the fetch call. Keep the guidance centered on the ci.md example
so readers can copy it safely without relying on an unset variable.
| 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 | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Potential stale file-read cache between baseline write and re-lint.
_write_committed_baseline runs _run_baseline in-process (reading CLAUDE.md), then the test overwrites CLAUDE.md and calls run_lint_in_process in the same process. If the utils file-read cache is keyed by path and not invalidated on write, the second lint could see the pre-edit content, silently passing or failing the "stale entry" assertion for the wrong reason.
Based on learnings: "the cache can return stale content and the re-lint may incorrectly use the old contents" when a test overwrites the same file path multiple times within a single test. Consider verifying the cache is invalidated on write, or writing to a distinct path/tmp_path per scenario to be safe.
🤖 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_git_baseline.py` around lines 419 - 431, The test may be reusing
cached file contents after rewriting CLAUDE.md in the same process, which can
make the re-lint use stale data. Update the test around
test_committed_baseline_stale_hint_suggests_refresh and
_write_committed_baseline so the file-read cache is invalidated after writes or
avoid reusing the same path by using a fresh file/path per scenario before
calling run_lint_in_process. Ensure the second lint is forced to read the
updated CLAUDE.md contents, not the pre-edit version.
Source: Learnings
3 participants
What
skillsaw lint --since origin/mainreports only the violations your changes introduced. Zero setup: no committed.skillsaw-baseline.json, nothing to regenerate, nothing to go stale.A repo with hundreds of legacy violations, you add one embedded secret,
--sincereports exactly that one and exits 1.Why
Legacy noise is the #1 adoption blocker: pointing skillsaw at an existing repo produces a wall of pre-existing violations, and CI either fails on all of them or requires committing and continually refreshing a baseline file.
--sincekills both problems — CI fails only on your own regressions, and there is no baseline file to drift out of date.How it works
Not diff line ranges — an ephemeral git baseline:
git worktree(always removed, even on failure).--sincemeasures how the repo changed, not how the rule config changed).BaselineFilefrom the result (INFO dropped, ratchetbaseline_modes captured — exactskillsaw baselinesemantics) and feed it through the existing baseline subtraction.The key trick: entries are fingerprinted against the snapshot toplevel, then
baseline.root_pathis pointed at the real repo toplevel — current violations fingerprint to identical relative paths, so everything downstream (suppressed counts, stale entries, formatters, grade, exit codes) works unchanged.Because baseline fingerprints hash rule ID + relative path + stripped line content (never line numbers):
context-budgetwith the delta; shrinking it stays quiet.Baseline: N violation(s) fixed since REF.--sincetakes precedence over a committed.skillsaw-baseline.jsonand is rejected when combined with--no-baseline.GitHub Action
New
sinceinput (default''— behavior is byte-compatible when unset):Composes with the existing
skillsaw/reviewworkflow_run action: the uploaded report only contains the PR's own regressions, so inline comments shrink accordingly.Limitations (documented in docs/baseline.md)
fetch-depth: 0/git fetch --unshallow.Test coverage
16 new end-to-end tests (
tests/test_lint_since.py) against a new static fixture (tests/fixtures/lint-since/, git-initialized per-test since fixtures can't contain.git):--since+--no-baseline→ error; non-git dir, unknown ref, shallow clone → precise errors, no tracebacks--sincewins over a committed.skillsaw-baseline.jsongit worktree listshows only the main worktree)Full suite: 2484 passed. Verified against a real repo (
openshift-eng/ai-helpers): default lint exits 0;--since HEAD~1on the shallow clone produces the precise shallow-history error, and aftergit fetch --deepen 2resolves the merge-base and exits 0 with the worktree cleaned up.🤖 Generated with Claude Code
Summary by CodeRabbit
skillsaw lint --since REFto report only violations introduced since the merge-base ofHEADand the chosen ref.sinceinput to enable the same behavior in CI.--sincecan’t be combined with--no-baseline; lint exits with an error.--sincebehavior, formatting, precedence, and cleanup.