feat(review): pull config-key implementation into review context - #450
Merged
Conversation
A review payload is changed hunks only, so a PR that only documents a config key shows the model the doc line and nothing about the key's behavior. PR #104 documented THRILLHOUSEBOT_REVIEW_MANUAL_TRIGGER_ALLOWED_LOGINS without saying the value is a comma-separated list, and the review reported no issues: the @withname mapping and the allowlist matching were both outside the diff, so the omission was not knowable. When a diff touches a *.md or .env* file, extract the config-key tokens it names (UPPER_SNAKE environment variables and dotted property keys), locate the repository's configuration files through one recursive tree listing, and append the matching definition lines to the review context. Both definition forms resolve: the explicit ${ENV:default} override in application.properties, and the SmallRye-derived environment name that exists only through a @withname mapping, matched by normalizing both sides to UPPER_SNAKE and dropping the key's prefix segments. The material rides the existing trailing-guidance slot rather than a new prompt variable, framed and escaped as untrusted data like the other fetched prose. Work is bounded by explicit caps — 8 files fetched, 5 keys rendered, 2 snippets per key, 3000 characters — and every fetch fails soft, so a GitHub failure degrades to no extra context instead of a failed review. Nothing is fetched at all when the diff names no key. Supplies the evidence the doc-completeness prompt rule (#109) needs. Refs #108
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
🤖 ThrillhouseBot PR SummaryWhat this PR doesExtracts config keys from documentation diffs, locates their definition sites in the repository (application.properties / config source files), and appends the relevant implementation snippets to the review context so the model can verify documentation completeness. Control-Flow Diagram🔀 Show diagramflowchart TD
A["ReviewContextLoader.load()"] --> B["ConfigKeyContextResolver.resolve()"]
B --> C["extractTokens(files)"]
C -->|no tokens|D["return empty"]
C -->|tokens found|E["candidatePaths() via getTree()"]
E -->|no candidates|D
E -->|candidates|F["collectDefinitions() fetch file contents"]
F --> G["match normalized tokens in lines"]
G -->|no matches|D
G -->|matches|H["render() prompt section"]
H --> I["ReviewPromptAssembler.configKeyContextSection()"]
I --> J["combineSections into repoInstructions"]
Changes Overview
Changed Files
Risk Assessment
Things to double-check1 lower-confidence finding
|
| Check | Type | Status | Detail |
|---|---|---|---|
| changes | check-run | ⏳ Pending | - |
| test | check-run | ⏳ Pending | - |
| trivy | check-run | ⏳ Pending | - |
| actionlint | check-run | ⏳ Pending | - |
| format | check-run | ⏳ Pending | - |
| frontend | check-run | ⏳ Pending | - |
| dependency-review | check-run | ⏳ Pending | - |
Automated review by ThrillhouseBot. Reply with /review to re-run.
![thrillhousebot[bot]](https://avatars.githubusercontent.com/in/3996985?s=60&v=4){kind=small}
There was a problem hiding this comment.
ThrillhouseBot noted 1 lower-confidence item(s) under Things to double-check in the PR summary (not posted as inline threads):
- LOW: Tree response truncation not handled (
src/main/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolver.java:237)
The call togetTreewithrecursive=1can return a truncated response for repositories with many files. Thetruncatedfield inTreeResponseis never checked, so when the tree is truncated the resolver may miss config files located in deep subtrees, silently producing no context. This violates the project's pagination/truncation guidance (a single-page fetch used to drive an action must paginate or justify one page is enough). Checkingtruncatedand logging a warning would at least make the limitation visible.
thrillhousebot
Bot
added
enhancement
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Follow-up on the config-key context resolver from the same branch, driven by the PR's own CI signals and ThrillhouseBot's review of it. Correctness and robustness: - Bound every quantifier in the token-extraction regexes. These run over Markdown supplied by a pull request, and Java compiles a repeated group into a recursive matcher, so an unbounded '+' on the segment group let a crafted line (thousands of "_A" repetitions) drive the match into deep recursion. The new bounds sit far above any real config key. - Surface GitHub's "truncated" flag on the recursive tree listing. The resolution stays best-effort, which is correct, but a key whose definition lives past GitHub's cut is now explicable from the logs instead of looking like a resolution miss. - Never split a surrogate pair when truncating a snippet or the section, the same guard BugFixContextResolver already applies, so an emoji in a comment cannot be halved into an unpaired surrogate. - Render line numbers with '\n' rather than String.format's platform-dependent '%n' — this text goes into a prompt, not to a console. - Drop two conditions that can never be true: a tree entry is never null (TreeResponse copies with List.copyOf, which rejects null elements before the loop runs) and containsSegment is only ever called with a non-empty needle. Coverage: the resolver now has full line and branch coverage. The added tests exercise the paths the first round left untested — both cap branches on the doc-file/token scan, the per-key snippet cap reached partway through a file, the fetch budget, an absent or bodyless or blank file response, a null tree response, a truncated listing, tree entries without a path, path classification for every supported layout, whole-segment matching that must keep searching past a partial hit, blank-line handling inside a snippet window, snippet and section truncation, and the default-branch fallback when a PR carries no head SHA. Refs #108
Addresses the maintainability issues static analysis raised on the new
resolver. This is the class a general cross-file context feature would later
generalize, so leaving its core gnarly would tax that work.
- collectDefinitions dropped from cognitive complexity 19 to about 8 by
extracting two cohesive helpers rather than shuffling lines: normalizedByToken
precomputes each token's normalized form, and absorbFile adds one file's
definition sites to the accumulator. The per-token snippet cap is now stated
as the room a token has left ("take at most this many more") instead of two
separate cap checks, which removes an inner break and continue outright.
- Replaced the nested ternary in normalize with a named normalizeChar, and
wrote its uppercasing as c - ('a' - 'A') so the offset explains itself.
- Extracted isWorthReading for the tree-entry filter: a blob, with a path,
small enough to be a config file, and not a test source. The candidate walk
now reads as one question instead of two stacked guards.
- Reordered snippetsFor's guard so the window it would render is computed
first and both conditions are stated positively.
Two loops keep a single break each — the doc-file scan and the candidate walk
— because "stop once the budget is spent" is exactly what a break says; a flag
variable would be strictly worse. No behavior changes: the resolver keeps full
line and branch coverage (1040/1040 instructions, 180/180 branches) under the
same tests, verified against JaCoCo after the restructure.
Refs #108
Absorbs #449 (per-repo ignore patterns), #451 (whole-change-set PR summary), #453 (decline re-check) and four dependency bumps. Two textual conflicts, both from independent additions at the same insertion point rather than any disagreement: - ReviewContextLoader: #449's resolveIgnoreGlobs and this branch's resolveConfigKeyContext are separate private helpers that git could not place. Kept both. - FindingPipelineTest: #451 parameterized the reviewContext helper with an explicit reviewable-file list while this branch added the configKeyContext record component. Kept both — the helper's parameter, with "" in the new component's position. One silent breakage git merged cleanly: #453's new declinedRaceContext helper constructs a ReviewContext without configKeyContext. Filled in. The interaction between the two features is the one worth noting. #449 made load() compute reviewableFiles from the global globs unioned with the repo's own, and config-key resolution already read that post-filter list, so a key documented only in an ignored file is now correctly never resolved — and it inherits per-repo ignore rules for free. Pinned with a test that fails if the raw file list is ever passed instead.
There was a problem hiding this comment.
ThrillhouseBot found no issues in this PR, but some checks are still pending or failed:
- Check frontend is pending
- Check test is pending
- Check changes is pending
- Check format is pending
- Check trivy is pending
- Check actionlint is pending
- Check dependency-review is pending
|
18 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
Description
A review payload is changed diff hunks only, so a PR that documents a config key shows the model the doc line and nothing about the key's behavior. Dogfood evidence: PR #104 documented
THRILLHOUSEBOT_REVIEW_MANUAL_TRIGGER_ALLOWED_LOGINSwithout saying the value is a comma-separated list, and the review reported 0/0/0/0 — the@WithNamemapping inThrillhouseConfigand the allowlist matching inManualReviewAuthorizerwere both outside the diff, so the omission was not knowable.This PR adds
ConfigKeyContextResolver:*.mdor.env*file, its added lines are scanned for config-key tokens:UPPER_SNAKEenvironment-variable names and dotted lowercase property keys of three or more segments (soapplication.propertiesandREADME.mdare not mistaken for keys).application*.{properties,yaml,yml}resources first, then source files whose name marks them as a config definition site (*Config.java,*Settings.kt, …). Test paths and oversized blobs are skipped.thrillhousebot.webhook.dedup-ttl=${WEBHOOK_DEDUP_TTL:24h}matches the env name literally;@WithName("manual-trigger-allowed-logins"), matches after both sides are normalized toUPPER_SNAKEand the key's prefix segments are dropped.@WithDefaultand the declared type come along) are rendered with their path and line numbers and appended to the review context.Design notes for reviewers:
repoInstructions) section alongside the bug-fix efficacy block, so the prompt constant, the@Vparameters onPrReviewer, andPromptInputsare untouched. A dedicated slot would be marginally cleaner but touches four more files for no behavioral gain; the section carries its own heading.PromptTemplateEscaper.escape(...)like the linked-issue text.getTreewas added to the existingGitHubPullRequestClientnext togetFileContent, whichProjectStackResolver/InstructionsResolveralready use for repo content.SoftLoaderspattern), never a failed review.This only supplies the context material; the doc-completeness prompt rule that consumes it is #109 and is not in this PR.
Related Issues
Fixes #108
How Has This Been Tested?
New
ConfigKeyContextResolverTest(18 cases: token extraction, both resolution forms, section framing, fail-soft on tree/content failures, candidate ranking, fetch budget, render caps, and the assembled-prompt acceptance case) plus three new cases inReviewContextLoaderTest.Red/green validation. With the tests in place, the production behavior was neutralized (
ConfigKeyContextResolver.resolve,ReviewContextLoader.resolveConfigKeyContextandReviewPromptAssembler.configKeyContextSectionmade to return""— the pre-change behavior) and the suites re-run. 9 of the new cases failed, verbatim:With the production change restored, all 69 cases in those two classes pass.
Full gates on Java 25:
./mvnw -B spotless:apply— clean./mvnw -B clean compile spotbugs:check spotless:check—BugInstance size is 0, BUILD SUCCESS./mvnw -B clean test— Tests run: 1898, Failures: 0, Errors: 0, Skipped: 0Checklist
Additional Notes
No new config key. The caps are compile-time constants rather than configuration, so there is nothing to add to the README table or
.env.example— hence the unticked documentation box. The resolver is unconditional likeProjectStackResolverand the linked-issue fetch; if a kill switch is wanted for the extra GitHub calls it is a small follow-up.Cost. A PR whose docs name no config key costs zero extra API calls. A PR that does costs one tree listing plus at most 8 content fetches, and stops early once every token has resolved (typically 2 fetches for this repo:
application.propertiesandThrillhouseConfig.java). Results are not cached — the material is read at the PR head SHA so a key added by the same PR resolves against the PR's own tree, which makes a per-repo cache mostly cold; a per-SHA cache is a reasonable follow-up if re-review latency matters.Deliberately out of scope. Issue #109 (the doc-completeness prompt rule) and #55 (general cross-file context) are not touched. No CHANGELOG entry was added to avoid conflicting with the other v0.6.0 wave branches.