diff --git a/.env.example b/.env.example index 1a868f7b..e1a12fc3 100644 --- a/.env.example +++ b/.env.example @@ -60,6 +60,11 @@ GITHUB_WEBHOOK_SECRET=your_webhook_secret # (set to false to disable) #REVIEW_GENERATE_TESTS_ENABLED=true +# Optional: let /describe REPLACE the PR's title and body with the generated suggestion instead of +# only posting it as a comment (set to true to enable). The previous title and description are +# preserved in the bot's confirmation comment, and only write-authorized users can trigger it. +#REVIEW_DESCRIBE_APPLY=false + # Optional: include an opt-in Mermaid control-flow diagram in the PR summary (set to true to enable) #REVIEW_DIAGRAM_ENABLED=false diff --git a/CHANGELOG.md b/CHANGELOG.md index 17c7120b..3f3ef0d6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,10 @@ All notable changes to ThrillhouseBot. ## [Unreleased] +### Added + +- **Opt-in apply mode for `/describe`** (#325): with `REVIEW_DESCRIBE_APPLY=true`, a write-authorized `/describe` replaces the PR's title and body with the generated suggestion instead of only posting it as a comment. Off by default — suggest-only stays the released behaviour. The confirmation comment preserves the previous title and description so the overwrite is never destructive, the apply is logged for audit, and a run whose output doesn't parse or whose GitHub update fails falls back to the plain suggestion comment + ## [0.6.2] — 2026-08-14 Follow-ups to the review threads on 0.6.1, plus the first piece of the release diff --git a/README.md b/README.md index 3d08b187..ddcefaab 100644 --- a/README.md +++ b/README.md @@ -89,7 +89,7 @@ not a reaction. | `/help` | List the available commands | anyone | | `/review` | Run (or re-run) a full review of the PR | write | | `/summary` | Post the PR summary if it isn't already on the PR — regenerates it if the comment was deleted, otherwise no-op | write | -| `/describe` | Suggest an improved PR title and description generated from the diff, as a comment to copy in (never overwrites the PR) | write | +| `/describe` | Suggest an improved PR title and description generated from the diff, as a comment to copy in. Never overwrites the PR by default; a deployment that opts in with `REVIEW_DESCRIBE_APPLY=true` has it replace the PR's title and body instead | write | | `/changelog` | Draft a CHANGELOG entry for the PR from the diff (Added/Changed/Fixed/Security…), as a comment to copy into `CHANGELOG.md` (never commits) | write | | `/add-docs` | Generate docstrings/inline docs for the symbols changed in the PR, posted as committable suggestions (or a note with the drafted docs when a multi-line declaration can't be pinned to a single diff hunk) | write | | `/improve` | Run a whole-PR improvement pass over the diff and post the improvements as committable suggestions (with copy-paste blocks for the ones that can't be pinned to the diff) | write | @@ -130,6 +130,16 @@ extra model call, reserved out of `REVIEW_MAX_AI_CALLS` and spent only when the more than one batch, so a run never exceeds the same ceiling as one review. Any file the budget could not cover is named in a partial-coverage note under the suggestion. +**`/describe` apply mode (opt-in)** — by default `/describe` only posts a suggestion comment +and never touches the PR. A deployment that sets `REVIEW_DESCRIBE_APPLY=true` changes that: +`/describe` then **replaces the PR's title and body** with the generated suggestion, so enable +it only where maintainers expect the bot to edit their PRs. The edit is gated twice — it needs +that explicit config *and* a commenter who passes the same write-access check as every command — +and it is never destructive: the confirmation comment the bot posts carries the previous title +and description, so anything replaced stays recoverable on the PR. When the model output doesn't +parse into a title and description, or the GitHub update fails, the run falls back to posting the +plain suggestion comment instead of editing the PR. + **`/add-docs`** — on demand, the bot reads the diff and proposes documentation comments for the public symbols changed in the PR, honoring the repository instructions and each file's language. Each suggestion is a committable `suggestion` block placed on the symbol's @@ -305,6 +315,7 @@ will change per provider: | `REVIEW_ADD_DOCS_ENABLED` | Allow the on-demand `/add-docs` command to generate docstrings as committable suggestions | `true` | | `REVIEW_IMPROVE_ENABLED` | Allow the on-demand `/improve` command to run a whole-PR improvement pass and post committable suggestions | `true` | | `REVIEW_GENERATE_TESTS_ENABLED` | Allow the on-demand `/generate-tests` command to propose unit tests for the changed code | `true` | +| `REVIEW_DESCRIBE_APPLY` | ⚠️ Opt-in auto-edit: make `/describe` **replace the PR's title and body** with the generated suggestion instead of only posting it as a comment (see [Commands](#commands)). The previous title and description are preserved in the confirmation comment, and the edit still requires a write-authorized commenter | `false` | | `REVIEW_DIAGRAM_ENABLED` | Include an opt-in Mermaid control-flow diagram in the PR summary | `false` | | `REVIEW_PATCH_COVERAGE_ENABLED` | Feed patch coverage into the review context: the added lines the repository's own coverage report records as never executed (see [Repository configuration](#repository-configuration)). Only takes effect for a repository that names its coverage artifact in `.github/thrillhousebot.yml` | `false` | | `REVIEW_FOLLOW_UP_SUMMARY_ENABLED` | Post a short delta comment on follow-up reviews with the new-finding, resolved, and still-open counts. Only the first review posts the full summary; a follow-up pass with no delta (nothing new, nothing resolved) posts nothing | `false` | diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/config/ThrillhouseConfig.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/config/ThrillhouseConfig.java index 5cd303b6..c79eee47 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/config/ThrillhouseConfig.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/config/ThrillhouseConfig.java @@ -396,6 +396,8 @@ static String normalizeCiGating(String raw) { LabelsConfig labels(); + DescribeConfig describe(); + DiagramConfig diagram(); @WithName("patch-coverage") @@ -516,6 +518,22 @@ interface LabelsConfig { int maxLabels(); } + /** + * Opt-in apply mode for the {@code /describe} command, mirroring {@link LabelsConfig#apply()}: + * suggestion-only is the released behaviour, and letting the bot mutate the PR is the operator's + * explicit call. + */ + interface DescribeConfig { + /** + * When {@code true}, a {@code /describe} from a write-authorized user replaces the PR's title + * and body with the generated suggestion; the previous title and body are preserved in the + * confirmation comment, so the overwrite is never destructive. When {@code false} (the + * default), the suggestion is only posted as a comment to copy in and the PR is never edited. + */ + @WithDefault("false") + boolean apply(); + } + interface DashboardConfig { /** Public base URL of the dashboard, used for session deep-links posted to GitHub. */ @WithName("url") diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubPullRequestClient.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubPullRequestClient.java index 85e9e486..d5eaa7af 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubPullRequestClient.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubPullRequestClient.java @@ -56,6 +56,38 @@ default PullRequestDetails getPullRequest( credential -> getPullRequestOnce(credential, accept, owner, repo, pullNumber)); } + /** One HTTP attempt at editing a PR. Callers want {@link #updatePullRequest} instead. */ + @PATCH + @Path("/repos/{owner}/{repo}/pulls/{pullNumber}") + @Produces(MediaType.APPLICATION_JSON) + @Consumes(MediaType.APPLICATION_JSON) + PullRequestDetails updatePullRequestOnce( + @HeaderParam("Authorization") String auth, + @HeaderParam("Accept") String accept, + @PathParam("owner") String owner, + @PathParam("repo") String repo, + @PathParam("pullNumber") int pullNumber, + UpdatePullRequestRequest request); + + /** + * Replaces a PR's title and body — the opt-in {@code /describe} apply path — with the same + * throttle backoff every other GitHub write gets. Losing this write to a throttle would post a + * confirmation comment describing an edit that never happened, so it retries like the comment + * writes do rather than only healing a rejected credential. + */ + default PullRequestDetails updatePullRequest( + String auth, + String accept, + String owner, + String repo, + int pullNumber, + UpdatePullRequestRequest request) { + return GitHubWriteRetry.DEFAULT.call( + "an update of the title/body of PR " + owner + "/" + repo + "#" + pullNumber, + auth, + credential -> updatePullRequestOnce(credential, accept, owner, repo, pullNumber, request)); + } + /** One HTTP attempt at a files page. Callers want {@link #getPullRequestFilesPage} instead. */ @GET @Path("/repos/{owner}/{repo}/pulls/{pullNumber}/files") @@ -194,6 +226,29 @@ public Ref(String sha) { } } + /** GitHub's hard maximum PR-title length, in characters; a longer title is rejected with 422. */ + int TITLE_MAX_LENGTH = 256; + + /** + * Body of the PR update PATCH. Only the title and body fields are sent, so nothing else about the + * PR (state, base, …) can change. Both fields are capped to the limit GitHub enforces for them, + * like every other outgoing text field: the body with the shared truncation notice, the title + * with a plain cut because the multi-line notice cannot go in a single-line field. + */ + record UpdatePullRequestRequest(String title, String body) { + public UpdatePullRequestRequest { + if (title != null && title.length() > TITLE_MAX_LENGTH) { + int keep = TITLE_MAX_LENGTH; + // Never leave a dangling high surrogate at the cut point. + if (Character.isHighSurrogate(title.charAt(keep - 1))) { + keep--; + } + title = title.substring(0, keep); + } + body = CommentBodyLimit.cap(body); + } + } + record FileDiff( String filename, String status, // added, modified, removed, renamed diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/PrDescriptionGenerator.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/PrDescriptionGenerator.java index 095e5e15..2d659b17 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/PrDescriptionGenerator.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/PrDescriptionGenerator.java @@ -29,12 +29,16 @@ import jakarta.inject.Inject; import java.util.ArrayList; import java.util.List; +import java.util.regex.Pattern; import org.eclipse.microprofile.rest.client.inject.RestClient; /** * Builds the {@code /describe} suggestion: an improved PR title and description generated from the - * diff, posted as a comment the author may copy in. It never edits the pull request, so the - * author's own title and body are never overwritten. + * diff, posted as a comment the author may copy in. By default it never edits the pull request, so + * the author's own title and body are never overwritten; a deployment that opts in with {@code + * thrillhousebot.review.describe.apply=true} instead has the caller apply the suggestion to the PR, + * for which {@link #generateSuggestion} also parses the title and description out of the model + * output and prepares a confirmation comment that preserves what was replaced. * *

