CM-72985 Add scan local-diff command for IDE plugin integration - #550
Open
aaron-butler-cy-int wants to merge 10 commits into
Open
aaron-butler-cy-int wants to merge 10 commits into
aaron-butler-cy-int wants to merge 10 commits into
Conversation
Diffs a git ref (default HEAD) against the current working directory -- staged, unstaged, and untracked changes -- with optional path scoping, for secret/SCA/SAST scans. Existing commands don't fit an IDE plugin wanting real-time feedback: commit-history only diffs between two commits, and pre-commit only looks at staged changes against HEAD. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
aaron-butler-cy-int
requested review from
avishaiamiel and
omer-roth
as code owners
September 17, 2026 20:02
Two lines collapsed to fit within the 120-char line length after ruff format normalization; CI caught this via `ruff format --check`. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Two platform-specific test bugs surfaced by the Windows/Python 3.10 CI job: - test_combines_staged_and_unstaged_changes wrote fixture files in default text mode, which translates '\n' to the OS line ending on write. On Windows this committed a CRLF blob, and reading it back via `git show` (which does not apply newline translation) surfaced a trailing '\r' that the hardcoded 'line1' assertion didn't expect. Fixed by opening the fixture files with newline='' so the written bytes are exactly what's given, matching the pattern already used for content in other tests here. - test_relative_path_argument_is_resolved_to_absolute compared a Click-resolved path against a raw tempfile path without normalizing either side. Windows can spell the same directory two ways (8.3 short name vs long name), so the two originally-identical paths diverged after each went through its own resolution step. Fixed by wrapping both sides in os.path.realpath(), the same pattern already used successfully by the sibling TestResolveRepoRoot and TestLocalDiffCommandFromSubdirectory tests in this same file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The Windows/Python 3.9 CI job showed patch('os.getcwd', ...) has no
reliable effect on Click's internal path resolution there: the resolved
path came back rooted at the real CI checkout directory (D:\a\...)
instead of the patched temp dir, meaning the mock was silently bypassed
rather than merely producing a differently-spelled equivalent path (which
is what happened on Windows/3.10 with 8.3 short names).
Replaced both patch('os.getcwd', ...) call sites with monkeypatch.chdir(),
which changes the process's actual working directory instead of mocking
one specific accessor, so it's honored consistently regardless of which
underlying API a given Python/platform combination uses to resolve
relative paths.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The chdir() fix from the previous commit introduced a new Windows-only
failure: monkeypatch only restores the cwd at fixture teardown, which
runs after the test function returns. But temporary_git_repository()'s
tempfile.TemporaryDirectory() cleanup runs inside the test function, at
the end of the `with` block -- while the process cwd was still pointed
at (or inside) that very directory. Windows refuses to delete a directory
that is the current working directory, raising PermissionError: WinError
32 ("used by another process").
Fixed by explicitly chdir-ing back to the original cwd immediately after
the CliRunner invocation, before the `with temporary_git_repository()`
block ends and triggers cleanup.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
PO feedback on the local-diff PR: it's an edge case of pre-commit, not a new scan mode; the two implementations must have full scanner parity (pre-commit's deleted-line filtering and SCA timeout weren't reaching local-diff); and maintaining two near-duplicate scan modes forever isn't worth it. Decision: fold the capability into pre-commit as opt-in flags instead of a separate command. Untracked-file scanning is dropped entirely per follow-up direction. - pre-commit gains --base-ref (default HEAD, so the git hook's default invocation is untouched), --include-unstaged, and --path (repeatable, new named option -- NOT the existing hidden positional argument, since the pre-commit framework already auto-passes staged filenames there and reusing that slot as a path filter would silently change hook behavior for every existing user). - Unified get_pre_commit_modified_documents/get_local_diff_documents into one collector, dispatching to three cases: the untouched default (staged index vs HEAD), include_unstaged (base_ref vs full working tree, reusing the proven local-diff mechanism), and the one genuinely new combination -- staged-only vs an arbitrary base_ref, verified against raw `git diff --cached` to confirm correct diff direction before relying on it. - Collapsed the parallel *_pre_commit/*_local_diff scanner-dispatch functions into one set, which is what actually fixes the parity complaints: every invocation now reports ctx.info_name == 'pre-commit', so the existing hard-coded deleted-line exclusion and SCA timeout apply uniformly with no new logic needed. - Removed the standalone `local-diff` command and its scan-type identity. "Local diff scanning" remains as a documented IDE-facing use case of `pre-commit --include-unstaged`, not a command name. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
omer-roth
reviewed
Sep 30, 2026
omer-roth
left a comment
omer-roth
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed against main: read every hunk with surrounding context and empirically verified the new git-diff logic against a scratch repository using this project's .venv GitPython 3.1.62. The branch's own test suite passes (78 tests).
Verified correct, since these were the highest-risk parts of the change: diff polarity is right in every new mode (commit.diff(None) and commit.diff() both yield a=base_ref, b=new, matching the legacy index.diff('HEAD', R=True) path, so added lines show as +). Path scoping works for all three diff variants, the empty-tree fallback resolves and diffs correctly, the _SCAN_TYPE_TO_PRE_COMMIT_HANDLER dispatch signatures line up, and -b doesn't collide with anything in the scan app.
The main thing worth fixing before merge is the perf regression on the default pre-commit hook path — it affects every existing user, not only the new flags. commit_range_documents.py:493 has a fix that addresses it for all three scan types at once. The rest is polish.
Review assisted by Claude Code.
omer-roth
reviewed
Sep 30, 2026
Fixes 5 issues raised in review of cycodehq#550: - Replace `git show <ref>:<path>` subprocess calls with in-process `diff.a_blob.data_stream.read()` in get_pre_commit_modified_documents. This removes a process spawn per changed file from the default hook path (not just the new --base-ref/--include-unstaged flags), and incidentally fixes a stale-resolved_ref bug and a guaranteed-failing git show against the empty tree on a brand-new repo. - Add collect_file_contents=False opt-out so the secret-scan dispatch path (which never used file contents) skips the read entirely. - Revert the base-ref side of the loop back to `if file_content:` (truthy) to match the working-copy side and preserve prior behavior of skipping empty files; commented to explain the deliberate divergence from get_commit_range_modified_documents's `is not None`. - Fix a stale test docstring in TestPreCommitCommandPathResolution that described a failure mode which no longer exists. - Add ScanPathOutsideRepositoryError and UnresolvedGitRefError so a bad --base-ref or an out-of-repo --path go through the CLI's normal error envelope (handle_scan_exception) instead of a raw typer.BadParameter or an unfriendly git error message, which matters for IDE plugins parsing `-o json` output. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
windows-latest/3.11 failed test_relative_path_option_is_resolved_to_absolute after the previous commit's --path-outside-repo check. GitPython's working_tree_dir can come back in Windows' 8.3 short-name form (e.g. RUNNE~1), while Click's resolve_path=True expands --path to the long form, so os.path.commonpath saw two different-looking prefixes and wrongly flagged an in-repo path as outside the repository. Normalize both sides with os.path.realpath before the comparison. Added a cross-platform regression test that reproduces the same class of mismatch via an unresolved ".." path segment (since 8.3 short names are Windows-only), plus a test for the actual outside-repo rejection path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
2 participants
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.
Diffs a git ref (default HEAD) against the current working directory -- staged, unstaged, and untracked changes -- with optional path scoping, for secret/SCA/SAST scans. Existing commands don't fit an IDE plugin wanting real-time feedback: commit-history only diffs between two commits, and pre-commit only looks at staged changes against HEAD.