diff --git a/CHANGELOG.md b/CHANGELOG.md index 128c7a09..b5de8ef8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ All notable changes to ThrillhouseBot. ### Fixed +- **The three recurring container defects are graded the same way in every pull request** (#773): a round of dogfood scoring read 65 findings over twelve pull requests against the code — 62 of them true — and found the grading, not the findings, to be the problem. 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 that was 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. Three classes now carry a stated grade, decided by what the defect costs and who it reaches rather than by the review's impression of it: a mutable external reference (a base image, action or chart named by a tag instead of a digest) is medium, a path the running user cannot write is high, and a container that never drops privilege is medium, each at medium confidence. The review prompt states them, and a calibration stage applies them after the finding verifier, so a downgrade there cannot re-spread the class. It regrades only a finding anchored in a Dockerfile, manifest, compose file, workflow or Terraform file that states the class in its own words, and it 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 exactly the one the review produced. Medium confidence puts every anchored finding on the diff instead of in the collapsed "Things to double-check" block, and leaves the merge verdict to the rest of the review: under the default `REVIEW_BLOCKING_STRICTNESS=balanced`, which needs high confidence, no anchored finding requests changes on its own, and one the review over-graded at critical with high confidence stops doing so, which is the calibration working rather than a side effect of it. The prompt also states the mirror of the severity rule for confidence, which is the half no deterministic stage can decide: equally provable defects get equal confidence, and an unequal one must name the fact it could not check - **Repeated timeouts on one AI call stop spending the whole retry budget** (#862): a streaming attempt waits `THRILLHOUSEBOT_REVIEW_AI_TIMEOUT_SECONDS`, 900 seconds in production, and a timeout was then retried like any other transient failure up to `THRILLHOUSEBOT_REVIEW_MAX_AI_RETRIES`, so one call could spend 75 minutes of wall clock while its review held the pull request's dispatcher slot. Production saw 20 timed-out attempts in 24 hours, all on one 503-file pull request, and the failed reviews of that day cost more than the completed ones. At most two attempts of one call may now end at the deadline: the second one fails the call instead of spending the attempts that are left, and the decision is logged at WARN with the session id, the attempt and the wait. Every other transient failure keeps the whole budget, a timeout followed by a successful attempt still succeeds, and the reasoning step-down's repeat (#839) shares the bound rather than getting a second pair of waits. The repeat keeps the full deadline, since it is there for the attempt whose first token never arrived because the provider queued the request, and the bound already brings the ceiling down from 75 minutes to 30. The setting still means one attempt's wait. The final summary call shares the loop and behaves the same way, and a review whose batches time out still discloses the files it did not read - **A review interrupted by a restart no longer stays `in_progress` for good** (#863): a session row is written `in_progress` when the review starts and updated when it ends, so a review killed between those two writes — a deploy restart, a crash, a `docker kill` — never got the terminal one and stayed `in_progress` for the life of the database. Production had 10 such rows, the oldest from 2026-06-09 and the newest from the 2026-09-09 restart, each counting as a running review and hiding the genuinely in-flight ones among them. Startup now moves every `in_progress` row to `failed` with "Review interrupted before it finished (bot restart or crash)" as the reason, which is what tells it apart on the dashboard from a review that failed on its own. Nothing carries a review across a restart, so a row still in progress at boot belongs to a review that is over: the sweep needs no age threshold and reconciles the rows stranded before it existed, with no manual SQL. Only the status and the reason are written, so the tokens and the cost the review had already paid for stay on the row. Sweeping every row at boot is safe because the bot is a single process; running more than one replica would need the rows to carry an owner first diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipeline.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipeline.java index d875b1bd..5dec485f 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipeline.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipeline.java @@ -52,10 +52,11 @@ import org.jboss.logging.Logger; /** - * The post-AI finding chain: validate quotes, dedupe, verify against the diff, drop already-replied - * duplicates, backfill missing content anchors, and persist the response. Extracted from {@code - * ReviewOrchestrator}; the ordering is preserved verbatim — quote validation runs before dedupe so - * a merged finding cannot inherit a phantom quote while a verbatim sibling is discarded. + * The post-AI finding chain: validate quotes, dedupe, verify against the diff, calibrate the graded + * fields, drop already-replied duplicates, backfill missing content anchors, and persist the + * response. Extracted from {@code ReviewOrchestrator}; the ordering is preserved verbatim — quote + * validation runs before dedupe so a merged finding cannot inherit a phantom quote while a verbatim + * sibling is discarded. */ @ApplicationScoped public class FindingPipeline { @@ -1444,6 +1445,10 @@ private ReviewResponse refine( promptInputs.previousFindings(), located, plan::recordVerificationCoverage); + // #773: the last word on the two graded fields, so the anchored infrastructure classes cannot + // be re-spread by the verifier's own lowering, and the grade the publisher routes on is the + // one persisted below for the next round to compare against. + aiResponse = SeverityCalibrator.calibrate(aiResponse); aiResponse = followUpAnalyzer.dropRepliedDuplicates( aiResponse, ctx.priorAiResponseJsons(), ctx.inlineComments(), botIdentity); diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/SeverityCalibrator.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/SeverityCalibrator.java new file mode 100644 index 00000000..183de21b --- /dev/null +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/SeverityCalibrator.java @@ -0,0 +1,464 @@ +/* + * Copyright 2026 Thiago Gonzaga + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package dev.thiagogonzaga.thrillhousebot.review; + +import dev.thiagogonzaga.thrillhousebot.LogSafe; +import dev.thiagogonzaga.thrillhousebot.review.ai.FindingVerificationService; +import dev.thiagogonzaga.thrillhousebot.review.ai.ReviewResponse; +import io.quarkus.logging.Log; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.Locale; +import java.util.regex.Pattern; + +/** + * Deterministic grade for the three infrastructure defect classes that recur in every containerized + * repository, whatever language it is written in (#773). + * + *

A round-8 corpus of 65 findings over twelve pull requests was 97% precise and still graded + * inconsistently: the same unpinned base image drew "medium" on four pull requests and "low" on + * three, and a root-owned volume under a non-root USER drew "low" on the pull request where it + * fails first and "medium" on another of the same shape. Nothing in those pull requests + * distinguished the findings — the level moved with the review, not with the defect. A severity + * that moves that way teaches a maintainer to read every finding at the same weight, which is the + * one thing the scale exists to prevent. + * + *

