Run /xwiki-review LLM skill automatically every day on new commits

Development
, ,
1

Hi devs,

I’d like to propose that we run the xwiki-review skill automatically, once a day, over everything pushed since the previous run — and possibly on PRs too. Below: what it checks, what it actually found today on the last 24h of master, where the output would go, and what it costs. The cost is real and it’s the main thing I want your opinion on.

What xwiki-review checks

It’s not “an LLM reads the diff”. It fans out eleven narrow reviewers over the same change, each reading it through one lens, each required to ground every finding in a rule this project actually holds — an OKF file, a skill, a CLAUDE.md, or a code fact. Generic best-practice folklore is rejected by construction.

Angle Runs when
Conventions any source file (code style, comments, commit messages, logging)
Architecture any .java (component system, module deps, oldcore placement)
Backward compatibility .java outside test/legacy (@since, @Unstable, signatures)
Defensive conventions always — see note below
Performance always
Tests always (a change with no test is itself the finding)
Accessibility .vm, .html, .vue, .js, .less, UI .xml
i18n & UX any user-facing string, .properties, .vm, sheets
Documentation new/changed public API, user-visible behaviour
Data & migration *.hbm.xml, R*XWIKI*.java, Solr schema, XClass, XAR pages
Spec conformance when a JIRA key is found — diff vs. the issue’s own text

Then — and this is the part that makes it usable — every finding is handed to an independent skeptic whose job is to refute it. The skeptic must open the cited file and confirm the rule actually says what was claimed, including any clause that limits its scope. Only confirmed and certain survive; everything else is dropped silently. A review nobody trusts is worse than no review, so the whole protocol is built around suppressing false positives rather than maximising findings.

One deliberate design note on the “defensive conventions” angle: it checks mechanisms, not attacks — is the escaper there, is the check there, is the value treated as untrusted — phrased exactly as a linter would. It does not assess exploitability and never names a problem class. That’s because CI logs on a public repo are public, so there is no safe place in an automated pipeline to say that something is a hole. It covers the conventions; it explicitly does not replace a security review and must not be described as one.

Real example: today’s run on the last 24h of master

I ran it this morning over b88754f93a5..c291686077 — 7 commits, 48 files, +1082/−87 (XWIKI-24796 Live Data/PDF export, XWIKI-22936 SearchSuggestCode, and two [Misc] test-framework commits).

12 raw findings → 5 survived the skeptic stage. All five are minor. Five angles (architecture, defensive conventions, performance, accessibility, data & migration) came back clean from the outset.

What survived:

  1. A dead event listener, plus a comment that says otherwise. searchSuggest.js now lazily creates the suggest widget on mouseenter or focus of the quick-search field. But that field is rendered disabled, and action-menus.less collapses it to width:0; padding:0 while the button is aria-expanded='false' — a zero-area disabled control can’t receive a pointer, and mouseenter doesn’t bubble. So the mouseenter half is unreachable 100% of the time in Flamingo, and the comment claiming the pointer is taken into account is wrong. Nothing breaks (minchars:3 and a catch-up call cover it) — but the intended optimisation isn’t there. Three of the eleven reviewers found this independently.
  2. Documentation in Release Notes = N/A on XWIKI-24796, which ships three new public @Unstable script-service methods ($services.uix.render() / renderExtensions()). The OKF rule reserves N/A for a fix with no user- or developer-visible impact. A new scripting API is developer-visible, so 18.8.0’s release notes would carry no record of it.
  3. @param filters doesn’t document that null is accepted on the new renderExtensions(), although the implementation branches on it and a test asserts it. Worse, the @see target’s own @param calls the filters “Optional” — on a method that NPEs on a null map. A reader following the link draws the opposite of the truth.
  4. A new member on BasePage with no @since — page objects in *-test-* modules carry @since “whatever the visibility”, and Revapi is skipped in that tree so nothing in the build catches it.
  5. A new test importing javax.inject while the new class it tests uses jakarta.inject — both files added in the same commit.

