ci: scope the ThreatCrush PR comment to the files the PR changes - #409
Merged
ralyodio merged 1 commit intoAug 16, 2026
Merged
Conversation
Copy link
Copy Markdown
Contributor
Sorry, something went wrong.
The scan runs against the whole tree and the comment reported the whole tree, so every pull request got the same table regardless of what it touched. #408 changes five files under the games code and drew 53 rows about `install.sh`, the service worker, the DNS client and the Moshpit SQL helpers — nothing it went near. A comment that says the same thing on every PR says nothing. Reviewers learn to scroll past it, and the finding that *is* theirs scrolls past with it. The tree is still scanned in full and the full SARIF still goes to the Security tab, so coverage is unchanged; only what the comment talks about is narrower. The findings outside the diff are counted and pointed at rather than dropped, so "no findings" can never be misread as "the repository is clean". Scoping fails *open*: if the changed-file list cannot be read, every finding is shown and the header says so. That is the opposite of the rest of this workflow, which treats an unknown as NOT RUN — the risk there is claiming a clean scan, and the risk here is hiding a real finding behind an empty scope. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
ThreatCrush Security Scan0 finding(s) in the files this PR changes No findings in the files this PR changes. 53 further finding(s) elsewhere in the repository are not shown: they are pre-existing and untouched by this PR. The full tree was scanned — see the Security tab. |
Sorry, something went wrong.
ralyodio
added a commit
that referenced
this pull request
Aug 16, 2026
…ix (#412) #409 scoped the PR comment to the files under review by hand. It was the right change and it is already obsolete: the workflow carries a `# Managed by sh1pt Actions Fleet` header, `planDiff` classifies a drifted managed file as `update-managed` rather than a conflict, and the next `sh1pt actions` sync would have overwritten it without saying so. This repo was pinned at pack 1.1.0 — four minor versions behind. Pack 1.7.0 solves the same problem, better. It reads the diff from the merge ref instead of the API: refs/pull/N/merge has the base branch as its first parent and the PR head as its second, so HEAD^1..HEAD is exactly this PR's diff — no API call, no token, and no `pull-requests: read` question. It also handles a case #409 did not: when a PR has conflicts GitHub cannot produce a merge ref, checkout falls back to the head commit, and `HEAD^1` silently becomes "the previous commit on this branch" — a plausible answer to a different question. The pack verifies the commit really has two parents before trusting it, and falls back to reporting everything unscoped rather than scoping to the wrong set. Its report is better too: pre-existing findings go into a collapsed `<details>` block rather than only being counted, so they stay one click away instead of out of reach. Everything else in 1.7.0 comes along: the CLI is pinned to 0.11.2 with a subresource-integrity hash that is verified before install (rather than `@latest`, unverified), third-party actions are pinned to commit SHAs, checkout runs with `persist-credentials: false`, and permissions are computed from which outputs are enabled. Native SARIF makes the compatibility converter dead code, so .github/threatcrush-to-sarif.py goes with it — the old workflow was its only caller. Verified by extracting the rendered report script and running it against the real 53-finding SARIF from run 31942492400: - five games files changed -> "0 finding(s) in the 5 file(s) this pull request changes", 53 folded into the details block - a PR touching apps/pwa/src/moshpit.mjs -> 16 findings shown, which matches what #409 measured independently - no merge ref -> "53 finding(s)", unscoped, as designed - scan did not complete -> NOT RUN, unchanged Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#408 changes five files under the games code. Its ThreatCrush comment is a 53-row table about install.sh, the service worker, the DNS client and the Moshpit SQL helpers — nothing the PR went near.
That is not a #408 problem. The workflow scans . and the comment reports everything the scan returned, so every PR gets the same table. A comment that says the same thing every time says nothing: reviewers learn to scroll past it, and the finding that actually is theirs scrolls past with it.
What changed
The comment now reports the files the PR touches. Findings outside the diff are counted and pointed at, not dropped:
The tree is still scanned in full and the full SARIF still uploads to the Security tab. Coverage is unchanged — only the comment's scope is narrower. --fail-on is empty as before, so nothing about pass/fail moves either.
Scoping fails open. If the changed-file list can't be read, every finding renders and the header says repository-wide — the changed-file list was unavailable, so nothing could be scoped out. That is deliberately the opposite of the rest of this workflow, which treats an unknown as NOT RUN: there the risk is claiming a clean scan, here it is hiding a real finding behind an empty scope.
The file list is paginated (listFiles caps at 100/page) — a truncated list would silently mark real findings out-of-scope. Removed files are dropped, since the scan never saw them.
Verified
The report script was extracted verbatim from the workflow and run against the real 53-finding SARIF from run 31942143248 and #408's real file list:
Two things this does not fix
The 53 findings are ~all false positives. I checked every HIGH and sampled the MEDIUMs; I did not find a true positive:
Those are ThreatCrush rule bugs and belong in @profullstack/threatcrush (0.11.2). Scoping the comment stops them landing on unrelated PRs; it does not make them right, and they will still fire the moment someone edits one of those files.
This file is fleet-managed. The header says pack: threatcrush-scan@1.1.0, and planDiff classifies a drifted managed file as update-managed — a future sh1pt actions sync overwrites it rather than flagging a conflict. The hash was already stale from #320. This change should be folded into the pack so it survives the next sync.
🤖 Generated with Claude Code