Skip to content

fix: honor changed_files from every config source and stop failing silently - #98

Open
John-David Dalton (jdalton) wants to merge 9 commits into
mainfrom
fix/changed-files-scope-observability
Open

fix: honor changed_files from every config source and stop failing silently#98
John-David Dalton (jdalton) wants to merge 9 commits into
mainfrom
fix/changed-files-scope-observability

Conversation

@jdalton

@jdalton John-David Dalton (jdalton) commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

LLM Description written by CLAUDE_CODE:claude-opus-5

Summary

A customer reported that changed_files does not work: they tried auto and they tried pr, and in every case the action scanned the whole repository and commented on files their PR never touched. Adding fetch-depth: 0 to their checkout made no difference. Diff-only scoping shipped in v2.1.0 (#77), so this was supposed to already work.

I reproduced the full pipeline against a real PR-shaped git checkout. The happy path in #77 is correct on a plain checkout — with fetch-depth: 0 and GITHUB_BASE_REF set, changed_files: 'auto' really does scope the scan to one file. What #77 missed is everything around it, and every one of the misses is completely silent.

The biggest one is that it never worked in the action at all. The scan ships as a Docker container action, so it runs as root over a workspace owned by the runner user, and git refuses to read a repository in that state. Every diff failed, the scope came back empty, and an empty scope is indistinguishable from a pull request that changed nothing. No workflow could fix it.

On top of that, the scope request only reached one of the ways a config gets built, a raw string like "auto" was iterated character by character, and scan_all threw a correctly-resolved scope away without a word.

This PR makes the git reads work inside the container, routes every source of changed_files through one resolver, and makes each remaining failure say what went wrong and what to do about it.

Nothing reaches a workflow until a release — and the reporting customer needs a newer tag no matter what. This action is consumed by tag, and the tag their mirror pins ships an image that predates the changed_files input entirely, so it accepts the setting and silently drops it. Merging this is safe on its own; after it merges, a release plus a re-mirror on their side is what actually changes their scans. See The tag a lot of people are pinned to, the first fold below.

Release & rollout

The tag a lot of people are pinned to runs a much older image

Worth checking separately from this PR, because it explains the same symptom
without any code being wrong.

action.yml pins the container image, and the release script keeps that pin in
sync with pyproject.toml. The v2.1.0 tag was cut before either was bumped:

Tag action.yml declares changed_files? Image it runs
v2.0.3 no 2.0.3
v2.1.0 yes 2.0.3
v2.2.0 yes 2.2.0
v2.2.1 yes 2.2.1

So on @v2.1.0 the input is declared, GitHub passes INPUT_CHANGED_FILES to the
container without complaint, and the 2.0.3 image never reads that variable —
it has no INPUT_CHANGED_FILES handling at all, only the --changed-files CLI
flag, and the container entrypoint passes no arguments. The request is accepted
and dropped, the whole repository is scanned, and fetch-depth: 0 makes no
difference. auto, pr and an explicit file list all behave identically,
because none of them are read.

Anyone pinned to @v2.1.0, including through an internal mirror of it, needs to
move to @v2.2.1 or later; no code change reaches them until they do.

scripts/sync_release_version.py --check runs in CI and would catch an
action.yml pin that disagrees with pyproject.toml today. It cannot catch this
one, because v2.1.0's pyproject.toml also still said 2.0.3 — the tag name
is the only thing that disagreed, and nothing compares a tag to the version it
claims to be. That gap is worth closing on its own.

Is the customer's mirrored copy the problem?

Partly, possibly — but not entirely, and it does not need to be settled to merge this.

The customer runs internally mirrored copies of the actions rather than upstream tags. A mirror pinned to a pre-#77 image would ignore changed_files completely and scan the whole repo, which matches their report exactly. That is worth checking on their side.

But it is not the only explanation, and the other two are in our code, not theirs. scan_all from an enterprise dashboard config produces precisely the reported symptom — full-repo scanning, unchanged across auto, pr and an explicit list — and produces it on current main. So does any config path that does not go through create_config_from_args. Both are fixed here.

The part that matters most either way is the observability. Every one of these states used to be indistinguishable from "it worked and found nothing". After this PR, the customer's next run tells them which one they are in from the log, without another round trip.

Diagnosis

What I found: three ways the scope is discarded, and they all happen without a single log line

I built a script that stands up an upstream repo, a PR merge ref, and a CI-style checkout, then drives the real Config code with the environment a Docker container action actually sees. Results:

Scenario Before this PR Silent?
fetch-depth: 0 + GITHUB_BASE_REF + auto scoped correctly
fetch-depth: 0 + GITHUB_BASE_REF + pr scoped correctly
Shallow checkout (no fetch-depth: 0) scanned nothing yes
GITHUB_BASE_REF absent (non-pull_request trigger) scanned nothing yes
git cannot read the checkout scanned nothing yes
changed_files never reaches the config layer scanned the whole repo yes
scan_all also set scanned the whole repo yes
changed_files: "auto" from a JSON/dashboard config scanned nothing yes

1. The input only reached one config path. changed_files was resolved in exactly one place, create_config_from_args(). load_config_from_env() handles INPUT_SCAN_ALL and INPUT_SCAN_FILES but had no changed_files entry at all, and neither did load_explicit_env_config(). So a Config built any other way — the library entry point, or anything that constructs Config() from the environment — never saw the request and scanned the whole repository.

2. A raw string value was never resolved, and then iterated character by character. A --config JSON file or a Socket dashboard config can carry "changed_files": "auto". That string went straight into the config, and get_scan_targets() handed it to _resolve_file_targets(), which iterates its argument. Iterating the string "auto" yields 'a', 'u', 't', 'o', so it looked for four one-character filenames, found none, and scoped the scan to nothing — logging four "Scan target does not exist" warnings naming <workspace>/a, <workspace>/u and so on.

3. scan_all discarded a correctly-resolved scope without a word. get_scan_targets() checks scan_all first and returns the whole workspace. I confirmed the sequence: the diff resolves to one file, changed_files is ['app.py'], and then the whole workspace is returned anyway with no output. scan_all can be set in a Socket dashboard config or a shared workflow template rather than in the workflow that asked for diff-only scoping, so the person who set it and the person debugging it are often different people. This is the mechanism that best matches "we tried three configurations and nothing changed" — the scope is computed correctly every time and thrown away every time.

And the base resolution only ever looked at GITHUB_BASE_REF. That variable is set only on pull_request and pull_request_target triggers. On any other trigger _diff_against_base('') returned None immediately and the scope resolved to nothing. The customer's workflow sets GITHUB_PR_NUMBER: ${{ github.event.pull_request.number || github.event.issue.number }}, which says they also run this on issue_comment events, so they land in that hole.

The fix

The fix: one resolver, and no failure mode that stays quiet

One resolver. A new resolve_changed_files_request(request, workspace) handles auto, pr, current-commit, a commit hash, and a comma-separated list. Config.__init__ calls it, so the action input, the --changed-files CLI flag, INPUT_CHANGED_FILES, a --config JSON file and a Socket dashboard config all get the same treatment. create_config_from_args() now just records the raw CLI value and lets Config resolve it, which deleted about 50 lines of duplicated per-mode branching. A value that is already a resolved list passes through untouched, so nothing is resolved twice.

More ways to find the PR base. auto and pr now try, in order: GITHUB_BASE_REF, then pull_request.base.sha from the event payload at GITHUB_EVENT_PATH, then pull_request.base.ref. base.sha is the best of the three because it is an exact commit and does not need a remote-tracking branch to exist. Each candidate is tried as origin/<ref> and then bare, exactly as before.

The payload steps cover the triggers whose payload has a top-level pull_request: pull_request, pull_request_target, pull_request_review and pull_request_review_comment. They do not cover issue_comment, which Bugbot caught after the first version of this description claimed otherwise. That payload has issue.pull_request instead — a set of URLs with no base ref or sha in it — so the base cannot be worked out without a GitHub API call, and config.py makes no network calls at all. Rather than guess a base (diffing against the wrong one silently is the failure this PR exists to kill), that shape now gets a warning that names the trigger and tells the workflow author to look the base up and pass GITHUB_BASE_REF in. docs/parameters.md carries the two-step workflow snippet.

Every failure names itself. These all used to be return []:

Situation New log line (WARNING)
Shallow checkout none of the candidate PR bases (main) could be resolved ... The checkout is shallow, so the base branch is not in it -- set fetch-depth: 0 on actions/checkout.
No PR base anywhere no pull request base was found. GITHUB_BASE_REF is unset and the GitHub event payload has no pull_request.base ... Use changed_files: 'current-commit', or pass an explicit file list.
Workspace is not a git repo is not a git repository. Check out the repository (actions/checkout) before running the scan
git refuses the checkout git refused to read ... usually a repository-ownership mismatch inside a container ('detected dubious ownership'). Run git config --global --add safe.directory ...
Scope resolved to zero files resolved to zero files. The scanners will be SKIPPED rather than scanning the whole repository.
scan_all overrides the scope scan_all is enabled, so the whole workspace will be scanned and the requested changed-files scope (1 file(s)) is being ignored.

The success path is loud too, so you can confirm the scope took effect: Resolved PR diff base to 'origin/main', then resolved 12 changed file(s), then Diff-only scan scoping active: 12 scan target(s).

Connectors stop substituting their own scope. TruffleHog and Trivy each re-derive a changed-file list with mode='staged' when the config has none. When the user explicitly asked for a scope and it resolved to nothing, that fallback replaced "what the PR changed" with "whatever happens to be staged" — a different set of files, and never the one that was asked for. It now only runs when no scope was requested, so the existing default behavior is unchanged.

The container could not read the repository at all, so none of this worked in the action

This action ships as a Docker container action. GitHub runs the container as
root and mounts the workspace, which belongs to the runner user. That ownership
mismatch is exactly what git refuses with detected dubious ownership, so every
git diff in the scan failed and changed_files: 'auto' or 'pr' resolved to
nothing — indistinguishable, before this PR, from a pull request that changed
no files.

I confirmed it by driving the real Config code over a real PR-shaped checkout
with GIT_TEST_ASSUME_DIFFERENT_OWNER=1, which is git's own switch for that
state. On main and on the first version of this branch the scope came back
empty; the only difference the branch made was that it explained itself.

The explanation it gave could not be acted on, either. It told the reader to run
git config --global --add safe.directory "$GITHUB_WORKSPACE" before the scan
step, and that writes the runner's git config — the container has its own,
so the advice does nothing. Every workflow hits this and no workflow can get out
of it.

So the git reads now declare the trust themselves, and only when git is actually
refusing:

git -c safe.directory=<workspace> diff --name-only ...

The decision is a probe. If plain git can read the workspace — any ordinary
local run — that is what runs and nothing is relaxed at all. Only when it cannot
does the scan retry with that one directory trusted, and it logs that it did.
safe.directory is a real protection against picking up a repository config you
do not control, and giving it up where it was not in the way would buy nothing.
It is never safe.directory=*.

Eight tests cover it: the "workspace only, never a wildcard" property against
the command builder, and the behaviour with git actually refusing. The second
group needs git to refuse for real, and GIT_TEST_ASSUME_DIFFERENT_OWNER only
forces git to consult safe.directory rather than forcing a refusal — a global
safe.directory = * makes it inert, and the hosted runners have one. Those
tests skip with a reason there instead of passing vacuously.

Scope

What I deliberately did not change
  • scan_all still wins over changed_files. That precedence is documented in get_scan_targets() and other people rely on it. Flipping it would be a silent behavior change for everyone. It now warns instead. Note the precedence is only partial and always has been: SAST widens because it asks get_scan_targets() for its paths, while TruffleHog and Trivy read changed_files directly and stay scoped. Making scan_all reach those two would change what a lot of existing runs scan, so the warning says the run will be a mix rather than pretending the override is clean.
  • auto still falls back to staged changes when there is no PR base, which is what makes it useful for pre-commit hooks. It now warns first and logs how many staged files it found, so a CI run that lands there is obvious.
  • current-commit and commit-hash modes still include deletions. Only pr/auto use --diff-filter=ACMR. The deleted paths get dropped when targets are resolved, so the behavior is right; the asymmetry is pre-existing and out of scope here. There is a test pinning it.
  • No version bump, no tag, no image rebuild. action.yml still points at the released 2.2.1 image.

Testing & review

Ran — exit codes read directly from the harness, not through a pipe.

Command Result
uv run --no-sync pytest -q tests/ exit 0, 261 passed (was 216 on main; 45 new)
python scripts/sync_release_version.py --check exit 0, version metadata in sync at 2.2.1
Scenario reproduction over a real git PR checkout 14 CI shapes before/after, through both the library entry point and the one the action uses; every previously-silent failure now logs
Same reproduction with GIT_TEST_ASSUME_DIFFERENT_OWNER=1 scoped correctly; empty on main and empty before the ownership fix
Suite again with HOME pointed at a config holding safe.directory = * exit 0, 3 passed and 5 skipped — reproduces the hosted-runner shape exactly
YAML parse of action.yml OK

New tests in tests/test_changed_files_scope.py:

  • TestScopeRequestReachesEveryConfigPath (8) — env loader, raw "auto" string, raw comma list, already-resolved list, empty value, CLI-over-env precedence
  • TestPrBaseResolution (7) — base.sha and base.ref from the event payload, a payload with no pull request, an unreadable payload, a deep checkout resolving origin/<ref>, an issue_comment payload getting its own named warning, and a plain issue comment getting the generic one
  • TestTrivyVulnScanHonorsTheResolvedScope (5) — Trivy's filesystem vulnerability scan is the one scanner that does not go through get_scan_targets(), so it needed the empty-scope check of its own, and that check has to yield to scan_all
  • TestScopeFailuresAreLoud (5) — shallow checkout, missing base, non-git workspace, zero resolution, and a success case asserting no warning
  • TestScanAllOverrideIsLoud (5) — the override still wins, warns when it discards a scope, stays quiet when no scope was requested, and the warning describes the mixed run rather than a clean override
  • TestResolveChangedFilesRequest (5) and TestConnectorsHonorTheResolvedScope (2)
Mutation checks: every fix was broken on purpose and a named test went red

Each mutation was applied, the full suite was run, the mutation was reverted, and the suite was re-run green.

Mutation Named tests that failed Exit
Env loader stops reading INPUT_CHANGED_FILES (the original gap) TestScopeRequestReachesEveryConfigPath::test_env_only_config_honors_input_changed_files, ::test_env_value_is_used_when_no_cli_value 1
String scope requests stored verbatim instead of resolved test_raw_auto_string_is_resolved_not_iterated, test_raw_comma_list_string_is_split, test_env_only_config_honors_input_changed_files, test_cli_value_overrides_env_value, test_env_value_is_used_when_no_cli_value, TestDetectGitChangedFiles::test_delete_only_pr_config_creation_keeps_empty_scope, TestResolveChangedFilesRequest::test_current_commit_drops_deleted_paths_from_targets 1
PR base no longer read from the event payload TestPrBaseResolution::test_uses_base_sha_from_event_payload, ::test_uses_base_ref_from_event_payload 1
issue_comment no longer gets its own warning TestPrBaseResolution::test_issue_comment_payload_yields_no_base_and_says_why 1
Any issue payload treated as a PR comment TestPrBaseResolution::test_plain_issue_comment_gets_the_generic_warning 1
Trivy vuln scan widens an empty scope to the workspace TestTrivyVulnScanHonorsTheResolvedScope::test_unresolvable_scope_does_not_widen_to_the_whole_workspace, ::test_scope_whose_paths_all_vanished_does_not_widen_either 1
scan_all warning goes back to claiming a clean override TestScanAllOverrideIsLoud::test_warning_says_the_run_will_be_a_mix_not_a_clean_override 1
Trivy empty-scope skip stops yielding to scan_all TestTrivyVulnScanHonorsTheResolvedScope::test_scan_all_still_gets_the_whole_workspace 1
scan_all discards the scope silently again TestScanAllOverrideIsLoud::test_scan_all_warns_when_it_discards_a_scope_request, ::test_scan_all_warns_for_a_scope_that_resolved_to_nothing 1
Failed base resolution goes back to being silent TestScopeFailuresAreLoud::test_shallow_checkout_warns_and_names_fetch_depth, ::test_missing_pr_base_warns 1
Zero-resolution warning removed TestScopeFailuresAreLoud::test_zero_resolution_warns_that_scanners_will_be_skipped 1
Connectors resume substituting the staged scope TestConnectorsHonorTheResolvedScope::test_scope_resolved_to_nothing_is_not_replaced_by_staged 1

After restoring all seven: exit 0, 245 passed.

Review feedback addressed: two more places where the scope was still being thrown away

Bugbot found both.

Trivy's filesystem vulnerability scan widened an empty scope back out to the whole workspace. Every other scanner inherits the empty-scope behaviour from Config.get_scan_targets(); this one builds its own path list from changed_files and had a if not scan_paths: scan_paths = [workspace_path] fallback right after it. Declining the staged-file substitution left that fallback in charge, so an unresolvable scope still produced a full-repository scan — and it was a widening this PR introduced, because before it the staged fallback would at least have narrowed to the staged directories. Fixed in 5a7fe28 with four tests.

The scan_all warning overstated what it does. scan_all only reaches the scanners that ask get_scan_targets() for their paths, so saying the changed-files scope is "ignored" would send someone looking for a full-repo secret scan that never happens. Fixed in ee741d7: the warning now says the run will be a mix and names which side does which. Chasing that also turned up a regression from 5a7fe28 -- with scan_all on and a scope that resolved to nothing, the new skip made Trivy's vulnerability scan do nothing at all, turning an explicit "scan everything" into scanning nothing. The skip now yields to scan_all.

The event-payload base fallback does not cover issue_comment, and this description said it did. It reads a top-level pull_request.base; issue_comment payloads have issue.pull_request, which is URLs only. Fixed in 8fa4908 by correcting the claim in the docstring, the docs and above, and by giving that shape a warning that names the trigger and says how to supply the base. Two tests, including one making sure a comment on a real issue still gets the generic message.

CI caught a test-isolation leak that the new event-payload fallback created

The first CI run failed one pre-existing test, test_auto_falls_back_to_staged_without_base_ref, which passes locally. The cause is a genuine consequence of this change: PR base resolution now reads GITHUB_EVENT_PATH, and when the suite runs inside a pull request that variable points at a real event payload naming a real base ref — main — which the fixture's throwaway repo happens to have a branch for. So the test diffed against it instead of falling back to staged changes, which is what it was written to check.

The fix is to clear GITHUB_EVENT_PATH in the pr_repo fixture alongside GITHUB_WORKSPACE and GITHUB_BASE_REF, which it already cleared for the same reason.

I reproduced the CI condition locally afterwards by running the suite with GITHUB_EVENT_PATH, GITHUB_BASE_REF and CI set the way Actions sets them: with the fixture fix reverted the same single test fails (exit 1), and with it in place the suite passes (exit 0, 245 passed). Worth noting because a test that only fails inside a pull request is the kind that comes back.

Did not run

  • No real GitHub Actions run. The reproduction drives the real Config and _detect_git_changed_files code against real git repositories built to look like an actions/checkout PR checkout (upstream remote, refs/pull/N/merge, detached HEAD, shallow and deep variants), with the container's GITHUB_* and INPUT_* environment set. It does not exercise the GitHub runner itself.
  • The Docker image was not rebuilt. This is Python-only with no new dependency.
  • The container repository-ownership case is now covered by tests that put git into that state with its own GIT_TEST_ASSUME_DIFFERENT_OWNER switch, not by running git as a second real user. The generic "git refused" branch is exercised by the non-git-workspace test.

Note

Medium Risk
Changes scan targeting and git invocation for all GitHub Action runs using changed_files; behavior is more correct but mixed scan_all + diff-only runs and trigger-specific base resolution need careful rollout.

Overview
Diff-only changed_files scoping is fixed end-to-end so PR scans stop silently scanning the whole repo or nothing.

Config now resolves every changed_files value through one resolve_changed_files_request() path (action input, INPUT_CHANGED_FILES, CLI, JSON, dashboard). Raw strings like "auto" are no longer iterated character-by-character. scan_all still wins for SAST via get_scan_targets() but logs a warning that secret/container scanners may stay scoped.

PR base detection adds pull_request.base.sha / base.ref from GITHUB_EVENT_PATH when GITHUB_BASE_REF is missing, with targeted warnings for shallow checkouts, non-git workspaces, issue_comment, and zero-file scopes. Git inside the Docker action uses per-workspace safe.directory only when ownership blocks reads.

TruffleHog and Trivy stop falling back to staged files or the full workspace when an explicit scope resolved empty; Trivy’s filesystem vuln scan gets the same guard. Docs and action.yml describe the new logging and config sources; tests cover the new behavior.

Reviewed by Cursor Bugbot for commit 93fe13b. Configure here.

changed_files was resolved in exactly one place, create_config_from_args,
and INPUT_CHANGED_FILES was missing from the environment loader entirely
(unlike INPUT_SCAN_ALL and INPUT_SCAN_FILES). A Config built any other
way silently scanned the whole repository, and a raw string value such
as 'auto' from a --config JSON file or a Socket dashboard config was
never git-resolved -- _resolve_file_targets iterated the string and
looked for files named a, u, t and o.

Every source now goes through one resolver, called from Config, so the
action input, the CLI flag, the env var, a JSON config and a dashboard
config are all honored identically.

Then make the failures visible. The PR base is now also read from
pull_request.base.sha/ref in the GitHub event payload, since
GITHUB_BASE_REF is only set on pull_request triggers. When no base can
be resolved the run names what it tried and why -- shallow checkout
(pointing at fetch-depth: 0), workspace is not a git repository, git
refusing to read the repository, or no PR base at all. A scope that
resolves to zero files warns that the scanners are being skipped, and
scan_all now warns when it discards a requested scope instead of
overriding it in silence. Every one of those previously returned an
empty list with no log output.

TruffleHog and Trivy no longer substitute their own staged-file scope
when an explicit request resolved to nothing.

Refs: SURF-1452
@jdalton
John-David Dalton (jdalton) requested a review from a team as a code owner August 3, 2026 19:03
PR base resolution now consults the GitHub event payload, so when these
tests run inside a pull request the ambient payload names a real base
ref and the fixture's temp repo happens to have a branch by that name.
test_auto_falls_back_to_staged_without_base_ref then diffed against it
instead of falling back to staged changes. Passed locally, failed in CI,
which is exactly the leak.
@jdalton

Copy link
Copy Markdown
Contributor Author

bugbot run

Comment thread socket_basics/core/connector/trivy/trivy.py
… the whole workspace

Trivy's filesystem vulnerability scan is the one scanner that builds its
own path list instead of going through Config.get_scan_targets(). Declining
the staged-file substitution was not enough on its own: when the requested
scope resolved to no scannable paths, scan_paths stayed empty and the
existing fallback assigned the whole workspace, so the scan expanded to the
full repository instead of skipping.

Caught by Cursor Bugbot on #98.
@jdalton

Copy link
Copy Markdown
Contributor Author

bugbot run

Comment thread socket_basics/core/config.py
… the event-payload base fallback

The event-payload fallback reads a top-level pull_request.base, which covers
pull_request, pull_request_target, pull_request_review and
pull_request_review_comment. It does not cover issue_comment: that payload
carries issue.pull_request, a set of URLs with no base ref or sha, so the base
cannot be worked out without a GitHub API call.

The docstring and docs claimed otherwise, and issue_comment is the trigger the
change was motivated by. Correct the claim, and give that shape its own warning
telling the workflow author to look the base up and pass GITHUB_BASE_REF, so an
unsupported trigger reports itself instead of looking like an empty diff.

Caught by Cursor Bugbot on #98.
@jdalton

Copy link
Copy Markdown
Contributor Author

bugbot run

Comment thread socket_basics/core/config.py
…t meaning scan everything

The scan_all warning claimed the changed-files scope was simply ignored. Only
the scanners that ask Config.get_scan_targets() for their paths widen -- SAST
does, while TruffleHog and Trivy read changed_files off the config themselves
and stay scoped. Both settings together produce a mixed run, so the warning now
says that instead of sending someone looking for a full-repo secret scan that
never happens.

Also stop the new Trivy empty-scope skip from firing under scan_all. scan_all is
an explicit request to scan everything, and turning it into scanning nothing was
a regression in the previous commit on this branch.

Caught by Cursor Bugbot on #98.
@jdalton

Copy link
Copy Markdown
Contributor Author

bugbot run

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit ee741d7. Configure here.

The action ships as a Docker container action, so the scan runs as root
over a workspace owned by the runner user. git refuses that with
"detected dubious ownership", which failed every diff and made
changed_files: 'auto' and 'pr' resolve to nothing -- the same symptom as
a pull request that changed no files.

No workflow could fix it. Running `git config --global --add
safe.directory` before the scan step writes the runner's git config, and
the container has its own, so the advice the previous warning gave could
not work. The git reads now declare the trust themselves, for the one
directory the workflow asked us to scan and nothing else.

Five tests drive real repositories through git's own
GIT_TEST_ASSUME_DIFFERENT_OWNER switch, including one asserting the
workspace is the only path trusted.
@jdalton

Copy link
Copy Markdown
Contributor Author

[agent] One more way the scope was being thrown away, and it is the one that mattered most: the container could not read the repository at all.

This ships as a Docker container action. GitHub runs the container as root and mounts a workspace that belongs to the runner user, and git refuses to read a repository in that state — detected dubious ownership. So every git diff in the scan failed, changed_files: 'auto' and 'pr' resolved to nothing, and an empty scope looks exactly like a pull request that changed no files. That happened on every run of the action, and no workflow could get out of it.

The warning this branch added made it visible but told the reader to do something that cannot work: git config --global --add safe.directory "$GITHUB_WORKSPACE" before the scan step writes the runner's git config, and the container has its own.

Fixed in 946cac4. The git reads now declare the trust themselves, scoped to the one directory the workflow already asked us to scan:

git -c safe.directory=<workspace> diff --name-only --diff-filter=ACMR <base>...HEAD

When ownership already matches this changes nothing, and it is never safe.directory=*. Five tests put git into the failing state with its own GIT_TEST_ASSUME_DIFFERENT_OWNER switch and drive real repositories through it, including one asserting the workspace is the only path trusted. Breaking the fix on purpose fails all five. Suite: 258 passed.

I also verified this the other way round, by running the same 14-scenario reproduction against main and against this branch through both entry points. Worth recording because it narrows what this PR does and does not fix:

  • Through the entry point the action actually uses, main already scopes correctly when GITHUB_BASE_REF is set and the checkout is deep — on a workspace git is willing to read. Inside the container it never was.
  • Through Config(load_config_from_env()), main scans the whole repository for every value of changed_files, silently. That is the config-path gap this branch opened with.

Separately, and worth checking on its own: the v2.1.0 tag declares the changed_files input but pins the 2.0.3 image, and that image has no INPUT_CHANGED_FILES handling at all — only the --changed-files CLI flag, which the container entrypoint never passes. So on @v2.1.0 the input is accepted and dropped, the whole repository is scanned, and auto, pr and an explicit list all behave identically because none of them are read. Anyone pinned there needs @v2.2.1 or later; no code change reaches them until they move.

scripts/sync_release_version.py --check would catch an action.yml pin that disagrees with pyproject.toml, but it cannot catch this one: v2.1.0's pyproject.toml also still said 2.0.3, so the tag name was the only thing that disagreed and nothing compares a tag to the version it claims to be. Closing that gap is a separate change.

@jdalton

Copy link
Copy Markdown
Contributor Author

bugbot run

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 946cac4. Configure here.

@jdalton

Copy link
Copy Markdown
Contributor Author

bugbot run

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 2be2e9e. Configure here.

@jdalton

Copy link
Copy Markdown
Contributor Author

bugbot run

Comment thread socket_basics/core/config.py
When both the plain and the trusting probe fail, _git_command_for falls
back to plain git, so the warning claiming the directory is trusted was
wrong for that path. It now says the retry was attempted and that the
problem is more than an ownership mismatch, which is true for the only
way that branch is now reachable.

Splits the trusting command out of the decision so the "workspace only,
never a wildcard" property can be asserted directly. It could not be
before: a global safe.directory entry makes git ignore
GIT_TEST_ASSUME_DIFFERENT_OWNER, the hosted runners have one, and the
test asserting a trusting command was therefore red in CI and green
locally. The tests that need git to actually be refusing now skip with a
reason instead of passing vacuously.
@jdalton

Copy link
Copy Markdown
Contributor Author

bugbot run

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 6451e30. Configure here.

@lelia lelia self-assigned this Aug 5, 2026

@lelia lelia left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes for two remaining changed-file scoping gaps. I reproduced both locally: Trivy scans a configured Dockerfile when the only changed file is unrelated, and TruffleHog is invoked with a deleted/nonexistent changed path even though the shared target resolver found nothing. The full test suite currently passes, so please add regression coverage for both cases.

changed_files = self.config.get('changed_files', []) if hasattr(self.config, '_config') else []
# Fallback: attempt to detect staged changed files if none present
if not changed_files:
if not changed_files and not self._changed_files_scope_requested():

@lelia lelia Aug 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suppressing the staged fallback here is not enough when changed_files is non-empty but contains no Dockerfile. In that case changed_dockerfiles later remains empty, the configured dockerfiles list is left unchanged, and Trivy scans those unchanged files. I reproduced this with changed_files=['app.py'] and dockerfiles='Dockerfile'. When a changed-files scope is active, replace/intersect dockerfiles unconditionally and skip if no Dockerfiles remain; please add a regression test for this case.

# substitute a different scope -- honor the empty result and skip.
changed_files = self.config.get('changed_files', []) if hasattr(self.config, '_config') else []
if not changed_files:
if not changed_files and not self._changed_files_scope_requested():

@lelia lelia Aug 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This guard prevents staged-file substitution when the resolved list is empty, but a non-empty list containing deleted or missing paths still bypasses the already-resolved targets below and passes the raw nonexistent path to TruffleHog. I reproduced trufflehog filesystem .../gone.py with changed_files=['gone.py']. Filter changed-file candidates for existence or reuse a changed-file-specific resolved target list so this skips cleanly, and add regression coverage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants