Skip to content

v0.6.7

Choose a tag to compare

@github-actions github-actions released this 07 Sep 11:42
· 50 commits to main since this release
Immutable release. Only release title and notes can be modified.
9ae348d

Two production reviews drove this one: a pull request that was approved after most of the model's answer was thrown away, and one that was pushed to while under review and lost every finding to the push. The rest is hardening found by auditing the merged pull requests and by dogfooding the repository configuration. No configuration changes; upgrading is a redeploy. The one behaviour a deployment may notice is that ignored-files globs now match the way the documentation always said they did, so a pattern that was silently doing nothing starts excluding files.

Fixed

  • A review response split across two JSON documents is read whole, and a response that was not read whole is not approved (#805): a reasoning model answered with prose, then an object carrying the summary, then a fenced second object carrying the findings. readTree returns after the first complete value and ignores the rest, so the second document was dropped with no log line, the root had no findings node, that read as an empty list, and the pull request was approved with 0 findings. The finding dropped in the observed run was a public-endpoint 500 on a registration path. The parser now reads every JSON object in the response and merges them: findings append in document order, an identical finding is kept once, and every other field keeps the first non-null value. One warning records how many documents and characters lay past the first. A closing sentence after the last document is discarded with a warning that says how much was dropped; a further document that is cut or malformed fails the response as malformed, so it is retried instead of approved from a partial read. A root with no findings node at all is a format failure for the same reason. Only an explicit "findings": [], which both prompts ask for, is a clean review
  • A run that stands down for a moved head hands its verified findings to the run that replaces it (#806): #704 made a review whose head moved abandon its post, since the coalesced run for the new head re-reviews in its place. That run's findings went with the post, and nothing about the replacement's fresh pass guaranteed it would find them again. On a pull request pushed to three times while under review, runs with four and two verified findings were discarded and the only outcome that reached GitHub was an approval from the one run that found nothing. The superseded run's verified findings are now folded into the round the replacement reports on: the model is asked to confirm or resolve each one against the new head, a finding whose code the push removed is closed by the vanish pass, and one the model confirms holds the verdict at its verified severity. The hand-off is keyed by pull request, released only to the review of the head that displaced the run, taken once and capped, so it cannot leak across pull requests or accumulate across pushes. The count carried is logged at WARN and named in the summary's review-scope note
  • Walkthrough rows go to the files the model summarized (#804): the Changed Files table rendered the first 20 files in GitHub's alphabetical order while the model summarized the 20 files it judged mattered, and the two lists were only intersected. #477 made the caps equal, which guarantees a fillable row only while the whole change set fits in the table; on a 148-file pull request the overlap was one file, 19 rows read "Not summarized" and the summarized views, auth and service files sat behind the "…and 128 more" note. The summarized files take the rows now, in diff order, and the remaining rows fill from the rest of the change set; the rollup count is unchanged. Only a changed file can be a row, so a summary naming a path outside the diff creates none. Both prompts now say an entry names one file and never a directory, after the same response spent an entry on billing/
  • Per-repo and deployment ignore globs match the gitignore syntax they are documented as (#481): the matcher was raw Java NIO globs, under which the common idioms matched nothing. build/ matched no file at all, a bare vendor matched only a file literally named that, *.lock matched only a root-level lockfile, and a leading / compiled to an absolute path that could never match. Each of those failed open: a path the repository asked to exclude was sent to the model anyway, with no warning at any log level, because a pattern that compiles and matches nothing is indistinguishable from one that had nothing to match. Declared patterns are now normalized to gitignore's reading (anchored when they carry a /, at any depth when they do not, a trailing / meaning that directory's tree) before compiling, !negations are dropped with a warning instead of compiling to a literal ! file name, and the shipped default list is pinned by test to exclude what it always did. The deployment key and a repository's own list go through one matcher, so they cannot drift apart
  • A declared ignore glob that matches nothing is disclosed (#481): the review summary's scope note names the repository's globs that excluded no file in the pull request, so a typo or a wrongly shaped pattern is visible instead of doing nothing forever
  • A brace glob survives the scalar form of ignored-files (#481): the scalar spelling is comma-split to match the environment-variable form, which cut **/*.{js,ts} in half; one fragment then failed to compile and the other became a matcher for a file named ts}. The split now ignores commas inside a brace group, and a brace that nothing closes is literal, so a truncated alternation costs its own entry and not every entry after it
  • A duplicate key no longer discards every setting in the file (#481): one repeated line threw out of the whole parse, far wider than the parser's per-entry rule that one bad entry costs only itself. It is warned about and read with YAML's usual last-one-wins reading
  • YAML anchors, aliases and merge keys are resolved (#481): jackson-dataformat-yaml never runs snakeyaml's composer, so *common arrived as the literal string common. An aliased ignore list became a glob for a file called common, and an aliased scope path became a short literal that passed every check and was rendered into the review prompt as the repository's rule for those files. The document is loaded through snakeyaml's own loader now, which also makes the nesting-depth and alias ceilings real; they were inert while their comments claimed protection. The alias ceiling counts uses, and a ladder of lists that each name the rung below twice stays under it while expanding to millions of nodes, so the parser also sizes what the aliases expand to and refuses a document past the code-point ceiling a spelled-out file could not pass
  • A transient GitHub failure is no longer cached as "this repository has no config" (#481): WebApplicationException covers ServerErrorException (500/502/503) and the 403 secondary rate limit as well as 404, so a blip pinned the negative cache for a minute and every review in that window ran on the deployment ignore list alone. Only a real 404 is cached now; anything else runs on the global list for that review and is asked again on the next
  • Config-key definition files under an ignored path are no longer fetched or rendered (#483): the definition sites the review pulls in for a documented config key come from a walk of the whole repository tree, and that walk was never handed the review's ignore globs; only the changed files that name the keys were filtered. A repository that excluded config/ from review still had config/application.yml read and quoted into the prompt whenever a doc line mentioned a key defined there. The candidate files now pass through the same compiled ignore set as the diff, with the same gitignore reading
  • The patch-coverage report reader attributes correctly (#483, #789): latent, since thrillhousebot.review.patch-coverage.enabled defaults to off, but each of these degraded to a wrong coverage signal that the prompt tells the model to treat as measured fact. One report entry was attributed to every same-named file across modules; only a unique report-to-path match contributes now. Coverage from a run that completed without succeeding was used as fact; runs are filtered on conclusion as well. A multi-module artifact kept only its first report; every one contributes now, and where two reports measure the same path their misses are intersected instead of unioned, so one module's misses are never charged to another module's file
  • A bound callId is registered under the lock that decides whether its session survives (#763, #788): ReviewSessionContext.bind looked the session's in-flight set up with computeIfAbsent and added to it in a second step, outside the per-bin lock. A sibling batch of the same session finishing in between unmapped that set, the add landed on an orphan, and a live stream's usage callback was discarded as stale. Those tokens never reached the ledger, so the per-review spend ceiling (#509) under-counted and a review that should have degraded kept spending. Reviews run their batches on virtual threads, so this interleaving is the ordinary case. A single compute puts the add back under the lock
  • LogSafe's documented reach is enforced by a test (#764, #788): five rounds had found INFO logs splicing in a raw model-supplied value (a finding's title, a path, a suggestion) that the class javadoc said always went through the sanitizer, and each fix enumerated the sites someone had noticed. LogSafeInvariantTest derives the untrusted accessor names from the response records themselves and scans every log call in src/main/java for one used without LogSafe, so a field added to a response record or a log line added anywhere is covered as soon as it exists
  • Live code is told from quoted text by a lexical state machine (#651, #791): RebuttalContradiction may only overrule a maintainer's "this runs serially" decline on code the revision runs, and it told the two apart with a character walk that toggled on any single quote plus one special case per shape the toggle got wrong. A dispatch named inside a block comment, a multi-line literal or after a case 1:// label counted as evidence against a maintainer who was right, while a Java text block or C++ raw string holding an interior quote cut the line early and hid the dispatch after it. The walk is replaced by LiveCodeScanner: one delimiter table drives text blocks, triple quotes, raw strings, template literals with their ${…} interpolations, block comments and backslash-continued strings, and the diff header decides whether a backtick literal escapes. It errs toward reading text as quoted, since firing overrules a human

Security

  • Aggregate decompression of a coverage artifact is bounded (#483, #789): the reader capped each entry but not the total. The next getNextEntry() inflates the remainder of a skipped entry, so a 16 MB artifact at the download ceiling reached roughly 16 GB of decompression per review, and a pull_request workflow that builds a fork's code lets a fork publish such an artifact under the configured name for the head under review. Every entry, directory-named ones included, is now drained through a counting copy against a 128 MB budget, and the archive is abandoned the moment the budget is blown. Latent until thrillhousebot.review.patch-coverage.enabled is turned on

Dependencies

  • Bumped Quarkus from 3.38.2 to 3.39.1 (#793, #799) and quarkus-langchain4j-bom from 1.12.2 to 1.13.1 (#794, #801)
  • Bumped spotbugs-maven-plugin 4.10.3.0 to 4.10.4.0, maven-compiler-plugin 3.15.0 to 3.16.0, and maven-surefire-plugin / maven-failsafe-plugin 3.5.6 to 3.6.0 (#794, #801)
  • Bumped the website docs group: astro 7.2.2 to 7.2.10, @astrojs/starlight 0.41.7 to 0.41.10, @astrojs/markdown-remark 7.2.2 to 7.3.0, starlight-versions 0.9.1 to 0.10.1, mermaid 11.16.1 to 11.17.2 and sharp 0.35.3 to 0.35.4 (#795, #802)
  • Bumped the frontend npm group: next 16.3.1 to 16.3.4, vitest 4.1.10 to 4.1.11, @testing-library/react 16.3.2 to 16.3.3, @testing-library/user-event 14.6.5 to 14.6.6, @types/node 26.2.0 to 26.4.0 and @types/react-dom 19.2.4 to 19.2.5 (#796, #800)
  • Bumped the actions group: actions/setup-java v5.7.0 to v6.0.0 (#798), github/codeql-action v4.37.7 to v4.37.9, docker/setup-qemu-action v4.2.0 to v4.3.0 and actions/deploy-pages v5.0.0 to v5.0.1 (#797, #803)

What's Changed

Fixes

  • fix: register a bound callId atomically, and enforce LogSafe's documented reach by @devops-thiago in #788
  • fix(review): harden the patch-coverage report reader (zip-bomb bound, cross-module attribution, failed-run, multi-module merge) by @devops-thiago in #789
  • fix(review): read ignore globs as the gitignore syntax they are documented as by @devops-thiago in #790
  • fix(review): read a diff line with a lexical state machine, not a character walk by @devops-thiago in #791
  • fix(review): give the walkthrough rows to the files the model summarized by @devops-thiago in #810
  • fix(review): read every JSON document in a review response and refuse one not read whole by @devops-thiago in #809
  • fix(review): apply the review's ignore globs to config-key candidate files by @devops-thiago in #811
  • fix(review): carry a superseded run's verified findings into its replacement by @devops-thiago in #812

Other changes

Full Changelog: v0.6.6...v0.6.7