Equally interesting is what the skeptics killed, because that’s the noise you’d otherwise be reading:

  • “XWIKI-24796 should be split or retitled” (raised as a blocker-ish major) — refuted: the doc fields were already filled, and no rule governs issue breadth.
  • “The logging call breaks the [] convention” — refuted: the value is already bracketed, and applying the suggested fix would produce the double brackets the same rule explicitly forbids. The skeptic also found that a committer-run logging audit had deliberately left an equivalent pattern alone.
  • “The JS change is untested” — refuted: the quote had been truncated before the strategy page’s “tests that do not rely on the DOM” clause, which routes this change to a functional test. And SearchSuggestIT turns out to depend on the new code.
  • “Failed fetch permanently disables suggestions” — pre-existing, identical before the change.

Four of the seven dropped findings were plausible-sounding and wrong. Without the skeptic stage this would be a nuisance, not a tool.

Where the digest gets published

It has to be somewhere publicly visible where you can answer inline, so the forum — this category. The alternatives don’t work for a daily range: GitHub commit comments are next to the code but unreadable as a stream and impossible to discuss, and JIRA is per-issue so a range spanning several issues has nowhere to land. The forum is the only one of the three where replying is natural.

Concretely:

  • One topic per month, one reply per day — e.g. “Automated review digests — September 2026”. A single permanent thread would mean that anyone who replies once gets notified every day forever, and they’d mute it, which kills the :+1:/:-1: signal the whole tuning loop depends on. A topic per day would isolate notifications properly and give a scannable list, but it’s ~250 topics/year and really wants its own subcategory — worth revisiting if the volume justifies it.
  • Posted by a dedicated bot account, not a committer’s. A digest arriving daily under a personal account reads as though that person wrote each one and floods their activity feed. Discourse supports this, and it’s worth settling before the first automated run since authorship can’t be migrated afterwards.
  • Quiet days don’t post. Most days will have no blockers and no majors. Posting “nothing found” twenty times a month is how people learn to skip it — better to post only when something survives verification, plus a one-line monthly tally so the silent days are still accounted for.

Here is what one actually looks like, using today’s run:

Example digest — master, 2026-09-08

Automated review — generated by Claude Code. It can be wrong. Treat every point below as a question, not a verdict: verify the claim against the code before acting on it, and dismiss anything that does not hold.

Automated review — master, 2026-09-07 → 2026-09-08

Reviewed 48 files (7 commits, +1082/−87) across all 11 angles. 12 raw findings, 5 after verification.

Range: b88754f93a5..c291686077 · XWIKI-24796, XWIKI-22936, 2 × [Misc]

Blockers — none.

Major — none.

Minor

  1. The mouseenter half of the new lazy-init guard is unreachable, and its comment states the opposite — the field is rendered disabled and collapsed to width:0; padding:0 while aria-expanded='false', so it cannot receive a pointer, and mouseenter does not bubble. Nothing breaks (minchars:3 plus the catch-up call cover it); the intended pre-warm is simply absent. Rule: okf/conventions/code-comments.md
    searchSuggest.js#L208-L221

  2. Documentation in Release Notes is N/A on XWIKI-24796, which ships three new public @Unstable script-service methods — N/A is reserved for a fix with no user- or developer-visible impact. Rule: okf/servers/jira.md (scoped “when closing”; the issue is still In Progress, so this is a heads-up, not a breach)
    UIExtensionScriptService.java#L160-L170

  3. @param filters does not document that null is accepted, though the implementation branches on it and UIExtensionScriptServiceTest:198 asserts it — and the @see target’s own @param calls the filters “Optional” on a method that NPEs on a null map. Rule: xwiki-javadoc skill, “document null / empty / absent behaviour”
    UIExtensionScriptService.java#L208-L212

  4. New member on BasePage carries no @since — okf/conventions/versioning.md covers page objects in *-test-* modules “whatever the module and whatever the visibility”, and Revapi is skipped in that tree (xwiki-platform-test/pom.xml:37) so the build cannot catch it.
    BasePage.java#L774-L780

  5. New test imports javax.inject while the new class under test uses jakarta.inject — both files added in the same commit. Rule: okf/conventions/code-style.md §javax→jakarta, and the org-wide CLAUDE.md
    UIExtensionRendererTest.java#L25-L28

