fix(review): grade the three recurring container defects the same way in every PR - #870
Conversation
… in every PR A round of dogfood scoring read 65 findings over twelve pull requests against the code. 62 were true, so precision was not the problem; the severity attached to them was. The same unpinned base image drew medium on four of those pull requests and low on three, with nothing in the changes to tell them apart. A root-owned volume under a non-root USER drew low on the pull request where the container dies on its first write and medium on another of the same shape. The same omission drew a confident inline finding in one half of a paired-language change and a low-confidence collapsed item in the other, on evidence equally provable from the diff in both. A severity that moves with the review rather than with the defect teaches a reader that the field carries no information, and unlike a wrong finding it does not correct itself. Three classes now carry a stated grade, decided by what the defect costs and who it reaches rather than by an impression of it: a mutable external reference is medium, a path the running user cannot write is high, a container that never drops privilege is medium, each at medium confidence. The review prompt states them, and SeverityCalibrator applies them after the finding verifier, so a downgrade there cannot re-spread the class. The stage regrades only a finding anchored in a Dockerfile, manifest, compose file, workflow or Terraform file that states the class in its own words, and leaves alone any finding that also asserts something the class does not cover: a privileged container, a host mount or namespace, an added capability, a credential, a named CVE. Nothing is dropped, added or reworded, so the finding set is the one the review produced. Medium confidence puts every anchored finding on the diff rather than in the collapsed double-check block, and keeps it from requesting changes on its own under the default blocking strictness. Confidence is the half no deterministic stage can decide, so the prompt carries the mirror of the severity rule for it: equally provable defects get equal confidence, and an unequal one must name the fact it could not check.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
🤖 ThrillhouseBot PR SummaryWhat this PR doesAdds SeverityCalibrator, a post-verifier stage in FindingPipeline.refine that pins risk and confidence for three recurring container defect classes (unpinned reference, unreadable runtime path, missing privilege drop), states the same anchors in the review prompt, adds a confidence-equivalence rule, and recounts the summary so the persisted grade matches the published one. Description vs. ImplementationNo mismatch found between the PR description and the change. Control-Flow Diagram🔀 Show diagramflowchart TD
V["Verifier output - risk possibly lowered"] --> C["SeverityCalibrator.calibrate"]
C --> E{"findings empty or all at anchor"}
E -->|"yes"| SAME["return same ReviewResponse"]
E -->|"no"| CL["classify each finding by file and title + description"]
CL --> ESC{"asserts escalation beyond the class"}
ESC -->|"yes"| KEEP["keep the review grade"]
ESC -->|"no"| G["pin anchored risk and medium confidence - log regrade"]
KEEP --> CH{"any finding regraded"}
G --> CH
CH -->|"no"| SAME
CH -->|"yes"| R["recount summary from regraded findings"]
R --> DD["dropRepliedDuplicates, then persist"]
Changes Overview
Changed Files
Risk Assessment
Key Findings
Things to double-check3 lower-confidence findings
Automated review by ThrillhouseBot. Reply with |
![thrillhousebot[bot]](https://avatars.githubusercontent.com/in/3996985?s=60&v=4){kind=small}
There was a problem hiding this comment.
ThrillhouseBot noted 3 lower-confidence item(s) under Things to double-check in the PR summary (not posted as inline threads):
- MEDIUM: NEVER_DROPS_PRIVILEGE matches a bare runAsUser/runAsNonRoot mention, pinning non-member findings (
src/main/java/dev/thiagogonzaga/thrillhousebot/review/SeverityCalibrator.java:156)
Heuristic edge probe. The last alternation of NEVER_DROPS_PRIVILEGE is\\brunAsNonRoot\\b|\\brunAsUser\\b(line 156), and the consume site isreturn NEVER_DROPS_PRIVILEGE.matcher(text).find() ? InfrastructureClass.MISSING_PRIVILEGE_DROP : null;— presence of the bare word alone anchors the never-drops-privilege class. The pattern is polarity-blind, and a manifest finding can mention these fields while asserting the OPPOSITE claim. Probe (input not in the diff): filedeploy/k8s/api.yaml, description "The securityContext sets runAsUser: 1000, but the service needs to bind port 80, so startup fails." — INFRASTRUCTURE_FILE matches, no escalation word appears, namesNonRootUser is false so the unwritable branch is skipped, and the bare\\brunAsUser\\bmatches, so the finding is pinned to medium/medium as MISSING_PRIVILEGE_DROP even though the container in that finding DOES run as uid 1000 (privilege is dropped — the opposite of the class) and the real defect is a port binding, which the review may have graded high/high. Second probe: "runAsNonRoot: true is set, but readOnlyRootFilesystem is missing, so the container can be tampered with." — same regrade. This wording is ordinary for Kubernetes-manifest findings (NON_ROOT_USER itself matches on \brunAsNonRoot\b, so the class vocabulary is expected in this domain), and it is a false positive of the very stage this PR adds — the calibrator's own javadoc states over-firing "would restate a different defect at this one's level". No in-diff test covers this phrasing: every test asserting the privilege-drop class states a genuine omission ("no USER directive", "runs as root"). Suggested fix below keeps the genuine cases green ("leaves runAsNonRoot unset, so the container runs as root" still anchors via the guarded runAsNonRoot term or via "runs as root"); note it also stops anchoring a barerunAsUsermention that never says "runs as root" — a real but rare under-fire direction. - LOW: Infrastructure file pattern matches any path segment containing 'dockerfile', not only the artifact (
src/main/java/dev/thiagogonzaga/thrillhousebot/review/SeverityCalibrator.java:107)
INFRASTRUCTURE_FILE's first alternative,(?:^|/)[^/]*(?:dockerfile|containerfile)[^/]*$(line 107 in the source text), requires only that ONE path segment CONTAIN the word, with[^/]*freely absorbing a trailing extension. Inputs not in the diff:src/main/java/dev/app/DockerfileSupport.java("dockerfile" + "Support.java") anddocs/Dockerfile-guide.mdboth match, though neither is a Dockerfile or Containerfile. A review finding anchored in such a file that also states the unpinned-reference vocabulary ("unpinned ... tag") would then be treated as if anchored in a declarative deployment artifact and regraded — the calibrator's javadoc says recognition requires "a declarative deployment artifact", which this rule over-delivers. The tightening below keeps every spelling the javadoc names (Dockerfile, Dockerfile.prod, prod.dockerfile, Containerfile) while rejecting a second extension after the optional first one; note it also narrows hyphenated variants like "dockerfile-backup" that the old rule accepted. All in-diff tests keep passing ("Dockerfile" and "rust/Dockerfile" still match; "src/main/java/dev/example/Builder.java" never matched). - LOW: External-reference subject check is satisfied by the bare word 'tag' or 'digest' in any workflow finding (
src/main/java/dev/thiagogonzaga/thrillhousebot/review/SeverityCalibrator.java:122)
EXTERNAL_REFERENCE_SUBJECT (line 122) matches the bare words "base image | image | chart | action | digest | tag" anywhere in title+description, and UNPINNED_REFERENCE matches the generic "not pinned". Together they anchor any workflow/manifest finding in which the two words happen to co-occur. Probe (input not in the diff): file.github/workflows/deploy.yml, description "The release job's image output tag is not pinned to the run id, so two concurrent runs can collide." — "not pinned" satisfies UNPINNED_REFERENCE and the word "tag" (inside "output tag") satisfies the subject check, so a naming/collision nit in a workflow is regraded as MUTABLE_EXTERNAL_REFERENCE (medium/medium) and, at medium confidence, moves from the collapsed block to an inline thread while asserting nothing about base-image/chart pinning — which the calibrator's javadoc says over-firing must not do. The in-diff negative test anUnpinnedSomethingElseInAWorkflowIsNotAnchored passes only because its cache-key text contains none of the six subject words; the guard is one generic word deep, and workflow findings routinely discuss "tags" and "digests". Direction to verify with the corpus: require the subject word to denote the artifact whose pinning is the defect (e.g. "image"/"chart"/"action" adjacent to the pinning claim, or exclude bare "tag" unless "image|chart|action" also appears).
thrillhousebot
Bot
added
bug
…s actually asserts
Review of the first commit found three ways a trigger could fire on a
finding that is not the class, and each is a real over-fire of the stage
this PR adds.
A manifest field name carries no polarity: runAsNonRoot and runAsUser are
written the same way by a finding that says the field is missing and by
one that says it is set, so "runAsUser: 1000 is set, but the service
cannot bind port 80" was regraded as the privilege-drop class it is the
opposite of. Neither field name is a trigger any more; a finding that
means either container class says so in prose, and the genuine manifest
case ("leaves runAsNonRoot unset, so the container runs as root") still
anchors on that prose.
Being named after the artifact is not being the artifact. The file test
now matches a whole dot-separated segment of the file's name, so
Dockerfile, Dockerfile.prod, prod.dockerfile and Containerfile are the
artifact while DockerfileSupport.java and Dockerfile-guide.md are source
and documentation. This also drops the regex the file test used, whose
two unbounded runs around the name backtracked over every segment of a
deep path.
Pinning is said of many things in a workflow. The reference trigger asked
only for the word "pinned" and a noun as generic as "tag", so "the image
output tag is not pinned to the run id" — a push-collision nit — was
regraded as the unpinned-base-image class. Every alternative now names
the immutable thing that is missing (a digest, a tag that does not move),
and the subject list no longer accepts the bare words the claim is
already made of.
The three probes are pinned as tests, and so are the two directions the
narrowing had to keep: a root-owned path under a non-root pod is still
the ownership class, and a non-root image whose base is unpinned is still
the reference class.
The regex rework also brings each trigger under the alternation
complexity the analyzer allows, as one flat list of phrases rather than
nested groups of synonyms.
|
@thrillhousebot resolved src/main/java/dev/thiagogonzaga/thrillhousebot/review/SeverityCalibrator.java:156 — NEVER_DROPS_PRIVILEGE matches a bare runAsUser/runAsNonRoot mention, pinning non-member findings Right, and the probe is the point: the field name carries no polarity. Fixed in d960709 by removing both field names from the trigger rather than guarding them. A proximity or negation guard would have had to tell "runAsNonRoot is unset" from "runAsNonRoot: true is set, but readOnlyRootFilesystem is missing" — the second probe — and the word that decides it sits at an arbitrary distance. A finding that means either container class says so in prose, so the trigger now asks for that: The under-fire you named is accepted and is the safe direction: a manifest finding that says only Both probes are pinned as |
|
@thrillhousebot resolved src/main/java/dev/thiagogonzaga/thrillhousebot/review/SeverityCalibrator.java:107 — Infrastructure file pattern matches any path segment containing 'dockerfile', not only the artifact Right. Fixed in d960709. The file test now takes the file's own name and matches a whole dot-separated segment of it against The same change removes the expression entirely, which also settles the super-linear backtracking SonarCloud reported on it: two unbounded runs around the name had to be re-tried at every segment of a deep path. |
|
The next review will close every previous finding this comment names by its |
|
@thrillhousebot resolved src/main/java/dev/thiagogonzaga/thrillhousebot/review/SeverityCalibrator.java:122 — External-reference subject check is satisfied by the bare word 'tag' or 'digest' in any workflow finding Right, and your probe would have gone through: Fixed in d960709, on the claim rather than only on the subject, because your probe carries the word Your probe is pinned as |
|
The next review will close every previous finding this comment names by its |
1 similar comment
|
The next review will close every previous finding this comment names by its |
🤖 ThrillhouseBot — changes since the last review
|
There was a problem hiding this comment.
ThrillhouseBot noted 1 lower-confidence item(s) under Things to double-check in the PR summary (not posted as inline threads):
- LOW: UNPINNED_REFERENCE alternative reads 'blacks a digest' — dead match, 'lacks a digest' never anchors (
src/main/java/dev/thiagogonzaga/thrillhousebot/review/SeverityCalibrator.java:106)
The unpinned-reference alternation contains|\\bblacks\\s+a\\s+digest\\b, which matches only the literal English phrase "blacks a digest" — a phrase no review finding about base-image pinning would naturally contain. Beside the neighbours in the same alternation ("no digest", "carries no digest", "pin it by digest"), the intended phrasing is evidently "lacks a digest" (\\bblack+s...is a one-extra-character misspelling of\\black). Input not in the diff: a finding on a Dockerfile whose description reads "The base image rust:1.81 lacks a digest, so the build is not reproducible." — UNPINNED_REFERENCE then finds no alternative (no 'unpinned', 'no digest', 'carries no digest', etc. appear) and the finding is left at the review's own grade instead of the anchored medium/medium, even though EXTERNAL_REFERENCE_SUBJECT would have matched 'base image'. This is the under-firing direction the class javadoc itself classifies as the benign half of the trade ('Under-firing costs a finding the calibration it should have had, which is where this class already stood'), so it is a low-risk defect — but it is a silently dead clause in a rule whose whole purpose is determinism, and none of the in-diff tests exercise the 'lacks a digest' phrasing. Verify with the corpus probe above.
ThrillhouseBot closed 3 previous finding(s) this round:
src/main/java/dev/thiagogonzaga/thrillhousebot/review/SeverityCalibrator.java:156— NEVER_DROPS_PRIVILEGE matches a bare runAsUser/runAsNonRoot mention, pinning non-member findingssrc/main/java/dev/thiagogonzaga/thrillhousebot/review/SeverityCalibrator.java:107— Infrastructure file pattern matches any path segment containing 'dockerfile', not only the artifactsrc/main/java/dev/thiagogonzaga/thrillhousebot/review/SeverityCalibrator.java:122— External-reference subject check is satisfied by the bare word 'tag' or 'digest' in any workflow finding
…er than alternations Each trigger was one expression per class, and its cost grows with every synonym and every whitespace run in it: the reference trigger reached an alternation complexity of 77 against the 20 the analyzer allows, and the other two were over as well. Splitting them further would have traded one unreadable expression for three. The triggers are now lists of phrases matched against the finding's own words, lower-cased and reduced to single-space-separated words first. A phrase carries its own boundaries, so "privileged" still does not match inside "unprivileged" and "image" does not match inside "images", and a list of phrases says what the class is made of more plainly than an alternation did. Two claims stay off that path and say why: the USER directive is read case-sensitively on the raw text, since the Dockerfile instruction is upper case and ordinary English about users is not, and ":latest" is read on the raw text because normalization drops the colon that makes it a reference. Behaviour is unchanged for every case the tests pin, and two more are pinned here: ":latest" is the same class as a missing digest, and a contraction states the same claim as the words it contracts.
🤖 ThrillhouseBot — changes since the last review
|
|
/review |
🤖 ThrillhouseBot — changes since the last review
|
…tion
Review of the phrase lists found four ways a finding could be sorted into
a class its own words contradict, and all four are the same mistake: a
word that appears in the class's vocabulary without carrying the claim.
A pinning claim now has to be about the reference. "Unpinned" on its own,
beside any incidental "image" elsewhere in the finding, anchored a
package-install nit as the base-image class; the phrases bind the two
together ("unpinned base image", "image is unpinned") or name the digest
that is missing.
An ownership claim now has to say which direction it is. "Chown" and
"ownership" are said as often of a chown that is present or redundant as
of one that is missing, so a layer-size nit about an existing RUN chown
was graded as a path nobody can write — at high, and blocking under
strict. The phrases name the absence instead.
The privilege-drop claim is read before the ownership one, because it
denies that class's premise: a container that runs as root has no
non-root user for a root-owned path to be unwritable by. "There is no
USER appuser directive, so the app runs as root and the files it writes
take root ownership" names an account and an ownership in one sentence,
and the lookahead that kept "no USER directive" out could not keep that
one out.
The escalation defeater is read on the words a finding uses, not only on
the manifest field spellings: "host pid namespace", "host mount" and the
singular "capability" now defeat it, since missing one pins a finding
DOWN whose level rests on the escalation.
The changelog's verdict sentence was also too broad. A pin is not only an
upgrade: a finding the review over-graded at critical with high
confidence stops requesting changes, and the entry now says that instead
of claiming no request changes at all.
🤖 ThrillhouseBot — changes since the last review
|
There was a problem hiding this comment.
ThrillhouseBot noted 1 lower-confidence item(s) under Things to double-check in the PR summary (not posted as inline threads):
- MEDIUM: USER directive lookahead misses negation prose, sorting privilege-drop omissions into the ownership class (
src/main/java/dev/thiagogonzaga/thrillhousebot/review/SeverityCalibrator.java:196)
The new rule at line 196 is "\bUSER\s+(?!(?:directive|instruction|line|statement|declaration)\b)[A-Za-z0-9_$.:-]+" and its javadoc states its purpose: 'The lookahead ... keeps the prose that talks ABOUT the instruction ("no USER directive") from reading as one that is present, which would otherwise sort a privilege-drop omission into the ownership class beside it.' The blocklist covers only five prose nouns. An omission that names the intended account WITHOUT any of those nouns and WITHOUT a 'runs as root' phrase defeats the guard. Input not in the diff, on a finding the design's own tests show is in this rule's expected domain (cf. anOmissionThatNamesTheIntendedAccountIsStillThePrivilegeDropClass, which needs both 'directive' and 'runs as root' to pass): file Dockerfile, description 'There is no USER appuser in this Dockerfile; the /data volume stays root-owned, so the first write fails.' Trace: ESCALATION_BEYOND_CLASS no match; NEVER_DROPS_PRIVILEGE no match ('no user directive' and 'runs as root' both absent); namesNonRootUser true because USER_DIRECTIVE matches 'USER appuser' ('in' passes the lookahead); UNWRITABLE_PATH matches 'root owned' and 'write fails' -> UNWRITABLE_RUNTIME_PATH, regraded high/medium. classify's own comment says this exact shape must be MISSING_PRIVILEGE_DROP ('a container that never drops privilege has no non-root user for a root-owned path to be unwritable by'), so the finding is published under a class label its own words contradict, at the wrong risk (medium -> high, an over-fire the design's 'pins rather than floors' rule forbids in the other direction). No in-diff test exercises this input: the omission test's wording deliberately contains both 'directive' and 'runs as root', so it still passes. Under-firing (finding 2's direction) is the side this design accepts; this is the over-fire side.
ThrillhouseBot closed 1 previous finding(s) this round:
src/main/java/dev/thiagogonzaga/thrillhousebot/review/SeverityCalibrator.java:236— Escalation-vocabulary gaps let escalated findings be pinned down instead of keeping their grade
|
/review |
🤖 ThrillhouseBot — changes since the last review
|
…ontainer classes A phrase carries its own word boundaries, but "non-root user" normalizes to three words and the privilege-drop list held "root user", which sits inside them. So a finding that states the ownership defect in the clearest way there is — "the image runs as a non-root user, but /data stays root-owned" — was read as the class that says no non-root user exists, and pinned to medium under a label its own words deny. The explicit non-root claim now decides: a finding that says the container does run as a non-root user is never the privilege-drop class, however the rest of the sentence reads. A USER token cannot decide it, since a finding writes "no USER appuser directive" with one, which is why the check is on the prose claim rather than on the token. "root user" is off the privilege list as well; "runs as root" carries the same finding without reaching inside a word that denies it. The escalation list gains "password" and "passwords". A committed password is the usual way a finding states a committed credential, and missing the spelling pins such a finding DOWN — the direction the design forbids, since its level rests on the credential rather than on anything an anchor can weigh.
🤖 ThrillhouseBot — changes since the last review
|
There was a problem hiding this comment.
ThrillhouseBot noted 1 lower-confidence item(s) under Things to double-check in the PR summary (not posted as inline threads):
- LOW: :latest anchor is discarded when the description omits the words image/chart/action (
src/main/java/dev/thiagogonzaga/thrillhousebot/review/SeverityCalibrator.java:372)
namesUnpinnedReference adds a raw-text special case for the ':latest' spelling (LATEST_TAG is read on the raw text because, per its own comment, 'normalization drops the colon that makes {@code :latest} a reference rather than the ordinary English word'), which shows the intent to anchor that spelling. But classify's conjunction 'return namesUnpinnedReference(text, raw) && states(text, EXTERNAL_REFERENCE_SUBJECT)' then requires the literal words image/images/chart/action/base image anywhere in the finding. Input not in the diff: a Dockerfile finding reading 'FROM alpine:latest — the tag drifts under the build.' matches no UNPINNED_REFERENCE phrase ('floating tag'/'mutable tag'/'moving tag'/'latest tag' are absent) and contains none of the four subject words, so the anchor silently misses and the finding keeps the review's grade — the exact inconsistency the PR exists to remove — while the same defect phrased '...alpine:latest base image...' pins to medium/medium. Worth verifying in CI with that probe; a fix would treat the ':latest' occurrence itself as naming the reference (it is Docker tag syntax, not prose), or extend EXTERNAL_REFERENCE_SUBJECT.
ThrillhouseBot closed 1 previous finding(s) this round:
src/main/java/dev/thiagogonzaga/thrillhousebot/review/SeverityCalibrator.java:376— 'root user' phrase matches inside 'non-root user', misgrading ownership findings as privilege drop
|
/review |
There was a problem hiding this comment.
No new issues in this revision, but 2 previous finding(s) remain unresolved — fix them, or reply on their review thread with why they are deferred. A finding listed only under "Things to double-check" has no thread: clear it by commenting @thrillhousebot resolved path/to/File.java:42 — <the finding's title> on this PR.
…ile type The whole-segment rule excluded DockerfileSupport.java and Dockerfile-guide.md, but not Dockerfile.md or Containerfile.kt: those carry the artifact's name as a segment of their own, so a finding about an example inside a documentation page was graded as a finding about the image that page describes. A Dockerfile has no file type — its name is the type — so a terminal extension is what separates Dockerfile.prod, which is the artifact, from Dockerfile.md, which is a page about it. The name now has to carry the artifact's name as a whole segment and end in something other than a source or documentation type. The list is the types a repository actually writes those in. A name outside it costs a finding its calibration rather than giving a document one, which is the direction to miss in.
🤖 ThrillhouseBot — changes since the last review
|
There was a problem hiding this comment.
ThrillhouseBot found no issues in this PR, but some checks are still pending or failed:
- Check test is pending
Additionally, No new issues in this revision, but 1 previous finding(s) remain unresolved — fix them, or reply on their review thread with why they are deferred. A finding listed only under "Things to double-check" has no thread: clear it by commenting @thrillhousebot resolved path/to/File.java:42 — <the finding's title> on this PR.
ThrillhouseBot closed 1 previous finding(s) this round:
src/main/java/dev/thiagogonzaga/thrillhousebot/review/SeverityCalibrator.java:379— Dot-segment rule anchors Dockerfile.md/Containerfile.java, contradicting the javadoc's exclusion claim
|
/review |
🤖 ThrillhouseBot — changes since the last review
|
There was a problem hiding this comment.
ThrillhouseBot noted 1 lower-confidence item(s) under Things to double-check in the PR summary (not posted as inline threads):
- LOW: Privilege-drop anchor misses the 'runs as the root user' spelling (article between 'as' and 'root') (
src/main/java/dev/thiagogonzaga/thrillhousebot/review/SeverityCalibrator.java:261)
Heuristic probe, input not in the diff. The list is quoted: 'runs as root', 'run as root', 'running as root', 'no user directive', 'no user instruction', 'missing user directive', 'missing user instruction', 'without a user directive', 'never adds a user', 'never drops privilege', 'never drops privileges', 'does not drop privilege', 'does not drop privileges', 'doesnt drop privileges'. Every phrase requires the words 'as' and 'root' (or 'user') to be adjacent after normalization, and USER_DIRECTIVE only fires on a literal 'USER ' that is not followed by a blocked noun. A finding worded 'The image runs as the root user in the final stage.' (or 'running as the root user') on a Dockerfile, or a missing-directive omission phrased 'no USER account is defined anywhere in this Dockerfile' (the lookahead blocks only directive/instruction/line/statement/declaration, so 'no USER account ...' would actually fire namesNonRootUser, but the privilege phrases still miss it), matches no phrase, so classify returns null and the finding keeps the review's grade — the same spread this calibration exists to remove, re-introduced by one article or one synonym ('entry', 'statement' is blocked but 'account'/'entry' are not). The corpus wordings pinned by this PR's own tests ('so the published service runs as root', 'the container runs as root user') are all in the adjacent register a reviewer model writes in, so the domain is visible, but the probe's actual word choice is my construction. Verification request: run 'The image runs as the root user in the final stage.' against SeverityCalibrator.calibrate in CI and decide whether article-tolerant phrasings belong on the list. The class javadoc states narrowness is the safe direction (under-firing costs only the calibration, the level stays where the review put it), which is why this is low rather than a blocker — but it is the same defect class as the open ':latest' finding from the previous round.
…tween the words Two wordings reached the end of classify and kept the spread this calibration exists to remove. ":latest" was read as a pinning claim that still needed one of the words image, chart or action beside it, so "FROM alpine:latest — the tag drifts under the build" matched nothing. The colon is Docker tag syntax: the occurrence IS the reference being named, and it now anchors on its own, while every prose claim still needs the subject it is about. "The image runs as the root user" matched no privilege-drop phrase because every one of them put "as" and "root" next to each other. The article forms are on the list. Both are the under-firing direction, where a finding keeps the grade the review gave it, and both are one word away from wordings the corpus does use.
|
@thrillhousebot resolved src/main/java/dev/thiagogonzaga/thrillhousebot/review/SeverityCalibrator.java:372 — :latest anchor is discarded when the description omits the words image/chart/action Right: the raw-text special case proved the spelling was meant to anchor, and the conjunction then threw it away unless the finding also used one of four prose words. Fixed in ce7f7aa along the first direction you named. Every prose claim still needs its subject, so the package-install and cache-key probes from the earlier rounds stay unanchored. Your probe is pinned as |
|
@thrillhousebot resolved src/main/java/dev/thiagogonzaga/thrillhousebot/review/SeverityCalibrator.java:261 — Privilege-drop anchor misses the 'runs as the root user' spelling (article between 'as' and 'root') Right, and the article is the cheapest of the misses: every phrase on the list put "as" and "root" next to each other. Fixed in ce7f7aa: The other half of your probe — "no USER account is defined anywhere" — is left as it is on purpose. Adding |
|
The next review will close every previous finding this comment names by its |
1 similar comment
|
The next review will close every previous finding this comment names by its |
🤖 ThrillhouseBot — changes since the last review
|
There was a problem hiding this comment.
ThrillhouseBot found no issues in this PR, but some checks are still pending or failed:
- Check test is pending
Additionally, No new issues in this revision, but 1 previous finding(s) remain unresolved — fix them, or reply on their review thread with why they are deferred. A finding listed only under "Things to double-check" has no thread: clear it by commenting @thrillhousebot resolved path/to/File.java:42 — <the finding's title> on this PR.
ThrillhouseBot closed 1 previous finding(s) this round:
src/main/java/dev/thiagogonzaga/thrillhousebot/review/SeverityCalibrator.java:261— Privilege-drop anchor misses the 'runs as the root user' spelling (article between 'as' and 'root')
|
|
/review |
1 participant
What type of PR is this?
Description
Round-8 scoring read 65 findings over twelve pull requests against the code: 62 true positives, so precision was not the problem. The grading was. The same defect, in the same round, from the same reviewer, landed at different severities depending only on which pull request it appeared in.
What the code actually does with
riskandconfidenceVerified before changing any wording:
PrReviewPrompts.SYSTEM— the four severity levels, the confidence calibration block, and a bullet that already said "Equivalent defects get equivalent severity" (Security severity is inconsistent across equivalent defects, and drifts downward #570). There was no equivalent bullet for confidence, and no level stated for any specific recurring class.RiskLevel.fromString/Confidence.fromStringparse the two fields, both defaulting permissively (an unrecognized risk isLOW, an absent confidence isHIGH).Finding.postsInline()is the inline-versus-double-check split:confidence != LOW || risk == CRITICAL || risk == HIGH. So a medium finding at low confidence never opens a thread on the diff —ReviewPublisherline 743 andReviewResultlines 353/364 route it into the collapsed "Things to double-check" list instead.VerdictBuildercounts the levels, takes the highest, and hands the findings toReviewState.fromFindings(outstanding, blockingStrictness);BlockingStrictness.isBlockingisseverityQualifies(risk) && (STRICT || confidence == HIGH). Under the shippedBALANCEDthat is critical/high and high confidence.FindingVerificationService.floorInjectionSinkRisk(Security severity is inconsistent across equivalent defects, and drifts downward #570), applied to the audit's output because the verifier can lower risk and a prompt-side floor alone did not hold.So the grading is decided in three places — the prompt, the verifier's lowering, and nothing at all after it — and the two graded fields are what decide both the surface and the verdict.
Each claim in the issue, checked
SYSTEMstates a level for this class, so it fell to per-review judgement between "medium: a real correctness or maintainability concern" and "low: rarely worth reporting". Both readings are defensible from the rubric as written, which is exactly why the grade moved.postsInline()sends a low-confidence medium finding to a collapsed block that carries no thread.One correction to the issue's framing: the severity half of the Rust/C# pair is not "partly defensible". The description in each half rests on the same in-diff shape, and the C# bound the issue credits (
int/DateTime.MaxValue) is a property of the consequence, not of the class — the grade for "unbounded request-driven work" should not move between the two, and this PR does not try to fix that class deterministically (see below).The fix
SeverityCalibratoris the deterministic component. It pinsriskandconfidencefor three defect classes whose consequence and reachability are fixed by the class rather than by the pull request:VOLUME/dir under a non-rootUSER, nochown)USER,runAsNonRootunset)It pins rather than floors: a level above the anchor is as much a miscalibration as one below, and a floor would close only the half of the observed spread that happened to point down. It runs in
FindingPipeline.refineafterfindingVerificationService.verify, because the verifier applies its verdict only in the lowering direction and would otherwise re-spread the class it just pinned — the same reason #570's floor wraps the audit rather than preceding it. The calibrated response is what gets persisted, so the grade the next round compares against is the calibrated one.Recognition needs both halves — an anchored declarative artifact (Dockerfile/Containerfile by name;
.yml/.yaml/.tf/.tfvarsby extension) and the class asserted in the finding's own words — and a finding that also asserts an escalation the class does not cover (privileged, a host mount or namespace, an added capability, a credential, a named CVE) keeps the review's own grade, because the level then rests on something an anchor cannot weigh. Under-firing costs a finding the calibration it should have had, which is where the class already stood; over-firing would restate a different defect at this one's level.The prompt states the same three anchors so the model usually lands there by itself, and carries the mirror of the existing severity bullet for confidence.
What is deterministic and what stays a model judgement. Deterministic: the risk and confidence of the three anchored classes, and therefore their surface. A model judgement, necessarily: whether a finding exists at all, everything outside those three classes, and confidence in general — the issue's Rust/C# pair is an unbounded-request-work claim in two languages, and no textual rule can tell "the same shape, graded unequally" from "two shapes that differ". That half is addressed in the prompt only: confidence answers one question, the answer cannot depend on the language the shape is written in, and an unequal rating must name the fact that could not be checked in one half and could in the other.
Effect on the verdict and the publication path
Stated explicitly because a rubric change moves both:
postsInline()is true regardless of risk, so the class stops landing on the diff in one language and in the collapsed block in the other.BALANCED, which needs high confidence — the anchors publish at medium. The pin moves the verdict in the other direction, though, and the PR body said so too loosely at first: a finding the review over-graded at critical with high confidence was blocking before and is not after, which is what a pin means and is asserted byanUnpinnedReferenceIsMediumWhateverTheReviewRatedIt. UnderLENIENT(critical only) the same downgrade applies and nothing starts blocking. UnderSTRICT, which drops the confidence gate, the unwritable-path class now qualifies at high risk, which is that mode's declared appetite: it blocks on critical/high regardless of confidence.summary.critical/high/medium/loware recomputed from the regraded findings through the existingFindingVerificationService.recount, so the summary block and the check-run counts stay consistent with the badges.everyFindingSurvivesWithItsTextAndAnchorIntactpins that.Related Issues
Fixes #773
How Has This Been Tested?
Red first. With the calibration stage reduced to a pass-through — which is the pre-fix behaviour, the grade published exactly as the review produced it — and the prompt anchors removed, the new tests fail on the issue's own evidence:
Every pairing test states the SAME defect twice — once as the pull request that drew the lower grade, once as the one that drew the higher — and asserts the two come out equal. Asserting one finding at a time would pass on a calibration that is merely consistent with itself.
Green, full suite:
Gates:
Patch coverage of the
src/maindiff againstorigin/main, line and branch:Checklist
Screenshots / Logs
Each regrade is logged at INFO with the class, the finding's anchor and both levels, so a corpus round can be scored for consistency from the logs rather than by reading badges:
Additional Notes