fix(review): clear the Sonar findings and coverage gaps on the release diff (#427) - #428
Conversation
…e diff (#427) Sonar (10 findings, all in code this release added): - java:S8786 super-linear regex, HeuristicCodeDetector x2. The declaration gap overlapped the captured name, so a long line with no "(" re-split on every backtrack; it is now atomic and must end on a separator, which cannot overlap the name. The threshold constant chained three unbounded runs in one expression; the declaration is now matched structurally and the name tokens checked in code, the same split HEURISTIC_NAME_STEMS already uses. - java:S135 x2: the diff scan loop and postInlineComments now branch with if/else instead of stacked continues. - java:S107: scheduleCaptureOnReviewReply takes the ReviewReply record it already used internally rather than three more positional parameters. - java:S6126 x5: diff fixtures converted to text blocks. Codecov: the three files below patch threshold are now fully covered. The gaps were all in the #386 pure-rename support plus two error paths — absent-file and zero-count-with-patch rename detection, the rollup's missing-previous-filename fallback, withPureRenames' merge path, the summary overview's rename rollup, unwrapParallelFailure with a real cause, and a bot root comment carrying no finding marker. Behavior is unchanged throughout; the detector's 26-case suite is the guard on the two regex rewrites. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
🤖 ThrillhouseBot PR SummaryWhat this PR doesClears Sonar and Codecov findings on the release diff: rewrites two super-linear regex patterns in HeuristicCodeDetector, refactors a method with many parameters, converts diff fixtures to text blocks, and adds tests to lift coverage.
|
| File | Change | Summary |
|---|---|---|
src/main/java/dev/thiagogonzaga/thrillhousebot/review/FindingFeedbackCaptureService.java |
Modified | Changed scheduleCaptureOnReviewReply signature to take ReviewReply record instead of three separate parameters; made ReviewReply public. |
src/main/java/dev/thiagogonzaga/thrillhousebot/review/HeuristicCodeDetector.java |
Modified | Replaced HEURISTIC_DECLARATION pattern with an atomic-group variant and THRESHOLD_CONSTANT with a new split pattern/code check; refactored loop to if/else. |
src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java |
Modified | Refactored postInlineComments loop to use if/else instead of stacked continues. |
src/main/java/dev/thiagogonzaga/thrillhousebot/webhook/WebhookController.java |
Modified | Updated call to scheduleCaptureOnReviewReply to pass ReviewReply record. |
src/test/java/dev/thiagogonzaga/thrillhousebot/review/FindingFeedbackCaptureServiceTest.java |
Modified | Updated tests to use new ReviewReply parameter; added test for bot comment without finding marker. |
src/test/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipelineTest.java |
Modified | Added tests for parallel failure cause unwrapping and summary overview leading with pure rename rollup. |
src/test/java/dev/thiagogonzaga/thrillhousebot/review/HeuristicCodeDetectorTest.java |
Modified | Converted diff string fixtures to text blocks for readability. |
src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewDiffFormatterTest.java |
Modified | Added tests for pure rename edge cases (null files, zero counts, missing previous filename) and rollup behaviour. |
src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestratorTest.java |
Modified | Updated test fixture to text block. |
src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPromptAssemblerTest.java |
Modified | - |
src/test/java/dev/thiagogonzaga/thrillhousebot/webhook/WebhookControllerTest.java |
Modified | Updated test to construct ReviewReply record for capture call. |
Risk Assessment
| Risk | Count |
|---|---|
| 🔴 Critical | 1 |
| 🟠 High | 0 |
| 🟡 Medium | 0 |
| 🔵 Low | 0 |
Key Findings
- CRITICAL: HEURISTIC_DECLARATION regex fails to match any method declaration with a return type (
src/main/java/dev/thiagogonzaga/thrillhousebot/review/HeuristicCodeDetector.java:100)
⚠️ CI Checks Status
Some checks are still pending or have failed:
| Check | Type | Status | Detail |
|---|---|---|---|
| trivy | check-run | ⏳ Pending | - |
| actionlint | check-run | ⏳ Pending | - |
| format | check-run | ⏳ Pending | - |
| changes | check-run | ⏳ Pending | - |
| frontend | check-run | ⏳ Pending | - |
| test | check-run | ⏳ Pending | - |
| dependency-review | check-run | ⏳ Pending | - |
Automated review by ThrillhouseBot. Reply with /review to re-run.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
|
Test image ready ( docker pull ghcr.io/devops-thiago/thrillhousebot:test-sha-ed00f4c-native
IMAGE_TAG=test-sha-ed00f4c-native docker compose up -dTags: |
|
/review |
|
/help |
🤖 ThrillhouseBot commands
You can also use the mention form, e.g. |
|
/changelog |
|
/add-docs |
|
📝 ThrillhouseBot found no changed symbols that need documentation in this PR. |
…validated (#430) ## What type of PR is this? - [x] 📝 Documentation ## Description Five commits landed on `release/v0.5.0` after the `[0.5.0]` section was written. Two of them changed what actually ships, leaving one feature undocumented and three entries describing behaviour that no longer exists. ### #429 was missing, and it changes existing deployments It widens the default `thrillhousebot.review.ignored-files` globs (closes #52) to cover `pnpm-lock.yaml`, `go.sum`, protobuf output, minified bundles and sourcemaps, and the directories `node_modules/`, `dist/`, `build/`, `out/`, `.next/`, `vendor/`, `__pycache__/`, `.venv/`, `bin/`, `obj/`. Filed under **Changed** rather than Added, because it is not additive for anyone already running the bot: the directory globs match by name, so a repository keeping handwritten source under `build/`, `bin/`, `out/` or `vendor/` silently stops having it reviewed on upgrade. The entry says so — the code's own javadoc carries the same warning. ### #423 invalidated three entries The audit commit touched 35 files (+2303/−159) and never touched the changelog: | Entry | Was wrong because | |---|---| | **#97** | Said the arithmetic/test-failure cap was absolute ("at most low confidence"). #423 made it evidence-dependent: with an execution or CI signal in the material, the finding keeps the confidence that evidence justifies | | **#324** | Described feedback capture with no permission model. It now records only verified write-capable collaborators on bot-authored threads, scopes `GET /api/dashboard/feedback` to repos the caller can access, bounds capture concurrency, and no longer logs source-line contents at INFO | | **#123** | Described a Java-shaped detector. It now spans JS/TS regex literals, `function`/arrow validators, package-private Java declarations, and multiline construction over a bounded window — and only confirms a synthesized failure when the expected domain or contract is visible | Also added a Dependencies line for the Node 22 requirement and the PostCSS/Sharp pins, which is what makes a clean `npm ci` resolve without known high-severity advisories. ## Related Issues N/A — release documentation accuracy. ## How Has This Been Tested? - [x] Manual testing Verified against the branch state: - every merged issue is cited except #136 (test hardening, intentional) - no intra-release fix numbers leaked in — #410, #421, #423, #428 all absent - `Fixed` audited entry by entry for pre-0.5.0 scope Prose checked against the house voice rather than written free-hand: bold marks the entry name and nothing else, matching every prior release section (0.4.0 and 0.3.x use no mid-sentence bold at all). Em dash density lands at 11.1 per 1000 words against 12.9 in 0.3.1 and 13.3 in 0.3.0, so it reads consistently with the sections around it. ## Checklist - [x] My code follows the project's coding standards - [x] I have performed a self-review of my own code



What type of PR is this?
Description
Clears everything #427 (
release/v0.5.0→main) reports. Opened againstrelease/v0.5.0ratherthan pushed to it, so it flows into #427 through the normal path.
Sonar — 10 findings
java:S8786super-linear regex ×2 (HeuristicCodeDetector) — the substantive ones.HEURISTIC_DECLARATIONmatched[^(=\r\n]*and then(\w+)\s*\(. The gap and the name classoverlap, so on a long line with no
(the engine re-split at every position. The gap is nowatomic and must end on a separator — it cannot consume the name, so there is nothing to backtrack
into. A possessive quantifier would have been wrong here: it stops exactly the backtracking the
name capture depends on.
THRESHOLD_CONSTANTchained three unbounded runs ([^=]* … \w*(?:ALT)\w* … [^=]*=). Replaced byTHRESHOLD_DECLARATION, one run terminated by=, with the name tokens checked in code — thesame regex/code split
HEURISTIC_NAME_STEMSalready uses in this file.java:S135×2 — the diff scan loop andReviewPublisher.postInlineCommentsnow branch withif/elseinstead of stackedcontinues.java:S107—scheduleCaptureOnReviewReplyhad 8 parameters. It now takes theReviewReplyrecord it already constructed internally from three of them, so the call site passes 6. The record
became
publicfor thewebhookcaller.java:S6126×5 — diff fixtures converted to text blocks.Codecov — three files under threshold, now fully covered
All the gaps were in the #386 pure-rename support plus two error paths, and each new test pins real
behaviour rather than just touching the line:
isPureRenamewith an absent file/status, and a rename reporting zero counts but carrying patchtext — that one stays reviewable, so a rename+edit is never silently dropped from the budget
previous_filename(no arrow, just the new path)withPureRenames— both the identity return and the merge path, asserting renames append afterreviewable files
dropping the disclosure on large multi-call reviews
unwrapParallelFailurepreserving a real cause (the null-cause side was already covered)Related Issues
Fixes the failing
codecov/patchand the Sonar findings on #427. No issue number — release cleanup.How Has This Been Tested?
Unit tests
./mvnw verify— 1863 tests, 0 failures, SpotBugsBugInstance size is 0, JaCoCo gate metJaCoCo confirms fully covered, 0 missed instructions/branches/lines for all five touched
production files:
HeuristicCodeDetector,ReviewDiffFormatter,FindingPipeline,FindingFeedbackCaptureService,ReviewPublisherThe detector's 26-case suite is the guard on the two regex rewrites — behaviour is unchanged,
including the
parseIntexclusion and the test-path/window-constant detection those patterns driveChecklist