Repository navigation
LLM-assisted PR triage - #5281
Conversation
Add a pr-triage-llm workflow that, once per PR, asks an LLM via the ASF gateway (llm.apache.org) whether the change warrants a changelog entry, Reference Guide update, upgrade notes, and tests. It judges the change on its merits, without seeing what the author included, then posts one comment comparing each verdict with what the PR contains and highlighting mismatches in either direction. The questions live in .github/pr-triage/judgements.yml. Since whether a PR needs a changelog entry is now judged (advisory), the validate-changelog check no longer fails PRs without one; it only validates the structure of entries present. The no-changelog label and its /no-changelog and /needs-changelog comment commands are removed. Requires repository secret ASF_LLM_API_KEY; optional variable LLM_MODEL. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KwTAxfG9oFYQQH7pZDB4op
github-actions
Bot
added
documentation
dsmiley
left a comment
dsmiley
left a comment
There was a problem hiding this comment.
Huge disclaimer here: I don't know python, nor GHA details... so really I'm the last one to try to author something of this nature. But I believe in this approach -- LLM based judgements. It's not gospel but it's something and we can tweak it to improve going forward. I could use any assistance in bringing this to reality. It'd be more helpful for reviewers to make changes rather than tell me what to do.
There was a problem hiding this comment.
Rather than give users a checklist for these things, we let an LLM make some judgements to help the user (and reviewers!).
|
🤖 AI text below 🤖 (reviewed by Nick Shanin) @dsmiley I put together a follow-up on top of your current head: changes from It implements the suggestions in your comments: an early warning for PRs targeting something other than I also fixed three cases in the helper: deleted files counted as included evidence, missing or malformed model answers became “no,” and model reasons could render active Markdown/HTML. The follow-up reports unknown answers explicitly and renders reasons as text. It uses the trusted workflow revision for executable code, installs PyYAML explicitly, and rejects changed or incomplete API snapshots. It adds 23 offline tests and a separate test workflow with no secrets or write permissions. Verified on Linux: all 23 tests pass. Three regressions fail against the original helper and pass with the fixes. GFM rendering checks and actionlint with ShellCheck pass; You can pull the tested commit into your PR branch with: git switch pr-triage-llm
git fetch https://github.com/nick-boss-tech/solr.git pr-5281-triage-followup
git merge --ff-only 6bcaa9e1191fb8d0c6def2eaf005b602bf882e40Live ASF gateway behavior and GitHub activation, including secret setup, event delivery, notifications and comment publication, still need a controlled smoke test once the workflow is available from the trusted branch. |
Generated-by: Claude Sonnet 5.5
|
Thanks Nick; I took your changes. I'm opinionated on dev-tools/README.md being at most 3 sentences per script -- each script should instead document itself. So I made an edit related to that. I think we basically need to just merge & try this thing out, iterate from there. Accept that the first iteration is just that; not perfection. Not sure if it's feasible to evaluate before merging? Does anyone have concerns at all here? Note: I'll need to get the LLM key and probably add it to the PMC's secret store. |
janhoy
left a comment
janhoy
left a comment
There was a problem hiding this comment.
This will be an interesting experiment and hopefully give contributors a quick and pleasant guidance. Let my 🤖🤖 analyze and below are the results. Some are arguably minor.
| choice = data["choices"][0] | ||
| content = choice["message"].get("content") or "" | ||
| if not content.strip(): | ||
| sys.exit(f"LLM returned no content (finish_reason={choice.get('finish_reason')})") |
There was a problem hiding this comment.
🤖 Worth also treating finish_reason == "length" and an empty parse_verdicts() as failures. A truncated or non-JSON reply currently posts a five-row all-❔ "No valid answer; needs human review" table, which is the likely outcome if the backend ignores chat_template_kwargs (vLLM-specific) and spends the 2000-token budget thinking.
| # 'ready_for_review' covers PRs opened as drafts; drafts themselves are skipped. | ||
| types: | ||
| - opened | ||
| - ready_for_review |
There was a problem hiding this comment.
Because triage runs only on these two events and never updates, the
That may have been the intended behavior, but perhaps it should be clearly stated in the note that it will not be re-evaluated?
| - id: needs_ref_guide | ||
| title: "Reference Guide update" | ||
| help_url: "https://github.com/apache/solr/tree/main/solr/solr-ref-guide" | ||
| evidence_files: ["solr/solr-ref-guide/**"] |
There was a problem hiding this comment.
This glob is a superset of the upgrade-notes glob below, so an upgrade-notes-only change also counts as Reference Guide evidence. Intended?
- Skip the comment when there is nothing to judge (only changelog/docs/test files changed) - Never fail the PR: model errors, truncated or unusable replies post no comment; judge job is continue-on-error - Dedup check reads one page of comments instead of paginating all - Config is now JSON, so the privileged job needs no PyYAML install; drop requirements-pr-triage.txt - Ref Guide evidence excludes upgrade-notes pages (evidence_exclude) - State up front in the comment that triage is not re-run on later pushes - Remove dead branches (empty-comment check, test-file diff ranking) Generated-by: Claude Sonnet 5.5
…--verdicts) - --from-git BASE triages the commits, staged, unstaged and untracked files since BASE; commit messages stand in for the PR title and description - --verdicts FILE renders answers given by an agent instead of calling the model - LLM_BASE_URL defaults to the ASF gateway; ASF_LLM_API_KEY is accepted as the key Generated-by: Claude Sonnet 5.5
Generated-by: Claude Sonnet 5.5
…ghten Jira and tests questions - skip_when_included: a judgement whose evidence is already present is not sent to the model; its row reads "Already present; not judged." - Jira is warranted only if a changelog entry is - Build, CI and developer tooling do not need tests Generated-by: Claude Sonnet 5.5
…t with the entry - The Changelog judgement chooses a type (added, changed, fixed, deprecated, removed, dependency_update, other) or "(not-worthy)", and the comment flags a missing entry, an unneeded one, or a type that differs from the entry's - summary_from: the changelog entry's title is given to every judgement as the author's own summary - Fixes to the build, tests or developer tooling are "other", not "fixed" - Trim the script docstring; the config format is described in the config's "_doc" key Generated-by: Claude Sonnet 5.5
|
🤖 Update on #5281 since the last round, including what differs from what was asked. New
Differences from the review feedback
Tuning so far Still open
🤖 Generated with Claude Code |
…verdict is false instead of answer no Generated-by: Claude Sonnet 5.5
Generated-by: Claude Sonnet 5.5
|
I set an API key using the I got the best success with I have a skill I use locally to invoke this: https://github.com/dsmiley/solr-claude-skills/blob/main/skills/pr-judgements/ Ready to merge and reap the rewards! |
|
🤖 AI text below 🤖 (reviewed by Nick Shanin) The test workflow merged here has its unittest command outside |
|
🤖 AI text below 🤖 (reviewed by Nick Shanin) I tried the existing runner on my workflow repair using the supported no-key agent mode ( |
|
Thanks Nick; I directly took that indentation change and pushed (without a PR, while I still have privileges to do so).
Sounds like a dev list thread. If you do that, include a link to such skills (probably in a repo you have but be a draft PR). |
|
Here's the first result from a PR! Automated PR triage (LLM-generated; may be wrong). Checked once, when the PR was opened or marked ready; not updated on later pushes. 1 possible mismatch(es) between what this change warrants and what the PR includes.
Overall I love seeing this in action and I think it's a great step forward 🎉 . It is kinda large & wordy. I think I'd prefer a series of bullets instead of a table. When there's full agreement -- no "Why" explanation needed. Maybe insert as a comment that can be viewed in the markdown view. Maybe confidence doesn't need to be shown when it's "high". WDYT @epugh ? |
|
🤖 Proposal: drop the "already triaged" check from the PR triage workflow. The
What changes: a PR that goes back to draft and then ready again gets a second triage comment and a second model call. The first comment stays and is not updated. If the author added a changelog entry or tests in between, the second comment reflects that. 🤖 Generated with Claude Code (and Smiley edited) |
|
I ported this to solr-operator :) apache/solr-operator#887 and it works beautifully |
|
Nice Jan! |
🤖 Add a pr-triage-llm workflow that, once per PR, asks an LLM via the ASF gateway (llm.apache.org) whether the change warrants a changelog entry, Reference Guide update, upgrade notes, and tests. It judges the change on its merits, without seeing what the author included, then posts one comment comparing each verdict with what the PR contains and highlighting mismatches in either direction. The questions live in .github/pr-triage/judgements.yml.
Since whether a PR needs a changelog entry is now judged (advisory), the validate-changelog check no longer fails PRs without one; it only validates the structure of entries present. The no-changelog label and its /no-changelog and /needs-changelog comment commands are removed.
Requires repository secret ASF_LLM_API_KEY; optional variable LLM_MODEL.
Claude-Session: https://claude.ai/code/session_01KwTAxfG9oFYQQH7pZDB4op