Skip to content

fix(commands): reply instead of declining a command in silence - #541

Merged
devops-thiago merged 1 commit into
release/v0.6.0from
fix/538-summary-decline-reply
Aug 11, 2026
Merged

fix(commands): reply instead of declining a command in silence#541
devops-thiago merged 1 commit into
release/v0.6.0from
fix/538-summary-decline-reply

Conversation

@devops-thiago

Copy link
Copy Markdown
Owner

What type of PR is this?

  • 🐛 Bug fix

Description

Every recognized comment command is acknowledged with a 👀 reaction before it runs, so any
handler that returns without posting leaves the commenter looking at an acknowledged command that
produced nothing — which reads as a hang, not as a decision.

/summary on a PR that already carries a summary did exactly that. The decline was deliberate and
logged (Ignoring /summary — a summary comment is already present on <repo> #<n>) but nothing
reached the PR. It now states the decline and how to get a fresh summary anyway:

📋 A ThrillhouseBot summary is already on this PR, so /summary did nothing. Delete that summary
comment and comment /summary again to regenerate it.

Audit of the same shape. Five other paths acknowledged and then declined in silence; all are
now answered:

Path Was Now
/add-docs, /improve, /generate-tests disabled by their kill switch logged only notice naming the config key an administrator would flip (thrillhousebot.review.<key>=true)
/describe, /changelog, /generate-tests whose generator returned nothing nothing posted notice saying there is no output, and to re-run

Two decisions worth flagging for review:

  • The kill-switch check moved to after the authorization check in all three handlers. Posting
    the "disabled" notice before authorizing would let any commenter make the bot speak on a
    repository whose maintainers switched the command off. Behaviour for authorized users is
    unchanged; unauthorized users now cost one permission lookup on a disabled command.
  • A command from someone without write access stays silent, logged only. That is the one
    deliberate exception to the new rule, for the same anti-abuse reason, and it is now stated in the
    class contract and pinned by a test.

The generator-returned-nothing notice deliberately names both possible causes ("it found nothing to
suggest, or the generation did not complete"): the generators return null for both, and the
handler has no way to tell them apart, so asserting either one would be a claim this code cannot
support.

Related Issues

Fixes #538

How Has This Been Tested?

  • Unit tests

CommentCommandServiceTest — 7 tests written/updated against the unfixed code first. All 7 failed
in exactly the way the issue describes: the command runs, decides, and never posts.

Tests run: 45, Failures: 7, Errors: 0, Skipped: 0
  summaryExplainsItselfWhenSummaryCommentIsLiveOnPr
  describeSaysSoWhenGeneratorReturnsNothing
  changelogSaysSoWhenGeneratorReturnsNothing
  generateTestsSaysSoWhenGeneratorReturnsNothing
  addDocsSaysSoWhenDisabled
  improveSaysSoWhenDisabled
  generateTestsSaysSoWhenDisabled

Verbatim red output for the issue's own case:

dev.thiagogonzaga.thrillhousebot.webhook.CommentCommandServiceTest.summaryExplainsItselfWhenSummaryCommentIsLiveOnPr -- Time elapsed: 0.002 s <<< FAILURE!
Wanted but not invoked:
commentClient.createComment(
    <any>,
    <any>,
    "owner",
    "repo",
    7,
    <Capturing argument: CreateCommentRequest>
);
-> at dev.thiagogonzaga.thrillhousebot.webhook.CommentCommandServiceTest.postedBody(CommentCommandServiceTest.java:115)
Actually, there were zero interactions with this mock.

	at dev.thiagogonzaga.thrillhousebot.webhook.CommentCommandServiceTest.summaryExplainsItselfWhenSummaryCommentIsLiveOnPr(CommentCommandServiceTest.java:168)

Green after the fix: Tests run: 45, Failures: 0, Errors: 0, Skipped: 0.

Gates (Java 25):

  • ./mvnw -B clean compile spotbugs:check spotless:checkBugInstance size is 0, BUILD SUCCESS
  • ./mvnw -B clean testTests run: 2595, Failures: 0, Errors: 0, Skipped: 0
  • Coverage ∩ git diff -U0 abd76e5...HEAD on changed main code → 17 executable changed lines,
    17 fully covered, 0 uncovered lines and 0 uncovered branches

Checklist

  • My code follows the project's coding standards
  • I have performed a self-review of my own code
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have updated the documentation accordingly
  • My changes generate no new warnings or errors

Additional Notes

/add-docs and /improve already post their own outcome from DocGenerationService /
PrImprovementService, so they need no reply here beyond the kill-switch case. No behaviour change
for any command that was already producing output.

Every recognized comment command gets a 👀 acknowledgment before it is
executed, so a handler that returns without posting leaves the commenter
looking at an acknowledged command that produced nothing — indistinguishable
from a hang. `/summary` on a PR that already carries a summary did exactly
that: the decline was deliberate and logged, but nothing reached the PR.

State the decline instead, and say how to get a fresh summary anyway
(delete the existing summary comment and re-run). The same shape was in
five other places, all now answered:

- `/add-docs`, `/improve` and `/generate-tests` switched off by their kill
  switch — the notice names the config key an administrator would flip
- `/describe`, `/changelog` and `/generate-tests` whose generator produced
  nothing to post

The kill-switch checks moved to after the authorization check so the new
notice cannot be used to make the bot post on a repository whose
maintainers turned the command off. A command from someone without write
access stays silent and logged — the one deliberate exception, since
replying there would let any commenter make the bot speak.

Fixes #538
@github-actions

Copy link
Copy Markdown
Contributor

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

@codecov

codecov Bot commented Aug 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@thrillhousebot

Copy link
Copy Markdown
Contributor

🤖 ThrillhouseBot PR Summary

What this PR does

Recognized slash-commands now produce a visible reply instead of silently declining: /summary says a summary already exists, disabled commands name the config key to re-enable, and null generator output gets a no-output notice. The kill-switch check is moved after the authorization check so only authorized users can trigger the disabled-command notice.

Control-Flow Diagram

🔀 Show diagram
flowchart TD
  A["Webhook: recognized command + 👀 ack"] --> B{"Authorized?"}
  B -- "No" --> C["Log only; no reply"]
  B -- "Yes" --> D{"Command disabled?"}
  D -- "Yes" --> E["Post disabled notice naming config key"]
  D -- "No" --> F["Run command handler"]
  F --> G{"Summary already live?"}
  G -- "Yes" --> H["Post summary-already-present notice"]
  G -- "No" --> I["Dispatch summary review"]
  F --> J{"Generator returned null?"}
  J -- "Yes" --> K["Post no-output notice"]
  J -- "No" --> L["Post generated output"]
Loading

Changes Overview

  • Files changed: 2
  • Lines added: +106
  • Lines removed: -41

Changed Files

File Change Summary
src/main/java/dev/thiagogonzaga/thrillhousebot/webhook/CommentCommandService.java Modified Adds visible PR replies for live-summary declines, disabled commands, and null generator output; reorders auth before kill-switch checks.
src/test/java/dev/thiagogonzaga/thrillhousebot/webhook/CommentCommandServiceTest.java Modified Updates unit tests to assert posted notices for summary, disabled, and no-output paths, plus silence for disabled-and-unauthorized.

Risk Assessment

Risk Count
🔴 Critical 0
🟠 High 0
🟡 Medium 0
🔵 Low 0

No new issues found in this PR, but the review cannot be approved until CI is confirmed green.

⚠️ CI Checks Status

Some checks are still pending or have failed:

Check Type Status Detail
trivy check-run ⏳ Pending -
frontend check-run ⏳ Pending -
test check-run ⏳ Pending -
format check-run ⏳ Pending -
changes check-run ⏳ Pending -
actionlint check-run ⏳ Pending -
dependency-review check-run ⏳ Pending -

Automated review by ThrillhouseBot. Reply with /review to re-run.

@thrillhousebot thrillhousebot Bot added bug Something isn't working java Pull requests that update java code testing Test coverage and test quality labels Aug 11, 2026
@sonarqubecloud

Copy link
Copy Markdown

@devops-thiago
devops-thiago merged commit 1d7df30 into release/v0.6.0 Aug 11, 2026
14 checks passed
@devops-thiago
devops-thiago deleted the fix/538-summary-decline-reply branch August 11, 2026 19:58
devops-thiago added a commit that referenced this pull request Aug 13, 2026
## What type of PR is this?

- [x] 📝 Documentation
- [x] 🏗️ CI/CD

## Description

Prepares the 0.6.0 release: bumps the project version from
`0.5.1-SNAPSHOT` to `0.6.0` and turns the
`[Unreleased]` changelog section into `[0.6.0]`.

The changelog is rewritten so it describes what changes for someone
upgrading from 0.5.0. Entries
recording fixes to work done inside this cycle are dropped, because the
defects they name never
shipped — a reader upgrading from 0.5.0 never saw them. Twenty-two issue
references go: the
truncation-salvager follow-ups (#582, #592, #617), the spend-ceiling
disclosure fixes (#521, #524,
#530), the write-pacing follow-ups (#541, #542, #577, #598), the
verifier hedging refinements (#594,
#610), and others of the same kind.

What stays is the features 0.6.0 adds, the behaviour it changes, and the
defects that were present in
0.5.0. Membership in that last group was decided against the `v0.5.0`
tag rather than from when an
issue was filed, since every issue in this cycle was filed after 0.5.0
shipped:

| check against `v0.5.0` | result | consequence |
|---|---|---|
| `Dockerfile.runtime` present | yes | image fixes (#561, #564) shipped,
kept |
| streaming in `AiReviewService` | yes | empty SSE frame (#555) shipped,
kept |
| `List.copyOf` in `DiffBudgetPlanner` | 4 uses | null-filename crash
(#472, #551) shipped, kept |
| retry loop in `AiReviewService` | yes | length-cap retry (#495, #504)
shipped, kept |
| `FindingVerifierPrompts` present | yes | verifier precision
(#611#614) shipped, kept |
| `TruncatedResponseSalvager` | absent | its follow-ups are
cycle-internal, dropped |
| `RebuttalContradiction`, `ConfigKeyContextResolver`, `PatchCoverage`,
`GitHubWriteRetry` | absent | same, dropped |

The prose follows the guidance on
[signs of AI
writing](https://en.wikipedia.org/wiki/Wikipedia:Signs_of_AI_writing):
plain sentences,
no promotional framing, and the em-dash and "X instead of Y" habits cut
back to where they carry
meaning.

Also corrects the `max-diff-lines` comment in `application.properties`,
which still listed
`/describe`, `/changelog` and `/add-docs` as capped by it after this
cycle moved those commands onto
token-budgeted batches. The README table already described the new
behaviour, so the two disagreed
and the properties file was the wrong one. Reported by the bot on #532.

## Also in this PR

Three gaps found after the first commit, each a convention the 0.5.0 cut
followed and this one had
missed.

**Dependency bumps.** 0.4.0 and 0.5.0 both carry a `### Dependencies`
section and 0.6.0 had none.
Added: the Quarkus platform 3.37.4 to 3.38.0 and `quarkus-langchain4j`
1.12.0 to 1.12.2;
`jackson-dataformat-yaml` declared explicitly now that reading a
repository's own
`.github/thrillhousebot.yml` uses it directly; frontend `next` to
16.2.12 and dev-only `jsdom` to
30.0.1; `github/codeql-action` to v4.37.6, `actions/setup-java` to
v5.7.0, `docker/login-action` to
v4.6.0; Spotless to 3.9.0. A fresh empty `[Unreleased]` is left above
the release section, as the
0.5.0 cut did.

**Architecture documentation.** `docs/ARCHITECTURE.md` was last touched
on 2026-08-08 and eight of
this cycle's capabilities appeared nowhere in it — verified at zero
occurrences each before the
change: the per-review spend ceiling, the `concise` model binding, patch
coverage as review context,
repository-supplied ignore globs, path-scoped instructions, the decline
re-check, write pacing, and
the coverage-honesty rules that decide when a file withholds APPROVE.
The Packages table also
predated the classes carrying them. Both updated, at the subsystem level
the document is written at.

README needed nothing: all 52 environment variables and all 12 commands
are already documented
there, and the website's Configuration and Commands pages are
include-wrappers over README sections,
so they track it automatically.

**Docs freeze.** `versions.json` listed `current: v0.5.0` with archives
stopping at 0.4.0, so 0.5.0
shipped without being frozen — the same lapse the 0.5.0 cut found for
0.4.0. Because
`archive-docs-version.mjs` expands the include markers at archive time,
archiving the working tree
would have produced a directory labelled v0.5.0 documenting `/improve`
and the spend ceiling. The doc
sources were restored to the `v0.5.0` tag, archived, then reverted.
Verified both directions:

| check | archived 0.5.0 pages | live tree |
|---|---|---|
| `REVIEW_MAX_TOKENS_PER_REVIEW` | 0 | 1 |
| `/improve` | 0 | present |
| `REVIEW_CI_GATING` | 1 | — |

Nine pages with matching assets and `src/content/versions/0.5.0.json`;
`current.label` moves to
v0.6.0. Unlike the 0.4.0 archive, the tag restore resurrected no deleted
files. The site builds: 75
pages, all internal links valid.

## Related Issues

N/A — release mechanics.

## How Has This Been Tested?

- [x] Unit tests

`./mvnw -B clean test` → `Tests run: 3067, Failures: 0, Errors: 0,
Skipped: 0`, building
`thrillhousebot 0.6.0`. `./mvnw -B clean compile spotbugs:check
spotless:check` → `BugInstance size
is 0`.

No production code changes, so there is nothing to cover: the diff is
the version string, the
changelog, the architecture document, one properties comment and the
frozen 0.5.0 docs.

The docs site was built as well (`npm ci && npm run build` in
`website/`): 75 pages, all internal
links valid.

## Checklist

- [x] My code follows the project's coding standards
- [x] I have performed a self-review of my own code
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working java Pull requests that update java code testing Test coverage and test quality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant