feat(review): add the Java backend reviewer agent to /dot-review - #36738
feat(review): add the Java backend reviewer agent to /dot-review#36738fabrizzio-dotCMS wants to merge 2 commits into
Conversation
Introduces the `dotcms-java-backend-reviewer` agent definition and the
`/dot-review` PR review command that dispatches it, so backend Java
changes get reviewed against the project standards documented in
CLAUDE.md and docs/backend/.
The agent reports only evidence-backed findings with confidence >= 75,
banded Critical/Important/Quality, and each rule carries its own
"how to apply without false positives" guidance.
Two rules in this first revision are worth calling out because both
fail silently in ways Java-level tests do not catch:
- Commit listeners: work that must only run if a preceding persistence
operation succeeded belongs in HibernateUtil.addCommitListener, and
data that only exists after commit must be read inside the listener
body. Registering outside an open transaction runs the listener
inline, and on rollback it never runs at all.
- Records reaching VTL: dotCMS forks Velocity 1.7 in-tree and its
introspection resolves $obj.foo through getFoo()/getfoo()/get("foo")/
isFoo() only, never a record's foo() accessor. With
runtime.references.strict=false the reference renders as literal
text, so a record replacing a getter-based type on the Velocity
context breaks templates while the build stays green.
The skill also documents which subagent types actually exist in the
repo, so the command falls back to inline review instead of dispatching
agents that are not committed yet.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
❌ Issue Linking RequiredThis PR could not be linked to an issue. All PRs must be linked to an issue for tracking purposes. How to fix this:Option 1: Add keyword to PR body (Recommended - auto-removes this comment)
Option 2: Link via GitHub UI (Note: won't clear the failed check)
Why is this required?Issue linking ensures proper tracking, documentation, and helps maintain project history. It connects your code changes to the problem they solve. |
|
Claude finished @fabrizzio-dotCMS's task in 19s —— View job I'll analyze this and get back to you. |
What
Adds the
dotcms-java-backend-revieweragent definition and the/dot-reviewPR review command that dispatches it, so backend Java changes get reviewed against the standards documented inCLAUDE.mdanddocs/backend/.Both files are new — neither was ever committed, so the review command and the agent land together. Tooling/config only: no product code, no build, no runtime impact.
.claude/agents/dotcms-java-backend-reviewer.mdgh pr view/gh pr diff, Grep, Glob, Read).claude/skills/dot-review/SKILL.md/dot-reviewcommand that classifies changed files and dispatches the agentHow the agent behaves
Rules cover: Config/Logger, Maven versions in
bom/application/pom.xmlonly, REST@Schemacorrectness and endpoint documentation for OpenAPI/AI-agent consumption, OpenAPI drift,@WrapInTransaction/@CloseDBIfOpenedlifecycle, commit listeners,respectFrontendRoles, distributed-FS I/O,Config.getPropertyin loops, IT suite registration, input/query/file-name security, cache-returnedContentletmutation, Page API REST↔GraphQL parity,SecurityLoggeraudit trail, PostgreSQL-only, third-party library wrapping, Karate/Postman coverage for new endpoints, virtual threads, records reaching VTL, and general quality/progressive-enhancement checks.Two rules worth highlighting
Both fail silently — the build stays green and Java-level tests pass, so a human reviewer is unlikely to catch them:
Commit listeners. Work that must only run if a preceding persistence operation succeeded belongs in
HibernateUtil.addCommitListener, and data that only exists after commit must be read inside the listener body. Verified semantics fromHibernateUtil: listeners fire afterconnection.commit(); on rollback the maps are cleared and they never run; registering with no open transaction runs the listener inline (else { listener.run(); }), silently degrading the post-commit contract; async listeners run on another thread without the transaction's connection or thread-locals. So an index write, cache flush, event notification, external call or file deletion executed inline after a write survives a rollback and leaves the DB and the outside world disagreeing.Records reaching VTL. dotCMS forks Velocity 1.7 in-tree (
dotCMS/src/main/java/org/apache/velocity/), andUberspectImpl.getPropertyGetresolves$obj.foothroughgetFoo()→getfoo()→Map.get("foo")→get("foo")→isFoo()— never a record's canonicalfoo()accessor. Withruntime.references.strict = falsethe unresolved reference renders as the literal text$obj.fooin the page. Reflection itself is fine ($rec.foo()with parens works), but only forpublicrecords:ClassMapreflects public classes and public methods only, so a package-private or non-public nested record exposes nothing at all. Net effect: replacing a getter-based type with a record on a Velocity context object breaks shipped templates and customer VTL while everything compiles.Note this does not contradict the "prefer immutable carriers" rule — records stay recommended for Java-internal DTOs and REST/GraphQL payloads (Jackson handles them natively). The requirement is scoped strictly to the Velocity boundary, and there are currently no records under
com.dotcms.rendering, so the rule is preventive rather than describing existing debt.Agent availability
The skill previously referenced five subagent types that exist in no branch:
dotcms-file-classifier,dotcms-typescript-reviewer,dotcms-angular-reviewer,dotcms-test-reviewer,dotcms-scss-html-style-reviewer. Dispatching a non-existent type fails, so the skill now documents which agents are actually committed and instructs the command to fall back to inline review for file classification and the frontend buckets — and to state that in its output rather than claiming a specialized agent ran. When those agents land, that note is what gets removed.Testing
Documentation/config only; nothing to build or run. Reviewed by exercising
/dot-reviewagainst backend PRs locally. Every semantic claim in the two highlighted rules was verified against the source rather than from memory:HibernateUtil.addCommitListener/finalizeCommitListeners/addRollbackListenerUberspectImpl.getPropertyGet,PropertyExecutor.discover,ClassMap.createMethodCache,SecureUberspectorsystem.properties(runtime.introspector.uberspect) andvelocity.properties(runtime.references.strict)🤖 Generated with Claude Code