diff --git a/CHANGELOG.md b/CHANGELOG.md index 31e6ad24..ca3c29c1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,7 @@ This release makes the bot interactive and controllable from the PR — conversa - **AI prompts dropped every context variable but the first**: each AI service (`PrReviewer`, `ReplyAssistant`, `FindingVerifier`) declared `@UserMessage` on a method *parameter*, which makes quarkus-langchain4j send only that parameter's raw value as the user message and never render the prompt template. So reviews ran on the diff alone — silently ignoring the repository instructions (`.github/thrillhousebot.md`), project stack, PR title/description, base comparison, related tests, and previous findings — the finding verifier audited candidates without the diff, and conversational replies saw only the maintainer's question with no diff, finding, or thread. Moved `@UserMessage` to the method so every `@V` variable is interpolated, and reduced `PromptTemplateEscaper` to marker-neutralization (its Qute unparsed-section wrapper was never stripped for data-bound values and corrupted any content containing `|}`). Added end-to-end and structural regression tests that pin the rendered prompt (#186) - **Reviewer corrupted the marker-handling code it was reviewing**: the prompt-injection defense rewrote the diff-section delimiters (`<<>>` / `<<>>`) found *inside* the diff, so whenever the bot reviewed code that legitimately contains those markers — the escaper, the prompt templates, and any PR that edits them — it saw altered source. That produced false "contradictory assertion"/no-op findings and silently degraded review accuracy of exactly those files. Replaced the fixed delimiters with a per-review unguessable random fence around the diff (the "random sequence enclosure"/spotlighting defense) and now pass the diff byte-exact; the small prose context slots keep the lightweight marker-neutralization as defense-in-depth (#187) - **Large PRs were silently truncated to 30 files**: `getPullRequestFiles` fetched only GitHub's default first page, so any PR with more than 30 changed files was reviewed — and described / changelog'd / replied to — on a partial diff, with no warning. It now paginates (100 files per page, bounded at 30 pages) so the whole diff is assembled before review (#190) +- **False "undefined / missing symbol" findings when the definition is just outside the diff**: a finding could confidently flag a variable, env var, import, or config key as undefined/unset when its definition sat in the same file a few unchanged lines outside the diff hunk's context window — GitHub serves only ~3 lines of context, so the definition was never in the reviewed material (a CRITICAL false positive on `release.yml` in PR #88 claimed `NEXT`/`TAG` were undefined when the step's `env:` block defined them). The reviewer now treats an unseen definition as unconfirmed rather than absent, and the verifier rejects an "undefined / missing symbol" finding only when the scope its definition would occupy isn't shown in the material (an unverifiable claim) — a genuinely missing symbol that the diff *does* demonstrate (e.g. the diff removes the definition) still stands (#192) ### Documentation diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerifierPrompts.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerifierPrompts.java index 0d0788da..2407f320 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerifierPrompts.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerifierPrompts.java @@ -48,6 +48,18 @@ public final class FindingVerifierPrompts { that location's content in the diff contradicts the assertion, or the two locations belong to different enclosing units (different functions, blocks, or scopes) without the finding acknowledging it. + - The finding claims a symbol is undefined, unset, missing, or never declared — a + variable, parameter, import, function, or config/env key — and the place its + definition would live (the file's import block, the enclosing scope, the config + or env section) is NOT shown in the provided material. The diff carries only a few + context lines around each change, so absence from it is not proof the symbol is + undefined: the definition can sit in the same file just outside the hunk (for + example, a finding that NEXT/TAG are undefined in a workflow run step when the + step's env: block — a few unchanged lines above the change, outside the hunk + window — defines them). Such a claim is unverifiable here, so reject it. Do NOT + reject when the material does show that scope and the symbol is genuinely absent + or misspelled there (e.g. the diff removes the definition, or the full block is + present and lacks it) — that finding is demonstrable and stands. - The finding misstates language semantics — for example, claiming the string escape "\\n" produces a literal backslash and n rather than a newline. - The diff already guards against the condition the finding claims is unhandled @@ -62,7 +74,10 @@ locations belong to different enclosing units (different functions, blocks, or "medium" risk with "low" confidence. Claims about the contents or behavior of artifacts not shown in the diff (base images, registries, installed packages, remote services) are not demonstrable here: downgrade any such finding above - "medium". + "medium". An "undefined / unset / missing symbol" claim is demonstrable only when + the provided material includes the scope a definition would occupy; when that scope + is outside the shown context the claim is unconfirmed (the definition may sit just + outside the hunk), so reject the finding (per above) rather than post it. Also audit each finding's suggested fix: when the underlying issue is real but suggestion_new is incorrect, incomplete, or would introduce a new defect, return 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 326a3a1c..b5a26e70 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/PrReviewPrompts.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/PrReviewPrompts.java @@ -89,9 +89,13 @@ same enclosing unit (the same function, block, or scope). When the two places ar images, registries, installed packages, remote services) cannot be verified here: they are never "critical" or "high", and the description must be phrased as a verification request naming the exact command or check to run. - - Before claiming a value is missing or a default is wrong, check the provided - material for configuration that already defines it, and name in the description - what you checked. + - Before claiming a name is undefined/unset or a value is missing — a variable, + parameter, import, function, env var, or config key — check the provided material + for its definition, and name in the description what you checked. The diff shows + only a few context lines around each change, so a definition can sit in the same + file just outside the visible hunk: its absence from the hunk is not proof the + name is undefined. When you cannot see the definition, do not assert the name is + undefined. - A suggestion must not contradict a convention visible in the provided material — for example, suggesting an unpinned reference when every similar reference nearby is pinned. When the obvious fix conflicts with such a convention, describe the