Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 (`<<<DIFF_START>>>` / `<<<DIFF_END>>>`) 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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 HIGH β€” Verifier rule will reject valid missing‑symbol findings

The new rejection rule in the verifier prompt instructs the LLM to "Reject such a finding unless the definition IS in the material and shown to be missing or misspelled." A genuine undefined/missing symbol (e.g., a missing import that is not in the diff) has no definition in the provided material. Consequently, the verifier will reject that valid finding, creating a false negative. This is a regression from the previous behaviour where such findings were not automatically discarded. The PR intends to suppress the false positive where a definition exists outside the hunk, but the rule is too broad and will suppress true positives as well.

- 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
Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading