From 005edbb67de6b0020ce074e864c456bbe9945d2e Mon Sep 17 00:00:00 2001 From: David Larsen Date: Mon, 10 Aug 2026 14:10:45 -0700 Subject: [PATCH 1/7] fix: mark the scan workspace as a git safe.directory for git subprocesses The pre-built Docker action runs as root while the checkout at GITHUB_WORKSPACE is owned by the runner user, so git's ownership check (git 2.35.2+) refused the repository. changed_files diff-only mode then resolved to zero files on every PR and the scanners silently skipped with a green run. Git-based repository/branch/commit and default-branch discovery failed the same way in local Docker runs. actions/checkout's own safe.directory entry cannot help: it lands in the runner's global config, which is not mounted into container actions. Inject safe.directory for the scan workspace into the environment of each git subprocess via command-scope GIT_CONFIG_* entries. No config files are touched, and caller-provided GIT_CONFIG_* entries (including the previously documented env-block workaround) are appended after, not clobbered. Tested: - unit: new TestDubiousOwnership tests drive the real git ownership check via GIT_TEST_ASSUME_DIFFERENT_OWNER; they fail on the unpatched code and pass with the fix. TestGitEnv covers append-after-caller, garbage GIT_CONFIG_COUNT, and the GITHUB_WORKSPACE default. Full suite: 222 passed. - container: on the published 3.0.0 and 2.2.1 images with a uid-1001 checkout and a root process, unpatched runs skip with zero targets; patched runs resolve the PR diff and report the seeded finding, with and without a pre-existing user GIT_CONFIG_* block. Same-owner and delete-only-PR behavior unchanged. --- CHANGELOG.md | 12 ++++ socket_basics/core/config.py | 68 +++++++++++++++++----- tests/test_changed_files_scope.py | 94 ++++++++++++++++++++++++++++++- 3 files changed, 159 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 525d50d..186a087 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,18 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## [Unreleased] +### Fixed +- `changed_files` diff-only mode always resolved to zero files in the pre-built + Docker GitHub Action: the container runs as root while the checkout is owned + by the runner user, so git's ownership check refused every diff lookup, the + scope silently resolved to nothing, and the scanners skipped with a green + run. Git subprocesses now mark the scan workspace as `safe.directory` via + command-scope `GIT_CONFIG_*` environment entries. No config files are + touched, and caller-provided `GIT_CONFIG_*` entries (including the previously + documented workaround) are preserved. The same mismatch broke git-based + repository/branch/commit and default-branch discovery in local Docker runs; + those lookups are covered by the same change. + ## [3.0.0] - 2026-08-06 Major release: Trivy-backed scanning returns, now built and published through diff --git a/socket_basics/core/config.py b/socket_basics/core/config.py index ac623b4..6682382 100644 --- a/socket_basics/core/config.py +++ b/socket_basics/core/config.py @@ -1639,6 +1639,36 @@ def create_config_from_args(args) -> Config: return Config(config_dict) +def _git_env(workspace_path: str | Path | None = None) -> Dict[str, str]: + """Environment for git subprocesses that marks the scan workspace safe to read. + + The pre-built GitHub Action runs as root inside a Docker container while the + checkout at ``GITHUB_WORKSPACE`` is owned by the runner user, so git's + ownership check (git 2.35.2+) refuses the repository and every git lookup + here fails. ``changed_files`` diff-only mode then resolves to zero files and + the scanners silently skip. ``actions/checkout`` cannot help: its + ``safe.directory`` entry is written to the runner's global config, which is + not mounted into container actions. + + The workspace is an explicit scan target, not an incidentally discovered + repository, so mark it safe for these subprocesses only. Injecting via + ``GIT_CONFIG_*`` (command-scope config, honored for ``safe.directory`` since + git 2.38; the bundled image ships newer) touches no config files, and + appending after any caller-provided ``GIT_CONFIG_*`` entries preserves + workarounds users already deployed. + """ + env = dict(os.environ) + try: + count = max(0, int(env.get('GIT_CONFIG_COUNT', '0') or '0')) + except ValueError: + count = 0 + ws = workspace_path or os.environ.get('GITHUB_WORKSPACE') or os.getcwd() + env[f'GIT_CONFIG_KEY_{count}'] = 'safe.directory' + env[f'GIT_CONFIG_VALUE_{count}'] = str(ws) + env['GIT_CONFIG_COUNT'] = str(count + 1) + return env + + def _detect_git_changed_files(workspace_path: str, mode: str = 'staged', commit: str | None = None, base_ref: str | None = None) -> List[str]: """Detect changed files in a git repository. @@ -1675,6 +1705,11 @@ def _detect_git_changed_files(workspace_path: str, mode: str = 'staged', commit: if not git_dir.exists(): return [] + # Mark the workspace safe for the git subprocesses below; without this + # every command fails under the container-action ownership mismatch and + # the diff silently resolves to nothing. + git_env = _git_env(ws) + # Change to workspace directory before running git commands # This ensures git runs in the correct repository context original_cwd = os.getcwd() @@ -1699,7 +1734,7 @@ def _diff_against_base(ref: str) -> Optional[List[str]]: try: out = check_output( ['git', 'diff', '--name-only', '--diff-filter=ACMR', f'{candidate}...HEAD'], - text=True, stderr=subprocess.DEVNULL, + text=True, stderr=subprocess.DEVNULL, env=git_env, ) return _split(out) except CalledProcessError: @@ -1713,21 +1748,21 @@ def _diff_against_base(ref: str) -> Optional[List[str]]: pr_files = _diff_against_base(base) if pr_files is not None: return pr_files - out = check_output(['git', 'diff', '--name-only', '--cached'], text=True, stderr=subprocess.DEVNULL) + out = check_output(['git', 'diff', '--name-only', '--cached'], text=True, stderr=subprocess.DEVNULL, env=git_env) return _split(out) elif mode == 'pr': base = base_ref or os.environ.get('GITHUB_BASE_REF', '') return _diff_against_base(base) or [] elif mode == 'staged': # staged but not yet committed - out = check_output(['git', 'diff', '--name-only', '--cached'], text=True, stderr=subprocess.DEVNULL) + out = check_output(['git', 'diff', '--name-only', '--cached'], text=True, stderr=subprocess.DEVNULL, env=git_env) return _split(out) elif mode == 'current-commit': # files that are part of HEAD commit - out = check_output(['git', 'diff-tree', '--no-commit-id', '--name-only', '-r', 'HEAD'], text=True, stderr=subprocess.DEVNULL) + out = check_output(['git', 'diff-tree', '--no-commit-id', '--name-only', '-r', 'HEAD'], text=True, stderr=subprocess.DEVNULL, env=git_env) return _split(out) elif mode == 'commit' and commit: - out = check_output(['git', 'diff-tree', '--no-commit-id', '--name-only', '-r', commit], text=True, stderr=subprocess.DEVNULL) + out = check_output(['git', 'diff-tree', '--no-commit-id', '--name-only', '-r', commit], text=True, stderr=subprocess.DEVNULL, env=git_env) return _split(out) else: return [] @@ -1934,9 +1969,10 @@ def _discover_repository(cli_repo: str | None, github_repository: str = '', gith # 4. Git information try: url = subprocess.check_output( - ['git', 'config', '--get', 'remote.origin.url'], - text=True, - stderr=subprocess.DEVNULL + ['git', 'config', '--get', 'remote.origin.url'], + text=True, + stderr=subprocess.DEVNULL, + env=_git_env() ).strip() if url.endswith('.git'): @@ -2003,9 +2039,10 @@ def _discover_branch(cli_branch: str | None, github_head_ref: str = '', github_r # 4. Git information try: branch = subprocess.check_output( - ['git', 'rev-parse', '--abbrev-ref', 'HEAD'], + ['git', 'rev-parse', '--abbrev-ref', 'HEAD'], text=True, - stderr=subprocess.DEVNULL + stderr=subprocess.DEVNULL, + env=_git_env() ).strip() if branch and branch != 'HEAD': @@ -2040,9 +2077,10 @@ def _discover_commit_hash() -> str: # 2. Git information try: commit = subprocess.check_output( - ['git', 'rev-parse', '--short', 'HEAD'], + ['git', 'rev-parse', '--short', 'HEAD'], text=True, - stderr=subprocess.DEVNULL + stderr=subprocess.DEVNULL, + env=_git_env() ).strip() if commit: @@ -2084,7 +2122,8 @@ def _discover_is_default_branch(current_branch: str, workspace_path: str = '') - ['git', 'symbolic-ref', 'refs/remotes/origin/HEAD'], text=True, stderr=subprocess.DEVNULL, - cwd=cwd + cwd=cwd, + env=_git_env(workspace_path) ).strip() # Extract branch name from refs/remotes/origin/branch-name @@ -2105,7 +2144,8 @@ def _discover_is_default_branch(current_branch: str, workspace_path: str = '') - ['git', 'ls-remote', '--symref', 'origin', 'HEAD'], text=True, stderr=subprocess.DEVNULL, - cwd=cwd + cwd=cwd, + env=_git_env(workspace_path) ).strip() # Parse the output: "ref: refs/heads/main\tHEAD" diff --git a/tests/test_changed_files_scope.py b/tests/test_changed_files_scope.py index 3eda06d..57102e3 100644 --- a/tests/test_changed_files_scope.py +++ b/tests/test_changed_files_scope.py @@ -11,7 +11,13 @@ import pytest -from socket_basics.core.config import Config, _detect_git_changed_files, create_config_from_args +from socket_basics.core.config import ( + Config, + _detect_git_changed_files, + _discover_repository, + _git_env, + create_config_from_args, +) def _make_config(workspace, **overrides): @@ -170,3 +176,89 @@ def test_delete_only_pr_config_creation_keeps_empty_scope(self, tmp_path, monkey assert cfg.get("changed_files") == [] assert cfg.get_scan_targets() == [] + + +def _git_refuses_repo(repo): + """True when git, told to assume a different owner, refuses to read `repo`. + + ``GIT_TEST_ASSUME_DIFFERENT_OWNER`` is git's own test knob for the + ownership check behind ``safe.directory``; it makes every repository look + like it belongs to another user, which is exactly what a checkout looks + like from inside the container action. Used as a control so these tests + skip (instead of passing vacuously) on a git build without the knob. + """ + probe = subprocess.run( + ["git", "-C", str(repo), "rev-parse", "--show-toplevel"], + capture_output=True, + text=True, + env={**os.environ, "GIT_TEST_ASSUME_DIFFERENT_OWNER": "1"}, + ) + return probe.returncode != 0 + + +class TestDubiousOwnership: + """Git subprocesses must survive the container-action ownership mismatch. + + The pre-built Docker action runs as root while the checkout is owned by + the runner user; without ``safe.directory`` git refuses the repo, the diff + resolves to zero files, and the scanners silently skip. + """ + + def test_pr_diff_survives_dubious_ownership(self, pr_repo, monkeypatch): + for i in range(3): + monkeypatch.delenv(f"GIT_CONFIG_KEY_{i}", raising=False) + monkeypatch.delenv(f"GIT_CONFIG_VALUE_{i}", raising=False) + monkeypatch.delenv("GIT_CONFIG_COUNT", raising=False) + + if not _git_refuses_repo(pr_repo): + pytest.skip("this git build does not honor GIT_TEST_ASSUME_DIFFERENT_OWNER") + + monkeypatch.setenv("GIT_TEST_ASSUME_DIFFERENT_OWNER", "1") + files = _detect_git_changed_files(str(pr_repo), mode="pr", base_ref="main") + assert sorted(files) == ["base.py", "feat.py"] + + def test_repo_discovery_survives_dubious_ownership(self, pr_repo, monkeypatch): + _git(pr_repo, "remote", "add", "origin", "https://github.com/acme/demo.git") + + if not _git_refuses_repo(pr_repo): + pytest.skip("this git build does not honor GIT_TEST_ASSUME_DIFFERENT_OWNER") + + monkeypatch.chdir(pr_repo) + monkeypatch.setenv("GIT_TEST_ASSUME_DIFFERENT_OWNER", "1") + assert _discover_repository(None, "", "") == "acme/demo" + + +class TestGitEnv: + """_git_env injects safe.directory without clobbering caller config.""" + + def test_injects_safe_directory_for_workspace(self, monkeypatch): + monkeypatch.delenv("GIT_CONFIG_COUNT", raising=False) + env = _git_env("/scan/me") + assert env["GIT_CONFIG_COUNT"] == "1" + assert env["GIT_CONFIG_KEY_0"] == "safe.directory" + assert env["GIT_CONFIG_VALUE_0"] == "/scan/me" + + def test_appends_after_caller_provided_entries(self, monkeypatch): + # A user already deploying the documented env-var workaround must not + # have their entry clobbered. + monkeypatch.setenv("GIT_CONFIG_COUNT", "1") + monkeypatch.setenv("GIT_CONFIG_KEY_0", "user.name") + monkeypatch.setenv("GIT_CONFIG_VALUE_0", "runner") + env = _git_env("/scan/me") + assert env["GIT_CONFIG_COUNT"] == "2" + assert env["GIT_CONFIG_KEY_0"] == "user.name" + assert env["GIT_CONFIG_VALUE_0"] == "runner" + assert env["GIT_CONFIG_KEY_1"] == "safe.directory" + assert env["GIT_CONFIG_VALUE_1"] == "/scan/me" + + def test_garbage_count_treated_as_zero(self, monkeypatch): + monkeypatch.setenv("GIT_CONFIG_COUNT", "not-a-number") + env = _git_env("/scan/me") + assert env["GIT_CONFIG_COUNT"] == "1" + assert env["GIT_CONFIG_KEY_0"] == "safe.directory" + + def test_defaults_to_github_workspace(self, monkeypatch): + monkeypatch.delenv("GIT_CONFIG_COUNT", raising=False) + monkeypatch.setenv("GITHUB_WORKSPACE", "/github/workspace") + env = _git_env() + assert env["GIT_CONFIG_VALUE_0"] == "/github/workspace" From 5dc95729cdc81faf93a0524825be23a9fd1d9b9a Mon Sep 17 00:00:00 2001 From: David Larsen Date: Mon, 10 Aug 2026 15:27:38 -0700 Subject: [PATCH 2/7] fix: resolve the safe.directory workspace path to an absolute path Git ignores relative safe.directory values, so a relative --workspace under an ownership mismatch still failed the check and the diff kept resolving to empty. Resolve the path before writing the entry. Regression tests: a relative --workspace now passes the end-to-end ownership test, and TestGitEnv asserts the injected value is absolute. Both fail without this change. Full suite: 224 passed. Re-ran the container check on the published 3.0.0 image: unchanged, finding still reported. --- socket_basics/core/config.py | 5 +++-- tests/test_changed_files_scope.py | 18 ++++++++++++++++++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/socket_basics/core/config.py b/socket_basics/core/config.py index 6682382..043e06f 100644 --- a/socket_basics/core/config.py +++ b/socket_basics/core/config.py @@ -1655,7 +1655,8 @@ def _git_env(workspace_path: str | Path | None = None) -> Dict[str, str]: ``GIT_CONFIG_*`` (command-scope config, honored for ``safe.directory`` since git 2.38; the bundled image ships newer) touches no config files, and appending after any caller-provided ``GIT_CONFIG_*`` entries preserves - workarounds users already deployed. + workarounds users already deployed. The path is resolved to an absolute one + first because git ignores relative ``safe.directory`` values. """ env = dict(os.environ) try: @@ -1664,7 +1665,7 @@ def _git_env(workspace_path: str | Path | None = None) -> Dict[str, str]: count = 0 ws = workspace_path or os.environ.get('GITHUB_WORKSPACE') or os.getcwd() env[f'GIT_CONFIG_KEY_{count}'] = 'safe.directory' - env[f'GIT_CONFIG_VALUE_{count}'] = str(ws) + env[f'GIT_CONFIG_VALUE_{count}'] = str(Path(ws).resolve()) env['GIT_CONFIG_COUNT'] = str(count + 1) return env diff --git a/tests/test_changed_files_scope.py b/tests/test_changed_files_scope.py index 57102e3..0c4d400 100644 --- a/tests/test_changed_files_scope.py +++ b/tests/test_changed_files_scope.py @@ -7,6 +7,7 @@ import os import subprocess +from pathlib import Path from argparse import Namespace import pytest @@ -217,6 +218,17 @@ def test_pr_diff_survives_dubious_ownership(self, pr_repo, monkeypatch): files = _detect_git_changed_files(str(pr_repo), mode="pr", base_ref="main") assert sorted(files) == ["base.py", "feat.py"] + def test_relative_workspace_survives_dubious_ownership(self, pr_repo, monkeypatch): + # git ignores relative safe.directory values, so the injected path must + # be absolutized even when --workspace is given as a relative path. + if not _git_refuses_repo(pr_repo): + pytest.skip("this git build does not honor GIT_TEST_ASSUME_DIFFERENT_OWNER") + + monkeypatch.chdir(pr_repo.parent) + monkeypatch.setenv("GIT_TEST_ASSUME_DIFFERENT_OWNER", "1") + files = _detect_git_changed_files(pr_repo.name, mode="pr", base_ref="main") + assert sorted(files) == ["base.py", "feat.py"] + def test_repo_discovery_survives_dubious_ownership(self, pr_repo, monkeypatch): _git(pr_repo, "remote", "add", "origin", "https://github.com/acme/demo.git") @@ -257,6 +269,12 @@ def test_garbage_count_treated_as_zero(self, monkeypatch): assert env["GIT_CONFIG_COUNT"] == "1" assert env["GIT_CONFIG_KEY_0"] == "safe.directory" + def test_workspace_value_is_absolute(self, monkeypatch, tmp_path): + monkeypatch.delenv("GIT_CONFIG_COUNT", raising=False) + monkeypatch.chdir(tmp_path) + env = _git_env("some/relative/dir") + assert Path(env["GIT_CONFIG_VALUE_0"]).is_absolute() + def test_defaults_to_github_workspace(self, monkeypatch): monkeypatch.delenv("GIT_CONFIG_COUNT", raising=False) monkeypatch.setenv("GITHUB_WORKSPACE", "/github/workspace") From 3e836bd902e9ac801a46e59b1483641e7097459b Mon Sep 17 00:00:00 2001 From: lelia <2418071+lelia@users.noreply.github.com> Date: Mon, 10 Aug 2026 16:06:38 -0700 Subject: [PATCH 3/7] fix: distinguish failed changed_files resolution from an empty diff A git failure during scope resolution previously collapsed into the same empty list as a genuinely empty diff, so any future breakage (beyond the safe.directory fix) would again skip every scanner and report green. - _detect_git_changed_files now captures git stderr (instead of DEVNULL), logs the failure reason, and returns None on failure vs [] for a truly empty diff. Ref-not-found is classified separately so the base-ref candidate loop still falls through, while unreadable-repo errors fail fast. - On failed resolution the config layer falls back to a full-repo scan with a prominent warning, never a silent zero-file skip. Delete-only diffs keep the empty-scope skip (existing test still guards this). - An unresolvable base ref in a PR context (e.g. shallow fetch) is now a failure rather than a quiet fall-through to the usually-empty staged diff. - Resolved scope is logged: file count at INFO, full list at DEBUG (the customer ask from the report). - Connector-internal staged-diff callers get 'or []' for the new contract. Full suite: 231 passed. Signed-off-by: lelia <2418071+lelia@users.noreply.github.com> --- CHANGELOG.md | 13 ++ socket_basics/core/config.py | 175 ++++++++++++------ socket_basics/core/connector/trivy/trivy.py | 6 +- .../core/connector/trufflehog/__init__.py | 2 +- tests/test_changed_files_scope.py | 62 +++++++ 5 files changed, 200 insertions(+), 58 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 186a087..d473927 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,19 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). documented workaround) are preserved. The same mismatch broke git-based repository/branch/commit and default-branch discovery in local Docker runs; those lookups are covered by the same change. +- A failed `changed_files` diff resolution is no longer indistinguishable from + an empty diff. Git errors are captured and logged (instead of discarded), and + when the scope cannot be resolved — unreadable repository, unresolvable base + ref (e.g. a shallow fetch without the base), or `pr` mode with no base ref — + Socket Basics now **falls back to a full-repo scan with a prominent warning** + rather than skipping every scanner and reporting a green run that scanned + nothing. A genuinely empty diff (e.g. a delete-only PR) still keeps the empty + scope and skips as before. + +### Added +- The resolved `changed_files` scope is now logged on every scoped run: file + count at INFO, the full file list at DEBUG — so an empty diff and a failed + lookup are visible and distinguishable in run logs. ## [3.0.0] - 2026-08-06 diff --git a/socket_basics/core/config.py b/socket_basics/core/config.py index 043e06f..1d1da7b 100644 --- a/socket_basics/core/config.py +++ b/socket_basics/core/config.py @@ -1568,39 +1568,55 @@ def create_config_from_args(args) -> Config: if changed_files_arg: val = str(changed_files_arg).strip() config_dict['changed_files_scope_requested'] = True - # 'auto' resolves to the PR base-ref diff in CI, else staged changes. - if val.lower() == 'auto': + _scope_log = logging.getLogger(__name__) + + def _apply_scoped_changed_files(mode_label: str, **detect_kwargs) -> None: + """Resolve the diff scope, distinguishing failure from an empty diff. + + A failed resolution (None) falls back to a full-repo scan with a + prominent warning: a scoped scan that silently resolves to nothing + reports a green run while scanning zero files, which is the + fail-open failure mode this guards against. A genuinely empty diff + (e.g. a delete-only PR) keeps the empty scope and skips, as before. + """ try: - git_changed = _detect_git_changed_files(config_dict.get('workspace', os.getcwd()), mode='auto') - config_dict['changed_files'] = git_changed + resolved = _detect_git_changed_files(config_dict.get('workspace', os.getcwd()), **detect_kwargs) except Exception as e: - logging.getLogger(__name__).warning("Warning: failed to detect git changed files (auto): %s", e) + _scope_log.warning("Warning: failed to detect git changed files (%s): %s", mode_label, e) + resolved = None + if resolved is None: + _scope_log.warning( + "changed_files scope could not be resolved (%s); falling back to a " + "full-repo scan so nothing is silently skipped. See the warnings " + "above for the underlying git error.", + mode_label, + ) config_dict['changed_files'] = [] + config_dict['changed_files_scope_requested'] = False + return + _scope_log.info("changed_files scope resolved to %d file(s) (%s)", len(resolved), mode_label) + if resolved: + _scope_log.debug("changed_files scope: %s", ", ".join(resolved)) + else: + _scope_log.info( + "changed_files diff is genuinely empty (e.g. delete-only change); " + "scoped scanners will be skipped" + ) + config_dict['changed_files'] = resolved + + # 'auto' resolves to the PR base-ref diff in CI, else staged changes. + if val.lower() == 'auto': + _apply_scoped_changed_files('auto', mode='auto') elif val.lower() == 'pr': # Explicit PR diff against the base branch (GITHUB_BASE_REF). - try: - git_changed = _detect_git_changed_files(config_dict.get('workspace', os.getcwd()), mode='pr') - config_dict['changed_files'] = git_changed - except Exception as e: - logging.getLogger(__name__).warning("Warning: failed to detect git changed files (pr): %s", e) - config_dict['changed_files'] = [] + _apply_scoped_changed_files('pr', mode='pr') elif val.lower() in ('current-commit', 'current_commit'): - try: - git_changed = _detect_git_changed_files(config_dict.get('workspace', os.getcwd()), mode='current-commit') - config_dict['changed_files'] = git_changed - except Exception as e: - logging.getLogger(__name__).warning("Warning: failed to detect git changed files (current-commit): %s", e) - config_dict['changed_files'] = [] + _apply_scoped_changed_files('current-commit', mode='current-commit') else: # If value looks like a commit hash, list files in that commit import re if re.match(r'^[0-9a-fA-F]{7,40}$', val): - try: - git_changed = _detect_git_changed_files(config_dict.get('workspace', os.getcwd()), mode='commit', commit=val) - config_dict['changed_files'] = git_changed - except Exception as e: - logging.getLogger(__name__).warning("Warning: failed to detect git changed files (commit %s): %s", val, e) - config_dict['changed_files'] = [] + _apply_scoped_changed_files(f'commit {val}', mode='commit', commit=val) else: # parse comma-separated list of files provided manually config_dict['changed_files'] = [f.strip() for f in val.split(',') if f.strip()] @@ -1670,7 +1686,20 @@ def _git_env(workspace_path: str | Path | None = None) -> Dict[str, str]: return env -def _detect_git_changed_files(workspace_path: str, mode: str = 'staged', commit: str | None = None, base_ref: str | None = None) -> List[str]: +class _GitScopeError(Exception): + """A git invocation needed for changed-files scoping failed outright. + + Carries git's first stderr line as the message. ``ref_miss`` is True when + the failure only means "this ref does not exist" (safe to try another + candidate) rather than "git could not read the repository at all." + """ + + def __init__(self, message: str, ref_miss: bool = False): + super().__init__(message) + self.ref_miss = ref_miss + + +def _detect_git_changed_files(workspace_path: str, mode: str = 'staged', commit: str | None = None, base_ref: str | None = None) -> Optional[List[str]]: """Detect changed files in a git repository. mode: @@ -1684,11 +1713,16 @@ def _detect_git_changed_files(workspace_path: str, mode: str = 'staged', commit: (``GITHUB_BASE_REF`` is set), otherwise staged changes. This is what ``--changed-files auto`` resolves to. - Returns a list of file paths relative to the workspace root. If not a git - repo or detection fails, returns []. + Returns a list of file paths relative to the workspace root. An empty list + means git resolved the diff and it is genuinely empty (e.g. a delete-only + change), or the workspace is not a git repo (nothing to diff). Returns + ``None`` when resolution *failed* — git could not read the repository, or a + requested base ref could not be resolved — so callers can distinguish "no + changed files" from "the lookup broke" instead of silently scanning + nothing. The specific git error is logged here at WARNING level. """ + log = logging.getLogger(__name__) try: - from subprocess import check_output, CalledProcessError import subprocess # Prefer GITHUB_WORKSPACE if set (GitHub Actions environment) @@ -1711,35 +1745,56 @@ def _detect_git_changed_files(workspace_path: str, mode: str = 'staged', commit: # the diff silently resolves to nothing. git_env = _git_env(ws) + # stderr markers that mean "this ref does not exist" — a soft miss the + # base-ref candidate loop may retry — as opposed to git being unable + # to read the repository at all (ownership, corruption, ...). + ref_miss_markers = ('unknown revision', 'bad revision', 'ambiguous argument') + + def _split(out: str) -> List[str]: + return [line.strip() for line in out.splitlines() if line.strip()] + + def _run_git(args: List[str]) -> List[str]: + """Run git, returning stdout lines; raise _GitScopeError on failure. + + stderr is captured rather than discarded so the failure reason — + e.g. git's self-diagnosing ``dubious ownership`` message — survives + into the logs instead of being indistinguishable from an empty diff. + """ + res = subprocess.run(args, text=True, capture_output=True, env=git_env) + if res.returncode != 0: + stderr = (res.stderr or '').strip() + first = stderr.splitlines()[0] if stderr else f'exit code {res.returncode}' + miss = any(m in stderr.lower() for m in ref_miss_markers) + raise _GitScopeError(first, ref_miss=miss) + return _split(res.stdout) + # Change to workspace directory before running git commands # This ensures git runs in the correct repository context original_cwd = os.getcwd() try: os.chdir(str(ws)) - def _split(out: str) -> List[str]: - return [line.strip() for line in out.splitlines() if line.strip()] - def _diff_against_base(ref: str) -> Optional[List[str]]: """Diff changed files (excluding deletions) against a base ref. Tries the remote-tracking ref (``origin/``) first, then the - bare ref. Returns None when neither ref can be resolved so the - caller can fall back to another detection strategy. The - ``--diff-filter=ACMR`` excludes deleted paths so they never - become scan targets. + bare ref. Returns None when neither candidate resolves (the ref + does not exist locally); raises _GitScopeError when git itself + cannot read the repository. The ``--diff-filter=ACMR`` excludes + deleted paths so they never become scan targets. """ if not ref: return None + last_miss = '' for candidate in (f'origin/{ref}', ref): try: - out = check_output( - ['git', 'diff', '--name-only', '--diff-filter=ACMR', f'{candidate}...HEAD'], - text=True, stderr=subprocess.DEVNULL, env=git_env, - ) - return _split(out) - except CalledProcessError: - continue + return _run_git(['git', 'diff', '--name-only', '--diff-filter=ACMR', f'{candidate}...HEAD']) + except _GitScopeError as e: + if e.ref_miss: + last_miss = str(e) + continue + raise + log.warning("changed_files scope: base ref %r could not be resolved (%s)", ref, last_miss or 'no candidates tried') return None if mode == 'auto': @@ -1749,32 +1804,44 @@ def _diff_against_base(ref: str) -> Optional[List[str]]: pr_files = _diff_against_base(base) if pr_files is not None: return pr_files - out = check_output(['git', 'diff', '--name-only', '--cached'], text=True, stderr=subprocess.DEVNULL, env=git_env) - return _split(out) + if base: + # A base ref was provided (we are in a PR context) but could + # not be resolved (e.g. shallow fetch without the base). The + # staged-diff fallback would almost always be empty in CI — + # silently scanning nothing — so report failure instead. + return None + return _run_git(['git', 'diff', '--name-only', '--cached']) elif mode == 'pr': base = base_ref or os.environ.get('GITHUB_BASE_REF', '') - return _diff_against_base(base) or [] + if not base: + log.warning("changed_files scope: mode 'pr' but no base ref available (GITHUB_BASE_REF unset)") + return None + return _diff_against_base(base) elif mode == 'staged': # staged but not yet committed - out = check_output(['git', 'diff', '--name-only', '--cached'], text=True, stderr=subprocess.DEVNULL, env=git_env) - return _split(out) + return _run_git(['git', 'diff', '--name-only', '--cached']) elif mode == 'current-commit': # files that are part of HEAD commit - out = check_output(['git', 'diff-tree', '--no-commit-id', '--name-only', '-r', 'HEAD'], text=True, stderr=subprocess.DEVNULL, env=git_env) - return _split(out) + return _run_git(['git', 'diff-tree', '--no-commit-id', '--name-only', '-r', 'HEAD']) elif mode == 'commit' and commit: - out = check_output(['git', 'diff-tree', '--no-commit-id', '--name-only', '-r', commit], text=True, stderr=subprocess.DEVNULL, env=git_env) - return _split(out) + return _run_git(['git', 'diff-tree', '--no-commit-id', '--name-only', '-r', commit]) else: return [] finally: # Always restore original working directory os.chdir(original_cwd) - except CalledProcessError: - return [] - except Exception: - return [] + except _GitScopeError as e: + msg = str(e) + hint = '' + if 'dubious ownership' in msg.lower(): + hint = (" — the checkout is owned by a different user; the workspace should be" + " marked safe.directory automatically as of this release, so please report this") + log.warning("changed_files scope: git failed: %s%s", msg, hint) + return None + except Exception as e: + log.warning("changed_files scope: unexpected error during git detection: %s", e) + return None def discover_all_files(workspace_path: str, respect_gitignore: bool = True) -> List[str]: diff --git a/socket_basics/core/connector/trivy/trivy.py b/socket_basics/core/connector/trivy/trivy.py index c4f518b..bedbc3c 100644 --- a/socket_basics/core/connector/trivy/trivy.py +++ b/socket_basics/core/connector/trivy/trivy.py @@ -144,7 +144,7 @@ def scan_dockerfiles(self) -> Dict[str, Any]: if not changed_files: try: from socket_basics.core.config import _detect_git_changed_files - changed_files = _detect_git_changed_files(str(self.config.workspace), mode='staged') + changed_files = _detect_git_changed_files(str(self.config.workspace), mode='staged') or [] except Exception: changed_files = [] @@ -174,7 +174,7 @@ def scan_dockerfiles(self) -> Dict[str, Any]: try: # import helper from config module from socket_basics.core.config import _detect_git_changed_files - changed_files = _detect_git_changed_files(str(self.config.workspace), mode='staged') + changed_files = _detect_git_changed_files(str(self.config.workspace), mode='staged') or [] except Exception: changed_files = [] if changed_files: @@ -325,7 +325,7 @@ def scan_vulnerabilities(self) -> Dict[str, Any]: if not changed_files: try: from socket_basics.core.config import _detect_git_changed_files - changed_files = _detect_git_changed_files(str(self.config.workspace), mode='staged') + changed_files = _detect_git_changed_files(str(self.config.workspace), mode='staged') or [] except Exception: changed_files = [] diff --git a/socket_basics/core/connector/trufflehog/__init__.py b/socket_basics/core/connector/trufflehog/__init__.py index 43eef14..522643c 100644 --- a/socket_basics/core/connector/trufflehog/__init__.py +++ b/socket_basics/core/connector/trufflehog/__init__.py @@ -228,7 +228,7 @@ def scan(self) -> Dict[str, Any]: if not changed_files: try: from socket_basics.core.config import _detect_git_changed_files - changed_files = _detect_git_changed_files(str(self.config.workspace), mode='staged') + changed_files = _detect_git_changed_files(str(self.config.workspace), mode='staged') or [] except Exception: changed_files = [] diff --git a/tests/test_changed_files_scope.py b/tests/test_changed_files_scope.py index 0c4d400..8858325 100644 --- a/tests/test_changed_files_scope.py +++ b/tests/test_changed_files_scope.py @@ -280,3 +280,65 @@ def test_defaults_to_github_workspace(self, monkeypatch): monkeypatch.setenv("GITHUB_WORKSPACE", "/github/workspace") env = _git_env() assert env["GIT_CONFIG_VALUE_0"] == "/github/workspace" + + +class TestScopeResolutionFailure: + """Failed diff resolution must be distinguishable from an empty diff. + + A git failure (unreadable repo, unresolvable base ref) returns None and the + config layer falls back to a full-repo scan with a warning — never a green + run that silently scanned nothing. A genuinely empty diff still returns [] + and keeps the skip behavior (see the delete-only test above). + """ + + def test_unreadable_repo_returns_none(self, pr_repo): + # Corrupt HEAD so every git command fails hard (not a ref miss). + (pr_repo / ".git" / "HEAD").write_text("garbage") + result = _detect_git_changed_files(str(pr_repo), mode="pr", base_ref="main") + assert result is None + + def test_unresolvable_base_ref_returns_none(self, pr_repo): + result = _detect_git_changed_files(str(pr_repo), mode="pr", base_ref="no-such-branch") + assert result is None + + def test_auto_with_unresolvable_base_ref_returns_none(self, pr_repo, monkeypatch): + # In a PR context (base ref set) an unresolvable base must NOT quietly + # fall back to the (usually empty) staged diff. + monkeypatch.setenv("GITHUB_BASE_REF", "no-such-branch") + result = _detect_git_changed_files(str(pr_repo), mode="auto") + assert result is None + + def test_pr_mode_without_base_ref_returns_none(self, pr_repo, monkeypatch): + monkeypatch.delenv("GITHUB_BASE_REF", raising=False) + result = _detect_git_changed_files(str(pr_repo), mode="pr") + assert result is None + + def test_failure_logs_git_stderr(self, pr_repo, caplog): + (pr_repo / ".git" / "HEAD").write_text("garbage") + with caplog.at_level("WARNING"): + _detect_git_changed_files(str(pr_repo), mode="pr", base_ref="main") + assert any("changed_files scope" in r.getMessage() for r in caplog.records) + + def test_config_creation_falls_back_to_full_scan_on_failure(self, pr_repo, monkeypatch, caplog): + monkeypatch.delenv("GITHUB_WORKSPACE", raising=False) + monkeypatch.setenv("GITHUB_BASE_REF", "main") + (pr_repo / ".git" / "HEAD").write_text("garbage") + + with caplog.at_level("WARNING"): + cfg = create_config_from_args(_config_args(pr_repo, "auto")) + + # Scope resolution failed -> full-repo scan, not a silent skip. + assert cfg.get("changed_files") == [] + assert cfg.get("changed_files_scope_requested") is False + assert cfg.get_scan_targets() == [str(pr_repo)] + assert any("falling back to a full-repo scan" in r.getMessage() for r in caplog.records) + + def test_config_creation_logs_resolved_count(self, pr_repo, monkeypatch, caplog): + monkeypatch.delenv("GITHUB_WORKSPACE", raising=False) + monkeypatch.setenv("GITHUB_BASE_REF", "main") + + with caplog.at_level("INFO"): + cfg = create_config_from_args(_config_args(pr_repo, "auto")) + + assert sorted(cfg.get("changed_files")) == ["base.py", "feat.py"] + assert any("resolved to 2 file(s)" in r.getMessage() for r in caplog.records) From a204ae79f6aa6ccf927e6a6de2d7972e9a3261fd Mon Sep 17 00:00:00 2001 From: lelia <2418071+lelia@users.noreply.github.com> Date: Wed, 12 Aug 2026 13:19:11 -0700 Subject: [PATCH 4/7] docs: document the diff-resolution fallback tradeoffs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The full-scan fallback is deliberate fail-toward-scanning behavior, with two known consequences on large repos (surprise full scans can be slow/OOM; the scan reports pre-existing findings until the checkout misconfiguration — usually a missing fetch-depth: 0 — is corrected). Document both, the fix, and how to tell an empty diff from a failed lookup in the logs. Signed-off-by: lelia <2418071+lelia@users.noreply.github.com> --- docs/github-action.md | 22 ++++++++++++++++++++++ docs/parameters.md | 10 ++++++++++ 2 files changed, 32 insertions(+) diff --git a/docs/github-action.md b/docs/github-action.md index 93cc83f..5965c1c 100644 --- a/docs/github-action.md +++ b/docs/github-action.md @@ -324,6 +324,28 @@ jobs: > nothing rather than falling back to the whole repo. To scan an explicit file > list regardless of git state, use the `scan_files` input instead. +> [!NOTE] +> **When the diff cannot be resolved** — the checkout is unreadable, or the +> base branch is missing (most commonly a shallow clone without +> `fetch-depth: 0`) — Socket Basics logs a warning naming the underlying git +> error and **falls back to a full-repository scan** rather than silently +> scanning nothing. This is deliberate fail-toward-scanning behavior for a +> security gate, and it comes with two tradeoffs worth planning for: +> +> - On very large repositories an unexpected full scan can be slow or exhaust +> CI memory. If the fallback warning appears on **every** PR, the cause is +> almost always the missing `fetch-depth: 0` — fix the checkout rather than +> sizing up the runner. +> - The full scan reports **pre-existing** findings, not just the PR's change, +> so a repo-wide checkout misconfiguration shows up as large PR comments or +> failing checks on every PR until corrected. The run log's warning names +> the actual git error — read it before triaging the findings. +> +> A genuinely *empty* diff (e.g. a delete-only PR) still skips the scanners; +> the fallback triggers only when resolution **fails**. The resolved file +> count is logged on every scoped run, so an empty diff and a failed lookup +> are always distinguishable in the logs. + ## PR Comment Customization Socket Basics automatically posts enhanced PR comments with **smart defaults that work out of the box** — clickable file links, collapsible sections, syntax highlighting, CVE links, CVSS scores, and auto-labels are all enabled by default. diff --git a/docs/parameters.md b/docs/parameters.md index 22adfeb..8cd8cd6 100644 --- a/docs/parameters.md +++ b/docs/parameters.md @@ -120,6 +120,16 @@ PR), the scanners are skipped rather than falling back to scanning the whole repository. For PR/`auto`/`pr` modes, check out with full history (e.g. `actions/checkout` with `fetch-depth: 0`) so the base branch is available. +If the diff **cannot be resolved** — the base ref is missing (shallow clone), +or git cannot read the repository — a warning with the underlying git error is +logged and the scan **falls back to the whole workspace** instead of silently +scanning nothing. On large repositories, prefer fixing the root cause (usually +checkout depth) over relying on the fallback: a full scan of a multi-GB repo +can be slow or hit CI limits, and it reports pre-existing findings rather than +just the change. The resolved scope is logged on every run (file count at +INFO, full file list at DEBUG), so an empty diff and a failed lookup are +distinguishable in run logs. + **Example:** ```bash socket-basics --changed-files auto From b7c757a8c0bca8951fac96b8ce47b7f53170a1f8 Mon Sep 17 00:00:00 2001 From: lelia <2418071+lelia@users.noreply.github.com> Date: Wed, 12 Aug 2026 16:51:06 -0700 Subject: [PATCH 5/7] fix: fail fast on shallow checkouts that cannot resolve the base ref MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A shallow checkout (missing fetch-depth: 0) can never resolve the PR base ref, so the full-scan fallback would fire on every PR — slow or OOM-prone on large monorepos, and reporting pre-existing findings instead of the actual problem. That case is deterministic, so exit with a configuration error naming the one-line fix (matching the existing SystemExit convention for repository/branch discovery failures). Non-deterministic resolution failures keep the full-scan fallback. Proposed by @dc-larsen in review. Shallowness probed via git rev-parse --is-shallow-repository; tests fake it by touching .git/shallow. Full suite: 236 passed. Signed-off-by: lelia <2418071+lelia@users.noreply.github.com> --- CHANGELOG.md | 5 ++++- docs/github-action.md | 6 ++++++ docs/parameters.md | 25 ++++++++++++++-------- socket_basics/core/config.py | 34 ++++++++++++++++++++++++++---- tests/test_changed_files_scope.py | 35 +++++++++++++++++++++++++++++++ 5 files changed, 91 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d473927..fe5975f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,7 +26,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). Socket Basics now **falls back to a full-repo scan with a prominent warning** rather than skipping every scanner and reporting a green run that scanned nothing. A genuinely empty diff (e.g. a delete-only PR) still keeps the empty - scope and skips as before. + scope and skips as before. One deterministic case fails fast instead of + falling back: a **shallow checkout** that cannot resolve the base ref exits + with a configuration error naming the fix (`fetch-depth: 0`), since it would + otherwise full-scan every PR — slow or OOM-prone on large repositories. ### Added - The resolved `changed_files` scope is now logged on every scoped run: file diff --git a/docs/github-action.md b/docs/github-action.md index 5965c1c..d037f0d 100644 --- a/docs/github-action.md +++ b/docs/github-action.md @@ -341,6 +341,12 @@ jobs: > failing checks on every PR until corrected. The run log's warning names > the actual git error — read it before triaging the findings. > +> **Exception — shallow checkouts fail fast instead.** If the base branch is +> missing *because the checkout is shallow* (the classic missing +> `fetch-depth: 0`), the failure is deterministic — every PR would full-scan — +> so Socket Basics exits with a configuration error naming that one-line fix +> rather than falling back. +> > A genuinely *empty* diff (e.g. a delete-only PR) still skips the scanners; > the fallback triggers only when resolution **fails**. The resolved file > count is logged on every scoped run, so an empty diff and a failed lookup diff --git a/docs/parameters.md b/docs/parameters.md index 8cd8cd6..46b8eb8 100644 --- a/docs/parameters.md +++ b/docs/parameters.md @@ -120,15 +120,22 @@ PR), the scanners are skipped rather than falling back to scanning the whole repository. For PR/`auto`/`pr` modes, check out with full history (e.g. `actions/checkout` with `fetch-depth: 0`) so the base branch is available. -If the diff **cannot be resolved** — the base ref is missing (shallow clone), -or git cannot read the repository — a warning with the underlying git error is -logged and the scan **falls back to the whole workspace** instead of silently -scanning nothing. On large repositories, prefer fixing the root cause (usually -checkout depth) over relying on the fallback: a full scan of a multi-GB repo -can be slow or hit CI limits, and it reports pre-existing findings rather than -just the change. The resolved scope is logged on every run (file count at -INFO, full file list at DEBUG), so an empty diff and a failed lookup are -distinguishable in run logs. +If the diff **cannot be resolved**, behavior depends on why: + +- **Shallow checkout with a missing base ref** (the classic missing + `fetch-depth: 0`): deterministic misconfiguration — the run **fails fast + with a configuration error** naming the fix, instead of full-scanning every + PR. +- **Any other resolution failure** (unreadable repository, non-shallow missing + ref): a warning with the underlying git error is logged and the scan **falls + back to the whole workspace** instead of silently scanning nothing. On large + repositories a surprise full scan can be slow or hit CI limits and reports + pre-existing findings, so treat the warning as the signal and fix the root + cause. + +The resolved scope is logged on every run (file count at INFO, full file list +at DEBUG), so an empty diff and a failed lookup are distinguishable in run +logs. **Example:** ```bash diff --git a/socket_basics/core/config.py b/socket_basics/core/config.py index 1d1da7b..10048ef 100644 --- a/socket_basics/core/config.py +++ b/socket_basics/core/config.py @@ -1774,6 +1774,27 @@ def _run_git(args: List[str]) -> List[str]: try: os.chdir(str(ws)) + def _fail_fast_if_shallow(ref: str) -> None: + """Deterministic misconfiguration check for an unresolvable base. + + A shallow checkout (no ``fetch-depth: 0``) can *never* resolve + the base ref, so every PR would take the full-scan fallback — + on large repos that is a slow/OOM red check instead of a clear + signal. Fail fast with the one-line fix instead. Follows the + existing SystemExit convention for unrecoverable configuration + errors (see repository/branch discovery below). + """ + try: + shallow = _run_git(['git', 'rev-parse', '--is-shallow-repository']) == ['true'] + except Exception: + return # probe failed; fall through to the generic fallback + if shallow: + raise SystemExit( + f"changed_files: base ref '{ref}' could not be resolved and this checkout is " + "shallow. Set 'fetch-depth: 0' on actions/checkout (or otherwise fetch the " + "base branch) so the diff has a base to compare against." + ) + def _diff_against_base(ref: str) -> Optional[List[str]]: """Diff changed files (excluding deletions) against a base ref. @@ -1806,9 +1827,11 @@ def _diff_against_base(ref: str) -> Optional[List[str]]: return pr_files if base: # A base ref was provided (we are in a PR context) but could - # not be resolved (e.g. shallow fetch without the base). The - # staged-diff fallback would almost always be empty in CI — - # silently scanning nothing — so report failure instead. + # not be resolved. A shallow checkout makes this deterministic + # (config error, fail fast); otherwise report failure so the + # caller falls back to a full scan rather than the staged + # diff, which is almost always empty in CI. + _fail_fast_if_shallow(base) return None return _run_git(['git', 'diff', '--name-only', '--cached']) elif mode == 'pr': @@ -1816,7 +1839,10 @@ def _diff_against_base(ref: str) -> Optional[List[str]]: if not base: log.warning("changed_files scope: mode 'pr' but no base ref available (GITHUB_BASE_REF unset)") return None - return _diff_against_base(base) + pr_files = _diff_against_base(base) + if pr_files is None: + _fail_fast_if_shallow(base) + return pr_files elif mode == 'staged': # staged but not yet committed return _run_git(['git', 'diff', '--name-only', '--cached']) diff --git a/tests/test_changed_files_scope.py b/tests/test_changed_files_scope.py index 8858325..e4a5aba 100644 --- a/tests/test_changed_files_scope.py +++ b/tests/test_changed_files_scope.py @@ -342,3 +342,38 @@ def test_config_creation_logs_resolved_count(self, pr_repo, monkeypatch, caplog) assert sorted(cfg.get("changed_files")) == ["base.py", "feat.py"] assert any("resolved to 2 file(s)" in r.getMessage() for r in caplog.records) + + +class TestShallowCheckoutFailFast: + """A shallow checkout that cannot resolve the base ref is a deterministic + misconfiguration (missing fetch-depth: 0) — fail fast with the one-line fix + instead of full-scanning every PR (slow/OOM on large repos). Non-shallow + failures keep the full-scan fallback. + """ + + def test_shallow_missing_base_fails_fast_pr_mode(self, pr_repo): + (pr_repo / ".git" / "shallow").touch() + with pytest.raises(SystemExit, match="fetch-depth"): + _detect_git_changed_files(str(pr_repo), mode="pr", base_ref="no-such-branch") + + def test_shallow_missing_base_fails_fast_auto_mode(self, pr_repo, monkeypatch): + (pr_repo / ".git" / "shallow").touch() + monkeypatch.setenv("GITHUB_BASE_REF", "no-such-branch") + with pytest.raises(SystemExit, match="fetch-depth"): + _detect_git_changed_files(str(pr_repo), mode="auto") + + def test_shallow_with_resolvable_base_still_diffs(self, pr_repo): + # Shallowness alone is fine — only shallow AND unresolvable-base fails. + (pr_repo / ".git" / "shallow").touch() + files = _detect_git_changed_files(str(pr_repo), mode="pr", base_ref="main") + assert sorted(files) == ["base.py", "feat.py"] + + def test_non_shallow_missing_base_keeps_fallback(self, pr_repo): + assert _detect_git_changed_files(str(pr_repo), mode="pr", base_ref="no-such-branch") is None + + def test_config_creation_propagates_config_error(self, pr_repo, monkeypatch): + monkeypatch.delenv("GITHUB_WORKSPACE", raising=False) + monkeypatch.setenv("GITHUB_BASE_REF", "no-such-branch") + (pr_repo / ".git" / "shallow").touch() + with pytest.raises(SystemExit, match="fetch-depth"): + create_config_from_args(_config_args(pr_repo, "auto")) From d5103a6f86d3d18b17d6a11970f60e2118726a35 Mon Sep 17 00:00:00 2001 From: lelia <2418071+lelia@users.noreply.github.com> Date: Wed, 12 Aug 2026 17:21:02 -0700 Subject: [PATCH 6/7] fix: apply the shallow fail-fast to no-merge-base failures too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bugbot: the fail-fast only ran on the soft ref-miss path, but the common partial-fetch shape (base tip fetched, history disconnected) fails with 'A...HEAD: no merge base' — a hard error that skipped the shallow check and took the full-scan fallback on every PR. Classify no-merge-base distinctly and route both failure shapes through the shallow check; non-shallow no-merge-base keeps the fallback (with a warning). Docs broadened to cover both shapes. Full suite: 238 passed. Signed-off-by: lelia <2418071+lelia@users.noreply.github.com> --- docs/github-action.md | 11 ++++--- docs/parameters.md | 5 +-- socket_basics/core/config.py | 54 ++++++++++++++++++++++--------- tests/test_changed_files_scope.py | 16 +++++++++ 4 files changed, 64 insertions(+), 22 deletions(-) diff --git a/docs/github-action.md b/docs/github-action.md index d037f0d..9ca5f36 100644 --- a/docs/github-action.md +++ b/docs/github-action.md @@ -341,11 +341,12 @@ jobs: > failing checks on every PR until corrected. The run log's warning names > the actual git error — read it before triaging the findings. > -> **Exception — shallow checkouts fail fast instead.** If the base branch is -> missing *because the checkout is shallow* (the classic missing -> `fetch-depth: 0`), the failure is deterministic — every PR would full-scan — -> so Socket Basics exits with a configuration error naming that one-line fix -> rather than falling back. +> **Exception — shallow checkouts fail fast instead.** If the diff cannot be +> computed *because the checkout is shallow* — the base branch is missing, or +> its tip was fetched without connecting history (`no merge base`), both +> classically a missing `fetch-depth: 0` — the failure is deterministic: +> every PR would full-scan. Socket Basics exits with a configuration error +> naming that one-line fix rather than falling back. > > A genuinely *empty* diff (e.g. a delete-only PR) still skips the scanners; > the fallback triggers only when resolution **fails**. The resolved file diff --git a/docs/parameters.md b/docs/parameters.md index 46b8eb8..2a09852 100644 --- a/docs/parameters.md +++ b/docs/parameters.md @@ -122,8 +122,9 @@ repository. For PR/`auto`/`pr` modes, check out with full history (e.g. If the diff **cannot be resolved**, behavior depends on why: -- **Shallow checkout with a missing base ref** (the classic missing - `fetch-depth: 0`): deterministic misconfiguration — the run **fails fast +- **Shallow checkout that cannot diff the base** — missing base ref or + disconnected history (`no merge base`), classically a missing + `fetch-depth: 0`: deterministic misconfiguration — the run **fails fast with a configuration error** naming the fix, instead of full-scanning every PR. - **Any other resolution failure** (unreadable repository, non-shallow missing diff --git a/socket_basics/core/config.py b/socket_basics/core/config.py index 10048ef..40c3278 100644 --- a/socket_basics/core/config.py +++ b/socket_basics/core/config.py @@ -1692,11 +1692,15 @@ class _GitScopeError(Exception): Carries git's first stderr line as the message. ``ref_miss`` is True when the failure only means "this ref does not exist" (safe to try another candidate) rather than "git could not read the repository at all." + ``merge_base_miss`` is True when the ref exists but shares no history with + HEAD (``A...HEAD: no merge base``) — the signature of a partial/shallow + fetch where the base tip was fetched without connecting history. """ - def __init__(self, message: str, ref_miss: bool = False): + def __init__(self, message: str, ref_miss: bool = False, merge_base_miss: bool = False): super().__init__(message) self.ref_miss = ref_miss + self.merge_base_miss = merge_base_miss def _detect_git_changed_files(workspace_path: str, mode: str = 'staged', commit: str | None = None, base_ref: str | None = None) -> Optional[List[str]]: @@ -1765,7 +1769,8 @@ def _run_git(args: List[str]) -> List[str]: stderr = (res.stderr or '').strip() first = stderr.splitlines()[0] if stderr else f'exit code {res.returncode}' miss = any(m in stderr.lower() for m in ref_miss_markers) - raise _GitScopeError(first, ref_miss=miss) + mb_miss = 'no merge base' in stderr.lower() + raise _GitScopeError(first, ref_miss=miss, merge_base_miss=mb_miss) return _split(res.stdout) # Change to workspace directory before running git commands @@ -1790,11 +1795,34 @@ def _fail_fast_if_shallow(ref: str) -> None: return # probe failed; fall through to the generic fallback if shallow: raise SystemExit( - f"changed_files: base ref '{ref}' could not be resolved and this checkout is " - "shallow. Set 'fetch-depth: 0' on actions/checkout (or otherwise fetch the " - "base branch) so the diff has a base to compare against." + f"changed_files: cannot diff against base ref '{ref}' (missing ref or no " + "shared history) and this checkout is shallow. Set 'fetch-depth: 0' on " + "actions/checkout (or otherwise fetch the base branch with full history) " + "so the diff has a base to compare against." ) + def _resolve_base_diff(ref: str) -> Optional[List[str]]: + """Base diff with the shallow fail-fast applied to both failure shapes. + + A shallow misconfiguration shows up either as a missing base ref + (nothing fetched) or as ``no merge base`` (base tip fetched but + history disconnected). Both are deterministic — fail fast when + shallow; otherwise preserve the original failure semantics. + """ + if not ref: + return None + try: + files = _diff_against_base(ref) + except _GitScopeError as e: + if e.merge_base_miss: + _fail_fast_if_shallow(ref) + log.warning("changed_files scope: base ref %r shares no merge base with HEAD (%s)", ref, e) + return None + raise + if files is None: + _fail_fast_if_shallow(ref) + return files + def _diff_against_base(ref: str) -> Optional[List[str]]: """Diff changed files (excluding deletions) against a base ref. @@ -1822,16 +1850,15 @@ def _diff_against_base(ref: str) -> Optional[List[str]]: # Prefer the PR base-ref diff in CI; fall back to staged changes # for local/pre-commit use. base = base_ref or os.environ.get('GITHUB_BASE_REF', '') - pr_files = _diff_against_base(base) + pr_files = _resolve_base_diff(base) if pr_files is not None: return pr_files if base: # A base ref was provided (we are in a PR context) but could - # not be resolved. A shallow checkout makes this deterministic - # (config error, fail fast); otherwise report failure so the - # caller falls back to a full scan rather than the staged - # diff, which is almost always empty in CI. - _fail_fast_if_shallow(base) + # not be diffed against, and the shallow fail-fast did not + # apply (non-shallow checkout). Report failure so the caller + # falls back to a full scan rather than the staged diff, + # which is almost always empty in CI. return None return _run_git(['git', 'diff', '--name-only', '--cached']) elif mode == 'pr': @@ -1839,10 +1866,7 @@ def _diff_against_base(ref: str) -> Optional[List[str]]: if not base: log.warning("changed_files scope: mode 'pr' but no base ref available (GITHUB_BASE_REF unset)") return None - pr_files = _diff_against_base(base) - if pr_files is None: - _fail_fast_if_shallow(base) - return pr_files + return _resolve_base_diff(base) elif mode == 'staged': # staged but not yet committed return _run_git(['git', 'diff', '--name-only', '--cached']) diff --git a/tests/test_changed_files_scope.py b/tests/test_changed_files_scope.py index e4a5aba..5a6a6d1 100644 --- a/tests/test_changed_files_scope.py +++ b/tests/test_changed_files_scope.py @@ -377,3 +377,19 @@ def test_config_creation_propagates_config_error(self, pr_repo, monkeypatch): (pr_repo / ".git" / "shallow").touch() with pytest.raises(SystemExit, match="fetch-depth"): create_config_from_args(_config_args(pr_repo, "auto")) + + def test_shallow_no_merge_base_fails_fast(self, pr_repo, monkeypatch): + # Base tip exists but shares no history with HEAD (partial fetch shape): + # `A...HEAD: no merge base`. Shallow -> config error, same as missing ref. + _git(pr_repo, "checkout", "--orphan", "disconnected") + _git(pr_repo, "add", "-A") + _git(pr_repo, "commit", "-m", "orphan") + (pr_repo / ".git" / "shallow").touch() + with pytest.raises(SystemExit, match="fetch-depth"): + _detect_git_changed_files(str(pr_repo), mode="pr", base_ref="main") + + def test_non_shallow_no_merge_base_keeps_fallback(self, pr_repo): + _git(pr_repo, "checkout", "--orphan", "disconnected") + _git(pr_repo, "add", "-A") + _git(pr_repo, "commit", "-m", "orphan") + assert _detect_git_changed_files(str(pr_repo), mode="pr", base_ref="main") is None From 9898a8408c297ccae2c7c0cc46be85f64dc002de Mon Sep 17 00:00:00 2001 From: lelia <2418071+lelia@users.noreply.github.com> Date: Wed, 12 Aug 2026 17:35:30 -0700 Subject: [PATCH 7/7] fix(scope): fail closed when changed_files cannot be resolved An unresolvable diff scope previously widened to a full-repo scan. Both that and skipping the scanners are dishonest outcomes: skipping exits green having scanned nothing, so a passing check inspected no code and a warning in a run log is not a signal anyone acts on; widening does the expensive thing on every PR, which is precisely what requesting a diff scope was avoiding, and it reports pre-existing findings rather than the PR's own. Diff-only scoping is an explicit instruction, so when it cannot be honored the run now stops with a configuration error naming the underlying git error. This generalizes the fail-fast already applied to shallow checkouts, which keep their more specific fetch-depth message. scan_all is the documented opt-in for the previous widening behavior: it already meant "when the scope resolves to nothing, scan everything", so it doubles as the fail-open escape hatch. It is now a declared action input rather than env-only. --- CHANGELOG.md | 20 +++++---- action.yml | 15 ++++++- docs/github-action.md | 51 +++++++++++---------- docs/parameters.md | 26 +++++------ socket_basics/core/config.py | 74 ++++++++++++++++++++++--------- tests/test_changed_files_scope.py | 73 +++++++++++++++++++++++++----- 6 files changed, 182 insertions(+), 77 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fe5975f..8056ad7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,21 +20,23 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). repository/branch/commit and default-branch discovery in local Docker runs; those lookups are covered by the same change. - A failed `changed_files` diff resolution is no longer indistinguishable from - an empty diff. Git errors are captured and logged (instead of discarded), and + an empty diff. Git errors are captured and logged instead of discarded, and when the scope cannot be resolved — unreadable repository, unresolvable base - ref (e.g. a shallow fetch without the base), or `pr` mode with no base ref — - Socket Basics now **falls back to a full-repo scan with a prominent warning** - rather than skipping every scanner and reporting a green run that scanned - nothing. A genuinely empty diff (e.g. a delete-only PR) still keeps the empty - scope and skips as before. One deterministic case fails fast instead of - falling back: a **shallow checkout** that cannot resolve the base ref exits - with a configuration error naming the fix (`fetch-depth: 0`), since it would - otherwise full-scan every PR — slow or OOM-prone on large repositories. + ref, or `pr` mode with no base ref — Socket Basics now **fails with a + configuration error** rather than reporting a green run that scanned nothing. + Shallow checkouts get a more specific error naming `fetch-depth: 0`. A + genuinely empty diff (e.g. a delete-only PR) is a successful resolution and + still skips the scanners as before. ### Added - The resolved `changed_files` scope is now logged on every scoped run: file count at INFO, the full file list at DEBUG — so an empty diff and a failed lookup are visible and distinguishable in run logs. +- `scan_all` is now a declared action input and doubles as the fail-open escape + hatch for `changed_files`: when the scope cannot be resolved, widen to a + full-repo scan with a warning instead of failing. The widening is partial — + only scanners that read scan targets widen, while secret and container + scanners stay scoped. ## [3.0.0] - 2026-08-06 diff --git a/action.yml b/action.yml index 06a1721..3bff066 100644 --- a/action.yml +++ b/action.yml @@ -11,6 +11,7 @@ runs: INPUT_WORKSPACE: ${{ inputs.workspace }} # Scan scope INPUT_CHANGED_FILES: ${{ inputs.changed_files }} + INPUT_SCAN_ALL: ${{ inputs.scan_all }} INPUT_SCAN_FILES: ${{ inputs.scan_files }} # Input mappings for all parameters INPUT_ALL_LANGUAGES_ENABLED: ${{ inputs.all_languages_enabled }} @@ -116,9 +117,21 @@ inputs: GITHUB_BASE_REF), or 'current-commit'. For PR/'auto' modes, check out with actions/checkout fetch-depth: 0 so the base branch is available. When the diff resolves to no existing files (e.g. a delete-only PR) the scanners - are skipped rather than scanning the whole repo. + are skipped rather than scanning the whole repo. When the diff cannot be + resolved at all (unreadable repo, missing base ref) the run fails with a + configuration error instead of reporting a green scan of nothing; set + scan_all to widen to a full-repo scan in that case instead. required: false default: "" + scan_all: + description: >- + Scan the entire workspace even when a narrower scope was requested but + could not be produced. Acts as the fail-open escape hatch for + changed_files: instead of failing when a diff cannot be resolved, widen to + a full-repo scan. Note the widening is partial — only scanners that read + scan targets widen, while secret and container scanners stay scoped. + required: false + default: "false" scan_files: description: >- Explicit comma-separated list of files to scan. Scopes SAST/OpenGrep, diff --git a/docs/github-action.md b/docs/github-action.md index 9ca5f36..f64569d 100644 --- a/docs/github-action.md +++ b/docs/github-action.md @@ -325,32 +325,37 @@ jobs: > list regardless of git state, use the `scan_files` input instead. > [!NOTE] -> **When the diff cannot be resolved** — the checkout is unreadable, or the -> base branch is missing (most commonly a shallow clone without -> `fetch-depth: 0`) — Socket Basics logs a warning naming the underlying git -> error and **falls back to a full-repository scan** rather than silently -> scanning nothing. This is deliberate fail-toward-scanning behavior for a -> security gate, and it comes with two tradeoffs worth planning for: +> **When the diff cannot be resolved** — the checkout is unreadable, or the base +> branch is missing (most commonly a shallow clone without `fetch-depth: 0`) — +> Socket Basics **fails with a configuration error** naming the underlying git +> error. It does not scan. > -> - On very large repositories an unexpected full scan can be slow or exhaust -> CI memory. If the fallback warning appears on **every** PR, the cause is -> almost always the missing `fetch-depth: 0` — fix the checkout rather than -> sizing up the runner. -> - The full scan reports **pre-existing** findings, not just the PR's change, -> so a repo-wide checkout misconfiguration shows up as large PR comments or -> failing checks on every PR until corrected. The run log's warning names -> the actual git error — read it before triaging the findings. +> This is deliberate. Diff-only scoping is an explicit instruction, and if it +> cannot be honored there is no honest result to report: > -> **Exception — shallow checkouts fail fast instead.** If the diff cannot be -> computed *because the checkout is shallow* — the base branch is missing, or -> its tip was fetched without connecting history (`no merge base`), both -> classically a missing `fetch-depth: 0` — the failure is deterministic: -> every PR would full-scan. Socket Basics exits with a configuration error -> naming that one-line fix rather than falling back. +> - **Skipping the scanners** would exit green having scanned zero files. A +> passing check that inspected nothing is worse than a failing one, and a +> warning buried in a run log is not something anyone acts on. +> - **Silently scanning everything** would do the expensive thing on every PR — +> precisely what asking for a diff scope was avoiding. On a large repository +> that is a slow or OOM-prone check, and it reports **pre-existing** findings +> rather than the PR's own, so a checkout misconfiguration surfaces as large PR +> comments on every PR until corrected. > -> A genuinely *empty* diff (e.g. a delete-only PR) still skips the scanners; -> the fallback triggers only when resolution **fails**. The resolved file -> count is logged on every scoped run, so an empty diff and a failed lookup +> If the error appears on **every** PR, the cause is almost always a missing +> `fetch-depth: 0` — fix the checkout rather than sizing up the runner. Shallow +> checkouts get a more specific error naming that fix directly, including the +> `no merge base` shape where the base tip was fetched without connecting +> history. +> +> **To scan anyway, set `scan_all: true`.** That widens an unresolvable scope to +> a full-repository scan with a warning instead of failing. Note the widening is +> partial: only scanners that read scan targets widen, while secret and container +> scanners stay scoped. +> +> A genuinely *empty* diff (e.g. a delete-only PR) is a successful resolution and +> still skips the scanners — only a **failed** resolution errors. The resolved +> file count is logged on every scoped run, so an empty diff and a failed lookup > are always distinguishable in the logs. ## PR Comment Customization diff --git a/docs/parameters.md b/docs/parameters.md index 2a09852..55b5d75 100644 --- a/docs/parameters.md +++ b/docs/parameters.md @@ -120,19 +120,19 @@ PR), the scanners are skipped rather than falling back to scanning the whole repository. For PR/`auto`/`pr` modes, check out with full history (e.g. `actions/checkout` with `fetch-depth: 0`) so the base branch is available. -If the diff **cannot be resolved**, behavior depends on why: - -- **Shallow checkout that cannot diff the base** — missing base ref or - disconnected history (`no merge base`), classically a missing - `fetch-depth: 0`: deterministic misconfiguration — the run **fails fast - with a configuration error** naming the fix, instead of full-scanning every - PR. -- **Any other resolution failure** (unreadable repository, non-shallow missing - ref): a warning with the underlying git error is logged and the scan **falls - back to the whole workspace** instead of silently scanning nothing. On large - repositories a surprise full scan can be slow or hit CI limits and reports - pre-existing findings, so treat the warning as the signal and fix the root - cause. +If the diff **cannot be resolved** — unreadable repository, missing base ref, or +a shallow checkout with no base to diff against — the run **fails with a +configuration error** naming the underlying git error, and nothing is scanned. +Neither alternative is honest: skipping the scanners exits green having scanned +zero files, and widening to the whole repository does the expensive thing on +every PR, which is what requesting a diff scope was avoiding. Shallow checkouts +get a more specific error naming `fetch-depth: 0`, covering both the missing-ref +and disconnected-history (`no merge base`) shapes. + +Set **`scan_all`** to widen instead of failing: an unresolvable scope then falls +back to a full-workspace scan with a warning. The widening is partial — only +scanners that read scan targets widen, while secret and container scanners stay +scoped to `changed_files`. The resolved scope is logged on every run (file count at INFO, full file list at DEBUG), so an empty diff and a failed lookup are distinguishable in run diff --git a/socket_basics/core/config.py b/socket_basics/core/config.py index 40c3278..a7089a1 100644 --- a/socket_basics/core/config.py +++ b/socket_basics/core/config.py @@ -1573,27 +1573,49 @@ def create_config_from_args(args) -> Config: def _apply_scoped_changed_files(mode_label: str, **detect_kwargs) -> None: """Resolve the diff scope, distinguishing failure from an empty diff. - A failed resolution (None) falls back to a full-repo scan with a - prominent warning: a scoped scan that silently resolves to nothing - reports a green run while scanning zero files, which is the - fail-open failure mode this guards against. A genuinely empty diff - (e.g. a delete-only PR) keeps the empty scope and skips, as before. + A failed resolution (None) is a configuration error and stops the + run. Diff-only scoping is an explicit instruction; if it cannot be + honored, Socket Basics cannot make any statement about the code, and + both alternatives are worse than stopping. Skipping the scanners + reports a green check having scanned nothing — false assurance, and a + warning in a run log is not a signal anyone acts on. Silently + widening to the whole repository does the expensive thing on every + PR, which is precisely what a caller asking for a diff scope was + avoiding. + + ``scan_all`` is the opt-in for that widening: it already means "when + the scope resolves to nothing, scan everything rather than nothing", + so it doubles as the documented fail-open escape hatch. + + A genuinely empty diff (e.g. a delete-only PR) is a successful + resolution — it keeps the empty scope and skips, as before. """ + fail_open = bool(config_dict.get('scan_all', False)) try: - resolved = _detect_git_changed_files(config_dict.get('workspace', os.getcwd()), **detect_kwargs) + resolved = _detect_git_changed_files( + config_dict.get('workspace', os.getcwd()), fail_open=fail_open, **detect_kwargs + ) except Exception as e: _scope_log.warning("Warning: failed to detect git changed files (%s): %s", mode_label, e) resolved = None if resolved is None: - _scope_log.warning( - "changed_files scope could not be resolved (%s); falling back to a " - "full-repo scan so nothing is silently skipped. See the warnings " - "above for the underlying git error.", - mode_label, + if fail_open: + _scope_log.warning( + "changed_files scope could not be resolved (%s); scan_all is set, so " + "falling back to a full-repo scan. See the warnings above for the " + "underlying git error.", + mode_label, + ) + config_dict['changed_files'] = [] + config_dict['changed_files_scope_requested'] = False + return + raise SystemExit( + f"changed_files: the requested scope ({mode_label}) could not be resolved, so " + "the scan would either report a green run having scanned nothing or silently " + "widen to the whole repository. See the warnings above for the underlying git " + "error. Fix the git problem, or set scan_all to widen to a full-repo scan when " + "the scope cannot be resolved." ) - config_dict['changed_files'] = [] - config_dict['changed_files_scope_requested'] = False - return _scope_log.info("changed_files scope resolved to %d file(s) (%s)", len(resolved), mode_label) if resolved: _scope_log.debug("changed_files scope: %s", ", ".join(resolved)) @@ -1703,7 +1725,7 @@ def __init__(self, message: str, ref_miss: bool = False, merge_base_miss: bool = self.merge_base_miss = merge_base_miss -def _detect_git_changed_files(workspace_path: str, mode: str = 'staged', commit: str | None = None, base_ref: str | None = None) -> Optional[List[str]]: +def _detect_git_changed_files(workspace_path: str, mode: str = 'staged', commit: str | None = None, base_ref: str | None = None, fail_open: bool = False) -> Optional[List[str]]: """Detect changed files in a git repository. mode: @@ -1724,6 +1746,13 @@ def _detect_git_changed_files(workspace_path: str, mode: str = 'staged', commit: requested base ref could not be resolved — so callers can distinguish "no changed files" from "the lookup broke" instead of silently scanning nothing. The specific git error is logged here at WARNING level. + + ``fail_open`` mirrors the caller's ``scan_all`` setting. When False (the + default) a deterministic misconfiguration raises SystemExit here with the + specific fix, rather than returning None for the caller to turn into a + generic error. When True the caller has opted into widening an unresolvable + scope to a full-repo scan, so those checks are skipped and the failure is + reported as None. """ log = logging.getLogger(__name__) try: @@ -1783,12 +1812,17 @@ def _fail_fast_if_shallow(ref: str) -> None: """Deterministic misconfiguration check for an unresolvable base. A shallow checkout (no ``fetch-depth: 0``) can *never* resolve - the base ref, so every PR would take the full-scan fallback — - on large repos that is a slow/OOM red check instead of a clear - signal. Fail fast with the one-line fix instead. Follows the - existing SystemExit convention for unrecoverable configuration - errors (see repository/branch discovery below). + the base ref, so every PR on this checkout would fail the same + way. Name the one-line fix here instead of letting the caller + report a generic unresolvable-scope error. Follows the existing + SystemExit convention for unrecoverable configuration errors + (see repository/branch discovery below). + + Skipped under ``fail_open``: the caller set ``scan_all`` and has + opted into widening an unresolvable scope instead of failing. """ + if fail_open: + return try: shallow = _run_git(['git', 'rev-parse', '--is-shallow-repository']) == ['true'] except Exception: diff --git a/tests/test_changed_files_scope.py b/tests/test_changed_files_scope.py index 5a6a6d1..c50b60c 100644 --- a/tests/test_changed_files_scope.py +++ b/tests/test_changed_files_scope.py @@ -285,10 +285,12 @@ def test_defaults_to_github_workspace(self, monkeypatch): class TestScopeResolutionFailure: """Failed diff resolution must be distinguishable from an empty diff. - A git failure (unreadable repo, unresolvable base ref) returns None and the - config layer falls back to a full-repo scan with a warning — never a green - run that silently scanned nothing. A genuinely empty diff still returns [] - and keeps the skip behavior (see the delete-only test above). + A git failure (unreadable repo, unresolvable base ref) returns None from the + detection helper, and the config layer turns that into a configuration error + — never a green run that silently scanned nothing, and never a silent widen + to the whole repository. ``scan_all`` opts into the widen instead. A + genuinely empty diff still returns [] and keeps the skip behavior (see the + delete-only test above). """ def test_unreadable_repo_returns_none(self, pr_repo): @@ -319,15 +321,36 @@ def test_failure_logs_git_stderr(self, pr_repo, caplog): _detect_git_changed_files(str(pr_repo), mode="pr", base_ref="main") assert any("changed_files scope" in r.getMessage() for r in caplog.records) - def test_config_creation_falls_back_to_full_scan_on_failure(self, pr_repo, monkeypatch, caplog): + def test_config_creation_fails_closed_on_failure(self, pr_repo, monkeypatch): monkeypatch.delenv("GITHUB_WORKSPACE", raising=False) + monkeypatch.delenv("INPUT_SCAN_ALL", raising=False) monkeypatch.setenv("GITHUB_BASE_REF", "main") (pr_repo / ".git" / "HEAD").write_text("garbage") + # Scope resolution failed -> configuration error. Neither a green run + # that scanned nothing nor a silent full-repo scan. + with pytest.raises(SystemExit, match="could not be resolved"): + create_config_from_args(_config_args(pr_repo, "auto")) + + def test_config_error_names_scan_all_escape_hatch(self, pr_repo, monkeypatch): + monkeypatch.delenv("GITHUB_WORKSPACE", raising=False) + monkeypatch.delenv("INPUT_SCAN_ALL", raising=False) + monkeypatch.setenv("GITHUB_BASE_REF", "main") + (pr_repo / ".git" / "HEAD").write_text("garbage") + + with pytest.raises(SystemExit, match="scan_all"): + create_config_from_args(_config_args(pr_repo, "auto")) + + def test_scan_all_opts_into_full_scan_fallback(self, pr_repo, monkeypatch, caplog): + monkeypatch.delenv("GITHUB_WORKSPACE", raising=False) + monkeypatch.setenv("GITHUB_BASE_REF", "main") + monkeypatch.setenv("INPUT_SCAN_ALL", "true") + (pr_repo / ".git" / "HEAD").write_text("garbage") + with caplog.at_level("WARNING"): cfg = create_config_from_args(_config_args(pr_repo, "auto")) - # Scope resolution failed -> full-repo scan, not a silent skip. + # scan_all is the documented fail-open opt-in: widen, do not fail. assert cfg.get("changed_files") == [] assert cfg.get("changed_files_scope_requested") is False assert cfg.get_scan_targets() == [str(pr_repo)] @@ -346,9 +369,11 @@ def test_config_creation_logs_resolved_count(self, pr_repo, monkeypatch, caplog) class TestShallowCheckoutFailFast: """A shallow checkout that cannot resolve the base ref is a deterministic - misconfiguration (missing fetch-depth: 0) — fail fast with the one-line fix - instead of full-scanning every PR (slow/OOM on large repos). Non-shallow - failures keep the full-scan fallback. + misconfiguration (missing fetch-depth: 0), so the detection helper raises + with that one-line fix rather than returning a generic failure. Other + failures return None and let the config layer report the generic + unresolvable-scope error. Under ``fail_open`` (the caller set ``scan_all``) + the check is skipped so the widen can happen instead. """ def test_shallow_missing_base_fails_fast_pr_mode(self, pr_repo): @@ -368,9 +393,35 @@ def test_shallow_with_resolvable_base_still_diffs(self, pr_repo): files = _detect_git_changed_files(str(pr_repo), mode="pr", base_ref="main") assert sorted(files) == ["base.py", "feat.py"] - def test_non_shallow_missing_base_keeps_fallback(self, pr_repo): + def test_non_shallow_missing_base_returns_none(self, pr_repo): assert _detect_git_changed_files(str(pr_repo), mode="pr", base_ref="no-such-branch") is None + def test_fail_open_skips_shallow_fail_fast(self, pr_repo): + # scan_all was set, so the caller wants to widen rather than fail: the + # shallow check must not pre-empt that with a SystemExit. + (pr_repo / ".git" / "shallow").touch() + result = _detect_git_changed_files( + str(pr_repo), mode="pr", base_ref="no-such-branch", fail_open=True + ) + assert result is None + + def test_fail_open_skips_no_merge_base_fail_fast(self, pr_repo): + _git(pr_repo, "checkout", "--orphan", "disconnected") + _git(pr_repo, "add", "-A") + _git(pr_repo, "commit", "-m", "orphan") + (pr_repo / ".git" / "shallow").touch() + result = _detect_git_changed_files( + str(pr_repo), mode="pr", base_ref="main", fail_open=True + ) + assert result is None + + def test_fail_open_still_resolves_a_good_diff(self, pr_repo): + # fail_open only affects failure handling, never a successful diff. + files = _detect_git_changed_files( + str(pr_repo), mode="pr", base_ref="main", fail_open=True + ) + assert sorted(files) == ["base.py", "feat.py"] + def test_config_creation_propagates_config_error(self, pr_repo, monkeypatch): monkeypatch.delenv("GITHUB_WORKSPACE", raising=False) monkeypatch.setenv("GITHUB_BASE_REF", "no-such-branch") @@ -388,7 +439,7 @@ def test_shallow_no_merge_base_fails_fast(self, pr_repo, monkeypatch): with pytest.raises(SystemExit, match="fetch-depth"): _detect_git_changed_files(str(pr_repo), mode="pr", base_ref="main") - def test_non_shallow_no_merge_base_keeps_fallback(self, pr_repo): + def test_non_shallow_no_merge_base_returns_none(self, pr_repo): _git(pr_repo, "checkout", "--orphan", "disconnected") _git(pr_repo, "add", "-A") _git(pr_repo, "commit", "-m", "orphan")