Loads the PR's current title/body and diff and the repository instructions (via {@link * AbstractPrSuggestionGenerator}), asks the {@link PrDescribeAssistant} for a suggestion, then @@ -78,6 +82,28 @@ public class PrDescriptionGenerator extends AbstractPrSuggestionGenerator { description. Re-run with `/describe`.* """; + static final String APPLIED_HEADER = "## 🤖 ThrillhouseBot — PR title & description updated\n\n"; + + static final String APPLIED_FOOTER = + """ + + + --- + *Applied by `/describe` — this deployment opts in with \ + `thrillhousebot.review.describe.apply=true`. The previous title and description are \ + preserved above. Edit the PR to adjust, or re-run `/describe` after more changes.* + """; + + /** + * The two required sections of the model's answer, in the exact shape the prompts demand. The + * title line's wrapping backticks (and a stray blank line before it) are tolerated and stripped, + * because the title goes into the PR's single-line title field verbatim. + */ + private static final Pattern SUGGESTION_SECTIONS = + Pattern.compile( + "###\\s+Suggested title\\s*\\R+(.+?)\\R+\\s*###\\s+Suggested description\\s*\\R+(.+)", + Pattern.DOTALL); + private final PrDescribeAssistant describeAssistant; @Inject @@ -101,6 +127,21 @@ public PrDescriptionGenerator( this.describeAssistant = describeAssistant; } + /** + * Everything one {@code /describe} run produced. {@code suggestBody} is the suggestion comment of + * the default suggest-only path. {@code title}, {@code description} and {@code applyBody} serve + * the opt-in apply path: the parsed pieces to PATCH onto the PR, and the confirmation comment — + * carrying the replaced title and body — to post once the PATCH succeeded. All three are {@code + * null} when the model output did not parse into the two required sections (or when there was + * nothing to describe at all), leaving the suggestion comment as the only thing to post. + */ + public record Suggestion(String title, String description, String suggestBody, String applyBody) { + /** Whether this suggestion parsed into pieces the apply path can put on the PR. */ + public boolean applicable() { + return title != null && description != null && applyBody != null; + } + } + /** * Generates the suggestion comment body for a PR, or {@code null} when there is nothing to * suggest (no diff) or the model produced no usable answer. The caller is responsible for posting @@ -117,6 +158,32 @@ public String generate( String defaultBranch, long installationId, String auth) { + var suggestion = doGenerate(owner, repo, prNumber, defaultBranch, installationId, auth); + return suggestion == null ? null : suggestion.suggestBody(); + } + + /** + * The {@link #generate} run with its pieces kept apart, for the opt-in apply path. Same contract: + * {@code null} when there is nothing to suggest or no usable answer came back. + */ + @ActivateRequestContext + public Suggestion generateSuggestion( + String owner, + String repo, + int prNumber, + String defaultBranch, + long installationId, + String auth) { + return doGenerate(owner, repo, prNumber, defaultBranch, installationId, auth); + } + + private Suggestion doGenerate( + String owner, + String repo, + int prNumber, + String defaultBranch, + long installationId, + String auth) { var inputs = loadInputs(owner, repo, prNumber, defaultBranch, installationId, auth, COMMAND); if (inputs == null) { return null; @@ -139,7 +206,9 @@ public String generate( // Nothing fitted, but the files that did not are known: name them rather than go quiet. A // plan that covered nothing and omitted nothing means no file was in scope at all (every one // ignored), which is genuinely nothing to say. - return plan.truncated() ? NOT_COVERED + disclosure(plan) : null; + return plan.truncated() + ? new Suggestion(null, null, NOT_COVERED + disclosure(plan), null) + : null; } var drafted = describeEachBatch(inputs, plan); if (drafted.partials().isEmpty()) { @@ -149,12 +218,82 @@ public String generate( if (suggestion == null) { return null; } - return HEADER - + suggestion - + batchFailureNote( - drafted.failedBatches(), COMMAND, "the files in them are not described here.") - + FOOTER - + disclosure(plan); + var note = + batchFailureNote( + drafted.failedBatches(), COMMAND, "the files in them are not described here."); + var suggestBody = HEADER + suggestion + note + FOOTER + disclosure(plan); + var parsed = parseSections(suggestion); + if (parsed == null) { + return new Suggestion(null, null, suggestBody, null); + } + var applyBody = + APPLIED_HEADER + previousContent(inputs) + note + APPLIED_FOOTER + disclosure(plan); + return new Suggestion(parsed.title(), parsed.description(), suggestBody, applyBody); + } + + /** The parsed sections of a well-formed answer; see {@link #parseSections}. */ + record TitleAndDescription(String title, String description) {} + + /** + * Extracts the proposed title and description from the model's answer, or {@code null} when the + * answer does not carry both sections in the demanded shape. Only the apply path needs this — a + * suggestion comment posts the answer as-is — so a shape the parser cannot read degrades the run + * to suggest-only rather than failing it. + */ + static TitleAndDescription parseSections(String suggestion) { + if (suggestion == null) { + return null; + } + var matcher = SUGGESTION_SECTIONS.matcher(suggestion); + if (!matcher.find()) { + return null; + } + var title = + matcher + .group(1) + .lines() + .map(String::strip) + .filter(line -> !line.isEmpty()) + .findFirst() + .orElse(""); + if (title.startsWith("`")) { + // A title opening with a backtick is only readable as one wrapping pair. Anything else — an + // opener with no close on the line, a multi-backtick wrapper, a bare ``` fence line — would + // be applied verbatim, backticks and all, so those degrade to suggest-only instead. (A title + // merely *ending* in an inline code span, like "fix: guard `null`", stays accepted: only an + // opening backtick makes the line read as a wrapper.) + if (title.length() < 2 || !title.endsWith("`")) { + return null; + } + String inner = title.substring(1, title.length() - 1).strip(); + if (inner.isEmpty() || inner.startsWith("`") || inner.endsWith("`")) { + return null; + } + title = inner; + } + var description = matcher.group(2).strip(); + if (title.isEmpty() || description.isEmpty()) { + return null; + } + return new TitleAndDescription(title, description); + } + + /** + * The replaced title and body, collapsed into the confirmation comment so the apply overwrite is + * never destructive: whatever `/describe` replaced stays recoverable on the PR itself. + */ + private static String previousContent(Inputs inputs) { + var title = inputs.title() == null || inputs.title().isBlank() ? "_(none)_" : inputs.title(); + var body = + inputs.body() == null || inputs.body().isBlank() ? "_(no description)_" : inputs.body(); + return "The title and description of this pull request were replaced with the suggestion" + + " ThrillhouseBot generated from the diff.\n\n" + + "

\nPrevious title and description\n\n" + + "**Title:** " + + title + + "\n\n" + + body + + "\n\n
"; } /** The per-batch partial descriptions that came back, plus how many batch calls failed. */ diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/webhook/CommentCommandService.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/webhook/CommentCommandService.java index 1bb26db6..de10b20f 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/webhook/CommentCommandService.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/webhook/CommentCommandService.java @@ -19,6 +19,7 @@ import dev.thiagogonzaga.thrillhousebot.config.ThrillhouseConfig; import dev.thiagogonzaga.thrillhousebot.github.GitHubAuthClient; import dev.thiagogonzaga.thrillhousebot.github.GitHubCommentClient; +import dev.thiagogonzaga.thrillhousebot.github.GitHubPullRequestClient; import dev.thiagogonzaga.thrillhousebot.github.GitHubReviewClient; import dev.thiagogonzaga.thrillhousebot.github.ReviewThreadService; import dev.thiagogonzaga.thrillhousebot.review.ChangelogEntryGenerator; @@ -95,6 +96,7 @@ public class CommentCommandService { private final ExecutorService executor; private final GitHubAuthClient authClient; private final GitHubCommentClient commentClient; + private final GitHubPullRequestClient prClient; private final GitHubReviewClient reviewClient; private final ReviewThreadService reviewThreadService; private final ReviewDispatcher reviewDispatcher; @@ -114,6 +116,7 @@ public CommentCommandService( @ReviewExecutor ExecutorService executor, GitHubAuthClient authClient, @RestClient GitHubCommentClient commentClient, + @RestClient GitHubPullRequestClient prClient, @RestClient GitHubReviewClient reviewClient, ReviewThreadService reviewThreadService, ReviewDispatcher reviewDispatcher, @@ -130,6 +133,7 @@ public CommentCommandService( this.executor = executor; this.authClient = authClient; this.commentClient = commentClient; + this.prClient = prClient; this.reviewClient = reviewClient; this.reviewThreadService = reviewThreadService; this.reviewDispatcher = reviewDispatcher; @@ -251,6 +255,10 @@ private void handleDescribe(CommandContext ctx, String auth) { postComment(auth, ctx, PAUSED_NOTICE); return; } + if (config.review().describe().apply()) { + describeAndApply(ctx, auth); + return; + } log.info( "Generating title/description suggestion for {}/{} #{} (triggered by @{})", ctx.owner(), @@ -268,6 +276,73 @@ private void handleDescribe(CommandContext ctx, String auth) { postComment(auth, ctx, suggestion != null ? suggestion : noOutputNotice("/describe")); } + /** + * The opt-in {@code /describe} apply path ({@code thrillhousebot.review.describe.apply=true}): + * replaces the PR's title and body with the generated suggestion, then posts the confirmation + * comment that preserves what was replaced. It reaches the PR only behind the same write-access + * gate as every command plus that explicit config, and it degrades to the suggest-only comment — + * never to silence, and never to a confirmation of an edit that did not happen — both when the + * model output did not parse into a title and description and when the PR update itself failed. + */ + private void describeAndApply(CommandContext ctx, String auth) { + log.info( + "Generating title/description for {}/{} #{} to apply (triggered by @{})", + ctx.owner(), + ctx.repo(), + num(ctx), + ctx.login()); + var suggestion = + descriptionGenerator.generateSuggestion( + ctx.owner(), + ctx.repo(), + ctx.prNumber(), + ctx.defaultBranch(), + ctx.installationId(), + auth); + if (suggestion == null) { + postComment(auth, ctx, noOutputNotice("/describe")); + return; + } + if (!suggestion.applicable()) { + log.info( + "/describe output for {}/{} #{} did not parse into a title and description —" + + " posting it as a suggestion instead", + ctx.owner(), + ctx.repo(), + num(ctx)); + postComment(auth, ctx, suggestion.suggestBody()); + return; + } + try { + prClient.updatePullRequest( + auth, + ACCEPT, + ctx.owner(), + ctx.repo(), + ctx.prNumber(), + new GitHubPullRequestClient.UpdatePullRequestRequest( + suggestion.title(), suggestion.description())); + } catch (RuntimeException e) { + log.warn( + "Failed to update the title/body of {}/{} #{} — posting the suggestion instead", + ctx.owner(), + ctx.repo(), + num(ctx), + e); + postComment(auth, ctx, suggestion.suggestBody()); + return; + } + // The audit trail of the overwrite: this log line for the operator, and the confirmation + // comment below — carrying the replaced title and body — for the PR itself. + log.info( + "Applied /describe to {}/{} #{} (triggered by @{}): PR title and description replaced", + ctx.owner(), + ctx.repo(), + num(ctx), + ctx.login()); + postComment(auth, ctx, suggestion.applyBody()); + } + private void handleChangelog(CommandContext ctx, String auth) { if (!authorized(ctx)) { log.info("Ignoring unauthorized /changelog from @{} on PR #{}", ctx.login(), num(ctx)); diff --git a/src/main/resources/application.properties b/src/main/resources/application.properties index 42e722cc..7c6e4177 100644 --- a/src/main/resources/application.properties +++ b/src/main/resources/application.properties @@ -240,6 +240,10 @@ thrillhousebot.review.add-docs-enabled=${REVIEW_ADD_DOCS_ENABLED:true} thrillhousebot.review.improve-enabled=${REVIEW_IMPROVE_ENABLED:true} # Allow the on-demand /generate-tests command to propose unit tests for the changed code thrillhousebot.review.generate-tests-enabled=${REVIEW_GENERATE_TESTS_ENABLED:true} +# Opt-in apply mode for /describe. false (default) = the suggested title/description is only posted +# as a comment to copy in. true = a write-authorized /describe REPLACES the PR's title and body with +# the suggestion; the previous title and body are preserved in the confirmation comment. +thrillhousebot.review.describe.apply=${REVIEW_DESCRIBE_APPLY:false} # Generated/vendored/build artifacts skipped by review across common ecosystems; never real source. thrillhousebot.review.ignored-files=**/pom.xml,**/package-lock.json,**/pnpm-lock.yaml,**/go.sum,**/*.lock,**/*.generated.*,**/*.pb.go,**/*_pb2.py,**/*.min.js,**/*.min.css,**/*.map,**/target/**,**/node_modules/**,**/dist/**,**/build/**,**/out/**,**/.next/**,**/vendor/**,**/__pycache__/**,**/.venv/**,**/bin/**,**/obj/** # Upper bound on the manual-trigger write-access check (token mint + collaborator-permission call) diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/config/DescribeApplyDefaultOffTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/config/DescribeApplyDefaultOffTest.java new file mode 100644 index 00000000..99dc5e98 --- /dev/null +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/config/DescribeApplyDefaultOffTest.java @@ -0,0 +1,40 @@ +/* + * 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.config; + +import static org.junit.jupiter.api.Assertions.assertFalse; + +import io.quarkus.test.junit.QuarkusTest; +import jakarta.inject.Inject; +import org.junit.jupiter.api.Test; + +/** + * Default profile: {@code /describe} apply mode is off, so an untouched deployment keeps the + * suggest-only command it has today and the bot never edits a PR's title or body without the + * operator's explicit opt-in. Asserted through the resolved configuration, not the + * {@code @WithDefault} annotation, so the {@code thrillhousebot.review.describe.apply} property + * wiring is covered too. + */ +@QuarkusTest +class DescribeApplyDefaultOffTest { + + @Inject ThrillhouseConfig config; + + @Test + void describeApplyIsOffByDefault() { + assertFalse(config.review().describe().apply()); + } +} diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubPullRequestClientTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubPullRequestClientTest.java index 300e6186..965ac42a 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubPullRequestClientTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubPullRequestClientTest.java @@ -16,6 +16,7 @@ package dev.thiagogonzaga.thrillhousebot.github; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.mock; @@ -114,4 +115,40 @@ void deserializesAuthoritativePrTotalsFromTheGitHubSnakeCaseFields() throws Exce assertEquals(975, pr.additions()); assertEquals(196, pr.deletions()); } + + @Test + void updateRequestPassesShortFieldsThroughUnchanged() { + var request = new GitHubPullRequestClient.UpdatePullRequestRequest("feat: title", "Body."); + + assertEquals("feat: title", request.title()); + assertEquals("Body.", request.body()); + } + + @Test + void updateRequestCapsTheTitleAtGitHubsLimitWithoutAMultiLineNotice() { + var request = new GitHubPullRequestClient.UpdatePullRequestRequest("t".repeat(300), "Body."); + + assertEquals(GitHubPullRequestClient.TITLE_MAX_LENGTH, request.title().length()); + // A title is a single-line field, so the cut is plain — the shared truncation notice would + // put newlines into it. + assertEquals(-1, request.title().indexOf('\n')); + } + + @Test + void updateRequestCapsTheBodyLikeEveryOtherOutgoingCommentField() { + var request = new GitHubPullRequestClient.UpdatePullRequestRequest("t", "b".repeat(70_000)); + + assertEquals(CommentBodyLimit.MAX_LENGTH, request.body().length()); + assertTrue(request.body().endsWith(CommentBodyLimit.TRUNCATION_NOTICE)); + } + + @Test + void updateRequestSerializesTitleAndBodyOnlySoNothingElseAboutThePrCanChange() throws Exception { + var json = + new ObjectMapper() + .writeValueAsString( + new GitHubPullRequestClient.UpdatePullRequestRequest("feat: title", "Body.")); + + assertEquals("{\"title\":\"feat: title\",\"body\":\"Body.\"}", json); + } } diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/PrDescriptionGeneratorTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/PrDescriptionGeneratorTest.java index 194d2313..066d7705 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/PrDescriptionGeneratorTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/PrDescriptionGeneratorTest.java @@ -526,4 +526,161 @@ private static FileDiff thirdFile() { +cache.put("k", "v"); +return cache;"""); } + + private static final String WELL_FORMED_ANSWER = + """ + ### Suggested title + `feat: add the widget` + + ### Suggested description + Adds the widget. + + - detail one + - detail two"""; + + private PrDescriptionGenerator.Suggestion generateSuggestion() { + return generator.generateSuggestion("owner", "repo", 7, "main", 12345L, AUTH); + } + + @Test + void suggestionCarriesTheParsedTitleAndDescriptionForTheApplyPath() { + prWithFiles(foo()); + describeReturns(WELL_FORMED_ANSWER); + + var suggestion = generateSuggestion(); + + assertNotNull(suggestion); + assertTrue(suggestion.applicable()); + assertEquals("feat: add the widget", suggestion.title()); + assertEquals("Adds the widget.\n\n- detail one\n- detail two", suggestion.description()); + // The suggest body is byte-identical to what generate() posts on the suggest-only path. + assertEquals(generate(), suggestion.suggestBody()); + } + + @Test + void applyBodyPreservesThePreviousTitleAndDescription() { + prWithFilesAndDetails(new PullRequestDetails("Old title", "Old body text", null, null)); + describeReturns(WELL_FORMED_ANSWER); + + var suggestion = generateSuggestion(); + + assertNotNull(suggestion); + assertTrue(suggestion.applicable()); + assertTrue(suggestion.applyBody().startsWith(PrDescriptionGenerator.APPLIED_HEADER)); + // The overwrite must never be destructive: the replaced title and body are on the comment. + assertTrue(suggestion.applyBody().contains("Old title"), suggestion.applyBody()); + assertTrue(suggestion.applyBody().contains("Old body text"), suggestion.applyBody()); + assertTrue( + suggestion.applyBody().contains(PrDescriptionGenerator.APPLIED_FOOTER), + suggestion.applyBody()); + } + + @Test + void suggestionDegradesToSuggestOnlyWhenTheAnswerLacksTheSections() { + prWithFiles(foo()); + describeReturns("Here is a nicer description of the change, with no sections at all."); + + var suggestion = generateSuggestion(); + + assertNotNull(suggestion); + assertFalse(suggestion.applicable()); + assertNull(suggestion.applyBody()); + assertTrue(suggestion.suggestBody().startsWith(PrDescriptionGenerator.HEADER)); + } + + @Test + void uncoverablePlanYieldsANonApplicableSuggestion() { + when(activeModel.maxInputTokens()).thenReturn(10); + prWithFiles(foo(), otherFile()); + + var suggestion = generateSuggestion(); + + assertNotNull(suggestion); + assertFalse(suggestion.applicable()); + assertTrue(suggestion.suggestBody().startsWith(PrDescriptionGenerator.NOT_COVERED)); + } + + @Test + void parseSectionsReadsTheDemandedShape() { + var parsed = PrDescriptionGenerator.parseSections(WELL_FORMED_ANSWER); + + assertNotNull(parsed); + assertEquals("feat: add the widget", parsed.title()); + assertEquals("Adds the widget.\n\n- detail one\n- detail two", parsed.description()); + } + + @Test + void parseSectionsToleratesAnUnbacktickedTitle() { + var parsed = + PrDescriptionGenerator.parseSections( + "### Suggested title\nfix: plain title\n\n### Suggested description\nBody."); + + assertNotNull(parsed); + assertEquals("fix: plain title", parsed.title()); + assertEquals("Body.", parsed.description()); + } + + @Test + void parseSectionsRejectsAMultiBacktickTitleWrapperInsteadOfApplyingItVerbatim() { + // Stripping one pair of a ``…`` wrapper would leave `…` as the title and PATCH the backticks + // onto the PR; the run must degrade to suggest-only instead. + assertNull( + PrDescriptionGenerator.parseSections( + "### Suggested title\n``feat: add the widget``\n\n### Suggested description\nBody.")); + // A bare code-fence line must not become the literal title "`". + assertNull( + PrDescriptionGenerator.parseSections( + "### Suggested title\n```\n\n### Suggested description\nBody.")); + } + + @Test + void parseSectionsRejectsAnUnclosedBacktickOpenerInsteadOfApplyingItVerbatim() { + // An opener with no close on the line would otherwise skip the strip branch entirely and be + // PATCHed onto the PR with the leading backtick(s) intact. + assertNull( + PrDescriptionGenerator.parseSections( + "### Suggested title\n`feat: add the widget\n\n### Suggested description\nBody.")); + assertNull( + PrDescriptionGenerator.parseSections( + "### Suggested title\n```feat: add the widget\n\n### Suggested description\nBody.")); + // The mirrored malformation — a wrapper whose inside still ends with a backtick — too. + assertNull( + PrDescriptionGenerator.parseSections( + "### Suggested title\n`feat: add the widget``\n\n### Suggested description\nBody.")); + } + + @Test + void parseSectionsKeepsATitleThatMerelyEndsWithAnInlineCodeSpan() { + // Only an opening backtick makes the line read as a wrapper; a trailing code span is a + // legitimate title shape and must not degrade the run. + var parsed = + PrDescriptionGenerator.parseSections( + "### Suggested title\nfix: guard `null`\n\n### Suggested description\nBody."); + + assertNotNull(parsed); + assertEquals("fix: guard `null`", parsed.title()); + } + + @Test + void parseSectionsKeepsInnerCodeSpansOfASinglyWrappedTitle() { + var parsed = + PrDescriptionGenerator.parseSections( + "### Suggested title\n`fix: guard `null` input`\n\n### Suggested description\nBody."); + + assertNotNull(parsed); + assertEquals("fix: guard `null` input", parsed.title()); + } + + @Test + void parseSectionsRejectsAnswersWithoutBothSections() { + assertNull(PrDescriptionGenerator.parseSections(null)); + assertNull(PrDescriptionGenerator.parseSections("no sections here")); + assertNull(PrDescriptionGenerator.parseSections("### Suggested title\n`only a title`")); + assertNull( + PrDescriptionGenerator.parseSections( + "### Suggested title\n``\n\n### Suggested description\nBody.")); + assertNull( + PrDescriptionGenerator.parseSections( + "### Suggested title\n`t`\n\n### Suggested description\n ")); + } } diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/webhook/CommentCommandServiceTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/webhook/CommentCommandServiceTest.java index 6ffce132..8db73504 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/webhook/CommentCommandServiceTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/webhook/CommentCommandServiceTest.java @@ -22,6 +22,7 @@ import dev.thiagogonzaga.thrillhousebot.config.ThrillhouseConfig; import dev.thiagogonzaga.thrillhousebot.github.GitHubAuthClient; import dev.thiagogonzaga.thrillhousebot.github.GitHubCommentClient; +import dev.thiagogonzaga.thrillhousebot.github.GitHubPullRequestClient; import dev.thiagogonzaga.thrillhousebot.github.GitHubReviewClient; import dev.thiagogonzaga.thrillhousebot.github.GitHubReviewClient.PullRequestComment; import dev.thiagogonzaga.thrillhousebot.github.GitHubReviewClient.ReviewResponse; @@ -59,8 +60,10 @@ class CommentCommandServiceTest { @Mock private DocGenerationService docGenerationService; @Mock private PrImprovementService improvementService; @Mock private UnitTestGenerator testGenerator; + @Mock private GitHubPullRequestClient prClient; @Mock private ThrillhouseConfig config; @Mock private ThrillhouseConfig.ReviewConfig reviewConfig; + @Mock private ThrillhouseConfig.DescribeConfig describeConfig; private CommentCommandService service; @@ -72,6 +75,8 @@ void setUp() { when(reviewConfig.addDocsEnabled()).thenReturn(true); when(reviewConfig.improveEnabled()).thenReturn(true); when(reviewConfig.generateTestsEnabled()).thenReturn(true); + when(reviewConfig.describe()).thenReturn(describeConfig); + when(describeConfig.apply()).thenReturn(false); // Run submitted work inline so the async handoff is exercised synchronously in tests. doAnswer( inv -> { @@ -85,6 +90,7 @@ void setUp() { executor, authClient, commentClient, + prClient, reviewClient, reviewThreadService, reviewDispatcher, @@ -240,6 +246,92 @@ void describeIgnoredWhenUnauthorized() { verifyNoInteractions(prPauseService); } + @Test + void describeSuggestOnlyNeverTouchesThePr() { + authorize(true); + when(prPauseService.isPaused("owner", "repo", 7)).thenReturn(false); + when(descriptionGenerator.generate("owner", "repo", 7, "main", 12345L, "token")) + .thenReturn("## suggestion body"); + + service.handle(ctx(CommentCommand.DESCRIBE)); + + // apply is off (the default), so the suggest-only contract holds: comment posted, PR untouched. + assertEquals("## suggestion body", postedBody()); + verifyNoInteractions(prClient); + } + + @Test + void describeAppliesSuggestionToThePrWhenApplyIsOptedIn() { + authorize(true); + when(prPauseService.isPaused("owner", "repo", 7)).thenReturn(false); + when(describeConfig.apply()).thenReturn(true); + when(descriptionGenerator.generateSuggestion("owner", "repo", 7, "main", 12345L, "token")) + .thenReturn( + new PrDescriptionGenerator.Suggestion( + "feat: new title", "New description.", "suggest body", "applied body")); + + service.handle(ctx(CommentCommand.DESCRIBE)); + + verify(prClient) + .updatePullRequest( + eq("token"), + any(), + eq("owner"), + eq("repo"), + eq(7), + eq( + new GitHubPullRequestClient.UpdatePullRequestRequest( + "feat: new title", "New description."))); + // The confirmation comment (which preserves the replaced title/body) is the one visible reply. + assertEquals("applied body", postedBody()); + } + + @Test + void describeApplyFallsBackToSuggestionWhenOutputDoesNotParse() { + authorize(true); + when(prPauseService.isPaused("owner", "repo", 7)).thenReturn(false); + when(describeConfig.apply()).thenReturn(true); + when(descriptionGenerator.generateSuggestion("owner", "repo", 7, "main", 12345L, "token")) + .thenReturn(new PrDescriptionGenerator.Suggestion(null, null, "suggest body", null)); + + service.handle(ctx(CommentCommand.DESCRIBE)); + + verifyNoInteractions(prClient); + assertEquals("suggest body", postedBody()); + } + + @Test + void describeApplyFallsBackToSuggestionWhenThePrUpdateFails() { + authorize(true); + when(prPauseService.isPaused("owner", "repo", 7)).thenReturn(false); + when(describeConfig.apply()).thenReturn(true); + when(descriptionGenerator.generateSuggestion("owner", "repo", 7, "main", 12345L, "token")) + .thenReturn( + new PrDescriptionGenerator.Suggestion( + "feat: new title", "New description.", "suggest body", "applied body")); + when(prClient.updatePullRequest(any(), any(), any(), any(), anyInt(), any())) + .thenThrow(new RuntimeException("boom")); + + service.handle(ctx(CommentCommand.DESCRIBE)); + + // Never confirm an edit that did not happen: the run degrades to the plain suggestion comment. + assertEquals("suggest body", postedBody()); + } + + @Test + void describeApplySaysSoWhenGeneratorReturnsNothing() { + authorize(true); + when(prPauseService.isPaused("owner", "repo", 7)).thenReturn(false); + when(describeConfig.apply()).thenReturn(true); + when(descriptionGenerator.generateSuggestion("owner", "repo", 7, "main", 12345L, "token")) + .thenReturn(null); + + service.handle(ctx(CommentCommand.DESCRIBE)); + + verifyNoInteractions(prClient); + assertTrue(postedBody().contains("no `/describe` output to post"), postedBody()); + } + @Test void changelogPostsTheGeneratedEntry() { authorize(true);