The calibration rule is that severity follows the consequence and its reachability, and for + * these three classes both are fixed by the class: an unpinned reference changes the build's inputs + * without a commit and breaks nothing at run time; a path the running user cannot write fails the + * first time it is written, on every deployment; a container that never drops privilege breaks + * nothing by itself but starts a compromise as root. So the grade is a property of the class and is + * written here rather than judged per review. {@link dev.thiagogonzaga.thrillhousebot.review.ai + * .PrReviewPrompts} states the same three anchors to the model — this is what makes them hold when + * it does not follow them, the way {@code FindingVerificationService.floorInjectionSinkRisk} holds + * the injection-sink floor (#570). + * + *

Confidence is anchored too, and at "medium" for all three, because the question confidence + * answers has the same answer in every pull request: the shape is settled by the file in front of + * the reviewer, so it is never "low", and whether the deployment exercises the consequence is not + * in that file, so it is never "high". That is what fixes the surface split the same corpus + * measured — {@link Finding#postsInline} routes a "low"-confidence medium finding to the collapsed + * "Things to double-check" block, so the same omission reached the diff in one language and nowhere + * in the other. At "medium" it is always on the diff, and because blocking under the default {@link + * BlockingStrictness#BALANCED} needs "high" confidence, an anchored finding informs the verdict + * without deciding it. + * + *

The anchor pins rather than floors: a level above it is as much a miscalibration as one below, + * and a floor would close only the half of the spread that happened to point down. Nothing is + * dropped, added, re-anchored or reworded here, so the precision of the finding set is + * arithmetically unchanged — only the two graded fields move. + * + *

Recognition is deliberately narrow in the safe direction. It needs the finding to be anchored + * in a declarative deployment artifact AND to assert the class in its own words, and any finding + * that also asserts an escalation beyond the class — a privileged container, host namespaces or + * mounts, an added capability, a credential, a named CVE — is left exactly as the review graded it, + * because the level then rests on something the class does not fix. Under-firing costs a finding + * the calibration it should have had, which is where this class already stood; over-firing would + * restate a different defect at this one's level. + */ +public final class SeverityCalibrator { + + /** + * One recurring infrastructure defect class and the risk its consequence fixes. Confidence is + * {@link #ANCHORED_CONFIDENCE} for all three, so it is not carried per constant. + */ + private enum InfrastructureClass { + /** A base image, action or chart referenced by a name that can move under the build. */ + MUTABLE_EXTERNAL_REFERENCE(RiskLevel.MEDIUM), + + /** A path the image's non-root user cannot write, which fails the first write on every run. */ + UNWRITABLE_RUNTIME_PATH(RiskLevel.HIGH), + + /** A container that never drops privilege, so a compromise of it starts as root. */ + MISSING_PRIVILEGE_DROP(RiskLevel.MEDIUM); + + private final RiskLevel risk; + + InfrastructureClass(RiskLevel risk) { + this.risk = risk; + } + } + + /** + * The confidence every anchored class publishes at; see the class javadoc for why it is fixed. + */ + private static final Confidence ANCHORED_CONFIDENCE = Confidence.MEDIUM; + + /** + * The extensions that carry a manifest, a compose file or a workflow. Read against the file's own + * name rather than the whole path, together with the container-image file names below: a + * directory component is not the artifact, and one expression over the path both allows that and + * backtracks over every segment of a deep one. + */ + private static final List INFRASTRUCTURE_EXTENSIONS = + List.of(".yml", ".yaml", ".tf", ".tfvars"); + + /** + * The container-image file, matched as a whole dot-separated segment of the file's name so every + * spelling of the artifact is covered: {@code Dockerfile}, {@code Dockerfile.prod}, {@code + * prod.dockerfile}, {@code Containerfile}. + */ + private static final List CONTAINER_FILE_NAMES = List.of("dockerfile", "containerfile"); + + /** + * File types that make a name ABOUT the artifact rather than the artifact. A Dockerfile has no + * file type — its name is the type — so a terminal extension from this list says the file is + * source or documentation however the rest of the name reads: {@code DockerfileSupport.java} and + * {@code Dockerfile-guide.md} are already excluded by the whole-segment rule, while {@code + * Dockerfile.md} and {@code Containerfile.kt} are not, and only the extension tells them from + * {@code Dockerfile.prod}. The list is the types a repository actually writes these in; a name + * outside it is one nobody writes, and the miss would cost a finding its calibration rather than + * give a document one. + */ + private static final List DOCUMENTARY_EXTENSIONS = + List.of( + "md", + "markdown", + "txt", + "rst", + "adoc", + "html", + "json", + "java", + "kt", + "kts", + "js", + "ts", + "tsx", + "py", + "go", + "rs", + "rb", + "cs", + "php", + "sh"); + + /** + * Every trigger below is a list of PHRASES, matched against the finding's own words after {@link + * #normalized} reduces them to lower-case words separated by single spaces. A phrase carries its + * own word boundaries, so "privileged" does not match inside "unprivileged" and "image" does not + * match inside "images", exactly as the expressions these lists replace did — without an + * alternation whose cost grows with every synonym and whitespace run in it. + * + *

The finding says the reference is not pinned to something immutable. Every phrase names + * either the immutable thing that is missing — a digest, a tag that does not move — or the + * reference it is missing for. A bare "unpinned" is not one of them, and neither is a bare "not + * pinned": pinning is said of many things in and around a build ("the cache key is not pinned to + * the lockfile hash", "the apt install is unpinned", "the output tag is not pinned to the run + * id"), so a bare negation would anchor any of them the moment the word "image" appeared anywhere + * else in the finding, which is the over-firing the class javadoc rules out. + */ + private static final List UNPINNED_REFERENCE = + phrases( + "unpinned base image", + "unpinned image", + "unpinned chart", + "unpinned action", + "base image is unpinned", + "image is unpinned", + "chart is unpinned", + "action is unpinned", + "image tag is unpinned", + "not pinned to a digest", + "not pinned by digest", + "not pinned to a version", + "without a digest", + "without a sha256 digest", + "no digest", + "no sha256", + "lacks a digest", + "carries no digest", + "pin it by digest", + "floating tag", + "mutable tag", + "moving tag", + "rolling tag", + "latest tag"); + + /** + * The one claim that has to be read on the raw text: normalization drops the colon that makes + * {@code :latest} a reference rather than the ordinary English word. + * + *

It also stands on its own, without the subject the other claims need. The colon is Docker + * tag syntax, so the occurrence IS the reference being named: "FROM alpine:latest — the tag + * drifts under the build" says the whole class in four characters and need not also use the word + * "image" for the class to be the one it is. + */ + private static final String LATEST_TAG = ":latest"; + + /** + * What the unpinned reference refers to, so a pinning claim alone never anchors a finding. The + * bare words "tag" and "digest" are deliberately not on this list: they are what the claim above + * is already made of, so accepting them here would make the second half of the test a restatement + * of the first. + */ + private static final List EXTERNAL_REFERENCE_SUBJECT = + phrases("base image", "image", "images", "chart", "action"); + + /** + * The finding says the container runs as someone other than root. A manifest FIELD NAME is not on + * this list, nor on the privilege-drop one below: {@code runAsNonRoot} and {@code runAsUser} are + * written the same way by a finding that says the field is missing and by one that says it is + * set, so the name carries no polarity and reading it as the claim would anchor the opposite of + * the class. A finding that means either class says so in prose — "runs as root", "non-root" — + * and one that says only "runAsUser: 1000 is set, but the port bind fails" keeps its own grade. + * + *

An explicit claim from this list also decides which of the two container classes a finding + * belongs to, in {@link #classify}: it says the container HAS a non-root user, which is the + * premise the privilege-drop class denies. + */ + private static final List NON_ROOT_USER = + phrases("non root", "nonroot", "unprivileged user"); + + /** + * A {@code USER} directive naming an account, read case-sensitively on the raw text: the + * Dockerfile instruction is written in upper case, and matching it either way would read the + * ordinary English "user" in any sentence as a privilege drop. 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. + */ + private static final Pattern USER_DIRECTIVE = + Pattern.compile( + "\\bUSER\\s+(?!(?:directive|instruction|line|statement|declaration)\\b)[A-Za-z0-9_$.:-]+"); + + /** + * The finding says that user cannot write the path, in ownership or in failure terms. Every + * phrase carries the direction: a bare "chown" or "ownership" is said as often of a chown that is + * present, redundant or merely mentioned as of one that is missing, so matching the word alone + * read a layer-size nit about an existing {@code RUN chown} as this class. + */ + private static final List UNWRITABLE_PATH = + phrases( + "root owned", + "owned by root", + "root root", + "root ownership", + "without a chown", + "no chown", + "nothing chowns", + "never chowned", + "permission denied", + "eacces", + "not writable", + "cannot write", + "cant write", + "unable to write", + "fails to write", + "fail to write", + "write fails", + "write fail", + "write will fail"); + + /** The finding says privilege is never dropped. */ + private static final List NEVER_DROPS_PRIVILEGE = + phrases( + "runs as root", + "run as root", + "running as root", + "runs as the root", + "run as the root", + "running as the 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"); + + /** + * What the finding must NOT also assert. Each of these puts the level somewhere the class does + * not decide — a container granted the host, a capability, a credential or a known vulnerability + * is severe for a reason an anchor cannot weigh — so the review's own grade stands. + */ + private static final List ESCALATION_BEYOND_CLASS = + phrases( + "privileged", + "hostpath", + "hostnetwork", + "hostpid", + "hostipc", + "sys admin", + "capabilities", + "capability", + "host mount", + "host mounts", + "host path", + "host namespace", + "host namespaces", + "host pid", + "host ipc", + "host network", + "docker sock", + "docker socket", + "cve", + "ghsa", + "secret", + "secrets", + "credential", + "credentials", + "password", + "passwords", + "private key"); + + /** The separator between a file name's stem and its extensions. */ + private static final Pattern SEGMENT = Pattern.compile("\\."); + + /** Everything {@link #normalized} turns into the single space that separates two words. */ + private static final Pattern NOT_A_WORD = Pattern.compile("[^a-z0-9]+"); + + /** The apostrophes a contraction is written with, dropped so "doesn't" normalizes to one word. */ + private static final Pattern APOSTROPHE = Pattern.compile("['\u2019]"); + + /** Each phrase padded with the spaces that make it match whole words and nothing else. */ + private static List phrases(String... words) { + return Arrays.stream(words).map(word -> " " + word + " ").toList(); + } + + /** + * The finding's words, lower-cased, stripped of punctuation and padded, so a phrase from the + * lists above matches whole words wherever the finding put them. + */ + private static String normalized(String text) { + String contracted = APOSTROPHE.matcher(text.toLowerCase(Locale.ROOT)).replaceAll(""); + return " " + NOT_A_WORD.matcher(contracted).replaceAll(" ").strip() + " "; + } + + private static boolean states(String normalized, List claim) { + return claim.stream().anyMatch(normalized::contains); + } + + private SeverityCalibrator() {} + + /** + * Rewrites the risk and confidence of every finding that states one of the three anchored classes + * and leaves every other finding, and the rest of the response, untouched. The same response + * instance comes back when nothing was regraded. + */ + public static ReviewResponse calibrate(ReviewResponse response) { + if (response.findings().isEmpty()) { + return response; + } + var adjusted = new ArrayList(response.findings().size()); + var changed = false; + for (ReviewResponse.Finding finding : response.findings()) { + InfrastructureClass anchored = classify(finding); + if (anchored == null || alreadyAnchored(finding, anchored)) { + adjusted.add(finding); + continue; + } + Log.infof( + "Calibrating %s finding '%s' (%s:%d) from %s/%s to %s/%s risk/confidence", + anchored, + LogSafe.oneLine(finding.title()), + LogSafe.oneLine(finding.file()), + finding.line(), + LogSafe.oneLine(finding.risk()), + LogSafe.oneLine(finding.confidence()), + label(anchored.risk), + label(ANCHORED_CONFIDENCE)); + adjusted.add( + new ReviewResponse.Finding( + label(anchored.risk), + label(ANCHORED_CONFIDENCE), + finding.file(), + finding.line(), + finding.title(), + finding.description(), + finding.suggestionOld(), + finding.suggestionNew())); + changed = true; + } + if (!changed) { + return response; + } + return new ReviewResponse( + adjusted, + response.previousFindingsStatus(), + FindingVerificationService.recount(response.summary(), adjusted)); + } + + /** The class the finding states, or {@code null} when it states none of them. */ + private static InfrastructureClass classify(ReviewResponse.Finding finding) { + if (finding.file() == null || !isInfrastructureFile(finding.file())) { + return null; + } + String raw = + (finding.title() == null ? "" : finding.title()) + + "\n" + + (finding.description() == null ? "" : finding.description()); + String text = normalized(raw); + if (states(text, ESCALATION_BEYOND_CLASS)) { + return null; + } + // The privilege-drop claim is read first because it DENIES the other container class's + // premise: a container that never drops privilege has no non-root user for a root-owned path + // to be unwritable by. The two share a vocabulary — "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 reading that as the ownership defect would publish it at that class's + // level under a class label its own words contradict. A finding that says in so many words + // that the container DOES run as a non-root user settles the same question the other way and + // is never this class, however the rest of the sentence reads: "the build stage runs as root, + // but the final image drops to a non-root user and /data stays root-owned" is the ownership + // defect. Only the explicit non-root claim tells that apart from the omission above — a USER + // token in the text does not, since a finding writes "no USER appuser directive" with one. + if (!states(text, NON_ROOT_USER) && states(text, NEVER_DROPS_PRIVILEGE)) { + return InfrastructureClass.MISSING_PRIVILEGE_DROP; + } + if (namesNonRootUser(text, raw) && states(text, UNWRITABLE_PATH)) { + return InfrastructureClass.UNWRITABLE_RUNTIME_PATH; + } + return namesLatestTag(raw) + || (states(text, UNPINNED_REFERENCE) && states(text, EXTERNAL_REFERENCE_SUBJECT)) + ? InfrastructureClass.MUTABLE_EXTERNAL_REFERENCE + : null; + } + + /** Whether the finding is anchored in one of the declarative artifacts these classes live in. */ + private static boolean isInfrastructureFile(String path) { + String name = path.substring(path.lastIndexOf('/') + 1).toLowerCase(Locale.ROOT); + return isContainerFile(name) || INFRASTRUCTURE_EXTENSIONS.stream().anyMatch(name::endsWith); + } + + private static boolean isContainerFile(String name) { + String[] segments = SEGMENT.split(name); + return Arrays.stream(segments).anyMatch(CONTAINER_FILE_NAMES::contains) + && !DOCUMENTARY_EXTENSIONS.contains(segments[segments.length - 1]); + } + + private static boolean namesNonRootUser(String text, String raw) { + return states(text, NON_ROOT_USER) || USER_DIRECTIVE.matcher(raw).find(); + } + + private static boolean namesLatestTag(String raw) { + return raw.toLowerCase(Locale.ROOT).contains(LATEST_TAG); + } + + private static boolean alreadyAnchored( + ReviewResponse.Finding finding, InfrastructureClass anchored) { + return RiskLevel.fromString(finding.risk()) == anchored.risk + && Confidence.fromString(finding.confidence()) == ANCHORED_CONFIDENCE; + } + + private static String label(Enum level) { + return level.name().toLowerCase(Locale.ROOT); + } +} diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/PrReviewPrompts.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/PrReviewPrompts.java index 93b68a81..7c467478 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/PrReviewPrompts.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/PrReviewPrompts.java @@ -292,6 +292,32 @@ collection is large is how a real O(n^2) goes unreported. What is not a finding (dimension 4): that is a false statement, not a wording preference. Prose style, tone and ordering remain nitpicks. + Anchored infrastructure classes — grade the consequence, not the pull request: + Three defects recur in almost every containerized repository, and for each one the + consequence and who it reaches are fixed by the class, not by the change it turns up + in. Grade them exactly as follows, in every language and every repository, and let the + description carry whatever is specific to this pull request: + - A MUTABLE EXTERNAL REFERENCE — a base image, action, or chart named by a tag, or by + no tag at all, instead of a digest: risk "medium", confidence "medium". The + consequence is a build whose inputs change without a commit; nothing fails at run + time. A lockfile elsewhere in the repository pins the dependency set, not the image, + so it neither raises nor lowers this. + - A PATH THE RUNNING USER CANNOT WRITE — a VOLUME, WORKDIR or data directory that stays + root-owned while the image drops to a non-root USER, with no mkdir/chown for it: + risk "high", confidence "medium". Docker creates that path root:root, so the + consequence is a deterministic failure the first time the container writes there, on + every deployment. Which write comes first, and how early it runs, does not change the + level — it is the same defect whether it kills the first request or the first flush. + - A CONTAINER THAT NEVER DROPS PRIVILEGE — no USER directive, runAsNonRoot unset or + false: risk "medium", confidence "medium". By itself it breaks nothing; it decides + what a compromise of that container starts with. + Confidence is "medium" on all three for the same reason in both directions: the shape + is settled by the file in front of you, so it is never "low", and whether this + deployment exercises the consequence is not in that file, so it is never "high". A + finding that ALSO asserts something the class does not cover — a privileged container, + a host mount or namespace, an added capability, a committed credential, a named CVE — + is a different claim and takes the severity that claim earns. + Severity is not confidence, and neither one is a reason to stay silent: - Emit a finding whose defect you can demonstrate from the provided material even when the confidence rules cap it at "medium" or "low". Those rules govern how you WORD the @@ -314,6 +340,17 @@ would give the same defect class in a different framework or language. If the an differs, the difference is coming from your uncertainty rather than from the defect, and it belongs in confidence — pin the risk to the class and lower the confidence instead. + - Equivalent evidence gets equivalent confidence, and this is the harder half. + Confidence answers ONE question — could another reviewer confirm this from the + material provided? — and the answer cannot depend on which language the same shape is + written in, on how familiar its idiom looks, or on how much of the file you happen to + have read. When one change carries the same defect in two places, grade both from the + same evidence: if you rate one lower, name in its description the fact you could not + check there and could check in the other. If you cannot name one, the two ratings + must match. This decides whether a maintainer sees the finding at all: a + "low"-confidence finding below "high" risk is collapsed into a summary block and + never opens a thread on the diff, so an unjustified confidence gap is the difference + between reporting a defect and burying it. - EVERY defect gets its OWN finding, on the dimension it belongs to. While writing one finding you will often state a SECOND, different defect as supporting evidence — a stale comment quoted to show what the code was meant to do, a stub that cannot diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipelineTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipelineTest.java index 250e7f42..fe3c1523 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipelineTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipelineTest.java @@ -1593,6 +1593,60 @@ void everyBatchOfAMultiCallReviewIsToldWhatWasWithheld() { } } + /** + * #773: the infrastructure anchor is in the chain, and it runs after the verifier. The verifier + * applies its verdict only in the lowering direction, so a downgrade there would otherwise + * re-spread exactly the class the anchor pins — which is how the same unpinned base image came + * out "medium" on four pull requests of one round and "low" on three. + */ + @Test + void theInfrastructureAnchorOutlivesAVerifierDowngrade() { + var session = ReviewSession.create("owner/repo", 1, "Add the container image", "sha"); + var ctx = reviewContext(); + var template = + new AiReviewService.PromptInputs("raw legacy diff", "ctx", "base", "s", "t", "", ""); + var plan = + new DiffBudgetPlanner.BudgetPlan( + List.of(batch("Dockerfile")), List.of(), List.of(), true, null, null, null, null); + var raised = + new ReviewResponse.Finding( + "medium", + "medium", + "Dockerfile", + 1, + "unpinned base image", + "FROM node:20-alpine is an unpinned base image, so two builds of this commit can" + + " resolve to different images.", + null, + null); + when(aiReviewService.review(eq(session), any())) + .thenReturn(new ReviewResponse(List.of(raised), List.of(), null)); + when(findingVerificationService.verify( + anyLong(), any(), any(), any(), any(), any(), any(), any())) + .thenAnswer( + inv -> + new ReviewResponse( + List.of( + new ReviewResponse.Finding( + "low", + "low", + raised.file(), + raised.line(), + raised.title(), + raised.description(), + null, + null)), + List.of(), + null)); + + var refined = + pipeline.run( + session, template, ctx, plan, new DiffLineResolver(Map.of()), NO_CITED_LOCATIONS); + + assertEquals("medium", refined.findings().get(0).risk()); + assertEquals("medium", refined.findings().get(0).confidence()); + } + @Test void singleCallCeilingRefusalPropagatesForTheOrchestratorToFailSoft() { // Characterization of the single-call contract: a mid-retry ceiling refusal has no paid batch diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/SeverityCalibratorTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/SeverityCalibratorTest.java new file mode 100644 index 00000000..c31bdd23 --- /dev/null +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/SeverityCalibratorTest.java @@ -0,0 +1,599 @@ +/* + * Copyright 2026 Thiago Gonzaga + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package dev.thiagogonzaga.thrillhousebot.review; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import dev.thiagogonzaga.thrillhousebot.review.ai.ReviewResponse; +import java.util.List; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; +import org.junit.jupiter.params.provider.ValueSource; + +/** + * #773. The round-8 corpus graded the same infrastructure defect differently in different pull + * requests, so every test here 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; the pair is the property the issue asks for. + */ +class SeverityCalibratorTest { + + /** The python half of the corpus pair: `ensure_schema()` writes first, and the grade was low. */ + private static final String ROOT_VOLUME_PYTHON = + "The image declares VOLUME /var/lib/app and then switches to USER appuser without a mkdir" + + " and chown for that path. Docker creates it root-owned, so the first write from" + + " ensure_schema() fails with permission denied and the container exits."; + + /** The node half: the same shape, and it drew medium. */ + private static final String ROOT_VOLUME_NODE = + "VOLUME /data is declared before USER node, and nothing chowns it, so the directory is" + + " root-owned at run time and the first flush() cannot write to it."; + + private static final String UNPINNED_NODE = + "FROM node:20-alpine is an unpinned base image: the tag moves, so two builds of this commit" + + " can resolve to different images. Pin it by digest."; + + private static final String UNPINNED_RUST = + "The base image rust:1.81 carries no digest, so the build is not reproducible even with" + + " Cargo.lock and --locked pinning the dependency set."; + + private static final String NO_USER_CSHARP = + "The Dockerfile never adds a USER directive, so the published service runs as root."; + + private static final String NO_USER_GO = + "No USER instruction is set anywhere in this Dockerfile; the container runs as root user."; + + private static ReviewResponse.Finding finding( + String risk, String confidence, String file, String description) { + return new ReviewResponse.Finding( + risk, confidence, file, 7, "container image finding", description, null, null); + } + + private static ReviewResponse response(ReviewResponse.Finding... findings) { + return new ReviewResponse( + List.of(findings), + List.of(), + new ReviewResponse.Summary( + findings.length, 0, 0, 0, findings.length, "assessment", "purpose", List.of())); + } + + private static ReviewResponse.Finding calibrateOne(ReviewResponse.Finding raw) { + return SeverityCalibrator.calibrate(response(raw)).findings().get(0); + } + + private static void assertGrade( + ReviewResponse.Finding calibrated, String risk, String confidence) { + assertEquals(risk, calibrated.risk(), "risk"); + assertEquals(confidence, calibrated.confidence(), "confidence"); + } + + @Test + void theSameUnpinnedBaseImageGradesTheSameInBothPullRequests() { + ReviewResponse.Finding node = calibrateOne(finding("low", "low", "Dockerfile", UNPINNED_NODE)); + ReviewResponse.Finding rust = + calibrateOne(finding("medium", "high", "rust/Dockerfile", UNPINNED_RUST)); + + assertGrade(node, "medium", "medium"); + assertGrade(rust, "medium", "medium"); + } + + @Test + void theSameRootOwnedVolumeGradesTheSameInBothPullRequests() { + ReviewResponse.Finding python = + calibrateOne(finding("low", "low", "python/Dockerfile", ROOT_VOLUME_PYTHON)); + ReviewResponse.Finding node = + calibrateOne(finding("medium", "high", "node/Dockerfile", ROOT_VOLUME_NODE)); + + assertGrade(python, "high", "medium"); + assertGrade(node, "high", "medium"); + } + + @Test + void theSameMissingUserDirectiveGradesTheSameInBothHalvesOfAPairedChange() { + ReviewResponse.Finding csharp = + calibrateOne(finding("low", "low", "csharp/Dockerfile", NO_USER_CSHARP)); + ReviewResponse.Finding go = + calibrateOne(finding("medium", "high", "go/Dockerfile", NO_USER_GO)); + + assertGrade(csharp, "medium", "medium"); + assertGrade(go, "medium", "medium"); + } + + /** + * The surface split the same corpus measured: a "low"-confidence finding below high risk is + * collapsed into the summary block and opens no thread, so the same omission reached the diff in + * one language and nowhere in the other. + */ + @Test + void anAnchoredFindingPostsInlineWhateverConfidenceTheReviewGaveIt() { + for (String description : List.of(UNPINNED_NODE, ROOT_VOLUME_PYTHON, NO_USER_CSHARP)) { + ReviewResponse.Finding calibrated = + calibrateOne(finding("low", "low", "Dockerfile", description)); + assertTrue( + Finding.fromAiResponse(calibrated).postsInline(), + "anchored finding must open a thread on the diff: " + description); + } + } + + /** The anchor pins: a level above it is as much a miscalibration as one below. */ + @ParameterizedTest + @CsvSource({"critical,high", "high,medium", "medium,low", "low,high"}) + void anUnpinnedReferenceIsMediumWhateverTheReviewRatedIt(String risk, String confidence) { + assertGrade( + calibrateOne(finding(risk, confidence, "Dockerfile", UNPINNED_NODE)), "medium", "medium"); + } + + @Test + void aManifestOutsideADockerfileIsAnchoredToo() { + ReviewResponse.Finding manifest = + calibrateOne( + finding( + "low", + "low", + "deploy/k8s/api.yaml", + "The pod template leaves runAsNonRoot unset, so the container runs as root.")); + + assertGrade(manifest, "medium", "medium"); + } + + /** The same ownership defect, said the way a Kubernetes manifest finding says it. */ + @Test + void aRootOwnedPathUnderANonRootPodIsTheSameClassAsUnderANonRootImage() { + ReviewResponse.Finding pod = + calibrateOne( + finding( + "low", + "low", + "deploy/k8s/api.yaml", + "The pod runs as non-root (uid 1000) but /var/lib/data is root-owned, so the first" + + " write is denied and the container restarts.")); + + assertGrade(pod, "high", "medium"); + } + + /** A non-root image with an unpinned base is the reference class, not the ownership one. */ + @Test + void aNonRootImageWhoseBaseIsUnpinnedGradesAsTheReferenceClass() { + ReviewResponse.Finding both = + calibrateOne( + finding( + "low", + "low", + "Dockerfile", + "The image drops to a non-root user, but FROM node:20-alpine is an unpinned base" + + " image, so the build is not reproducible.")); + + assertGrade(both, "medium", "medium"); + } + + /** Unpinned is said of things that are not external references, and those are not the class. */ + @Test + void anUnpinnedToolVersionIsNotAnExternalReference() { + ReviewResponse tool = + response( + new ReviewResponse.Finding( + "low", + "low", + ".github/workflows/ci.yml", + 4, + "unpinned linter version", + "The linter is installed unpinned, so a new release can change which warnings the" + + " job reports.", + null, + null)); + + assertSame(tool, SeverityCalibrator.calibrate(tool)); + } + + /** The colon is what makes {@code :latest} a reference rather than the English word. */ + @Test + void theLatestTagIsTheSameClassAsAMissingDigest() { + ReviewResponse.Finding latest = + calibrateOne( + finding( + "low", + "low", + "Dockerfile", + "FROM ubuntu:latest resolves to a different image on every build.")); + + assertGrade(latest, "medium", "medium"); + } + + /** The colon is Docker tag syntax, so the occurrence names the reference by itself. */ + @Test + void theLatestTagNamesTheReferenceWithoutTheWordImage() { + ReviewResponse.Finding terse = + calibrateOne( + finding( + "low", + "low", + "Dockerfile", + "FROM alpine:latest — the tag drifts under the build.")); + + assertGrade(terse, "medium", "medium"); + } + + /** An article between the words is the same claim. */ + @Test + void anArticleBeforeRootIsTheSameClaim() { + ReviewResponse.Finding article = + calibrateOne( + finding( + "low", "low", "Dockerfile", "The image runs as the root user in the final stage.")); + + assertGrade(article, "medium", "medium"); + } + + /** The claim is read on the finding's words, so a contraction is the same claim. */ + @Test + void aContractionStatesTheSameClaim() { + ReviewResponse.Finding contracted = + calibrateOne( + finding( + "low", + "low", + "Dockerfile", + "The container doesn't drop privileges before the entrypoint starts.")); + + assertGrade(contracted, "medium", "medium"); + } + + /** + * The two container classes share a vocabulary, and the privilege-drop claim denies the other's + * premise: a container that runs as root has no non-root user for a root-owned path to be + * unwritable by, however much ownership the finding goes on to discuss. + */ + @Test + void anOmissionThatNamesTheIntendedAccountIsStillThePrivilegeDropClass() { + ReviewResponse.Finding omission = + calibrateOne( + finding( + "low", + "low", + "Dockerfile", + "There is no USER appuser directive, so the app runs as root and the files it" + + " writes take root ownership.")); + + assertGrade(omission, "medium", "medium"); + } + + /** A chown that is present, redundant or merely mentioned is not a path nobody can write. */ + @Test + void aNitAboutARedundantChownIsNotTheOwnershipClass() { + ReviewResponse redundant = + response( + finding( + "low", + "low", + "Dockerfile", + "USER appuser is already set, so the explicit RUN chown -R appuser:appuser is" + + " redundant and adds a duplicate layer; use COPY --chown instead.")); + + assertSame(redundant, SeverityCalibrator.calibrate(redundant)); + } + + /** + * "non-root user" normalizes to three words, and a phrase list matches inside them: the explicit + * non-root claim settles which container class the finding is, so it is read before the words + * that merely appear in both. + */ + @Test + void anOwnershipFindingThatSaysNonRootUserIsNotThePrivilegeDropClass() { + ReviewResponse.Finding ownership = + calibrateOne( + finding( + "low", + "low", + "Dockerfile", + "The image runs as a non-root user, but /data stays root-owned, so the first write" + + " fails with permission denied and the container exits.")); + + assertGrade(ownership, "high", "medium"); + } + + /** A committed password is a credential, whatever the finding calls it. */ + @Test + void aCommittedPasswordKeepsTheReviewsGrade() { + ReviewResponse baked = + response( + finding( + "high", + "high", + "Dockerfile", + "The Dockerfile bakes the deploy password into an ARG and the container runs as" + + " root, so anyone who pulls the image reads it.")); + + assertSame(baked, SeverityCalibrator.calibrate(baked)); + } + + /** A digest is asked of things that are not external references, and those are not the class. */ + @Test + void aMissingDigestForSomethingOtherThanAReferenceIsNotAnchored() { + ReviewResponse archive = + response( + new ReviewResponse.Finding( + "low", + "low", + ".github/workflows/ci.yml", + 9, + "downloaded toolchain is not verified", + "The step pins the toolchain by version but carries no digest for the archive it" + + " downloads, so a replaced artifact would go unnoticed.", + null, + null)); + + assertSame(archive, SeverityCalibrator.calibrate(archive)); + } + + /** The claim has to be about the reference, not merely in the same finding as one. */ + @Test + void anUnpinnedPackageInstallBesideAPinnedImageIsNotAnchored() { + ReviewResponse aptInstall = + response( + finding( + "low", + "low", + "Dockerfile", + "The base image is correctly pinned by digest, but the apt install is unpinned, so" + + " runtime package versions drift between builds.")); + + assertSame(aptInstall, SeverityCalibrator.calibrate(aptInstall)); + } + + /** + * The escalation defeater is read on the words a finding uses, not only on the manifest field + * names: missing it would pin a high finding DOWN, which is the direction the design forbids. + */ + @Test + void anEscalationWrittenInProseAlsoKeepsTheReviewsGrade() { + ReviewResponse hostNamespace = + response( + finding( + "high", + "high", + "deploy/k8s/api.yaml", + "The base image is unpinned, and the pod also shares the host PID namespace with" + + " the node.")); + + assertSame(hostNamespace, SeverityCalibrator.calibrate(hostNamespace)); + } + + /** A variant suffix names the same artifact; only a file type says the file is about it. */ + @Test + void aVariantDockerfileIsStillTheArtifact() { + assertGrade( + calibrateOne(finding("low", "low", "docker/Dockerfile.prod", UNPINNED_RUST)), + "medium", + "medium"); + } + + /** Podman spells the same artifact differently, and it is the same artifact. */ + @Test + void aContainerfileIsTheSameArtifactAsADockerfile() { + assertGrade( + calibrateOne(finding("low", "low", "build/Containerfile", UNPINNED_RUST)), + "medium", + "medium"); + } + + @Test + void aFindingThatAlsoAssertsAnEscalationBeyondTheClassKeepsItsOwnGrade() { + ReviewResponse escalating = + response( + finding( + "critical", + "high", + "deploy/k8s/api.yaml", + "The container runs as root AND mounts hostPath /var/run/docker.sock, so a" + + " compromise owns the node.")); + + assertSame(escalating, SeverityCalibrator.calibrate(escalating)); + } + + /** "Not pinned" on its own is not the class: the anchored one is about external references. */ + @Test + void anUnpinnedSomethingElseInAWorkflowIsNotAnchored() { + ReviewResponse cacheKey = + response( + new ReviewResponse.Finding( + "low", + "low", + ".github/workflows/ci.yml", + 7, + "stale dependency cache", + "The cache key is not pinned to the lockfile hash, so a stale dependency cache" + + " can be restored on a change that should have missed.", + null, + null)); + + assertSame(cacheKey, SeverityCalibrator.calibrate(cacheKey)); + } + + /** + * A manifest field name is written the same way by a finding that says the field is missing and + * by one that says it is set, so naming it is not the class. + */ + @ParameterizedTest + @ValueSource( + strings = { + "The securityContext sets runAsUser: 1000, but the service binds port 80, so startup fails.", + "runAsNonRoot: true is set, but readOnlyRootFilesystem is missing, so the filesystem can be" + + " tampered with." + }) + void aManifestFindingThatSetsThePrivilegeFieldKeepsItsOwnGrade(String description) { + ReviewResponse set = response(finding("high", "high", "deploy/k8s/api.yaml", description)); + + assertSame(set, SeverityCalibrator.calibrate(set)); + } + + /** Being named after the artifact is not being the artifact. */ + @ParameterizedTest + @ValueSource( + strings = { + "src/main/java/dev/app/DockerfileSupport.java", + "docs/Dockerfile-guide.md", + "docs/Dockerfile.md", + "ui/Containerfile.kt" + }) + void aFileMerelyNamedAfterTheDockerfileIsNotOne(String path) { + ReviewResponse named = response(finding("low", "low", path, UNPINNED_NODE)); + + assertSame(named, SeverityCalibrator.calibrate(named)); + } + + /** + * Pinning is said of many things in a workflow, so the claim has to name the immutable thing that + * is missing before a generic noun beside it can anchor anything. + */ + @Test + void anOutputTagNotPinnedToTheRunIdIsNotAnchored() { + ReviewResponse collision = + response( + finding( + "low", + "low", + ".github/workflows/deploy.yml", + "The release job's image output tag is not pinned to the run id, so two concurrent" + + " runs can overwrite each other's push.")); + + assertSame(collision, SeverityCalibrator.calibrate(collision)); + } + + @Test + void aFindingOutsideADeclarativeArtifactIsNotAnchored() { + ReviewResponse code = + response( + finding( + "low", + "low", + "src/main/java/dev/example/Builder.java", + "The image name built here is unpinned, so the tag can move.")); + + assertSame(code, SeverityCalibrator.calibrate(code)); + } + + /** + * The two container classes share a vocabulary, and the omission is the weaker of the two: a + * finding that says there is no USER directive at all is the privilege-drop class even when it + * goes on to talk about ownership, because there is no non-root user for the path to be + * unwritable by. + */ + @Test + void talkingAboutOwnershipDoesNotPromoteAPrivilegeDropOmission() { + ReviewResponse.Finding omission = + calibrateOne( + finding( + "low", + "low", + "Dockerfile", + "There is no USER directive, so every file the entrypoint creates takes root" + + " ownership and the service runs as root.")); + + assertGrade(omission, "medium", "medium"); + } + + /** + * The {@code USER} directive is matched case-sensitively, so ordinary English about users in a + * manifest finding is not read as a privilege drop. + */ + @Test + void ordinaryProseAboutUsersIsNotReadAsAPrivilegeDrop() { + ReviewResponse prose = + response( + finding( + "low", + "low", + "deploy/values.yaml", + "The user quota default is 5, but the chart's README documents 10, so an operator" + + " who trusts the README gets the wrong limit.")); + + assertSame(prose, SeverityCalibrator.calibrate(prose)); + } + + @ParameterizedTest + @ValueSource(strings = {"low", "medium", "high", "critical"}) + void aFindingWithNoTitleOrDescriptionIsNotAnchored(String risk) { + ReviewResponse bare = + response(new ReviewResponse.Finding(risk, "low", "Dockerfile", 3, null, null, null, null)); + + assertSame(bare, SeverityCalibrator.calibrate(bare)); + } + + @Test + void aFindingWithNoFileIsNotAnchored() { + ReviewResponse unanchored = response(finding("low", "low", null, UNPINNED_NODE)); + + assertSame(unanchored, SeverityCalibrator.calibrate(unanchored)); + } + + @Test + void aResponseAlreadyAtTheAnchorIsReturnedUntouched() { + ReviewResponse graded = response(finding("medium", "medium", "Dockerfile", UNPINNED_NODE)); + + assertSame(graded, SeverityCalibrator.calibrate(graded)); + } + + @Test + void anEmptyFindingListIsReturnedUntouched() { + ReviewResponse empty = response(); + + assertSame(empty, SeverityCalibrator.calibrate(empty)); + } + + /** Calibration regrades; it never drops, adds or rewords, so precision cannot move. */ + @Test + void everyFindingSurvivesWithItsTextAndAnchorIntact() { + ReviewResponse.Finding raw = + new ReviewResponse.Finding( + "low", + "low", + "Dockerfile", + 12, + "unpinned base image", + UNPINNED_NODE, + "FROM node:20-alpine", + "FROM node:20-alpine@sha256:abc"); + + ReviewResponse calibrated = SeverityCalibrator.calibrate(response(raw)); + + assertEquals(1, calibrated.findings().size()); + ReviewResponse.Finding only = calibrated.findings().get(0); + assertEquals(raw.file(), only.file()); + assertEquals(raw.line(), only.line()); + assertEquals(raw.title(), only.title()); + assertEquals(raw.description(), only.description()); + assertEquals(raw.suggestionOld(), only.suggestionOld()); + assertEquals(raw.suggestionNew(), only.suggestionNew()); + } + + @Test + void theSummaryCountsFollowTheRegradedFindings() { + ReviewResponse calibrated = + SeverityCalibrator.calibrate( + response( + finding("low", "low", "python/Dockerfile", ROOT_VOLUME_PYTHON), + finding("low", "low", "python/Dockerfile", UNPINNED_NODE))); + + assertEquals(2, calibrated.summary().totalFindings()); + assertEquals(1, calibrated.summary().high(), "high"); + assertEquals(1, calibrated.summary().medium(), "medium"); + assertEquals(0, calibrated.summary().low(), "low"); + } +} diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ai/PrReviewPromptsContentTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ai/PrReviewPromptsContentTest.java index ad723468..092f3acd 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ai/PrReviewPromptsContentTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ai/PrReviewPromptsContentTest.java @@ -1279,6 +1279,68 @@ void publishedRiskMustMatchTheReasoningTheFindingStates() { "the artifact cap must not apply to an unshown mitigation for a shown defect (#570)"); } + /** + * #773. Scoring a whole round for consistency rather than precision found the same unpinned base + * image at two severities across seven pull requests and a root-owned volume under a non-root + * USER at two more, with nothing in the changes to tell them apart. The prompt states the grade + * for each class so the model lands there by itself; {@code SeverityCalibrator} is what makes it + * hold when it does not. + */ + @Test + void theRecurringInfrastructureClassesCarryAStatedGrade() { + String sys = PrReviewPrompts.SYSTEM; + assertContains( + sys, + "Anchored infrastructure classes — grade the consequence, not the pull request", + "the recurring infrastructure classes must carry a stated grade (#773)"); + assertContains( + sys, + "A MUTABLE EXTERNAL REFERENCE", + "an unpinned base image, action or chart must have one stated level (#773)"); + assertContains( + sys, + "A PATH THE RUNNING USER CANNOT WRITE", + "a root-owned path under a non-root USER must have one stated level (#773)"); + assertContains( + sys, + "A CONTAINER THAT NEVER DROPS PRIVILEGE", + "a container that never drops privilege must have one stated level (#773)"); + assertContains( + sys, + "Which write comes first, and how early it runs, does not change the", + "how early the failure lands must not move the level (#773)"); + assertContains( + sys, + "so it is never \"low\", and whether this", + "the anchored classes' confidence must be fixed in both directions (#773)"); + } + + /** + * The harder half of #773: the same defect class in the two halves of one paired-language change + * drew a confident inline finding in one and a low-confidence collapsed item in the other, on + * evidence that was equally provable from the diff in both. + */ + @Test + void confidenceMustNotDependOnWhichLanguageTheShapeIsWrittenIn() { + String sys = PrReviewPrompts.SYSTEM; + assertContains( + sys, + "Equivalent evidence gets equivalent confidence", + "confidence must be compared across the halves of a change (#773)"); + assertContains( + sys, + "cannot depend on which language the same shape is", + "an idiom's familiarity must not move confidence (#773)"); + assertContains( + sys, + "name in its description the fact you could not", + "an unequal confidence must name what could not be checked (#773)"); + assertContains( + sys, + "between reporting a defect and burying it", + "the prompt must say what the collapsed surface costs (#773)"); + } + /** The same guard on both surfaces that emit {@code description_gaps}. */ @Test void bothPromptsKeepWithheldPathsOutOfDescriptionGaps() {