No angles were skipped. Architecture, defensive conventions, performance, accessibility and data & migration returned no findings.

7 findings dropped in verification

Finding Verdict Why
XWIKI-24796 should be split or retitled false Doc fields already filled; no rule governs issue breadth
Logging call breaks the [] convention false Value already bracketed; the fix would produce the double brackets the same rule forbids
The JS change is untested false Citation truncated before the “tests that do not rely on the DOM” clause; SearchSuggestIT depends on the new code
Failed fetch permanently disables suggestions false Pre-existing; identical before the change
Page test does not exercise the wiki-UIX chain false Cited rule governs commit-message format, not test depth
xhtml/1.0 → html/5.0 is scope creep unverified Two of three cited sites were already html/5.0
e.g. in new Javadoc nitpick Real rule, but 84 existing occurrences vs 131 “for example”

Cost: ~1.83M tokens across 20 subagents (11 reviewers, 9 skeptics).

Was this useful? React :+1: or :-1: — the reactions are read back monthly to tune the reviewer.

In the real thing every finding carries a full-SHA permalink into the code and a link to the rule it cites; I’ve trimmed those to filenames here to keep the example readable.

The cost

This is the honest downside. Today’s run consumed ~1.83M tokens across 20 subagents (11 reviewers averaging ~110k, 9 skeptics averaging ~68k), plus orchestration — call it ~2M tokens for one day’s worth of commits. Daily, that’s roughly 60M tokens/month for master alone. Running it on PRs as well would multiply that by our PR volume.

So the question isn’t “is it good” — I think today’s run answers that — it’s “is it worth that much”.

Proposal

Start narrow and measure:

  • Daily routine on master (main for the other repos), covering everything since the previous run, publishing one digest to this category as described above. Skip automatically when the diff is only a license/xar:format/l10n sync or a Renovate bump — the skill already gates on this.
  • Not on PRs initially. Revisit once we have a month of daily data on the true-positive rate.
  • A :+1:/:-1: on each digest, read back monthly, to tune which angles earn their keep. If an angle produces nothing but noise for a month, drop it from the routing and the cost falls with it.
  • Keep it advisory. It’s a second pair of eyes, never a gate, and never a substitute for human review.

Open questions

  • Is ~60M tokens/month acceptable for master only? Who pays — is this a shared org budget or does it run on one committer’s account?
  • Should it run per-commit-range daily, or only on days with substantial changes (say >200 changed lines)?
  • Any objection to a bot account posting daily in this category — and does someone with forum admin rights want to create it?
  • Several angles found no rule to cite for things they wanted to flag — e.g. the OKF has no home at all for front-end/network performance (lazy loading, cache headers, request count per page view), which is exactly what XWIKI-22936 was about. Those become candidate conventions. Are we happy for the routine to feed the OKF that way?
  • For those of you not on Claude Code — the skill is a plain Markdown protocol, so in principle another tool could run it. Worth keeping it tool-agnostic?

WDYT?

2

Hi,

thanks for the proposal. Conceptually I’m not against we test it, but I’m afraid that we’re starting to consume tokens for work we’re already doing in the CI.

So, it feels like some of the review comments might be interesting, but IMO we need to target a bit more specifically what we want: I really don’t think we should use those tokens to perform analysis on code checkstyle for example.

In the example report you provided there’s for example:

I don’t think it’s something we want to catch. Right now we don’t provide those annotations in test and it doesn’t bring much value to have that report.

Another solution not discussed would be to post those directly on XWiki.org in a dedicated space. Personally I’m not a huge fan of having a single forum topic per month: we will have discussions about some reports spanning over several days and it will quickly a mess IMO.

On the other hand, having a single XWiki page report per day, allow to have comments on each page, and a better follow up IMO.

3

I had already fixed this before seeing it, but that’s an exception because I usually wait a bit before writing the documentation, to see if there are any complains or failures that might lead to an update of the new API/UI or even a rollback. But more importantly, the issue in this case is not yet closed. We can’t expect the JIRA issue to have documentation before it’s closed.

It refers to this:

