From cc099777570fa25edb3f7ec30bf5d5ca93a8208c Mon Sep 17 00:00:00 2001 From: Aaron Butler Date: Thu, 17 Sep 2026 15:59:48 -0400 Subject: [PATCH 1/8] CM-72985 Add scan local-diff command for IDE plugin integration 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 --- README.md | 69 +++++++- cycode/cli/apps/scan/__init__.py | 6 + cycode/cli/apps/scan/commit_range_scanner.py | 101 +++++++++++ cycode/cli/apps/scan/local_diff/__init__.py | 0 .../scan/local_diff/local_diff_command.py | 81 +++++++++ cycode/cli/consts.py | 2 + .../files_collector/commit_range_documents.py | 86 +++++++++ .../files_collector/sca/sca_file_collector.py | 11 ++ .../scan/test_commit_range_scanner.py | 55 ++++++ .../commands/scan/test_local_diff_command.py | 150 ++++++++++++++++ .../test_commit_range_documents.py | 164 ++++++++++++++++++ 11 files changed, 717 insertions(+), 8 deletions(-) create mode 100644 cycode/cli/apps/scan/local_diff/__init__.py create mode 100644 cycode/cli/apps/scan/local_diff/local_diff_command.py create mode 100644 tests/cli/commands/scan/test_local_diff_command.py diff --git a/README.md b/README.md index da428ec8..b149e252 100644 --- a/README.md +++ b/README.md @@ -47,8 +47,10 @@ This guide walks you through both installation and usage. 1. [Terraform Plan Scan](#terraform-plan-scan) 4. [Commit History Scan](#commit-history-scan) 1. [Commit Range Option (Diff Scanning)](#commit-range-option-diff-scanning) - 5. [Pre-Commit Scan](#pre-commit-scan) - 6. [Pre-Push Scan](#pre-push-scan) + 5. [Local Diff Scan](#local-diff-scan) + 1. [What Gets Scanned](#what-gets-scanned) + 6. [Pre-Commit Scan](#pre-commit-scan) + 7. [Pre-Push Scan](#pre-push-scan) 2. [Scan Results](#scan-results) 1. [Show/Hide Secrets](#showhide-secrets) 2. [Soft Fail](#soft-fail) @@ -798,12 +800,13 @@ The Cycode CLI application offers several types of scans so that you can choose | `--maven-settings-file` | For Maven only, allows using a custom [settings.xml](https://maven.apache.org/settings.html) file when scanning for dependencies | | `--help` | Show options for given command. | -| Command | Description | -|----------------------------------------|-----------------------------------------------------------------------| -| [commit-history](#commit-history-scan) | Scan commit history or perform diff scanning between specific commits | -| [path](#path-scan) | Scan the files in the path supplied in the command | -| [pre-commit](#pre-commit-scan) | Use this command to scan the content that was not committed yet | -| [repository](#repository-scan) | Scan git repository including its history | +| Command | Description | +|-----------------------------------------|-----------------------------------------------------------------------------| +| [commit-history](#commit-history-scan) | Scan commit history or perform diff scanning between specific commits | +| [local-diff](#local-diff-scan) | Scan uncommitted changes (staged, unstaged, and untracked) against a commit | +| [path](#path-scan) | Scan the files in the path supplied in the command | +| [pre-commit](#pre-commit-scan) | Use this command to scan the content that was not committed yet | +| [repository](#repository-scan) | Scan git repository including its history | ### Options @@ -1075,6 +1078,56 @@ cycode scan commit-history -r HEAD~3..HEAD ~/home/git/codebase > [!TIP] > For CI/CD pipelines, you can use environment variables like `${{ github.event.pull_request.base.sha }}..${{ github.sha }}` (GitHub Actions) or `$CI_MERGE_REQUEST_TARGET_BRANCH_SHA..$CI_COMMIT_SHA` (GitLab CI) to scan only PR/MR changes. +### Local Diff Scan + +> [!NOTE] +> Local Diff Scan is not available for IaC scans. + +A local diff scan compares a commit (by default, `HEAD`) against your current working directory — including staged changes, unstaged edits, and files you haven't added to Git yet. Unlike Pre-Commit Scan, which only looks at staged changes, and Commit History Scan, which compares two existing commits, Local Diff Scan reflects exactly what's currently on disk. This makes it well suited for IDE integrations and other tools that need continuous, real-time feedback as you work. + +To execute a local diff scan, run: + +`cycode scan local-diff` + +By default, this compares your working directory against `HEAD`. To compare against a different commit, branch, or tag, use the `--commit` (`-c`) option: + +`cycode scan local-diff --commit main` + +You can also scope the scan to one or more specific files or directories — useful for scanning only the file currently open in an editor: + +`cycode scan local-diff path/to/file.py` + +The following options are available for use with this command: + +| Option | Description | +|---------------------|------------------------------------------------------------------------------------| +| `[PATHS]...` | Optional paths to scope the diff scan to; defaults to the entire working directory | +| `-c, --commit TEXT` | Git ref (commit, branch, or tag) to diff against; defaults to `HEAD` | + +#### What Gets Scanned + +Local Diff Scan collects: +- **Staged changes** – content added with `git add` +- **Unstaged changes** – edits to tracked files that haven't been staged yet +- **Untracked files** – new files that haven't been added to Git at all + +#### Local Diff Scanning Examples + +**Scan everything currently changed against the last commit:** +```bash +cycode scan local-diff +``` + +**Scan changes against a specific commit or branch:** +```bash +cycode scan local-diff --commit main +``` + +**Scan only a specific file (e.g., the file currently open in your IDE):** +```bash +cycode scan local-diff src/app.py +``` + ### Pre-Commit Scan A pre-commit scan automatically identifies any issues before you commit changes to your repository. There is no need to manually execute this scan; configure the pre-commit hook as detailed under the Installation section of this guide. diff --git a/cycode/cli/apps/scan/__init__.py b/cycode/cli/apps/scan/__init__.py index 629c3b8f..658c0952 100644 --- a/cycode/cli/apps/scan/__init__.py +++ b/cycode/cli/apps/scan/__init__.py @@ -1,6 +1,7 @@ import typer from cycode.cli.apps.scan.commit_history.commit_history_command import commit_history_command +from cycode.cli.apps.scan.local_diff.local_diff_command import local_diff_command from cycode.cli.apps.scan.path.path_command import path_command from cycode.cli.apps.scan.pre_commit.pre_commit_command import pre_commit_command from cycode.cli.apps.scan.pre_push.pre_push_command import pre_push_command @@ -26,6 +27,11 @@ app.command(name='commit-history', short_help='Scan commit history or perform diff scanning between specific commits.')( commit_history_command ) +app.command( + name='local-diff', + short_help='Scan uncommitted changes (staged, unstaged, and untracked) against a commit. ' + 'Useful for IDE integrations.', +)(local_diff_command) app.command( name='pre-commit', short_help='Use this command in pre-commit hook to scan any content that was not committed yet.', diff --git a/cycode/cli/apps/scan/commit_range_scanner.py b/cycode/cli/apps/scan/commit_range_scanner.py index 70b7e8e4..c4117bcb 100644 --- a/cycode/cli/apps/scan/commit_range_scanner.py +++ b/cycode/cli/apps/scan/commit_range_scanner.py @@ -26,6 +26,7 @@ get_commit_range_modified_documents, get_diff_file_content, get_diff_file_path, + get_local_diff_documents, get_pre_commit_modified_documents, get_staged_diff_index, parse_commit_range, @@ -34,6 +35,7 @@ from cycode.cli.files_collector.file_excluder import excluder from cycode.cli.files_collector.models.in_memory_zip import InMemoryZip from cycode.cli.files_collector.sca.sca_file_collector import ( + perform_sca_local_diff_scan_actions, perform_sca_pre_commit_range_scan_actions, perform_sca_pre_hook_range_scan_actions, ) @@ -420,3 +422,102 @@ def scan_pre_commit(ctx: typer.Context, repo_path: str) -> None: _SCAN_TYPE_TO_PRE_COMMIT_HANDLER[scan_type](ctx, repo_path) logger.debug('Pre-commit scan completed successfully') + + +def _scan_sca_local_diff( + ctx: typer.Context, repo_path: str, commit_rev: str, paths: Optional[list[str]] = None, **_ +) -> None: + scan_parameters = get_scan_parameters(ctx, (repo_path,)) + + from_commit_documents, working_tree_documents, _diff_documents = get_local_diff_documents( + progress_bar=ctx.obj['progress_bar'], + progress_bar_section=ScanProgressBarSection.PREPARE_LOCAL_FILES, + repo_path=repo_path, + commit_rev=commit_rev, + paths=paths, + ) + + from_commit_documents = excluder.exclude_irrelevant_documents_to_scan(consts.SCA_SCAN_TYPE, from_commit_documents) + working_tree_documents = excluder.exclude_irrelevant_documents_to_scan( + consts.SCA_SCAN_TYPE, working_tree_documents + ) + + is_cycodeignore_allowed = is_cycodeignore_allowed_by_scan_config(ctx) + from_commit_documents = filter_documents_with_cycodeignore( + from_commit_documents, repo_path, is_cycodeignore_allowed + ) + working_tree_documents = filter_documents_with_cycodeignore( + working_tree_documents, repo_path, is_cycodeignore_allowed + ) + + perform_sca_local_diff_scan_actions(repo_path, from_commit_documents, commit_rev, working_tree_documents) + + _scan_commit_range_documents(ctx, from_commit_documents, working_tree_documents, scan_parameters=scan_parameters) + + +def _scan_secret_local_diff( + ctx: typer.Context, repo_path: str, commit_rev: str, paths: Optional[list[str]] = None, **_ +) -> None: + _from_commit_documents, _working_tree_documents, diff_documents = get_local_diff_documents( + progress_bar=ctx.obj['progress_bar'], + progress_bar_section=ScanProgressBarSection.PREPARE_LOCAL_FILES, + repo_path=repo_path, + commit_rev=commit_rev, + paths=paths, + ) + + diff_documents = excluder.exclude_irrelevant_documents_to_scan(consts.SECRET_SCAN_TYPE, diff_documents) + + is_cycodeignore_allowed = is_cycodeignore_allowed_by_scan_config(ctx) + diff_documents = filter_documents_with_cycodeignore(diff_documents, repo_path, is_cycodeignore_allowed) + + scan_documents(ctx, diff_documents, get_scan_parameters(ctx, (repo_path,)), is_git_diff=True) + + +def _scan_sast_local_diff( + ctx: typer.Context, repo_path: str, commit_rev: str, paths: Optional[list[str]] = None, **_ +) -> None: + scan_parameters = get_scan_parameters(ctx, (repo_path,)) + + _from_commit_documents, working_tree_documents, diff_documents = get_local_diff_documents( + progress_bar=ctx.obj['progress_bar'], + progress_bar_section=ScanProgressBarSection.PREPARE_LOCAL_FILES, + repo_path=repo_path, + commit_rev=commit_rev, + paths=paths, + ) + + working_tree_documents = excluder.exclude_irrelevant_documents_to_scan( + consts.SAST_SCAN_TYPE, working_tree_documents + ) + diff_documents = excluder.exclude_irrelevant_documents_to_scan(consts.SAST_SCAN_TYPE, diff_documents) + + is_cycodeignore_allowed = is_cycodeignore_allowed_by_scan_config(ctx) + working_tree_documents = filter_documents_with_cycodeignore( + working_tree_documents, repo_path, is_cycodeignore_allowed + ) + diff_documents = filter_documents_with_cycodeignore(diff_documents, repo_path, is_cycodeignore_allowed) + + _scan_commit_range_documents(ctx, working_tree_documents, diff_documents, scan_parameters=scan_parameters) + + +_SCAN_TYPE_TO_LOCAL_DIFF_HANDLER = { + consts.SCA_SCAN_TYPE: _scan_sca_local_diff, + consts.SECRET_SCAN_TYPE: _scan_secret_local_diff, + consts.SAST_SCAN_TYPE: _scan_sast_local_diff, +} + + +def scan_local_diff( + ctx: typer.Context, repo_path: str, commit_rev: str, paths: Optional[list[str]] = None, **kwargs +) -> None: + scan_type = ctx.obj['scan_type'] + + progress_bar = ctx.obj['progress_bar'] + progress_bar.start() + + if scan_type not in _SCAN_TYPE_TO_LOCAL_DIFF_HANDLER: + raise click.ClickException(f'Local diff scanning for {scan_type.upper()} is not supported') + + _SCAN_TYPE_TO_LOCAL_DIFF_HANDLER[scan_type](ctx, repo_path, commit_rev, paths=paths, **kwargs) + logger.debug('Local diff scan completed successfully') diff --git a/cycode/cli/apps/scan/local_diff/__init__.py b/cycode/cli/apps/scan/local_diff/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/cycode/cli/apps/scan/local_diff/local_diff_command.py b/cycode/cli/apps/scan/local_diff/local_diff_command.py new file mode 100644 index 00000000..7a3b878e --- /dev/null +++ b/cycode/cli/apps/scan/local_diff/local_diff_command.py @@ -0,0 +1,81 @@ +import os +from pathlib import Path +from typing import Annotated, Optional + +import typer + +from cycode.cli import consts +from cycode.cli.apps.scan.commit_range_scanner import scan_local_diff +from cycode.cli.exceptions.handle_scan_errors import handle_scan_exception +from cycode.cli.logger import logger +from cycode.cli.utils.git_proxy import git_proxy + + +def _resolve_repo_root(cwd: str) -> str: + """Resolve the repository root from `cwd`, which may be any subdirectory of the repo. + + `git_proxy.get_repo()` requires an exact match (the root or a `.git` dir) unless told to + search parent directories, so without this, running the command from anywhere but the + repo root raises a misleading "not a git repository" error. + """ + repo = git_proxy.get_repo(cwd, search_parent_directories=True) + return repo.working_tree_dir or cwd + + +def _validate_commit_ref(repo_path: str, commit: str) -> None: + """Raise a clear, user-facing error for an unresolvable `--commit` ref. + + A repository with no commits at all is a valid state (the diff falls back to comparing + against the empty tree), so validation is skipped in that case. + """ + repo = git_proxy.get_repo(repo_path) + + try: + repo.rev_parse(consts.GIT_HEAD_COMMIT_REV) + except Exception as e: + logger.debug('Repository has no commits yet; skipping --commit validation', exc_info=e) + return + + try: + repo.commit(commit) + except Exception as e: + raise typer.BadParameter(f'Could not resolve git ref: {commit!r}', param_hint='--commit') from e + + +def local_diff_command( + ctx: typer.Context, + paths: Annotated[ + Optional[list[Path]], + typer.Argument( + help='Optional paths to scope the diff scan to (e.g. the file currently open in an IDE). ' + 'Defaults to the entire working directory.', + show_default=False, + resolve_path=True, + ), + ] = None, + commit: Annotated[ + str, + typer.Option( + '--commit', + '-c', + help='Git ref (commit, branch, or tag) to diff against. ' + 'Compares this ref to the current staged, unstaged, and untracked changes in the working directory.', + ), + ] = 'HEAD', +) -> None: + try: + repo_path = _resolve_repo_root(os.getcwd()) + _validate_commit_ref(repo_path, commit) + except typer.BadParameter: + raise + except Exception as e: + handle_scan_exception(ctx, e) + return + + logger.debug('Starting local diff scan process, %s', {'commit': commit, 'paths': paths}) + + try: + str_paths = [str(path) for path in paths] if paths else None + scan_local_diff(ctx, repo_path=repo_path, commit_rev=commit, paths=str_paths) + except Exception as e: + handle_scan_exception(ctx, e) diff --git a/cycode/cli/consts.py b/cycode/cli/consts.py index 104cfc9b..20b7bf86 100644 --- a/cycode/cli/consts.py +++ b/cycode/cli/consts.py @@ -8,6 +8,7 @@ PRE_RECEIVE_COMMAND_SCAN_TYPE_OLD = 'pre_receive' COMMIT_HISTORY_COMMAND_SCAN_TYPE = 'commit-history' COMMIT_HISTORY_COMMAND_SCAN_TYPE_OLD = 'commit_history' +LOCAL_DIFF_COMMAND_SCAN_TYPE = 'local-diff' SECRET_SCAN_TYPE = 'secret' IAC_SCAN_TYPE = 'iac' @@ -195,6 +196,7 @@ PRE_RECEIVE_COMMAND_SCAN_TYPE_OLD, COMMIT_HISTORY_COMMAND_SCAN_TYPE, COMMIT_HISTORY_COMMAND_SCAN_TYPE_OLD, + LOCAL_DIFF_COMMAND_SCAN_TYPE, ] DEFAULT_CYCODE_DOMAIN = 'cycode.com' diff --git a/cycode/cli/files_collector/commit_range_documents.py b/cycode/cli/files_collector/commit_range_documents.py index 2fb63581..430f56a3 100644 --- a/cycode/cli/files_collector/commit_range_documents.py +++ b/cycode/cli/files_collector/commit_range_documents.py @@ -457,6 +457,92 @@ def get_pre_commit_modified_documents( return git_head_documents, pre_committed_documents, diff_documents +def _is_path_included(file_path: str, paths: Optional[list[str]]) -> bool: + if not paths: + return True + + normalized_file_path = os.path.normpath(file_path) + for path in paths: + normalized_path = os.path.normpath(path) + if normalized_file_path == normalized_path or normalized_file_path.startswith(normalized_path + os.sep): + return True + + return False + + +def get_local_diff_documents( + progress_bar: 'BaseProgressBar', + progress_bar_section: 'ProgressBarSection', + repo_path: str, + commit_rev: str, + paths: Optional[list[str]] = None, +) -> tuple[list[Document], list[Document], list[Document]]: + """Diffs `commit_rev` against the current working tree: staged, unstaged, and untracked changes. + + Returns: + (from_commit_documents, working_tree_documents, diff_documents) - the same shape as + `get_pre_commit_modified_documents`, but compared against an arbitrary commit (not just HEAD) + and against the full working tree (not just the staged index), and including untracked files. + """ + from_commit_documents = [] + working_tree_documents = [] + diff_documents = [] + + repo = git_proxy.get_repo(repo_path) + + try: + diff_target = repo.commit(commit_rev) + except Exception as e: + # Repository has no commits yet; diff against the empty tree instead. + # (git_proxy.get_null_tree() is only a sentinel usable as the `other` side of a diff, + # so we resolve the well-known empty tree object itself to diff from.) + logger.debug( + 'Could not resolve commit_rev, falling back to the empty tree, %s', {'commit_rev': commit_rev}, exc_info=e + ) + diff_target = repo.tree(consts.GIT_EMPTY_TREE_OBJECT) + + diff_index = diff_target.diff(None, create_patch=True, paths=paths or None) + progress_bar.set_section_length(progress_bar_section, len(diff_index)) + for diff in diff_index: + progress_bar.update(progress_bar_section) + + file_path = get_path_by_os(get_diff_file_path(diff, repo=repo)) + + diff_documents.append( + Document( + path=file_path, + content=get_diff_file_content(diff), + is_git_diff_format=True, + ) + ) + + file_content = _get_file_content_from_commit_diff(repo, commit_rev, diff) + if file_content is not None: + from_commit_documents.append(Document(file_path, file_content)) + + if os.path.exists(file_path): + file_content = get_file_content(file_path) + if file_content: + working_tree_documents.append(Document(file_path, file_content)) + + for relative_path in repo.untracked_files if repo.working_tree_dir else []: + absolute_path = get_path_by_os(os.path.join(repo.working_tree_dir, relative_path)) + if not _is_path_included(absolute_path, paths): + continue + + file_content = get_file_content(absolute_path) + if not file_content: + continue + + # A new, untracked file has no real diff to show against `commit_rev`; its full content + # stands in for both the "current" content and the diff channel (matches how `path`/ + # `repository` scans already treat whole files: is_git_diff_format=False). + working_tree_documents.append(Document(absolute_path, file_content)) + diff_documents.append(Document(absolute_path, file_content)) + + return from_commit_documents, working_tree_documents, diff_documents + + def parse_commit_range(commit_range: str, path: str) -> tuple[Optional[str], Optional[str], Optional[str]]: """Parses a git commit range string and returns the full SHAs for the 'from' and 'to' commits. Also, it returns the separator in the commit range. diff --git a/cycode/cli/files_collector/sca/sca_file_collector.py b/cycode/cli/files_collector/sca/sca_file_collector.py index 4db5cd04..72c694d3 100644 --- a/cycode/cli/files_collector/sca/sca_file_collector.py +++ b/cycode/cli/files_collector/sca/sca_file_collector.py @@ -72,6 +72,17 @@ def perform_sca_pre_hook_range_scan_actions( _add_ecosystem_related_files_if_exists(pre_committed_documents) +def perform_sca_local_diff_scan_actions( + repo_path: str, from_commit_documents: list[Document], from_commit_rev: str, working_tree_documents: list[Document] +) -> None: + """Same as `perform_sca_pre_hook_range_scan_actions`, but `from_commit_rev` can be any ref, not just HEAD.""" + repo = git_proxy.get_repo(repo_path) + _add_ecosystem_related_files_if_exists(from_commit_documents, repo, from_commit_rev) + # working tree documents reflect the live filesystem (staged + unstaged + untracked), so their + # related project files must also be read from disk, not from a commit tree. + _add_ecosystem_related_files_if_exists(working_tree_documents) + + def _get_doc_ecosystem_related_project_files( doc: Document, documents: list[Document], ecosystem: str, commit_rev: Optional[str], repo: Optional['Repo'] ) -> list[Document]: diff --git a/tests/cli/commands/scan/test_commit_range_scanner.py b/tests/cli/commands/scan/test_commit_range_scanner.py index a4a6c58b..74fd2609 100644 --- a/tests/cli/commands/scan/test_commit_range_scanner.py +++ b/tests/cli/commands/scan/test_commit_range_scanner.py @@ -47,3 +47,58 @@ def test_commit_range_scan_falls_back_to_api_when_presigned_upload_raises_wrappe mock_v4_async.assert_called_once() mock_async.assert_called_once() mock_handle_exception.assert_not_called() + + +class TestScanLocalDiff: + """Test the scan_local_diff dispatcher.""" + + def _make_ctx(self, scan_type: str) -> MagicMock: + mock_ctx = MagicMock() + mock_ctx.obj = {'scan_type': scan_type, 'progress_bar': MagicMock()} + return mock_ctx + + def test_unsupported_scan_type_raises(self) -> None: + import click + import pytest + + from cycode.cli.apps.scan.commit_range_scanner import scan_local_diff + + mock_ctx = self._make_ctx(consts.IAC_SCAN_TYPE) + + with pytest.raises(click.ClickException, match='IAC'): + scan_local_diff(mock_ctx, repo_path='/repo', commit_rev='HEAD') + + def test_dispatches_secret_scan_type(self) -> None: + from cycode.cli.apps.scan import commit_range_scanner + from cycode.cli.apps.scan.commit_range_scanner import scan_local_diff + + mock_handler = Mock() + with patch.dict( + commit_range_scanner._SCAN_TYPE_TO_LOCAL_DIFF_HANDLER, {consts.SECRET_SCAN_TYPE: mock_handler} + ): + mock_ctx = self._make_ctx(consts.SECRET_SCAN_TYPE) + scan_local_diff(mock_ctx, repo_path='/repo', commit_rev='abc123', paths=['/repo/file.py']) + + mock_handler.assert_called_once_with(mock_ctx, '/repo', 'abc123', paths=['/repo/file.py']) + + def test_dispatches_sca_scan_type(self) -> None: + from cycode.cli.apps.scan import commit_range_scanner + from cycode.cli.apps.scan.commit_range_scanner import scan_local_diff + + mock_handler = Mock() + with patch.dict(commit_range_scanner._SCAN_TYPE_TO_LOCAL_DIFF_HANDLER, {consts.SCA_SCAN_TYPE: mock_handler}): + mock_ctx = self._make_ctx(consts.SCA_SCAN_TYPE) + scan_local_diff(mock_ctx, repo_path='/repo', commit_rev='HEAD') + + mock_handler.assert_called_once_with(mock_ctx, '/repo', 'HEAD', paths=None) + + def test_dispatches_sast_scan_type(self) -> None: + from cycode.cli.apps.scan import commit_range_scanner + from cycode.cli.apps.scan.commit_range_scanner import scan_local_diff + + mock_handler = Mock() + with patch.dict(commit_range_scanner._SCAN_TYPE_TO_LOCAL_DIFF_HANDLER, {consts.SAST_SCAN_TYPE: mock_handler}): + mock_ctx = self._make_ctx(consts.SAST_SCAN_TYPE) + scan_local_diff(mock_ctx, repo_path='/repo', commit_rev='HEAD') + + mock_handler.assert_called_once_with(mock_ctx, '/repo', 'HEAD', paths=None) diff --git a/tests/cli/commands/scan/test_local_diff_command.py b/tests/cli/commands/scan/test_local_diff_command.py new file mode 100644 index 00000000..e66285b4 --- /dev/null +++ b/tests/cli/commands/scan/test_local_diff_command.py @@ -0,0 +1,150 @@ +import os +import tempfile +from collections.abc import Generator +from contextlib import contextmanager +from unittest.mock import MagicMock, patch + +import pytest +import typer +from git import Repo +from typer.testing import CliRunner + +from cycode.cli.apps.scan.local_diff.local_diff_command import ( + _resolve_repo_root, + _validate_commit_ref, + local_diff_command, +) + + +@contextmanager +def temporary_git_repository() -> Generator[tuple[str, Repo], None, None]: + with tempfile.TemporaryDirectory() as temp_dir: + repo = Repo.init(temp_dir, b='main') + try: + yield temp_dir, repo + finally: + repo.close() + + +class TestValidateCommitRef: + def test_valid_commit_ref_does_not_raise(self) -> None: + with temporary_git_repository() as (temp_dir, repo): + file_path = os.path.join(temp_dir, 'file.txt') + with open(file_path, 'w') as f: + f.write('content') + repo.index.add(['file.txt']) + repo.index.commit('initial') + + _validate_commit_ref(temp_dir, 'HEAD') + + def test_invalid_commit_ref_raises_bad_parameter(self) -> None: + with temporary_git_repository() as (temp_dir, repo): + file_path = os.path.join(temp_dir, 'file.txt') + with open(file_path, 'w') as f: + f.write('content') + repo.index.add(['file.txt']) + repo.index.commit('initial') + + with pytest.raises(typer.BadParameter): + _validate_commit_ref(temp_dir, 'not-a-real-ref') + + def test_empty_repository_with_default_head_does_not_raise(self) -> None: + with temporary_git_repository() as (temp_dir, _repo): + _validate_commit_ref(temp_dir, 'HEAD') + + +class TestResolveRepoRoot: + """Running from any subdirectory of the repo must resolve to the actual repo root. + + Regression test: git_proxy.get_repo() requires an exact match (root or .git dir) unless + told to search parent directories, so a naive `os.getcwd()` breaks the moment the command + is invoked from a subdirectory -- exactly how an IDE plugin scoped to the open file's + folder, or a workspace subdirectory in a monorepo, would invoke it. + """ + + def test_resolves_root_from_subdirectory(self) -> None: + with temporary_git_repository() as (temp_dir, repo): + os.makedirs(os.path.join(temp_dir, 'sub')) + file_path = os.path.join(temp_dir, 'sub', 'app.py') + with open(file_path, 'w') as f: + f.write('content') + repo.index.add(['sub/app.py']) + repo.index.commit('initial') + + resolved_root = _resolve_repo_root(os.path.join(temp_dir, 'sub')) + + assert os.path.realpath(resolved_root) == os.path.realpath(temp_dir) + + def test_resolves_root_when_already_at_root(self) -> None: + with temporary_git_repository() as (temp_dir, repo): + file_path = os.path.join(temp_dir, 'file.txt') + with open(file_path, 'w') as f: + f.write('content') + repo.index.add(['file.txt']) + repo.index.commit('initial') + + resolved_root = _resolve_repo_root(temp_dir) + + assert os.path.realpath(resolved_root) == os.path.realpath(temp_dir) + + +class TestLocalDiffCommandPathResolution: + """A relative path argument must reach scan_local_diff already resolved to absolute. + + Regression test: get_local_diff_documents/_is_path_included compare an always-absolute + path (derived from the repo's working tree) against the raw `paths` strings. Without + `resolve_path=True` on the CLI argument, a relative path (the natural way to invoke this + command, e.g. `cycode scan local-diff sub/app.py` from the repo root) would never match, + silently dropping untracked files from the scoped scan. + """ + + def test_relative_path_argument_is_resolved_to_absolute(self) -> None: + with temporary_git_repository() as (temp_dir, repo): + os.makedirs(os.path.join(temp_dir, 'sub')) + file_path = os.path.join(temp_dir, 'sub', 'app.py') + with open(file_path, 'w') as f: + f.write('content') + repo.index.add(['sub/app.py']) + repo.index.commit('initial') + + app = typer.Typer() + app.command()(local_diff_command) + + with ( + patch('cycode.cli.apps.scan.local_diff.local_diff_command.scan_local_diff') as mock_scan, + patch('os.getcwd', return_value=temp_dir), + ): + result = CliRunner().invoke(app, ['sub/app.py'], obj=MagicMock()) + + assert result.exit_code == 0, result.output + mock_scan.assert_called_once() + _, kwargs = mock_scan.call_args + assert kwargs['paths'] == [os.path.join(temp_dir, 'sub', 'app.py')] + + +class TestLocalDiffCommandFromSubdirectory: + """End-to-end: invoking the command from a repo subdirectory must not fail.""" + + def test_scan_local_diff_called_with_repo_root_not_subdirectory(self) -> None: + with temporary_git_repository() as (temp_dir, repo): + os.makedirs(os.path.join(temp_dir, 'sub')) + file_path = os.path.join(temp_dir, 'sub', 'app.py') + with open(file_path, 'w') as f: + f.write('content') + repo.index.add(['sub/app.py']) + repo.index.commit('initial') + + app = typer.Typer() + app.command()(local_diff_command) + + subdirectory = os.path.join(temp_dir, 'sub') + with ( + patch('cycode.cli.apps.scan.local_diff.local_diff_command.scan_local_diff') as mock_scan, + patch('os.getcwd', return_value=subdirectory), + ): + result = CliRunner().invoke(app, [], obj=MagicMock()) + + assert result.exit_code == 0, result.output + mock_scan.assert_called_once() + _, kwargs = mock_scan.call_args + assert os.path.realpath(kwargs['repo_path']) == os.path.realpath(temp_dir) diff --git a/tests/cli/files_collector/test_commit_range_documents.py b/tests/cli/files_collector/test_commit_range_documents.py index d972144c..01f13f34 100644 --- a/tests/cli/files_collector/test_commit_range_documents.py +++ b/tests/cli/files_collector/test_commit_range_documents.py @@ -15,6 +15,7 @@ calculate_pre_receive_commit_range, collect_commit_range_diff_documents, get_diff_file_path, + get_local_diff_documents, get_safe_head_reference_for_diff, get_staged_diff_index, parse_commit_range, @@ -1167,3 +1168,166 @@ def test_collect_with_various_commit_range_formats(self) -> None: commit_range = a_commit.hexsha documents = collect_commit_range_diff_documents(mock_ctx, temp_dir, commit_range) assert len(documents) == 2, f'Expected 2 documents from single commit A, got {len(documents)}' + + +class TestGetLocalDiffDocuments: + """Test get_local_diff_documents: diffing a commit against the working directory.""" + + @staticmethod + def _mock_progress_bar() -> Mock: + mock_progress_bar = Mock() + mock_progress_bar.set_section_length = Mock() + mock_progress_bar.update = Mock() + return mock_progress_bar + + def test_combines_staged_and_unstaged_changes(self) -> None: + """A tracked file with both a staged and an unstaged edit should appear as a single diff/document.""" + with temporary_git_repository() as (temp_dir, repo): + file_path = os.path.join(temp_dir, 'tracked.txt') + with open(file_path, 'w') as f: + f.write('line1\n') + repo.index.add(['tracked.txt']) + repo.index.commit('initial') + + with open(file_path, 'a') as f: + f.write('staged\n') + repo.index.add(['tracked.txt']) + + with open(file_path, 'a') as f: + f.write('unstaged\n') + + from_docs, work_docs, diff_docs = get_local_diff_documents( + progress_bar=self._mock_progress_bar(), + progress_bar_section='prepare', + repo_path=temp_dir, + commit_rev='HEAD', + ) + + assert len(from_docs) == 1 + assert from_docs[0].content == 'line1' + + assert len(work_docs) == 1 + assert work_docs[0].content == 'line1\nstaged\nunstaged\n' + + assert len(diff_docs) == 1 + assert diff_docs[0].is_git_diff_format is True + assert '+staged' in diff_docs[0].content + assert '+unstaged' in diff_docs[0].content + + def test_includes_untracked_files(self) -> None: + """A brand-new, never-added file should show up as full content in both channels.""" + with temporary_git_repository() as (temp_dir, repo): + tracked_path = os.path.join(temp_dir, 'tracked.txt') + with open(tracked_path, 'w') as f: + f.write('line1\n') + repo.index.add(['tracked.txt']) + repo.index.commit('initial') + + untracked_path = os.path.join(temp_dir, 'new_secret.txt') + with open(untracked_path, 'w') as f: + f.write('super-secret-value\n') + + from_docs, work_docs, diff_docs = get_local_diff_documents( + progress_bar=self._mock_progress_bar(), + progress_bar_section='prepare', + repo_path=temp_dir, + commit_rev='HEAD', + ) + + assert not any(doc.path == get_path_by_os(untracked_path) for doc in from_docs) + + untracked_work_doc = next(doc for doc in work_docs if doc.path == get_path_by_os(untracked_path)) + assert untracked_work_doc.content == 'super-secret-value\n' + assert untracked_work_doc.is_git_diff_format is False + + untracked_diff_doc = next(doc for doc in diff_docs if doc.path == get_path_by_os(untracked_path)) + assert untracked_diff_doc.content == 'super-secret-value\n' + assert untracked_diff_doc.is_git_diff_format is False + + def test_scopes_to_requested_paths(self) -> None: + """Only files under the requested paths should be collected, tracked or untracked.""" + with temporary_git_repository() as (temp_dir, repo): + os.makedirs(os.path.join(temp_dir, 'sub')) + + included_path = os.path.join(temp_dir, 'sub', 'app.py') + excluded_path = os.path.join(temp_dir, 'excluded.py') + with open(included_path, 'w') as f: + f.write("print('old')\n") + with open(excluded_path, 'w') as f: + f.write("print('old')\n") + repo.index.add(['sub/app.py', 'excluded.py']) + repo.index.commit('initial') + + with open(included_path, 'a') as f: + f.write("print('new')\n") + with open(excluded_path, 'a') as f: + f.write("print('new')\n") + + untracked_included = os.path.join(temp_dir, 'sub', 'new_file.py') + untracked_excluded = os.path.join(temp_dir, 'new_file.py') + with open(untracked_included, 'w') as f: + f.write('secret\n') + with open(untracked_excluded, 'w') as f: + f.write('secret\n') + + _from_docs, work_docs, diff_docs = get_local_diff_documents( + progress_bar=self._mock_progress_bar(), + progress_bar_section='prepare', + repo_path=temp_dir, + commit_rev='HEAD', + paths=[os.path.join(temp_dir, 'sub')], + ) + + diff_paths = {doc.path for doc in diff_docs} + work_paths = {doc.path for doc in work_docs} + + assert diff_paths == {get_path_by_os(included_path), get_path_by_os(untracked_included)} + assert work_paths == {get_path_by_os(included_path), get_path_by_os(untracked_included)} + + def test_uses_explicit_commit_rev_not_just_head(self) -> None: + """Diffing against an older commit should show changes made since that commit, not just since HEAD.""" + with temporary_git_repository() as (temp_dir, repo): + file_path = os.path.join(temp_dir, 'tracked.txt') + with open(file_path, 'w') as f: + f.write('A\n') + repo.index.add(['tracked.txt']) + first_commit = repo.index.commit('A') + + with open(file_path, 'a') as f: + f.write('B\n') + repo.index.add(['tracked.txt']) + repo.index.commit('B') + + with open(file_path, 'a') as f: + f.write('unstaged-C\n') + + _from_docs, _work_docs, diff_docs = get_local_diff_documents( + progress_bar=self._mock_progress_bar(), + progress_bar_section='prepare', + repo_path=temp_dir, + commit_rev=first_commit.hexsha, + ) + + assert len(diff_docs) == 1 + assert '+B' in diff_docs[0].content + assert '+unstaged-C' in diff_docs[0].content + + def test_empty_repository_falls_back_to_empty_tree(self) -> None: + """A repository with zero commits should treat all current content as new, not error out.""" + with temporary_git_repository() as (temp_dir, _repo): + file_path = os.path.join(temp_dir, 'new_file.txt') + with open(file_path, 'w') as f: + f.write('brand-new-content\n') + + from_docs, work_docs, diff_docs = get_local_diff_documents( + progress_bar=self._mock_progress_bar(), + progress_bar_section='prepare', + repo_path=temp_dir, + commit_rev='HEAD', + ) + + assert from_docs == [] + assert len(work_docs) == 1 + assert work_docs[0].content == 'brand-new-content\n' + assert len(diff_docs) == 1 + assert diff_docs[0].content == 'brand-new-content\n' From e56fca315a48277ce50d265ec1d457f6bd5bf71f Mon Sep 17 00:00:00 2001 From: Aaron Butler Date: Fri, 18 Sep 2026 09:06:35 -0400 Subject: [PATCH 2/8] CM-72985 Fix ruff formatting in local-diff scanner and tests 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 --- cycode/cli/apps/scan/commit_range_scanner.py | 4 +--- tests/cli/commands/scan/test_commit_range_scanner.py | 4 +--- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/cycode/cli/apps/scan/commit_range_scanner.py b/cycode/cli/apps/scan/commit_range_scanner.py index c4117bcb..f915a644 100644 --- a/cycode/cli/apps/scan/commit_range_scanner.py +++ b/cycode/cli/apps/scan/commit_range_scanner.py @@ -438,9 +438,7 @@ def _scan_sca_local_diff( ) from_commit_documents = excluder.exclude_irrelevant_documents_to_scan(consts.SCA_SCAN_TYPE, from_commit_documents) - working_tree_documents = excluder.exclude_irrelevant_documents_to_scan( - consts.SCA_SCAN_TYPE, working_tree_documents - ) + working_tree_documents = excluder.exclude_irrelevant_documents_to_scan(consts.SCA_SCAN_TYPE, working_tree_documents) is_cycodeignore_allowed = is_cycodeignore_allowed_by_scan_config(ctx) from_commit_documents = filter_documents_with_cycodeignore( diff --git a/tests/cli/commands/scan/test_commit_range_scanner.py b/tests/cli/commands/scan/test_commit_range_scanner.py index 74fd2609..f18d59bc 100644 --- a/tests/cli/commands/scan/test_commit_range_scanner.py +++ b/tests/cli/commands/scan/test_commit_range_scanner.py @@ -73,9 +73,7 @@ def test_dispatches_secret_scan_type(self) -> None: from cycode.cli.apps.scan.commit_range_scanner import scan_local_diff mock_handler = Mock() - with patch.dict( - commit_range_scanner._SCAN_TYPE_TO_LOCAL_DIFF_HANDLER, {consts.SECRET_SCAN_TYPE: mock_handler} - ): + with patch.dict(commit_range_scanner._SCAN_TYPE_TO_LOCAL_DIFF_HANDLER, {consts.SECRET_SCAN_TYPE: mock_handler}): mock_ctx = self._make_ctx(consts.SECRET_SCAN_TYPE) scan_local_diff(mock_ctx, repo_path='/repo', commit_rev='abc123', paths=['/repo/file.py']) From 6824afe9c68a799c24a154f2faa6d42b9f4b839a Mon Sep 17 00:00:00 2001 From: Aaron Butler Date: Fri, 18 Sep 2026 09:17:36 -0400 Subject: [PATCH 3/8] CM-72985 Fix Windows-only test failures in local-diff tests 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 --- tests/cli/commands/scan/test_local_diff_command.py | 4 +++- .../files_collector/test_commit_range_documents.py | 14 ++++++++++---- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/tests/cli/commands/scan/test_local_diff_command.py b/tests/cli/commands/scan/test_local_diff_command.py index e66285b4..a51d59cc 100644 --- a/tests/cli/commands/scan/test_local_diff_command.py +++ b/tests/cli/commands/scan/test_local_diff_command.py @@ -119,7 +119,9 @@ def test_relative_path_argument_is_resolved_to_absolute(self) -> None: assert result.exit_code == 0, result.output mock_scan.assert_called_once() _, kwargs = mock_scan.call_args - assert kwargs['paths'] == [os.path.join(temp_dir, 'sub', 'app.py')] + assert [os.path.realpath(p) for p in kwargs['paths']] == [ + os.path.realpath(os.path.join(temp_dir, 'sub', 'app.py')) + ] class TestLocalDiffCommandFromSubdirectory: diff --git a/tests/cli/files_collector/test_commit_range_documents.py b/tests/cli/files_collector/test_commit_range_documents.py index 01f13f34..78ea647e 100644 --- a/tests/cli/files_collector/test_commit_range_documents.py +++ b/tests/cli/files_collector/test_commit_range_documents.py @@ -1181,19 +1181,25 @@ def _mock_progress_bar() -> Mock: return mock_progress_bar def test_combines_staged_and_unstaged_changes(self) -> None: - """A tracked file with both a staged and an unstaged edit should appear as a single diff/document.""" + """A tracked file with both a staged and an unstaged edit should appear as a single diff/document. + + Files are written with newline='' so the on-disk (and therefore committed) content is exactly + the LF bytes given, regardless of platform -- otherwise Python's default text-mode write + translates '\\n' to the OS line ending, and on Windows the resulting CRLF blob makes the + `from_docs[0].content == 'line1'` assertion below fail with a trailing '\\r'. + """ with temporary_git_repository() as (temp_dir, repo): file_path = os.path.join(temp_dir, 'tracked.txt') - with open(file_path, 'w') as f: + with open(file_path, 'w', newline='') as f: f.write('line1\n') repo.index.add(['tracked.txt']) repo.index.commit('initial') - with open(file_path, 'a') as f: + with open(file_path, 'a', newline='') as f: f.write('staged\n') repo.index.add(['tracked.txt']) - with open(file_path, 'a') as f: + with open(file_path, 'a', newline='') as f: f.write('unstaged\n') from_docs, work_docs, diff_docs = get_local_diff_documents( From 63b9c1c76cc3618d5935b3d3372b6557d5bd446c Mon Sep 17 00:00:00 2001 From: Aaron Butler Date: Fri, 18 Sep 2026 09:26:37 -0400 Subject: [PATCH 4/8] CM-72985 Use real chdir instead of mocking os.getcwd in local-diff tests 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 --- .../commands/scan/test_local_diff_command.py | 23 ++++++++++--------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/tests/cli/commands/scan/test_local_diff_command.py b/tests/cli/commands/scan/test_local_diff_command.py index a51d59cc..4c6349de 100644 --- a/tests/cli/commands/scan/test_local_diff_command.py +++ b/tests/cli/commands/scan/test_local_diff_command.py @@ -98,7 +98,7 @@ class TestLocalDiffCommandPathResolution: silently dropping untracked files from the scoped scan. """ - def test_relative_path_argument_is_resolved_to_absolute(self) -> None: + def test_relative_path_argument_is_resolved_to_absolute(self, monkeypatch: pytest.MonkeyPatch) -> None: with temporary_git_repository() as (temp_dir, repo): os.makedirs(os.path.join(temp_dir, 'sub')) file_path = os.path.join(temp_dir, 'sub', 'app.py') @@ -110,10 +110,12 @@ def test_relative_path_argument_is_resolved_to_absolute(self) -> None: app = typer.Typer() app.command()(local_diff_command) - with ( - patch('cycode.cli.apps.scan.local_diff.local_diff_command.scan_local_diff') as mock_scan, - patch('os.getcwd', return_value=temp_dir), - ): + # Actually chdir rather than patching os.getcwd(): on Windows/Python 3.9, Click's + # path resolution does not consistently go through the patched os.getcwd symbol, + # so the mock silently has no effect there and the test passes for the wrong reason + # (or, as happened in CI, resolves against the real process cwd instead). + monkeypatch.chdir(temp_dir) + with patch('cycode.cli.apps.scan.local_diff.local_diff_command.scan_local_diff') as mock_scan: result = CliRunner().invoke(app, ['sub/app.py'], obj=MagicMock()) assert result.exit_code == 0, result.output @@ -127,7 +129,7 @@ def test_relative_path_argument_is_resolved_to_absolute(self) -> None: class TestLocalDiffCommandFromSubdirectory: """End-to-end: invoking the command from a repo subdirectory must not fail.""" - def test_scan_local_diff_called_with_repo_root_not_subdirectory(self) -> None: + def test_scan_local_diff_called_with_repo_root_not_subdirectory(self, monkeypatch: pytest.MonkeyPatch) -> None: with temporary_git_repository() as (temp_dir, repo): os.makedirs(os.path.join(temp_dir, 'sub')) file_path = os.path.join(temp_dir, 'sub', 'app.py') @@ -139,11 +141,10 @@ def test_scan_local_diff_called_with_repo_root_not_subdirectory(self) -> None: app = typer.Typer() app.command()(local_diff_command) - subdirectory = os.path.join(temp_dir, 'sub') - with ( - patch('cycode.cli.apps.scan.local_diff.local_diff_command.scan_local_diff') as mock_scan, - patch('os.getcwd', return_value=subdirectory), - ): + # Actually chdir rather than patching os.getcwd() -- see the comment in + # TestLocalDiffCommandPathResolution for why the mock is unreliable on Windows. + monkeypatch.chdir(os.path.join(temp_dir, 'sub')) + with patch('cycode.cli.apps.scan.local_diff.local_diff_command.scan_local_diff') as mock_scan: result = CliRunner().invoke(app, [], obj=MagicMock()) assert result.exit_code == 0, result.output From 5b89fd1471ced1ab7366098baf0380749e42b2ed Mon Sep 17 00:00:00 2001 From: Aaron Butler Date: Fri, 18 Sep 2026 09:35:15 -0400 Subject: [PATCH 5/8] CM-72985 chdir back before temp dir cleanup in local-diff tests 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 --- tests/cli/commands/scan/test_local_diff_command.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/cli/commands/scan/test_local_diff_command.py b/tests/cli/commands/scan/test_local_diff_command.py index 4c6349de..36ab93cb 100644 --- a/tests/cli/commands/scan/test_local_diff_command.py +++ b/tests/cli/commands/scan/test_local_diff_command.py @@ -99,6 +99,7 @@ class TestLocalDiffCommandPathResolution: """ def test_relative_path_argument_is_resolved_to_absolute(self, monkeypatch: pytest.MonkeyPatch) -> None: + original_cwd = os.getcwd() with temporary_git_repository() as (temp_dir, repo): os.makedirs(os.path.join(temp_dir, 'sub')) file_path = os.path.join(temp_dir, 'sub', 'app.py') @@ -118,6 +119,12 @@ def test_relative_path_argument_is_resolved_to_absolute(self, monkeypatch: pytes with patch('cycode.cli.apps.scan.local_diff.local_diff_command.scan_local_diff') as mock_scan: result = CliRunner().invoke(app, ['sub/app.py'], obj=MagicMock()) + # monkeypatch only restores the cwd at fixture teardown, which runs after this test + # function returns -- but temporary_git_repository()'s cleanup (below, at the end of + # this `with` block) runs now, inside the test. Windows can't delete a directory that + # is still the process's cwd, so we must chdir back out before that cleanup happens. + monkeypatch.chdir(original_cwd) + assert result.exit_code == 0, result.output mock_scan.assert_called_once() _, kwargs = mock_scan.call_args @@ -130,6 +137,7 @@ class TestLocalDiffCommandFromSubdirectory: """End-to-end: invoking the command from a repo subdirectory must not fail.""" def test_scan_local_diff_called_with_repo_root_not_subdirectory(self, monkeypatch: pytest.MonkeyPatch) -> None: + original_cwd = os.getcwd() with temporary_git_repository() as (temp_dir, repo): os.makedirs(os.path.join(temp_dir, 'sub')) file_path = os.path.join(temp_dir, 'sub', 'app.py') @@ -147,6 +155,10 @@ def test_scan_local_diff_called_with_repo_root_not_subdirectory(self, monkeypatc with patch('cycode.cli.apps.scan.local_diff.local_diff_command.scan_local_diff') as mock_scan: result = CliRunner().invoke(app, [], obj=MagicMock()) + # See the matching comment in TestLocalDiffCommandPathResolution: must chdir back out + # before temporary_git_repository()'s cleanup runs, or Windows can't delete the dir. + monkeypatch.chdir(original_cwd) + assert result.exit_code == 0, result.output mock_scan.assert_called_once() _, kwargs = mock_scan.call_args From 2d035cead5594601b9562348c2c97024fec1b424 Mon Sep 17 00:00:00 2001 From: Aaron Butler Date: Wed, 30 Sep 2026 09:30:20 -0400 Subject: [PATCH 6/8] CM-72985 Fold local-diff into pre-commit per PO feedback 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 --- README.md | 78 +++---- cycode/cli/apps/scan/__init__.py | 9 +- cycode/cli/apps/scan/commit_range_scanner.py | 203 ++++++------------ cycode/cli/apps/scan/local_diff/__init__.py | 0 .../scan/local_diff/local_diff_command.py | 81 ------- .../scan/pre_commit/pre_commit_command.py | 80 ++++++- cycode/cli/consts.py | 2 - .../files_collector/commit_range_documents.py | 141 +++++------- .../files_collector/sca/sca_file_collector.py | 22 +- .../scan/test_commit_range_scanner.py | 40 ++-- ..._command.py => test_pre_commit_command.py} | 87 +++++--- .../test_commit_range_documents.py | 179 ++++++++++----- 12 files changed, 439 insertions(+), 483 deletions(-) delete mode 100644 cycode/cli/apps/scan/local_diff/__init__.py delete mode 100644 cycode/cli/apps/scan/local_diff/local_diff_command.py rename tests/cli/commands/scan/{test_local_diff_command.py => test_pre_commit_command.py} (62%) diff --git a/README.md b/README.md index b149e252..44b44521 100644 --- a/README.md +++ b/README.md @@ -47,10 +47,9 @@ This guide walks you through both installation and usage. 1. [Terraform Plan Scan](#terraform-plan-scan) 4. [Commit History Scan](#commit-history-scan) 1. [Commit Range Option (Diff Scanning)](#commit-range-option-diff-scanning) - 5. [Local Diff Scan](#local-diff-scan) - 1. [What Gets Scanned](#what-gets-scanned) - 6. [Pre-Commit Scan](#pre-commit-scan) - 7. [Pre-Push Scan](#pre-push-scan) + 5. [Pre-Commit Scan](#pre-commit-scan) + 1. [Local Diff Scanning (IDE Integrations)](#local-diff-scanning-ide-integrations) + 6. [Pre-Push Scan](#pre-push-scan) 2. [Scan Results](#scan-results) 1. [Show/Hide Secrets](#showhide-secrets) 2. [Soft Fail](#soft-fail) @@ -800,13 +799,12 @@ The Cycode CLI application offers several types of scans so that you can choose | `--maven-settings-file` | For Maven only, allows using a custom [settings.xml](https://maven.apache.org/settings.html) file when scanning for dependencies | | `--help` | Show options for given command. | -| Command | Description | -|-----------------------------------------|-----------------------------------------------------------------------------| -| [commit-history](#commit-history-scan) | Scan commit history or perform diff scanning between specific commits | -| [local-diff](#local-diff-scan) | Scan uncommitted changes (staged, unstaged, and untracked) against a commit | -| [path](#path-scan) | Scan the files in the path supplied in the command | -| [pre-commit](#pre-commit-scan) | Use this command to scan the content that was not committed yet | -| [repository](#repository-scan) | Scan git repository including its history | +| Command | Description | +|-----------------------------------------|----------------------------------------------------------------------------------------------------| +| [commit-history](#commit-history-scan) | Scan commit history or perform diff scanning between specific commits | +| [path](#path-scan) | Scan the files in the path supplied in the command | +| [pre-commit](#pre-commit-scan) | Scan content that was not committed yet; also supports local diff scanning via flags (IDE-friendly) | +| [repository](#repository-scan) | Scan git repository including its history | ### Options @@ -1078,64 +1076,44 @@ cycode scan commit-history -r HEAD~3..HEAD ~/home/git/codebase > [!TIP] > For CI/CD pipelines, you can use environment variables like `${{ github.event.pull_request.base.sha }}..${{ github.sha }}` (GitHub Actions) or `$CI_MERGE_REQUEST_TARGET_BRANCH_SHA..$CI_COMMIT_SHA` (GitLab CI) to scan only PR/MR changes. -### Local Diff Scan - -> [!NOTE] -> Local Diff Scan is not available for IaC scans. - -A local diff scan compares a commit (by default, `HEAD`) against your current working directory — including staged changes, unstaged edits, and files you haven't added to Git yet. Unlike Pre-Commit Scan, which only looks at staged changes, and Commit History Scan, which compares two existing commits, Local Diff Scan reflects exactly what's currently on disk. This makes it well suited for IDE integrations and other tools that need continuous, real-time feedback as you work. - -To execute a local diff scan, run: - -`cycode scan local-diff` - -By default, this compares your working directory against `HEAD`. To compare against a different commit, branch, or tag, use the `--commit` (`-c`) option: +### Pre-Commit Scan -`cycode scan local-diff --commit main` +A pre-commit scan automatically identifies any issues before you commit changes to your repository. There is no need to manually execute this scan; configure the pre-commit hook as detailed under the Installation section of this guide. -You can also scope the scan to one or more specific files or directories — useful for scanning only the file currently open in an editor: +After installing the pre-commit hook, you may occasionally wish to skip scanning during a specific commit. To do this, add the following to your `git` command to skip scanning for a single commit: -`cycode scan local-diff path/to/file.py` +```bash +SKIP=cycode git commit -m ` +``` The following options are available for use with this command: -| Option | Description | -|---------------------|------------------------------------------------------------------------------------| -| `[PATHS]...` | Optional paths to scope the diff scan to; defaults to the entire working directory | -| `-c, --commit TEXT` | Git ref (commit, branch, or tag) to diff against; defaults to `HEAD` | +| Option | Description | +|-----------------------|---------------------------------------------------------------------------------------------------------------------| +| `-b, --base-ref TEXT` | Git ref (commit, branch, or tag) to diff against; defaults to `HEAD`, matching the pre-commit hook behavior | +| `--include-unstaged` | Also scan unstaged changes to tracked files, not just what is staged; off by default | +| `--path PATH` | Optional path(s) to scope the diff scan to; repeatable; defaults to the entire working directory | -#### What Gets Scanned +#### Local Diff Scanning (IDE Integrations) -Local Diff Scan collects: -- **Staged changes** – content added with `git add` -- **Unstaged changes** – edits to tracked files that haven't been staged yet -- **Untracked files** – new files that haven't been added to Git at all +`--base-ref` and `--include-unstaged` turn `pre-commit` into a general-purpose **local diff scan**: comparing any commit (default `HEAD`) against your current working directory — including edits that aren't staged yet. This is intended for IDE plugins and other tools that need continuous, real-time feedback as you work, rather than the git hook flow. Combined with `--include-unstaged`, `--path` lets an IDE scope the scan to just the file currently open in the editor. -#### Local Diff Scanning Examples +> [!NOTE] +> Local diff scanning (via these flags) is not available for IaC scans. -**Scan everything currently changed against the last commit:** +**Scan everything currently changed (staged + unstaged) against the last commit:** ```bash -cycode scan local-diff +cycode scan pre-commit --include-unstaged ``` **Scan changes against a specific commit or branch:** ```bash -cycode scan local-diff --commit main +cycode scan pre-commit --include-unstaged --base-ref main ``` **Scan only a specific file (e.g., the file currently open in your IDE):** ```bash -cycode scan local-diff src/app.py -``` - -### Pre-Commit Scan - -A pre-commit scan automatically identifies any issues before you commit changes to your repository. There is no need to manually execute this scan; configure the pre-commit hook as detailed under the Installation section of this guide. - -After installing the pre-commit hook, you may occasionally wish to skip scanning during a specific commit. To do this, add the following to your `git` command to skip scanning for a single commit: - -```bash -SKIP=cycode git commit -m ` +cycode scan pre-commit --include-unstaged --path src/app.py ``` ### Pre-Push Scan diff --git a/cycode/cli/apps/scan/__init__.py b/cycode/cli/apps/scan/__init__.py index 658c0952..424fcaa5 100644 --- a/cycode/cli/apps/scan/__init__.py +++ b/cycode/cli/apps/scan/__init__.py @@ -1,7 +1,6 @@ import typer from cycode.cli.apps.scan.commit_history.commit_history_command import commit_history_command -from cycode.cli.apps.scan.local_diff.local_diff_command import local_diff_command from cycode.cli.apps.scan.path.path_command import path_command from cycode.cli.apps.scan.pre_commit.pre_commit_command import pre_commit_command from cycode.cli.apps.scan.pre_push.pre_push_command import pre_push_command @@ -27,14 +26,10 @@ app.command(name='commit-history', short_help='Scan commit history or perform diff scanning between specific commits.')( commit_history_command ) -app.command( - name='local-diff', - short_help='Scan uncommitted changes (staged, unstaged, and untracked) against a commit. ' - 'Useful for IDE integrations.', -)(local_diff_command) app.command( name='pre-commit', - short_help='Use this command in pre-commit hook to scan any content that was not committed yet.', + short_help='Use this command in pre-commit hook to scan any content that was not committed yet. ' + 'Also supports IDE-style local diff scanning via --base-ref/--include-unstaged/--path.', rich_help_panel=_AUTOMATION_COMMANDS_RICH_HELP_PANEL, )(pre_commit_command) app.command( diff --git a/cycode/cli/apps/scan/commit_range_scanner.py b/cycode/cli/apps/scan/commit_range_scanner.py index 93691df1..686bd898 100644 --- a/cycode/cli/apps/scan/commit_range_scanner.py +++ b/cycode/cli/apps/scan/commit_range_scanner.py @@ -24,25 +24,18 @@ from cycode.cli.files_collector.commit_range_documents import ( collect_commit_range_diff_documents, get_commit_range_modified_documents, - get_diff_file_content, - get_diff_file_path, - get_local_diff_documents, get_pre_commit_modified_documents, - get_staged_diff_index, parse_commit_range, ) from cycode.cli.files_collector.documents_walk_ignore import filter_documents_with_cycodeignore from cycode.cli.files_collector.file_excluder import excluder from cycode.cli.files_collector.models.in_memory_zip import InMemoryZip from cycode.cli.files_collector.sca.sca_file_collector import ( - perform_sca_local_diff_scan_actions, perform_sca_pre_commit_range_scan_actions, - perform_sca_pre_hook_range_scan_actions, + perform_sca_pre_commit_scan_actions, ) from cycode.cli.files_collector.zip_documents import zip_documents from cycode.cli.models import Document -from cycode.cli.utils.git_proxy import git_proxy -from cycode.cli.utils.path_utils import get_path_by_os from cycode.cli.utils.progress_bar import ScanProgressBarSection from cycode.cli.utils.scan_utils import ( generate_unique_scan_id, @@ -331,139 +324,57 @@ def scan_commit_range(ctx: typer.Context, repo_path: str, commit_range: str, **k _SCAN_TYPE_TO_COMMIT_RANGE_HANDLER[scan_type](ctx, repo_path, commit_range, **kwargs) -def _scan_sca_pre_commit(ctx: typer.Context, repo_path: str) -> None: - scan_parameters = get_scan_parameters(ctx) +def _scan_sca_pre_commit( + ctx: typer.Context, + repo_path: str, + base_ref: str = consts.GIT_HEAD_COMMIT_REV, + include_unstaged: bool = False, + paths: Optional[list[str]] = None, +) -> None: + scan_parameters = get_scan_parameters(ctx, (repo_path,)) - git_head_documents, pre_committed_documents, _ = get_pre_commit_modified_documents( + from_ref_documents, working_copy_documents, _diff_documents = get_pre_commit_modified_documents( progress_bar=ctx.obj['progress_bar'], progress_bar_section=ScanProgressBarSection.PREPARE_LOCAL_FILES, repo_path=repo_path, + base_ref=base_ref, + include_unstaged=include_unstaged, + paths=paths, ) - git_head_documents = excluder.exclude_irrelevant_documents_to_scan(consts.SCA_SCAN_TYPE, git_head_documents) - pre_committed_documents = excluder.exclude_irrelevant_documents_to_scan( - consts.SCA_SCAN_TYPE, pre_committed_documents - ) + from_ref_documents = excluder.exclude_irrelevant_documents_to_scan(consts.SCA_SCAN_TYPE, from_ref_documents) + working_copy_documents = excluder.exclude_irrelevant_documents_to_scan(consts.SCA_SCAN_TYPE, working_copy_documents) is_cycodeignore_allowed = is_cycodeignore_allowed_by_scan_config(ctx) - git_head_documents = filter_documents_with_cycodeignore(git_head_documents, repo_path, is_cycodeignore_allowed) - pre_committed_documents = filter_documents_with_cycodeignore( - pre_committed_documents, repo_path, is_cycodeignore_allowed + from_ref_documents = filter_documents_with_cycodeignore(from_ref_documents, repo_path, is_cycodeignore_allowed) + working_copy_documents = filter_documents_with_cycodeignore( + working_copy_documents, repo_path, is_cycodeignore_allowed ) - perform_sca_pre_hook_range_scan_actions(repo_path, git_head_documents, pre_committed_documents) + perform_sca_pre_commit_scan_actions(repo_path, from_ref_documents, base_ref, working_copy_documents) _scan_commit_range_documents( ctx, - git_head_documents, - pre_committed_documents, + from_ref_documents, + working_copy_documents, scan_parameters, configuration_manager.get_sca_pre_commit_timeout_in_seconds(), ) -def _scan_secret_pre_commit(ctx: typer.Context, repo_path: str) -> None: - progress_bar = ctx.obj['progress_bar'] - repo = git_proxy.get_repo(repo_path) - _, diff_index = get_staged_diff_index(repo) - - progress_bar.set_section_length(ScanProgressBarSection.PREPARE_LOCAL_FILES, len(diff_index)) - - documents_to_scan = [] - for diff in diff_index: - progress_bar.update(ScanProgressBarSection.PREPARE_LOCAL_FILES) - documents_to_scan.append( - Document( - get_path_by_os(get_diff_file_path(diff, repo=repo)), - get_diff_file_content(diff), - is_git_diff_format=True, - ) - ) - - documents_to_scan = excluder.exclude_irrelevant_documents_to_scan(consts.SECRET_SCAN_TYPE, documents_to_scan) - - is_cycodeignore_allowed = is_cycodeignore_allowed_by_scan_config(ctx) - documents_to_scan = filter_documents_with_cycodeignore(documents_to_scan, repo_path, is_cycodeignore_allowed) - - scan_documents(ctx, documents_to_scan, get_scan_parameters(ctx), is_git_diff=True) - - -def _scan_sast_pre_commit(ctx: typer.Context, repo_path: str, **_) -> None: - scan_parameters = get_scan_parameters(ctx, (repo_path,)) - - _, pre_committed_documents, diff_documents = get_pre_commit_modified_documents( - progress_bar=ctx.obj['progress_bar'], - progress_bar_section=ScanProgressBarSection.PREPARE_LOCAL_FILES, - repo_path=repo_path, - ) - - pre_committed_documents = excluder.exclude_irrelevant_documents_to_scan( - consts.SAST_SCAN_TYPE, pre_committed_documents - ) - diff_documents = excluder.exclude_irrelevant_documents_to_scan(consts.SAST_SCAN_TYPE, diff_documents) - - is_cycodeignore_allowed = is_cycodeignore_allowed_by_scan_config(ctx) - pre_committed_documents = filter_documents_with_cycodeignore( - pre_committed_documents, repo_path, is_cycodeignore_allowed - ) - diff_documents = filter_documents_with_cycodeignore(diff_documents, repo_path, is_cycodeignore_allowed) - - _scan_commit_range_documents(ctx, pre_committed_documents, diff_documents, scan_parameters=scan_parameters) - - -_SCAN_TYPE_TO_PRE_COMMIT_HANDLER = { - consts.SCA_SCAN_TYPE: _scan_sca_pre_commit, - consts.SECRET_SCAN_TYPE: _scan_secret_pre_commit, - consts.SAST_SCAN_TYPE: _scan_sast_pre_commit, -} - - -def scan_pre_commit(ctx: typer.Context, repo_path: str) -> None: - scan_type = ctx.obj['scan_type'] - if scan_type not in _SCAN_TYPE_TO_PRE_COMMIT_HANDLER: - raise click.ClickException(f'Pre-commit scanning for {scan_type.upper()} is not supported') - - _SCAN_TYPE_TO_PRE_COMMIT_HANDLER[scan_type](ctx, repo_path) - logger.debug('Pre-commit scan completed successfully') - - -def _scan_sca_local_diff( - ctx: typer.Context, repo_path: str, commit_rev: str, paths: Optional[list[str]] = None, **_ -) -> None: - scan_parameters = get_scan_parameters(ctx, (repo_path,)) - - from_commit_documents, working_tree_documents, _diff_documents = get_local_diff_documents( - progress_bar=ctx.obj['progress_bar'], - progress_bar_section=ScanProgressBarSection.PREPARE_LOCAL_FILES, - repo_path=repo_path, - commit_rev=commit_rev, - paths=paths, - ) - - from_commit_documents = excluder.exclude_irrelevant_documents_to_scan(consts.SCA_SCAN_TYPE, from_commit_documents) - working_tree_documents = excluder.exclude_irrelevant_documents_to_scan(consts.SCA_SCAN_TYPE, working_tree_documents) - - is_cycodeignore_allowed = is_cycodeignore_allowed_by_scan_config(ctx) - from_commit_documents = filter_documents_with_cycodeignore( - from_commit_documents, repo_path, is_cycodeignore_allowed - ) - working_tree_documents = filter_documents_with_cycodeignore( - working_tree_documents, repo_path, is_cycodeignore_allowed - ) - - perform_sca_local_diff_scan_actions(repo_path, from_commit_documents, commit_rev, working_tree_documents) - - _scan_commit_range_documents(ctx, from_commit_documents, working_tree_documents, scan_parameters=scan_parameters) - - -def _scan_secret_local_diff( - ctx: typer.Context, repo_path: str, commit_rev: str, paths: Optional[list[str]] = None, **_ +def _scan_secret_pre_commit( + ctx: typer.Context, + repo_path: str, + base_ref: str = consts.GIT_HEAD_COMMIT_REV, + include_unstaged: bool = False, + paths: Optional[list[str]] = None, ) -> None: - _from_commit_documents, _working_tree_documents, diff_documents = get_local_diff_documents( + _from_ref_documents, _working_copy_documents, diff_documents = get_pre_commit_modified_documents( progress_bar=ctx.obj['progress_bar'], progress_bar_section=ScanProgressBarSection.PREPARE_LOCAL_FILES, repo_path=repo_path, - commit_rev=commit_rev, + base_ref=base_ref, + include_unstaged=include_unstaged, paths=paths, ) @@ -475,50 +386,58 @@ def _scan_secret_local_diff( scan_documents(ctx, diff_documents, get_scan_parameters(ctx, (repo_path,)), is_git_diff=True) -def _scan_sast_local_diff( - ctx: typer.Context, repo_path: str, commit_rev: str, paths: Optional[list[str]] = None, **_ +def _scan_sast_pre_commit( + ctx: typer.Context, + repo_path: str, + base_ref: str = consts.GIT_HEAD_COMMIT_REV, + include_unstaged: bool = False, + paths: Optional[list[str]] = None, + **_, ) -> None: scan_parameters = get_scan_parameters(ctx, (repo_path,)) - _from_commit_documents, working_tree_documents, diff_documents = get_local_diff_documents( + _from_ref_documents, working_copy_documents, diff_documents = get_pre_commit_modified_documents( progress_bar=ctx.obj['progress_bar'], progress_bar_section=ScanProgressBarSection.PREPARE_LOCAL_FILES, repo_path=repo_path, - commit_rev=commit_rev, + base_ref=base_ref, + include_unstaged=include_unstaged, paths=paths, ) - working_tree_documents = excluder.exclude_irrelevant_documents_to_scan( - consts.SAST_SCAN_TYPE, working_tree_documents + working_copy_documents = excluder.exclude_irrelevant_documents_to_scan( + consts.SAST_SCAN_TYPE, working_copy_documents ) diff_documents = excluder.exclude_irrelevant_documents_to_scan(consts.SAST_SCAN_TYPE, diff_documents) is_cycodeignore_allowed = is_cycodeignore_allowed_by_scan_config(ctx) - working_tree_documents = filter_documents_with_cycodeignore( - working_tree_documents, repo_path, is_cycodeignore_allowed + working_copy_documents = filter_documents_with_cycodeignore( + working_copy_documents, repo_path, is_cycodeignore_allowed ) diff_documents = filter_documents_with_cycodeignore(diff_documents, repo_path, is_cycodeignore_allowed) - _scan_commit_range_documents(ctx, working_tree_documents, diff_documents, scan_parameters=scan_parameters) + _scan_commit_range_documents(ctx, working_copy_documents, diff_documents, scan_parameters=scan_parameters) -_SCAN_TYPE_TO_LOCAL_DIFF_HANDLER = { - consts.SCA_SCAN_TYPE: _scan_sca_local_diff, - consts.SECRET_SCAN_TYPE: _scan_secret_local_diff, - consts.SAST_SCAN_TYPE: _scan_sast_local_diff, +_SCAN_TYPE_TO_PRE_COMMIT_HANDLER = { + consts.SCA_SCAN_TYPE: _scan_sca_pre_commit, + consts.SECRET_SCAN_TYPE: _scan_secret_pre_commit, + consts.SAST_SCAN_TYPE: _scan_sast_pre_commit, } -def scan_local_diff( - ctx: typer.Context, repo_path: str, commit_rev: str, paths: Optional[list[str]] = None, **kwargs +def scan_pre_commit( + ctx: typer.Context, + repo_path: str, + base_ref: str = consts.GIT_HEAD_COMMIT_REV, + include_unstaged: bool = False, + paths: Optional[list[str]] = None, ) -> None: scan_type = ctx.obj['scan_type'] + if scan_type not in _SCAN_TYPE_TO_PRE_COMMIT_HANDLER: + raise click.ClickException(f'Pre-commit scanning for {scan_type.upper()} is not supported') - progress_bar = ctx.obj['progress_bar'] - progress_bar.start() - - if scan_type not in _SCAN_TYPE_TO_LOCAL_DIFF_HANDLER: - raise click.ClickException(f'Local diff scanning for {scan_type.upper()} is not supported') - - _SCAN_TYPE_TO_LOCAL_DIFF_HANDLER[scan_type](ctx, repo_path, commit_rev, paths=paths, **kwargs) - logger.debug('Local diff scan completed successfully') + _SCAN_TYPE_TO_PRE_COMMIT_HANDLER[scan_type]( + ctx, repo_path, base_ref=base_ref, include_unstaged=include_unstaged, paths=paths + ) + logger.debug('Pre-commit scan completed successfully') diff --git a/cycode/cli/apps/scan/local_diff/__init__.py b/cycode/cli/apps/scan/local_diff/__init__.py deleted file mode 100644 index e69de29b..00000000 diff --git a/cycode/cli/apps/scan/local_diff/local_diff_command.py b/cycode/cli/apps/scan/local_diff/local_diff_command.py deleted file mode 100644 index 7a3b878e..00000000 --- a/cycode/cli/apps/scan/local_diff/local_diff_command.py +++ /dev/null @@ -1,81 +0,0 @@ -import os -from pathlib import Path -from typing import Annotated, Optional - -import typer - -from cycode.cli import consts -from cycode.cli.apps.scan.commit_range_scanner import scan_local_diff -from cycode.cli.exceptions.handle_scan_errors import handle_scan_exception -from cycode.cli.logger import logger -from cycode.cli.utils.git_proxy import git_proxy - - -def _resolve_repo_root(cwd: str) -> str: - """Resolve the repository root from `cwd`, which may be any subdirectory of the repo. - - `git_proxy.get_repo()` requires an exact match (the root or a `.git` dir) unless told to - search parent directories, so without this, running the command from anywhere but the - repo root raises a misleading "not a git repository" error. - """ - repo = git_proxy.get_repo(cwd, search_parent_directories=True) - return repo.working_tree_dir or cwd - - -def _validate_commit_ref(repo_path: str, commit: str) -> None: - """Raise a clear, user-facing error for an unresolvable `--commit` ref. - - A repository with no commits at all is a valid state (the diff falls back to comparing - against the empty tree), so validation is skipped in that case. - """ - repo = git_proxy.get_repo(repo_path) - - try: - repo.rev_parse(consts.GIT_HEAD_COMMIT_REV) - except Exception as e: - logger.debug('Repository has no commits yet; skipping --commit validation', exc_info=e) - return - - try: - repo.commit(commit) - except Exception as e: - raise typer.BadParameter(f'Could not resolve git ref: {commit!r}', param_hint='--commit') from e - - -def local_diff_command( - ctx: typer.Context, - paths: Annotated[ - Optional[list[Path]], - typer.Argument( - help='Optional paths to scope the diff scan to (e.g. the file currently open in an IDE). ' - 'Defaults to the entire working directory.', - show_default=False, - resolve_path=True, - ), - ] = None, - commit: Annotated[ - str, - typer.Option( - '--commit', - '-c', - help='Git ref (commit, branch, or tag) to diff against. ' - 'Compares this ref to the current staged, unstaged, and untracked changes in the working directory.', - ), - ] = 'HEAD', -) -> None: - try: - repo_path = _resolve_repo_root(os.getcwd()) - _validate_commit_ref(repo_path, commit) - except typer.BadParameter: - raise - except Exception as e: - handle_scan_exception(ctx, e) - return - - logger.debug('Starting local diff scan process, %s', {'commit': commit, 'paths': paths}) - - try: - str_paths = [str(path) for path in paths] if paths else None - scan_local_diff(ctx, repo_path=repo_path, commit_rev=commit, paths=str_paths) - except Exception as e: - handle_scan_exception(ctx, e) diff --git a/cycode/cli/apps/scan/pre_commit/pre_commit_command.py b/cycode/cli/apps/scan/pre_commit/pre_commit_command.py index e0cbc7a8..95a9c94a 100644 --- a/cycode/cli/apps/scan/pre_commit/pre_commit_command.py +++ b/cycode/cli/apps/scan/pre_commit/pre_commit_command.py @@ -1,18 +1,94 @@ import os +from pathlib import Path from typing import Annotated, Optional import typer +from cycode.cli import consts from cycode.cli.apps.scan.commit_range_scanner import scan_pre_commit +from cycode.cli.exceptions.handle_scan_errors import handle_scan_exception +from cycode.cli.logger import logger +from cycode.cli.utils.git_proxy import git_proxy + + +def _resolve_repo_root(cwd: str) -> str: + """Resolve the repository root from `cwd`, which may be any subdirectory of the repo. + + `git_proxy.get_repo()` requires an exact match (the root or a `.git` dir) unless told to + search parent directories, so without this, running the command from anywhere but the repo + root raises a misleading "not a git repository" error. The git pre-commit hook framework + always invokes from the repo root, so this is a no-op there; it matters for IDE-style + invocations (--include-unstaged etc.), which may run from any subdirectory. + """ + repo = git_proxy.get_repo(cwd, search_parent_directories=True) + return repo.working_tree_dir or cwd + + +def _validate_base_ref(repo_path: str, base_ref: str) -> None: + """Raise a clear, user-facing error for an unresolvable `--base-ref`. + + A repository with no commits at all is a valid state (the diff falls back to comparing + against the empty tree), so validation is skipped in that case. + """ + repo = git_proxy.get_repo(repo_path) + + try: + repo.rev_parse(consts.GIT_HEAD_COMMIT_REV) + except Exception as e: + logger.debug('Repository has no commits yet; skipping --base-ref validation', exc_info=e) + return + + try: + repo.commit(base_ref) + except Exception as e: + raise typer.BadParameter(f'Could not resolve git ref: {base_ref!r}', param_hint='--base-ref') from e def pre_commit_command( ctx: typer.Context, _: Annotated[Optional[list[str]], typer.Argument(help='Ignored arguments', hidden=True)] = None, + base_ref: Annotated[ + str, + typer.Option( + '--base-ref', + '-b', + help='Git ref (commit, branch, or tag) to diff against. Defaults to HEAD, matching the ' + 'pre-commit hook behavior. Combine with --include-unstaged for IDE-style local diff scanning.', + ), + ] = consts.GIT_HEAD_COMMIT_REV, + include_unstaged: Annotated[ + bool, + typer.Option( + '--include-unstaged', + help='Also scan unstaged changes to tracked files, not just what is staged. ' + 'Off by default, so the pre-commit hook flow is unaffected.', + ), + ] = False, + paths: Annotated[ + Optional[list[Path]], + typer.Option( + '--path', + help='Optional paths to scope the diff scan to (e.g. the file currently open in an IDE). ' + 'Defaults to the entire working directory. Repeatable.', + show_default=False, + resolve_path=True, + ), + ] = None, ) -> None: - repo_path = os.getcwd() # change locally for easy testing + try: + repo_path = _resolve_repo_root(os.getcwd()) + _validate_base_ref(repo_path, base_ref) + except typer.BadParameter: + raise + except Exception as e: + handle_scan_exception(ctx, e) + return progress_bar = ctx.obj['progress_bar'] progress_bar.start() - scan_pre_commit(ctx, repo_path) + try: + str_paths = [str(path) for path in paths] if paths else None + scan_pre_commit(ctx, repo_path, base_ref=base_ref, include_unstaged=include_unstaged, paths=str_paths) + except Exception as e: + handle_scan_exception(ctx, e) diff --git a/cycode/cli/consts.py b/cycode/cli/consts.py index 7c4f77ec..e2d09ce1 100644 --- a/cycode/cli/consts.py +++ b/cycode/cli/consts.py @@ -8,7 +8,6 @@ PRE_RECEIVE_COMMAND_SCAN_TYPE_OLD = 'pre_receive' COMMIT_HISTORY_COMMAND_SCAN_TYPE = 'commit-history' COMMIT_HISTORY_COMMAND_SCAN_TYPE_OLD = 'commit_history' -LOCAL_DIFF_COMMAND_SCAN_TYPE = 'local-diff' SECRET_SCAN_TYPE = 'secret' IAC_SCAN_TYPE = 'iac' @@ -196,7 +195,6 @@ PRE_RECEIVE_COMMAND_SCAN_TYPE_OLD, COMMIT_HISTORY_COMMAND_SCAN_TYPE, COMMIT_HISTORY_COMMAND_SCAN_TYPE_OLD, - LOCAL_DIFF_COMMAND_SCAN_TYPE, ] DEFAULT_CYCODE_DOMAIN = 'cycode.com' diff --git a/cycode/cli/files_collector/commit_range_documents.py b/cycode/cli/files_collector/commit_range_documents.py index 430f56a3..488567d8 100644 --- a/cycode/cli/files_collector/commit_range_documents.py +++ b/cycode/cli/files_collector/commit_range_documents.py @@ -47,7 +47,7 @@ def get_safe_head_reference_for_diff(repo: 'Repo') -> str: return consts.GIT_EMPTY_TREE_OBJECT -def get_staged_diff_index(repo: 'Repo') -> tuple[str, 'DiffIndex']: +def get_staged_diff_index(repo: 'Repo', paths: Optional[list[str]] = None) -> tuple[str, 'DiffIndex']: """Diff the index against HEAD, or against the empty tree in repositories with no commits. GitPython only inverts the `R` flag for HEAD, so `R` must be off for the empty tree to keep @@ -55,13 +55,14 @@ def get_staged_diff_index(repo: 'Repo') -> tuple[str, 'DiffIndex']: Args: repo: Git repository object + paths: Optional pathspec to scope the diff to Returns: The reference that was diffed against, and the resulting diff index """ head_reference = get_safe_head_reference_for_diff(repo) reverse = head_reference == consts.GIT_HEAD_COMMIT_REV - return head_reference, repo.index.diff(head_reference, create_patch=True, R=reverse) + return head_reference, repo.index.diff(head_reference, create_patch=True, R=reverse, paths=paths or None) def _does_reach_to_max_commits_to_scan_limit(commit_ids: list[str], max_commits_count: Optional[int]) -> bool: @@ -422,86 +423,59 @@ def get_pre_commit_modified_documents( progress_bar: 'BaseProgressBar', progress_bar_section: 'ProgressBarSection', repo_path: str, -) -> tuple[list[Document], list[Document], list[Document]]: - git_head_documents = [] - pre_committed_documents = [] - diff_documents = [] - - repo = git_proxy.get_repo(repo_path) - head_reference, diff_index = get_staged_diff_index(repo) - progress_bar.set_section_length(progress_bar_section, len(diff_index)) - for diff in diff_index: - progress_bar.update(progress_bar_section) - - file_path = get_path_by_os(get_diff_file_path(diff, repo=repo)) - - diff_documents.append( - Document( - path=file_path, - content=get_diff_file_content(diff), - is_git_diff_format=True, - ) - ) - - # Only get file content from HEAD if HEAD exists (not the empty tree hash) - if head_reference == consts.GIT_HEAD_COMMIT_REV: - file_content = _get_file_content_from_commit_diff(repo, head_reference, diff) - if file_content: - git_head_documents.append(Document(file_path, file_content)) - - if os.path.exists(file_path): - file_content = get_file_content(file_path) - if file_content: - pre_committed_documents.append(Document(file_path, file_content)) - - return git_head_documents, pre_committed_documents, diff_documents - - -def _is_path_included(file_path: str, paths: Optional[list[str]]) -> bool: - if not paths: - return True - - normalized_file_path = os.path.normpath(file_path) - for path in paths: - normalized_path = os.path.normpath(path) - if normalized_file_path == normalized_path or normalized_file_path.startswith(normalized_path + os.sep): - return True - - return False - - -def get_local_diff_documents( - progress_bar: 'BaseProgressBar', - progress_bar_section: 'ProgressBarSection', - repo_path: str, - commit_rev: str, + base_ref: str = consts.GIT_HEAD_COMMIT_REV, + include_unstaged: bool = False, paths: Optional[list[str]] = None, ) -> tuple[list[Document], list[Document], list[Document]]: - """Diffs `commit_rev` against the current working tree: staged, unstaged, and untracked changes. + """Diffs `base_ref` against the staged index (default) or the full working tree. + + Args: + base_ref: Git ref to diff against. Defaults to HEAD, matching the pre-commit hook's + historical behavior exactly. + include_unstaged: If False (default), diffs the staged index only -- this is the exact + pre-commit hook flow. If True, diffs the full working tree (staged + unstaged). + paths: Optional pathspec to scope the diff to. Returns: - (from_commit_documents, working_tree_documents, diff_documents) - the same shape as - `get_pre_commit_modified_documents`, but compared against an arbitrary commit (not just HEAD) - and against the full working tree (not just the staged index), and including untracked files. + (from_ref_documents, working_copy_documents, diff_documents). `from_ref_documents` holds + each changed file's content at `base_ref` (skipped if it didn't exist there); + `working_copy_documents` holds each changed file's current on-disk content (skipped if + the file no longer exists); `diff_documents` holds the unified diff per changed file. """ - from_commit_documents = [] - working_tree_documents = [] + from_ref_documents = [] + working_copy_documents = [] diff_documents = [] repo = git_proxy.get_repo(repo_path) - try: - diff_target = repo.commit(commit_rev) - except Exception as e: - # Repository has no commits yet; diff against the empty tree instead. - # (git_proxy.get_null_tree() is only a sentinel usable as the `other` side of a diff, - # so we resolve the well-known empty tree object itself to diff from.) - logger.debug( - 'Could not resolve commit_rev, falling back to the empty tree, %s', {'commit_rev': commit_rev}, exc_info=e - ) - diff_target = repo.tree(consts.GIT_EMPTY_TREE_OBJECT) + if base_ref == consts.GIT_HEAD_COMMIT_REV and not include_unstaged: + # The exact pre-commit hook flow, byte-for-byte unchanged: diff the staged index against + # HEAD (or the empty tree in a brand-new repo), via get_staged_diff_index()'s proven + # R-flag handling. Keeping this as its own branch (rather than folding it into the + # `diff_target.diff(...)` calls below) means this default, most-used path can't regress. + resolved_ref, diff_index = get_staged_diff_index(repo, paths=paths) + else: + try: + diff_target = repo.commit(base_ref) + except Exception as e: + # Repository has no commits yet, or base_ref doesn't resolve; diff against the empty + # tree instead. (git_proxy.get_null_tree() is only a sentinel usable as the `other` + # side of a diff, so we resolve the well-known empty tree object itself to diff from.) + logger.debug( + 'Could not resolve base_ref, falling back to the empty tree, %s', {'base_ref': base_ref}, exc_info=e + ) + diff_target = repo.tree(consts.GIT_EMPTY_TREE_OBJECT) + + resolved_ref = base_ref + if include_unstaged: + diff_index = diff_target.diff(None, create_patch=True, paths=paths or None) + else: + # `Diffable.diff()` defaults `other` to the staged index, so omitting it here diffs + # `base_ref` against the index only -- the same "staged changes against an arbitrary + # ref" semantics as `git diff --cached `, verified to produce identical, + # correctly-directed patches. + diff_index = diff_target.diff(create_patch=True, paths=paths or None) - diff_index = diff_target.diff(None, create_patch=True, paths=paths or None) progress_bar.set_section_length(progress_bar_section, len(diff_index)) for diff in diff_index: progress_bar.update(progress_bar_section) @@ -516,31 +490,16 @@ def get_local_diff_documents( ) ) - file_content = _get_file_content_from_commit_diff(repo, commit_rev, diff) + file_content = _get_file_content_from_commit_diff(repo, resolved_ref, diff) if file_content is not None: - from_commit_documents.append(Document(file_path, file_content)) + from_ref_documents.append(Document(file_path, file_content)) if os.path.exists(file_path): file_content = get_file_content(file_path) if file_content: - working_tree_documents.append(Document(file_path, file_content)) - - for relative_path in repo.untracked_files if repo.working_tree_dir else []: - absolute_path = get_path_by_os(os.path.join(repo.working_tree_dir, relative_path)) - if not _is_path_included(absolute_path, paths): - continue - - file_content = get_file_content(absolute_path) - if not file_content: - continue - - # A new, untracked file has no real diff to show against `commit_rev`; its full content - # stands in for both the "current" content and the diff channel (matches how `path`/ - # `repository` scans already treat whole files: is_git_diff_format=False). - working_tree_documents.append(Document(absolute_path, file_content)) - diff_documents.append(Document(absolute_path, file_content)) + working_copy_documents.append(Document(file_path, file_content)) - return from_commit_documents, working_tree_documents, diff_documents + return from_ref_documents, working_copy_documents, diff_documents def parse_commit_range(commit_range: str, path: str) -> tuple[Optional[str], Optional[str], Optional[str]]: diff --git a/cycode/cli/files_collector/sca/sca_file_collector.py b/cycode/cli/files_collector/sca/sca_file_collector.py index 72c694d3..3ac2d3e9 100644 --- a/cycode/cli/files_collector/sca/sca_file_collector.py +++ b/cycode/cli/files_collector/sca/sca_file_collector.py @@ -64,23 +64,19 @@ def perform_sca_pre_commit_range_scan_actions( _add_ecosystem_related_files_if_exists(to_commit_documents, repo, to_commit_rev) -def perform_sca_pre_hook_range_scan_actions( - repo_path: str, git_head_documents: list[Document], pre_committed_documents: list[Document] +def perform_sca_pre_commit_scan_actions( + repo_path: str, from_ref_documents: list[Document], base_ref: str, working_copy_documents: list[Document] ) -> None: - repo = git_proxy.get_repo(repo_path) - _add_ecosystem_related_files_if_exists(git_head_documents, repo, consts.GIT_HEAD_COMMIT_REV) - _add_ecosystem_related_files_if_exists(pre_committed_documents) - + """Add ecosystem-related project files for a pre-commit-style scan (staged-index-or-working-tree vs `base_ref`). -def perform_sca_local_diff_scan_actions( - repo_path: str, from_commit_documents: list[Document], from_commit_rev: str, working_tree_documents: list[Document] -) -> None: - """Same as `perform_sca_pre_hook_range_scan_actions`, but `from_commit_rev` can be any ref, not just HEAD.""" + `base_ref` can be any ref, not just HEAD -- covers both the default pre-commit hook flow and + the opt-in `--base-ref`/`--include-unstaged` flags. + """ repo = git_proxy.get_repo(repo_path) - _add_ecosystem_related_files_if_exists(from_commit_documents, repo, from_commit_rev) - # working tree documents reflect the live filesystem (staged + unstaged + untracked), so their + _add_ecosystem_related_files_if_exists(from_ref_documents, repo, base_ref) + # working copy documents reflect the live filesystem (staged, or staged+unstaged), so their # related project files must also be read from disk, not from a commit tree. - _add_ecosystem_related_files_if_exists(working_tree_documents) + _add_ecosystem_related_files_if_exists(working_copy_documents) def _get_doc_ecosystem_related_project_files( diff --git a/tests/cli/commands/scan/test_commit_range_scanner.py b/tests/cli/commands/scan/test_commit_range_scanner.py index f18d59bc..24743e55 100644 --- a/tests/cli/commands/scan/test_commit_range_scanner.py +++ b/tests/cli/commands/scan/test_commit_range_scanner.py @@ -49,8 +49,8 @@ def test_commit_range_scan_falls_back_to_api_when_presigned_upload_raises_wrappe mock_handle_exception.assert_not_called() -class TestScanLocalDiff: - """Test the scan_local_diff dispatcher.""" +class TestScanPreCommit: + """Test the scan_pre_commit dispatcher, including the folded-in local-diff-style flags.""" def _make_ctx(self, scan_type: str) -> MagicMock: mock_ctx = MagicMock() @@ -61,42 +61,46 @@ def test_unsupported_scan_type_raises(self) -> None: import click import pytest - from cycode.cli.apps.scan.commit_range_scanner import scan_local_diff + from cycode.cli.apps.scan.commit_range_scanner import scan_pre_commit mock_ctx = self._make_ctx(consts.IAC_SCAN_TYPE) with pytest.raises(click.ClickException, match='IAC'): - scan_local_diff(mock_ctx, repo_path='/repo', commit_rev='HEAD') + scan_pre_commit(mock_ctx, repo_path='/repo') - def test_dispatches_secret_scan_type(self) -> None: + def test_dispatches_secret_scan_type_with_default_args(self) -> None: from cycode.cli.apps.scan import commit_range_scanner - from cycode.cli.apps.scan.commit_range_scanner import scan_local_diff + from cycode.cli.apps.scan.commit_range_scanner import scan_pre_commit mock_handler = Mock() - with patch.dict(commit_range_scanner._SCAN_TYPE_TO_LOCAL_DIFF_HANDLER, {consts.SECRET_SCAN_TYPE: mock_handler}): + with patch.dict(commit_range_scanner._SCAN_TYPE_TO_PRE_COMMIT_HANDLER, {consts.SECRET_SCAN_TYPE: mock_handler}): mock_ctx = self._make_ctx(consts.SECRET_SCAN_TYPE) - scan_local_diff(mock_ctx, repo_path='/repo', commit_rev='abc123', paths=['/repo/file.py']) + scan_pre_commit(mock_ctx, repo_path='/repo') - mock_handler.assert_called_once_with(mock_ctx, '/repo', 'abc123', paths=['/repo/file.py']) + mock_handler.assert_called_once_with(mock_ctx, '/repo', base_ref='HEAD', include_unstaged=False, paths=None) - def test_dispatches_sca_scan_type(self) -> None: + def test_dispatches_sca_scan_type_with_folded_in_flags(self) -> None: from cycode.cli.apps.scan import commit_range_scanner - from cycode.cli.apps.scan.commit_range_scanner import scan_local_diff + from cycode.cli.apps.scan.commit_range_scanner import scan_pre_commit mock_handler = Mock() - with patch.dict(commit_range_scanner._SCAN_TYPE_TO_LOCAL_DIFF_HANDLER, {consts.SCA_SCAN_TYPE: mock_handler}): + with patch.dict(commit_range_scanner._SCAN_TYPE_TO_PRE_COMMIT_HANDLER, {consts.SCA_SCAN_TYPE: mock_handler}): mock_ctx = self._make_ctx(consts.SCA_SCAN_TYPE) - scan_local_diff(mock_ctx, repo_path='/repo', commit_rev='HEAD') + scan_pre_commit( + mock_ctx, repo_path='/repo', base_ref='abc123', include_unstaged=True, paths=['/repo/file.py'] + ) - mock_handler.assert_called_once_with(mock_ctx, '/repo', 'HEAD', paths=None) + mock_handler.assert_called_once_with( + mock_ctx, '/repo', base_ref='abc123', include_unstaged=True, paths=['/repo/file.py'] + ) def test_dispatches_sast_scan_type(self) -> None: from cycode.cli.apps.scan import commit_range_scanner - from cycode.cli.apps.scan.commit_range_scanner import scan_local_diff + from cycode.cli.apps.scan.commit_range_scanner import scan_pre_commit mock_handler = Mock() - with patch.dict(commit_range_scanner._SCAN_TYPE_TO_LOCAL_DIFF_HANDLER, {consts.SAST_SCAN_TYPE: mock_handler}): + with patch.dict(commit_range_scanner._SCAN_TYPE_TO_PRE_COMMIT_HANDLER, {consts.SAST_SCAN_TYPE: mock_handler}): mock_ctx = self._make_ctx(consts.SAST_SCAN_TYPE) - scan_local_diff(mock_ctx, repo_path='/repo', commit_rev='HEAD') + scan_pre_commit(mock_ctx, repo_path='/repo') - mock_handler.assert_called_once_with(mock_ctx, '/repo', 'HEAD', paths=None) + mock_handler.assert_called_once_with(mock_ctx, '/repo', base_ref='HEAD', include_unstaged=False, paths=None) diff --git a/tests/cli/commands/scan/test_local_diff_command.py b/tests/cli/commands/scan/test_pre_commit_command.py similarity index 62% rename from tests/cli/commands/scan/test_local_diff_command.py rename to tests/cli/commands/scan/test_pre_commit_command.py index 36ab93cb..5c3f6263 100644 --- a/tests/cli/commands/scan/test_local_diff_command.py +++ b/tests/cli/commands/scan/test_pre_commit_command.py @@ -9,10 +9,10 @@ from git import Repo from typer.testing import CliRunner -from cycode.cli.apps.scan.local_diff.local_diff_command import ( +from cycode.cli.apps.scan.pre_commit.pre_commit_command import ( _resolve_repo_root, - _validate_commit_ref, - local_diff_command, + _validate_base_ref, + pre_commit_command, ) @@ -26,8 +26,8 @@ def temporary_git_repository() -> Generator[tuple[str, Repo], None, None]: repo.close() -class TestValidateCommitRef: - def test_valid_commit_ref_does_not_raise(self) -> None: +class TestValidateBaseRef: + def test_valid_base_ref_does_not_raise(self) -> None: with temporary_git_repository() as (temp_dir, repo): file_path = os.path.join(temp_dir, 'file.txt') with open(file_path, 'w') as f: @@ -35,9 +35,9 @@ def test_valid_commit_ref_does_not_raise(self) -> None: repo.index.add(['file.txt']) repo.index.commit('initial') - _validate_commit_ref(temp_dir, 'HEAD') + _validate_base_ref(temp_dir, 'HEAD') - def test_invalid_commit_ref_raises_bad_parameter(self) -> None: + def test_invalid_base_ref_raises_bad_parameter(self) -> None: with temporary_git_repository() as (temp_dir, repo): file_path = os.path.join(temp_dir, 'file.txt') with open(file_path, 'w') as f: @@ -46,11 +46,11 @@ def test_invalid_commit_ref_raises_bad_parameter(self) -> None: repo.index.commit('initial') with pytest.raises(typer.BadParameter): - _validate_commit_ref(temp_dir, 'not-a-real-ref') + _validate_base_ref(temp_dir, 'not-a-real-ref') def test_empty_repository_with_default_head_does_not_raise(self) -> None: with temporary_git_repository() as (temp_dir, _repo): - _validate_commit_ref(temp_dir, 'HEAD') + _validate_base_ref(temp_dir, 'HEAD') class TestResolveRepoRoot: @@ -59,7 +59,8 @@ class TestResolveRepoRoot: Regression test: git_proxy.get_repo() requires an exact match (root or .git dir) unless told to search parent directories, so a naive `os.getcwd()` breaks the moment the command is invoked from a subdirectory -- exactly how an IDE plugin scoped to the open file's - folder, or a workspace subdirectory in a monorepo, would invoke it. + folder, or a workspace subdirectory in a monorepo, would invoke it. The git pre-commit hook + framework always invokes from the repo root, so this is a no-op there. """ def test_resolves_root_from_subdirectory(self) -> None: @@ -88,17 +89,17 @@ def test_resolves_root_when_already_at_root(self) -> None: assert os.path.realpath(resolved_root) == os.path.realpath(temp_dir) -class TestLocalDiffCommandPathResolution: - """A relative path argument must reach scan_local_diff already resolved to absolute. +class TestPreCommitCommandPathResolution: + """A relative --path value must reach scan_pre_commit already resolved to absolute. - Regression test: get_local_diff_documents/_is_path_included compare an always-absolute - path (derived from the repo's working tree) against the raw `paths` strings. Without - `resolve_path=True` on the CLI argument, a relative path (the natural way to invoke this - command, e.g. `cycode scan local-diff sub/app.py` from the repo root) would never match, - silently dropping untracked files from the scoped scan. + Regression test: get_pre_commit_modified_documents compares an always-absolute path + (derived from the repo's working tree) against the raw `paths` strings. Without + `resolve_path=True` on the CLI option, a relative path (the natural way to invoke this, + e.g. `--path sub/app.py` from the repo root) would never match, silently dropping files + from the scoped scan. """ - def test_relative_path_argument_is_resolved_to_absolute(self, monkeypatch: pytest.MonkeyPatch) -> None: + def test_relative_path_option_is_resolved_to_absolute(self, monkeypatch: pytest.MonkeyPatch) -> None: original_cwd = os.getcwd() with temporary_git_repository() as (temp_dir, repo): os.makedirs(os.path.join(temp_dir, 'sub')) @@ -109,15 +110,15 @@ def test_relative_path_argument_is_resolved_to_absolute(self, monkeypatch: pytes repo.index.commit('initial') app = typer.Typer() - app.command()(local_diff_command) + app.command()(pre_commit_command) # Actually chdir rather than patching os.getcwd(): on Windows/Python 3.9, Click's # path resolution does not consistently go through the patched os.getcwd symbol, # so the mock silently has no effect there and the test passes for the wrong reason # (or, as happened in CI, resolves against the real process cwd instead). monkeypatch.chdir(temp_dir) - with patch('cycode.cli.apps.scan.local_diff.local_diff_command.scan_local_diff') as mock_scan: - result = CliRunner().invoke(app, ['sub/app.py'], obj=MagicMock()) + with patch('cycode.cli.apps.scan.pre_commit.pre_commit_command.scan_pre_commit') as mock_scan: + result = CliRunner().invoke(app, ['--path', 'sub/app.py'], obj=MagicMock()) # monkeypatch only restores the cwd at fixture teardown, which runs after this test # function returns -- but temporary_git_repository()'s cleanup (below, at the end of @@ -133,10 +134,10 @@ def test_relative_path_argument_is_resolved_to_absolute(self, monkeypatch: pytes ] -class TestLocalDiffCommandFromSubdirectory: +class TestPreCommitCommandFromSubdirectory: """End-to-end: invoking the command from a repo subdirectory must not fail.""" - def test_scan_local_diff_called_with_repo_root_not_subdirectory(self, monkeypatch: pytest.MonkeyPatch) -> None: + def test_scan_pre_commit_called_with_repo_root_not_subdirectory(self, monkeypatch: pytest.MonkeyPatch) -> None: original_cwd = os.getcwd() with temporary_git_repository() as (temp_dir, repo): os.makedirs(os.path.join(temp_dir, 'sub')) @@ -147,19 +148,49 @@ def test_scan_local_diff_called_with_repo_root_not_subdirectory(self, monkeypatc repo.index.commit('initial') app = typer.Typer() - app.command()(local_diff_command) + app.command()(pre_commit_command) # Actually chdir rather than patching os.getcwd() -- see the comment in - # TestLocalDiffCommandPathResolution for why the mock is unreliable on Windows. + # TestPreCommitCommandPathResolution for why the mock is unreliable on Windows. monkeypatch.chdir(os.path.join(temp_dir, 'sub')) - with patch('cycode.cli.apps.scan.local_diff.local_diff_command.scan_local_diff') as mock_scan: + with patch('cycode.cli.apps.scan.pre_commit.pre_commit_command.scan_pre_commit') as mock_scan: result = CliRunner().invoke(app, [], obj=MagicMock()) - # See the matching comment in TestLocalDiffCommandPathResolution: must chdir back out + # See the matching comment in TestPreCommitCommandPathResolution: must chdir back out # before temporary_git_repository()'s cleanup runs, or Windows can't delete the dir. monkeypatch.chdir(original_cwd) + assert result.exit_code == 0, result.output + mock_scan.assert_called_once() + args, _kwargs = mock_scan.call_args + assert os.path.realpath(args[1]) == os.path.realpath(temp_dir) + + +class TestPreCommitCommandDefaults: + """The default invocation (no new flags) must match the pre-existing hook behavior exactly.""" + + def test_default_invocation_uses_head_and_staged_only(self, monkeypatch: pytest.MonkeyPatch) -> None: + original_cwd = os.getcwd() + with temporary_git_repository() as (temp_dir, repo): + file_path = os.path.join(temp_dir, 'file.txt') + with open(file_path, 'w') as f: + f.write('content') + repo.index.add(['file.txt']) + repo.index.commit('initial') + + app = typer.Typer() + app.command()(pre_commit_command) + + monkeypatch.chdir(temp_dir) + # Simulate the pre-commit framework's own auto-passed staged filenames -- must be + # silently swallowed, not treated as a --path scope, per the ignored hidden argument. + with patch('cycode.cli.apps.scan.pre_commit.pre_commit_command.scan_pre_commit') as mock_scan: + result = CliRunner().invoke(app, ['file.txt'], obj=MagicMock()) + monkeypatch.chdir(original_cwd) + assert result.exit_code == 0, result.output mock_scan.assert_called_once() _, kwargs = mock_scan.call_args - assert os.path.realpath(kwargs['repo_path']) == os.path.realpath(temp_dir) + assert kwargs['base_ref'] == 'HEAD' + assert kwargs['include_unstaged'] is False + assert kwargs['paths'] is None diff --git a/tests/cli/files_collector/test_commit_range_documents.py b/tests/cli/files_collector/test_commit_range_documents.py index 78ea647e..ccd0e202 100644 --- a/tests/cli/files_collector/test_commit_range_documents.py +++ b/tests/cli/files_collector/test_commit_range_documents.py @@ -15,7 +15,7 @@ calculate_pre_receive_commit_range, collect_commit_range_diff_documents, get_diff_file_path, - get_local_diff_documents, + get_pre_commit_modified_documents, get_safe_head_reference_for_diff, get_staged_diff_index, parse_commit_range, @@ -1170,8 +1170,8 @@ def test_collect_with_various_commit_range_formats(self) -> None: assert len(documents) == 2, f'Expected 2 documents from single commit A, got {len(documents)}' -class TestGetLocalDiffDocuments: - """Test get_local_diff_documents: diffing a commit against the working directory.""" +class TestGetPreCommitModifiedDocuments: + """Test get_pre_commit_modified_documents across its (base_ref, include_unstaged) combinations.""" @staticmethod def _mock_progress_bar() -> Mock: @@ -1180,13 +1180,52 @@ def _mock_progress_bar() -> Mock: mock_progress_bar.update = Mock() return mock_progress_bar - def test_combines_staged_and_unstaged_changes(self) -> None: - """A tracked file with both a staged and an unstaged edit should appear as a single diff/document. + def test_default_call_matches_default_arguments(self) -> None: + """Regression-critical: calling with no base_ref/include_unstaged/paths at all (the exact + pre-commit hook invocation) must behave identically to passing the documented defaults. + """ + with temporary_git_repository() as (temp_dir, repo): + file_path = os.path.join(temp_dir, 'tracked.txt') + with open(file_path, 'w') as f: + f.write('line1\n') + repo.index.add(['tracked.txt']) + repo.index.commit('initial') + + with open(file_path, 'a') as f: + f.write('staged\n') + repo.index.add(['tracked.txt']) + + no_args_result = get_pre_commit_modified_documents( + progress_bar=self._mock_progress_bar(), + progress_bar_section='prepare', + repo_path=temp_dir, + ) + explicit_defaults_result = get_pre_commit_modified_documents( + progress_bar=self._mock_progress_bar(), + progress_bar_section='prepare', + repo_path=temp_dir, + base_ref='HEAD', + include_unstaged=False, + paths=None, + ) - Files are written with newline='' so the on-disk (and therefore committed) content is exactly - the LF bytes given, regardless of platform -- otherwise Python's default text-mode write - translates '\\n' to the OS line ending, and on Windows the resulting CRLF blob makes the - `from_docs[0].content == 'line1'` assertion below fail with a trailing '\\r'. + no_args_contents = [(d.path, d.content, d.is_git_diff_format) for docs in no_args_result for d in docs] + explicit_contents = [ + (d.path, d.content, d.is_git_diff_format) for docs in explicit_defaults_result for d in docs + ] + assert no_args_contents == explicit_contents + + def test_default_staged_only_ignores_unstaged_changes(self) -> None: + """The default (include_unstaged=False) diff/patch channel must only ever reflect staged + content, matching today's pre-commit hook behavior exactly -- an unstaged edit on top must + not leak into `diff_docs`. `work_docs` always reflects the live on-disk file regardless of + staging state (true both before and after this change -- it's a plain file read, with no + awareness of the index), so it legitimately includes the unstaged line too. + + Files are written with newline='' so the on-disk (and therefore committed) content is + exactly the LF bytes given, regardless of platform -- otherwise Python's default text-mode + write translates '\\n' to the OS line ending, and on Windows the resulting CRLF blob makes + the `from_docs[0].content == 'line1\\n'` assertion below fail with a trailing '\\r'. """ with temporary_git_repository() as (temp_dir, repo): file_path = os.path.join(temp_dir, 'tracked.txt') @@ -1202,11 +1241,10 @@ def test_combines_staged_and_unstaged_changes(self) -> None: with open(file_path, 'a', newline='') as f: f.write('unstaged\n') - from_docs, work_docs, diff_docs = get_local_diff_documents( + from_docs, work_docs, diff_docs = get_pre_commit_modified_documents( progress_bar=self._mock_progress_bar(), progress_bar_section='prepare', repo_path=temp_dir, - commit_rev='HEAD', ) assert len(from_docs) == 1 @@ -1218,40 +1256,47 @@ def test_combines_staged_and_unstaged_changes(self) -> None: assert len(diff_docs) == 1 assert diff_docs[0].is_git_diff_format is True assert '+staged' in diff_docs[0].content - assert '+unstaged' in diff_docs[0].content + assert '+unstaged' not in diff_docs[0].content - def test_includes_untracked_files(self) -> None: - """A brand-new, never-added file should show up as full content in both channels.""" + def test_include_unstaged_combines_staged_and_unstaged_changes(self) -> None: + """A tracked file with both a staged and an unstaged edit should appear as a single diff/document + once include_unstaged=True. Files use newline='' -- see the comment in + test_default_staged_only_ignores_unstaged_changes for why. + """ with temporary_git_repository() as (temp_dir, repo): - tracked_path = os.path.join(temp_dir, 'tracked.txt') - with open(tracked_path, 'w') as f: + file_path = os.path.join(temp_dir, 'tracked.txt') + with open(file_path, 'w', newline='') as f: f.write('line1\n') repo.index.add(['tracked.txt']) repo.index.commit('initial') - untracked_path = os.path.join(temp_dir, 'new_secret.txt') - with open(untracked_path, 'w') as f: - f.write('super-secret-value\n') + with open(file_path, 'a', newline='') as f: + f.write('staged\n') + repo.index.add(['tracked.txt']) + + with open(file_path, 'a', newline='') as f: + f.write('unstaged\n') - from_docs, work_docs, diff_docs = get_local_diff_documents( + from_docs, work_docs, diff_docs = get_pre_commit_modified_documents( progress_bar=self._mock_progress_bar(), progress_bar_section='prepare', repo_path=temp_dir, - commit_rev='HEAD', + include_unstaged=True, ) - assert not any(doc.path == get_path_by_os(untracked_path) for doc in from_docs) + assert len(from_docs) == 1 + assert from_docs[0].content == 'line1' - untracked_work_doc = next(doc for doc in work_docs if doc.path == get_path_by_os(untracked_path)) - assert untracked_work_doc.content == 'super-secret-value\n' - assert untracked_work_doc.is_git_diff_format is False + assert len(work_docs) == 1 + assert work_docs[0].content == 'line1\nstaged\nunstaged\n' - untracked_diff_doc = next(doc for doc in diff_docs if doc.path == get_path_by_os(untracked_path)) - assert untracked_diff_doc.content == 'super-secret-value\n' - assert untracked_diff_doc.is_git_diff_format is False + assert len(diff_docs) == 1 + assert diff_docs[0].is_git_diff_format is True + assert '+staged' in diff_docs[0].content + assert '+unstaged' in diff_docs[0].content def test_scopes_to_requested_paths(self) -> None: - """Only files under the requested paths should be collected, tracked or untracked.""" + """Only files under the requested paths should be collected.""" with temporary_git_repository() as (temp_dir, repo): os.makedirs(os.path.join(temp_dir, 'sub')) @@ -1268,30 +1313,23 @@ def test_scopes_to_requested_paths(self) -> None: f.write("print('new')\n") with open(excluded_path, 'a') as f: f.write("print('new')\n") + repo.index.add(['sub/app.py', 'excluded.py']) - untracked_included = os.path.join(temp_dir, 'sub', 'new_file.py') - untracked_excluded = os.path.join(temp_dir, 'new_file.py') - with open(untracked_included, 'w') as f: - f.write('secret\n') - with open(untracked_excluded, 'w') as f: - f.write('secret\n') - - _from_docs, work_docs, diff_docs = get_local_diff_documents( + _from_docs, work_docs, diff_docs = get_pre_commit_modified_documents( progress_bar=self._mock_progress_bar(), progress_bar_section='prepare', repo_path=temp_dir, - commit_rev='HEAD', paths=[os.path.join(temp_dir, 'sub')], ) diff_paths = {doc.path for doc in diff_docs} work_paths = {doc.path for doc in work_docs} - assert diff_paths == {get_path_by_os(included_path), get_path_by_os(untracked_included)} - assert work_paths == {get_path_by_os(included_path), get_path_by_os(untracked_included)} + assert diff_paths == {get_path_by_os(included_path)} + assert work_paths == {get_path_by_os(included_path)} - def test_uses_explicit_commit_rev_not_just_head(self) -> None: - """Diffing against an older commit should show changes made since that commit, not just since HEAD.""" + def test_include_unstaged_uses_explicit_base_ref_not_just_head(self) -> None: + """Diffing against an older ref should show changes made since that ref, not just since HEAD.""" with temporary_git_repository() as (temp_dir, repo): file_path = os.path.join(temp_dir, 'tracked.txt') with open(file_path, 'w') as f: @@ -1307,33 +1345,76 @@ def test_uses_explicit_commit_rev_not_just_head(self) -> None: with open(file_path, 'a') as f: f.write('unstaged-C\n') - _from_docs, _work_docs, diff_docs = get_local_diff_documents( + _from_docs, _work_docs, diff_docs = get_pre_commit_modified_documents( progress_bar=self._mock_progress_bar(), progress_bar_section='prepare', repo_path=temp_dir, - commit_rev=first_commit.hexsha, + base_ref=first_commit.hexsha, + include_unstaged=True, ) assert len(diff_docs) == 1 assert '+B' in diff_docs[0].content assert '+unstaged-C' in diff_docs[0].content + def test_staged_only_against_non_head_base_ref_shows_correct_diff_direction(self) -> None: + """The new combination: staged-only (include_unstaged=False) diffed against an older ref. + + Verifies GitPython's Diffable.diff() defaulting `other` to the index produces the same, + correctly-directed patch as `git diff --cached ` -- this is the one code path + with no prior coverage (see plan: verified manually against raw git before implementing). + """ + with temporary_git_repository() as (temp_dir, repo): + file_path = os.path.join(temp_dir, 'tracked.txt') + with open(file_path, 'w') as f: + f.write('A\n') + repo.index.add(['tracked.txt']) + first_commit = repo.index.commit('A') + + with open(file_path, 'a') as f: + f.write('B\n') + repo.index.add(['tracked.txt']) + repo.index.commit('B') + + with open(file_path, 'a') as f: + f.write('staged-C\n') + repo.index.add(['tracked.txt']) + + with open(file_path, 'a') as f: + f.write('unstaged-D\n') + + _from_docs, _work_docs, diff_docs = get_pre_commit_modified_documents( + progress_bar=self._mock_progress_bar(), + progress_bar_section='prepare', + repo_path=temp_dir, + base_ref=first_commit.hexsha, + include_unstaged=False, + ) + + assert len(diff_docs) == 1 + assert diff_docs[0].content.count('+B') == 1 + assert diff_docs[0].content.count('+staged-C') == 1 + assert 'unstaged-D' not in diff_docs[0].content + # every changed line must be an addition, never a removal, for this pure-append history + assert '-A' not in diff_docs[0].content + assert '-B' not in diff_docs[0].content + def test_empty_repository_falls_back_to_empty_tree(self) -> None: - """A repository with zero commits should treat all current content as new, not error out.""" - with temporary_git_repository() as (temp_dir, _repo): + """A repository with zero commits should treat all staged content as new, not error out.""" + with temporary_git_repository() as (temp_dir, repo): file_path = os.path.join(temp_dir, 'new_file.txt') with open(file_path, 'w') as f: f.write('brand-new-content\n') + repo.index.add(['new_file.txt']) - from_docs, work_docs, diff_docs = get_local_diff_documents( + from_docs, work_docs, diff_docs = get_pre_commit_modified_documents( progress_bar=self._mock_progress_bar(), progress_bar_section='prepare', repo_path=temp_dir, - commit_rev='HEAD', ) assert from_docs == [] assert len(work_docs) == 1 assert work_docs[0].content == 'brand-new-content\n' assert len(diff_docs) == 1 - assert diff_docs[0].content == 'brand-new-content\n' + assert '+brand-new-content' in diff_docs[0].content From 0251e5bd266395ac01df7d04dd25c271e759083c Mon Sep 17 00:00:00 2001 From: Aaron Butler Date: Wed, 30 Sep 2026 13:40:05 -0400 Subject: [PATCH 7/8] CM-72985 Address PR review feedback on pre-commit diff collection Fixes 5 issues raised in review of #550: - Replace `git show :` 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 --- cycode/cli/apps/scan/commit_range_scanner.py | 4 ++ .../scan/pre_commit/pre_commit_command.py | 10 +++-- cycode/cli/exceptions/custom_exceptions.py | 19 ++++++++ cycode/cli/exceptions/handle_scan_errors.py | 10 +++++ .../files_collector/commit_range_documents.py | 43 ++++++++++++++++--- .../commands/scan/test_pre_commit_command.py | 15 ++++--- .../test_commit_range_documents.py | 4 +- 7 files changed, 85 insertions(+), 20 deletions(-) diff --git a/cycode/cli/apps/scan/commit_range_scanner.py b/cycode/cli/apps/scan/commit_range_scanner.py index 686bd898..f1f52318 100644 --- a/cycode/cli/apps/scan/commit_range_scanner.py +++ b/cycode/cli/apps/scan/commit_range_scanner.py @@ -369,6 +369,9 @@ def _scan_secret_pre_commit( include_unstaged: bool = False, paths: Optional[list[str]] = None, ) -> None: + # collect_file_contents=False: the secret scan only ever uses diff_documents below, so skip + # building from_ref_documents/working_copy_documents (a disk read per changed file it would + # otherwise discard immediately). _from_ref_documents, _working_copy_documents, diff_documents = get_pre_commit_modified_documents( progress_bar=ctx.obj['progress_bar'], progress_bar_section=ScanProgressBarSection.PREPARE_LOCAL_FILES, @@ -376,6 +379,7 @@ def _scan_secret_pre_commit( base_ref=base_ref, include_unstaged=include_unstaged, paths=paths, + collect_file_contents=False, ) diff_documents = excluder.exclude_irrelevant_documents_to_scan(consts.SECRET_SCAN_TYPE, diff_documents) diff --git a/cycode/cli/apps/scan/pre_commit/pre_commit_command.py b/cycode/cli/apps/scan/pre_commit/pre_commit_command.py index 95a9c94a..aa6e8c9a 100644 --- a/cycode/cli/apps/scan/pre_commit/pre_commit_command.py +++ b/cycode/cli/apps/scan/pre_commit/pre_commit_command.py @@ -6,6 +6,7 @@ from cycode.cli import consts from cycode.cli.apps.scan.commit_range_scanner import scan_pre_commit +from cycode.cli.exceptions.custom_exceptions import ScanPathOutsideRepositoryError, UnresolvedGitRefError from cycode.cli.exceptions.handle_scan_errors import handle_scan_exception from cycode.cli.logger import logger from cycode.cli.utils.git_proxy import git_proxy @@ -41,7 +42,7 @@ def _validate_base_ref(repo_path: str, base_ref: str) -> None: try: repo.commit(base_ref) except Exception as e: - raise typer.BadParameter(f'Could not resolve git ref: {base_ref!r}', param_hint='--base-ref') from e + raise UnresolvedGitRefError(base_ref) from e def pre_commit_command( @@ -78,8 +79,10 @@ def pre_commit_command( try: repo_path = _resolve_repo_root(os.getcwd()) _validate_base_ref(repo_path, base_ref) - except typer.BadParameter: - raise + str_paths = [str(path) for path in paths] if paths else None + for str_path in str_paths or []: + if os.path.commonpath([repo_path, str_path]) != repo_path: + raise ScanPathOutsideRepositoryError(str_path, repo_path) except Exception as e: handle_scan_exception(ctx, e) return @@ -88,7 +91,6 @@ def pre_commit_command( progress_bar.start() try: - str_paths = [str(path) for path in paths] if paths else None scan_pre_commit(ctx, repo_path, base_ref=base_ref, include_unstaged=include_unstaged, paths=str_paths) except Exception as e: handle_scan_exception(ctx, e) diff --git a/cycode/cli/exceptions/custom_exceptions.py b/cycode/cli/exceptions/custom_exceptions.py index a9a1505f..7aa10c19 100644 --- a/cycode/cli/exceptions/custom_exceptions.py +++ b/cycode/cli/exceptions/custom_exceptions.py @@ -98,6 +98,25 @@ def __str__(self) -> str: return f'Error occurred while parsing terraform plan file. Path: {self.file_path}' +class ScanPathOutsideRepositoryError(CycodeError): + def __init__(self, path: str, repo_path: str) -> None: + self.path = path + self.repo_path = repo_path + super().__init__() + + def __str__(self) -> str: + return f'The path {self.path!r} is outside the repository {self.repo_path!r}' + + +class UnresolvedGitRefError(CycodeError): + def __init__(self, ref: str) -> None: + self.ref = ref + super().__init__() + + def __str__(self) -> str: + return f'Could not resolve git ref: {self.ref!r}' + + _SSL_ERROR_CA_BUNDLE_HINT = ( 'set the REQUESTS_CA_BUNDLE (or CURL_CA_BUNDLE) environment variable to the path of a valid .pem or similar' ) diff --git a/cycode/cli/exceptions/handle_scan_errors.py b/cycode/cli/exceptions/handle_scan_errors.py index 0b1e975b..05df95d7 100644 --- a/cycode/cli/exceptions/handle_scan_errors.py +++ b/cycode/cli/exceptions/handle_scan_errors.py @@ -54,6 +54,16 @@ def handle_scan_exception(ctx: typer.Context, err: Exception, *, return_exceptio message='The path you supplied does not correlate to a Git repository. ' 'If you still wish to scan this path, use: `cycode scan path `', ), + custom_exceptions.ScanPathOutsideRepositoryError: CliError( + soft_fail=False, + code='invalid_scan_path_error', + message=f'\n{err!s}\n--path must point to a location inside the scanned repository', + ), + custom_exceptions.UnresolvedGitRefError: CliError( + soft_fail=False, + code='invalid_git_ref_error', + message=f'\n{err!s}\nPass a commit, branch, or tag that exists in this repository', + ), } return handle_errors(ctx, err, errors, return_exception=return_exception) diff --git a/cycode/cli/files_collector/commit_range_documents.py b/cycode/cli/files_collector/commit_range_documents.py index 488567d8..16ad3fb4 100644 --- a/cycode/cli/files_collector/commit_range_documents.py +++ b/cycode/cli/files_collector/commit_range_documents.py @@ -15,7 +15,7 @@ from cycode.logger import get_logger if TYPE_CHECKING: - from git import Diff, DiffIndex, Repo + from git import Blob, Diff, DiffIndex, Repo from cycode.cli.utils.progress_bar import BaseProgressBar, ProgressBarSection @@ -180,6 +180,20 @@ def _get_file_content_from_commit_diff(repo: 'Repo', commit: str, diff: 'Diff') return get_file_content_from_commit_path(repo, commit, file_path) +def _get_blob_content(blob: Optional['Blob']) -> Optional[str]: + """Read a diff-side blob's content in-process, with no subprocess spawn. + + `diff.a_blob`/`diff.b_blob` already hold whatever git loaded to compute the patch, so this is + the same content `git show :` would return -- for every `Diffable.diff()` variant + (`index.diff(HEAD, R=True)`, `commit.diff()`, `commit.diff(None)`), `a_blob` is exactly the + "before" side, and it's `None` precisely when the file didn't exist there (e.g. diffed against + the empty tree for a brand-new repo/file) -- verified against a scratch repo for each variant. + """ + if blob is None: + return None + return blob.data_stream.read().decode('UTF-8', errors='replace') + + def get_commit_range_modified_documents( progress_bar: 'BaseProgressBar', progress_bar_section: 'ProgressBarSection', @@ -426,6 +440,7 @@ def get_pre_commit_modified_documents( base_ref: str = consts.GIT_HEAD_COMMIT_REV, include_unstaged: bool = False, paths: Optional[list[str]] = None, + collect_file_contents: bool = True, ) -> tuple[list[Document], list[Document], list[Document]]: """Diffs `base_ref` against the staged index (default) or the full working tree. @@ -435,12 +450,18 @@ def get_pre_commit_modified_documents( include_unstaged: If False (default), diffs the staged index only -- this is the exact pre-commit hook flow. If True, diffs the full working tree (staged + unstaged). paths: Optional pathspec to scope the diff to. + collect_file_contents: If False, skip building `from_ref_documents`/`working_copy_documents` + entirely -- only `diff_documents` (the patch itself) is produced. The secret scan path + only ever uses `diff_documents`, so this avoids a disk read per changed file (and, + before the `a_blob` fix below, a subprocess spawn per file) that it would otherwise + discard immediately. Defaults to True so existing callers (SCA/SAST) are unaffected. Returns: (from_ref_documents, working_copy_documents, diff_documents). `from_ref_documents` holds - each changed file's content at `base_ref` (skipped if it didn't exist there); + each changed file's content at `base_ref` (skipped if it didn't exist there, or was empty); `working_copy_documents` holds each changed file's current on-disk content (skipped if - the file no longer exists); `diff_documents` holds the unified diff per changed file. + the file no longer exists, or is empty); `diff_documents` holds the unified diff per + changed file. """ from_ref_documents = [] working_copy_documents = [] @@ -453,7 +474,7 @@ def get_pre_commit_modified_documents( # HEAD (or the empty tree in a brand-new repo), via get_staged_diff_index()'s proven # R-flag handling. Keeping this as its own branch (rather than folding it into the # `diff_target.diff(...)` calls below) means this default, most-used path can't regress. - resolved_ref, diff_index = get_staged_diff_index(repo, paths=paths) + _, diff_index = get_staged_diff_index(repo, paths=paths) else: try: diff_target = repo.commit(base_ref) @@ -466,7 +487,6 @@ def get_pre_commit_modified_documents( ) diff_target = repo.tree(consts.GIT_EMPTY_TREE_OBJECT) - resolved_ref = base_ref if include_unstaged: diff_index = diff_target.diff(None, create_patch=True, paths=paths or None) else: @@ -490,8 +510,17 @@ def get_pre_commit_modified_documents( ) ) - file_content = _get_file_content_from_commit_diff(repo, resolved_ref, diff) - if file_content is not None: + if not collect_file_contents: + continue + + # `a_blob` is exactly the content at `base_ref` -- see `_get_blob_content`'s docstring. + # Deliberately `if file_content:` (skip empty string) rather than `is not None`: this + # matches the working_copy branch immediately below, treating an empty file the same as + # a nonexistent one. `get_commit_range_modified_documents` uses `is not None` instead, but + # for a different reason (there, `None` only ever means "no such commit"); don't "fix" + # this one to match it. + file_content = _get_blob_content(diff.a_blob) + if file_content: from_ref_documents.append(Document(file_path, file_content)) if os.path.exists(file_path): diff --git a/tests/cli/commands/scan/test_pre_commit_command.py b/tests/cli/commands/scan/test_pre_commit_command.py index 5c3f6263..57ba5831 100644 --- a/tests/cli/commands/scan/test_pre_commit_command.py +++ b/tests/cli/commands/scan/test_pre_commit_command.py @@ -14,6 +14,7 @@ _validate_base_ref, pre_commit_command, ) +from cycode.cli.exceptions.custom_exceptions import UnresolvedGitRefError @contextmanager @@ -37,7 +38,7 @@ def test_valid_base_ref_does_not_raise(self) -> None: _validate_base_ref(temp_dir, 'HEAD') - def test_invalid_base_ref_raises_bad_parameter(self) -> None: + def test_invalid_base_ref_raises_unresolved_git_ref_error(self) -> None: with temporary_git_repository() as (temp_dir, repo): file_path = os.path.join(temp_dir, 'file.txt') with open(file_path, 'w') as f: @@ -45,7 +46,7 @@ def test_invalid_base_ref_raises_bad_parameter(self) -> None: repo.index.add(['file.txt']) repo.index.commit('initial') - with pytest.raises(typer.BadParameter): + with pytest.raises(UnresolvedGitRefError): _validate_base_ref(temp_dir, 'not-a-real-ref') def test_empty_repository_with_default_head_does_not_raise(self) -> None: @@ -92,11 +93,11 @@ def test_resolves_root_when_already_at_root(self) -> None: class TestPreCommitCommandPathResolution: """A relative --path value must reach scan_pre_commit already resolved to absolute. - Regression test: get_pre_commit_modified_documents compares an always-absolute path - (derived from the repo's working tree) against the raw `paths` strings. Without - `resolve_path=True` on the CLI option, a relative path (the natural way to invoke this, - e.g. `--path sub/app.py` from the repo root) would never match, silently dropping files - from the scoped scan. + Regression test: `paths` is handed to git as a pathspec, and git resolves relative + pathspecs against the repo root -- not the process cwd. Invoking from a subdirectory + (`--path app.py` from inside `sub/`) would therefore target `/app.py` instead + of `/sub/app.py`, silently scanning the wrong file or nothing at all. + `resolve_path=True` pins the path to the caller's cwd before git ever sees it. """ def test_relative_path_option_is_resolved_to_absolute(self, monkeypatch: pytest.MonkeyPatch) -> None: diff --git a/tests/cli/files_collector/test_commit_range_documents.py b/tests/cli/files_collector/test_commit_range_documents.py index ccd0e202..4a74f82e 100644 --- a/tests/cli/files_collector/test_commit_range_documents.py +++ b/tests/cli/files_collector/test_commit_range_documents.py @@ -1248,7 +1248,7 @@ def test_default_staged_only_ignores_unstaged_changes(self) -> None: ) assert len(from_docs) == 1 - assert from_docs[0].content == 'line1' + assert from_docs[0].content == 'line1\n' assert len(work_docs) == 1 assert work_docs[0].content == 'line1\nstaged\nunstaged\n' @@ -1285,7 +1285,7 @@ def test_include_unstaged_combines_staged_and_unstaged_changes(self) -> None: ) assert len(from_docs) == 1 - assert from_docs[0].content == 'line1' + assert from_docs[0].content == 'line1\n' assert len(work_docs) == 1 assert work_docs[0].content == 'line1\nstaged\nunstaged\n' From 1d8d2a9421c40214448c7318abc292dda9448495 Mon Sep 17 00:00:00 2001 From: Aaron Butler Date: Wed, 30 Sep 2026 15:18:22 -0400 Subject: [PATCH 8/8] CM-72985 Fix Windows CI failure: realpath the --path containment check 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 --- .../scan/pre_commit/pre_commit_command.py | 6 +- .../commands/scan/test_pre_commit_command.py | 61 +++++++++++++++++++ 2 files changed, 66 insertions(+), 1 deletion(-) diff --git a/cycode/cli/apps/scan/pre_commit/pre_commit_command.py b/cycode/cli/apps/scan/pre_commit/pre_commit_command.py index aa6e8c9a..beacf734 100644 --- a/cycode/cli/apps/scan/pre_commit/pre_commit_command.py +++ b/cycode/cli/apps/scan/pre_commit/pre_commit_command.py @@ -80,8 +80,12 @@ def pre_commit_command( repo_path = _resolve_repo_root(os.getcwd()) _validate_base_ref(repo_path, base_ref) str_paths = [str(path) for path in paths] if paths else None + # realpath both sides: repo_path (from GitPython) and str_path (from Click's + # resolve_path=True) can disagree on 8.3 short-name vs long-name form on Windows, + # which would otherwise make an in-repo path look like it's outside the repo. + repo_path_real = os.path.realpath(repo_path) for str_path in str_paths or []: - if os.path.commonpath([repo_path, str_path]) != repo_path: + if os.path.commonpath([repo_path_real, os.path.realpath(str_path)]) != repo_path_real: raise ScanPathOutsideRepositoryError(str_path, repo_path) except Exception as e: handle_scan_exception(ctx, e) diff --git a/tests/cli/commands/scan/test_pre_commit_command.py b/tests/cli/commands/scan/test_pre_commit_command.py index 57ba5831..d62159a1 100644 --- a/tests/cli/commands/scan/test_pre_commit_command.py +++ b/tests/cli/commands/scan/test_pre_commit_command.py @@ -135,6 +135,67 @@ def test_relative_path_option_is_resolved_to_absolute(self, monkeypatch: pytest. ] +class TestPreCommitCommandPathContainment: + """--path must be validated against the repo root using normalized (realpath) forms. + + Regression test: CI on windows-latest surfaced a false-positive rejection here. 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 -- an in-repo path then looks + like it's outside the repo under a naive string comparison. Reproduced here in a way that + doesn't depend on Windows-only short names: repo_path is returned with an unresolved `..` + segment, which only a realpath-normalized comparison sees through. + """ + + def test_unresolved_repo_root_form_does_not_reject_an_in_repo_path(self, monkeypatch: pytest.MonkeyPatch) -> None: + original_cwd = os.getcwd() + with temporary_git_repository() as (temp_dir, repo): + os.makedirs(os.path.join(temp_dir, 'sub')) + file_path = os.path.join(temp_dir, 'sub', 'app.py') + with open(file_path, 'w') as f: + f.write('content') + repo.index.add(['sub/app.py']) + repo.index.commit('initial') + + app = typer.Typer() + app.command()(pre_commit_command) + + monkeypatch.chdir(temp_dir) + unresolved_repo_path = os.path.join(temp_dir, 'sub', '..') + with ( + patch( + 'cycode.cli.apps.scan.pre_commit.pre_commit_command._resolve_repo_root', + return_value=unresolved_repo_path, + ), + patch('cycode.cli.apps.scan.pre_commit.pre_commit_command.scan_pre_commit') as mock_scan, + ): + result = CliRunner().invoke(app, ['--path', 'sub/app.py'], obj=MagicMock()) + monkeypatch.chdir(original_cwd) + + assert result.exit_code == 0, result.output + mock_scan.assert_called_once() + + def test_path_outside_repo_raises_scan_path_outside_repository_error(self, monkeypatch: pytest.MonkeyPatch) -> None: + original_cwd = os.getcwd() + with temporary_git_repository() as (temp_dir, repo): + file_path = os.path.join(temp_dir, 'file.txt') + with open(file_path, 'w') as f: + f.write('content') + repo.index.add(['file.txt']) + repo.index.commit('initial') + + with tempfile.TemporaryDirectory() as outside_dir: + app = typer.Typer() + app.command()(pre_commit_command) + + monkeypatch.chdir(temp_dir) + with patch('cycode.cli.apps.scan.pre_commit.pre_commit_command.scan_pre_commit') as mock_scan: + result = CliRunner().invoke(app, ['--path', os.path.join(outside_dir, 'file.txt')], obj=MagicMock()) + monkeypatch.chdir(original_cwd) + + assert result.exit_code == 0, result.output + mock_scan.assert_not_called() + + class TestPreCommitCommandFromSubdirectory: """End-to-end: invoking the command from a repo subdirectory must not fail."""