fix(threatcrush-scan): report the pull request, not the whole backlog - #965
Merged
Conversation
qryptchat-web#258 changed two files and got a comment with 91 findings, five of them HIGH, none of them from the diff under review. Nothing in that comment was actionable by its author, and the one finding that mattered was in the middle of seventy-six fixture matches. The scan still covers the whole tree and the Security tab still gets all of it — narrowing what is scanned would be a real loss. What changes is the comment, which is a review artifact and should be about the change under review. - Lead with findings in the files the pull request touches, and fold the rest into one <details> line carrying its severity counts. - Order by severity. The 50-row cap used to be applied in SARIF order, which is file order, so which findings survived truncation depended on where they sat in the tree: a HIGH in the last file scanned could be cut while fifty notes from the first printed in full. The diff comes from HEAD^1..HEAD. refs/pull/N/merge has the base as its first parent and the head as its second, so that range is exactly the pull request — no API call, no token, no added permission. It needs fetch-depth: 2, which checkout now asks for. A conflicted pull request has no merge ref; checkout falls back to the head commit and HEAD^1 quietly starts answering a different question. So the shape is verified before it is trusted, and a failure reports everything unscoped and says so, rather than scoping to the wrong files. Tests assert the scoping, the ordering, the unscoped fallback, and that the rendered workflow is still parseable YAML across three input variants — the report builder is a heredoc'd Python program inside a run: block, where an indentation slip produces a file GitHub refuses to load while every content assertion still passes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
vu1nz Security Review0 finding(s) in PR #? No security issues found. |
ThreatCrush Security Scan311 finding(s) HIGH/CRITICAL: 24 | MEDIUM: 51 | LOW: 236
…and 261 more. Full results in the Security tab. Snippets are redacted; ThreatCrush never prints matched credential material. |
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.
What prompted this
qryptchat-web#258 changed two files and received a ThreatCrush comment with 91 findings — 76 LOW, 10 MEDIUM, 5 HIGH — none of them from the diff under review.
I reproduced it exactly and hand-verified every HIGH and MEDIUM against the source. One of the 91 was real (a nested quantifier in a URL validator). The rest were false positives, and they're being fixed separately in profullstack/threatcrush#148.
But even with a perfect scanner, this comment would still have been wrong to post. A repository with a legacy backlog hands every author the whole backlog on every pull request. That is how a scanner teaches a team to scroll past it.
What changes
The scan still covers the whole tree, and the Security tab still receives all of it. Narrowing what gets scanned would be a real loss of coverage.
The comment is a review artifact, and a review is about the change under review. So:
<details>line carrying its severity counts, listing the twenty most serious.highin the last file scanned could be cut while fiftynotes from the first printed in full.Same repository, same scan, rendered through the new builder:
How the diff is identified
refs/pull/N/mergehas the base branch as its first parent and the pull request head as its second, soHEAD^1..HEADis exactly the pull request — no API call, no token, no added permission. It needsfetch-depth: 2, which checkout now requests.A conflicted pull request has no merge ref. Checkout falls back to the head commit, where
HEAD^1means "the previous commit on this branch" — a plausible-looking answer to a different question. The workflow verifies HEAD really has two parents before trusting the range; when it doesn't, it reports everything unscoped and says so in the log. Scoping to the wrong set of files is worse than not scoping.Verification
NOT RUNpath (all still correct).pnpm vitest run packages/actions— 84 passed, including new assertions for the scoping, the ordering, the unscoped fallback, and that the rendered workflow is valid YAML across three input variants. That last one matters: the report builder is a heredoc'd Python program inside arun:block, where an indentation slip yields a workflow GitHub refuses to load while every content assertion still passes.pnpm --filter @profullstack/sh1pt-action-packs typecheckclean.pnpm vitest runhas 662 suites failing to resolve workspacedist/in a fresh worktree. Confirmed pre-existing by stashing this branch and reproducing the identical failure on the base commit.Pack version
1.5.1→1.6.0, so consumers re-sync.Note for whoever lands this
qryptchat-web installs
@profullstack/threatcrush@latest(pack v1.1.0). npm is at0.11.1, which equals threatcrush HEAD — so merging #148 will not change that PR comment until a release is cut, and re-syncing qryptchat to pack 1.6.0 moves it onto the pinned0.11.0spec with an integrity hash.🤖 Generated with Claude Code