@param filters Optional filters to apply before retrieving the list

I think the agent misunderstood “Optional” as a flag / marker that applies to the parameter name “filters” (text before), while in fact it is part of the description that follows, and the way I read it is:

there is no mandatory filter, as far as the API is concerned. All filters are optional, meaning you can specify zero, one or more filters; this doesn’t mean zero filters is null; zero filters is an empty collection of filters, and since we use a Map to specify the filters, zero filters is an empty Map.

I’m quite surprised that Claude didn’t consider this alternative interpretation.

This looks like something that static code analysis can catch with less resources.

In any case, I’m fine to try it out.

Thanks,
Marius

4

I think LLM-based review is a good idea in general, but I’m not convinced by this particular approach:

  • While testing against following our conventions is good, rejecting generic best-practice folklore feels harmful - we should follow (modern) generic best practices, even if we don’t have any project-specific rules, e.g., because we weren’t aware of those best practices or because we didn’t feel it was necessary to document them as they seemed obvious.
  • I wonder if we really need 11 narrow reviewers, or if it wouldn’t be better from a token usage and maybe even result point of view if we had fewer, broader reviewers that are grouped by the context they need to explore.
  • Killing noise might seem good at first glance, but I would have preferred if the LLM had kept the information about a pre-existing issue and instead checked if it already exists in Jira instead of ignoring it and reported the finding together with the information if we have a Jira issue. If I work/worked on some area and there is an important bug in it that can easily be fixed, I would seriously consider just fixing it/asking AI to fix it as I already have the context in my head to validate the fix.
  • It feels to me like after the commit has been pushed is too late - in particular for PRs, I would prefer if we had this on the PR itself so we have a proper context to discuss it before we merge and potentially release the change. It doesn’t need to be automatic on the PR, it could also be manually triggered by either the reviewer or reporter (if the reporter has sufficient permissions). If it is a manual trigger, maybe we could also give the possibility for a manual input regarding what should specifically be checked in addition to the generic checks. Having it on the PR instead of being executed by the author of the PR locally means that we can discuss the findings together and have them validated/disputed by the reviewers in addition to the PR author.
  • If the review is not bound to a PR, it’s another place to check and discuss every day basically (in addition to the CI).

I’m not against trying it out, though.

Personally, when using models like Opus 5 with just my AGENTS.md in context with a very simple prompt like “Review the last commit in xwiki-platform” I normally get very useful output with valid findings. I would be careful with giving agents too many instructions and context as my impression is that it just confuses them. I think at this point, frontier LLMs have already been trained a lot to provide useful reviews.

5

Helloo! Here to give my 2 cents as well.

+1

Important to remember and to update the skills accordingly.

+1

IMO, having daily checks on commits feels wasteful. Also, using expensive models for this kind of tasks also feels wasteful, when not as smart, but comparable models, are available for waaay cheaper.

I’d also like to add that just because Claude Code is a familiar tool, it doesn’t mean that it’s the best one for this kind of job. There are several specialized tools for AI code review, which may provide better results (and may also be cheaper over all?). Some examples:

I’d propose something along the lines of:

  1. Automatic AI code review for each PR, and each commit of a PR, but with a specialized tool, or with a cheaper, less capable model
    1.1. Since the code in a PR is usually a smaller chunk, a cheaper agent with sufficient instructions can usually figure out common issues with it
  2. Have Claude Code do a comprehensive code review at the end of each week (or month?) instead of daily. It will consume more tokens per run, since it would need to ingest more commits, but it would need to do it for 4 runs a month instead of 30. The lower frequency should cut down some of the cost.

I feel like this approach would offer just enough guidance while PRs are being created.
If a critical bug is introduced in a PR, a cheap model from China is at this point smart enough to identify it. And if the issue is really small, and it goes under the radar of an AI model and of a maintainer, it probably isn’t something that needs urgent attention. I think having that 1 week delay (until Claude validates the commits as well) for small issues is an acceptable tradeoff given the potential reduction in cost.

6

YouTube video testing out some of these automatic code review tools:

7

Also stumbled upon Open Code Review from Alibaba. It may be of interest.