spring-review
README.md
# spring-review
Line-level review of Spring and MyBatis changes. Give it a git diff; get back
findings with a file, a line number, and the rule that fired.
It exists for the mistakes you have to know Spring to see:
- `@Transactional` on a method called from inside the same class. The proxy never
sees that call, so nothing is transactional. It compiles, it starts, it passes review.
- `@Transactional` on a method that `throws IOException`. Spring rolls back on
RuntimeException only, so a checked exception commits the half-finished write.
- `orderLineMapper.selectPrice(id)` inside a `for` loop. One query per element.
- `where name = '${keyword}'` in a Mapper XML. `${}` is string concatenation.
- `like concat('%', #{kw}, '%')`. A leading `%` is where the index stops helping.
- `Executors.newFixedThreadPool(8)` called from a singleton bean's method.
Rules make these calls, offline and without an API key, so the same diff gives the
same output. `--llm` rewrites the summary paragraph and nothing else.
> [中文 README](./README.zh-CN.md) · [Rules](#rules) · [Why not just ask the model](#why-not-just-ask-the-model) · [What it does not do](#what-it-does-not-do)
Exit codes: `0` nothing blocking, `1` at least one error-severity finding, `2` the
tool could not run — which includes an argument or a config file it cannot honour, so
a mistyped severity is a red build rather than a clean pass.
## Install
The npm package is not published yet. The CLI comes from this repository's
Releases instead, and Node 20 or newer is the only requirement:
```bash
npm i -g https://github.com/JingYu-create520/spring-review/releases/latest/download/spring-review.tgz
spring-review --diff origin/main..HEAD
```
`releases/latest` moves; `releases/download/v0.1.3/spring-review.tgz` does not.
To work on the tool rather than use it, clone it and run the build:
```bash
git clone https://github.com/JingYu-create520/spring-review.git
cd spring-review
npm ci && npm run build
node dist/cli.js --patch examples/sample.patch
```
Below, `spring-review` means either of those two CLIs.
## Usage
```bash
spring-review # uncommitted changes, incl. new files
spring-review --diff origin/main..HEAD # a commit range
spring-review --staged
spring-review --file src/main/java/demo/UserService.java
spring-review src/main/java # a directory, recursively
spring-review "src/**/*Service.java" # or a glob
spring-review --patch pr.patch --format github
```
A path may be a file, a directory or a glob; `target/`, `build/`, `node_modules/`
and `generated/` are never walked. An input that yields nothing reviewable is
named in the output rather than passing quietly — a linter that reports "clean"
over zero files is worse than one that fails. A patch whose context lines do not
match the file it claims to change is named the same way: the review falls back to
the patch's own lines, because scoring added numbers against unrelated code
produces the one result nobody should trust.
The default mode covers files that have never been `git add`ed, because a new
class is what "review my changes" means on the first run; `--diff` and `--patch` do
not, since they describe history. Only added lines are reported, so existing code
does not come back to haunt you.
Other flags: `--min-severity error|warn|info`, `--exclude '**/generated/**'`,
`--disable SPR005`, `--experimental`, `--list-rules`. A repo can keep its own
settings in `.spring-review.json` — `exclude`, `disable`, `only`, `minSeverity`,
`experimental`. It is looked for in the run directory and then upwards, stopping at
the repository root, so a module in a multi-module build still gets the file the team
wrote; the nearest one wins outright rather than being merged with the one above it,
and a file that cannot be parsed is exit 2 rather than a quietly default run.
To silence one finding, say why:
```java
// spring-review:disable MYB001 "sortField 来自服务端白名单映射"
```
That works on the offending line or the line above it; `disable-file` covers a whole
file.
### GitHub Action
```yaml
on: [pull_request]
permissions: { contents: read, checks: write }
jobs:
review:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v7
with: { fetch-depth: 0 }
- uses: JingYu-create520/spring-review@v0
with:
exclude: "**/generated/**"
```
Findings arrive as check-run annotations, which is why there is no token to hand
over and no comment thread to de-duplicate. `fail-on-error: false` keeps the job
green and still marks the lines; it does that by re-emitting at `notice` level,
because a `::error` workflow command fails the run whatever the exit code says.
The Action downloads the prebuilt `spring-review.tgz` from this repository's
Releases and runs it with node in a temporary directory, so the runner builds
nothing and there is no npm package to wait for. `ref: v0.1.3` pins the CLI to a
version. `install-from: npm` switches over once the package exists;
`install-from: local` runs a build already present in the job. CI exercises the
github and local paths on every push.
`@v0` is a moving tag that CI keeps pointed at the `main` commit that passed
those jobs; `@v0.1.3` freezes the Action itself as well.
### Code scanning
`--format sarif` writes SARIF 2.1.0 with all rule descriptions embedded, so findings
sit on the Security tab instead of scrolling past:
```yaml
- run: spring-review --diff origin/main..HEAD --format sarif > spring-review.sarif
- uses: github/codeql-action/upload-sarif@v4
with: { sarif_file: spring-review.sarif, category: spring-review }
```
This repo runs it against its own fixtures on every push to `main`. The Security
tab holds 21 open alerts; a 22nd closed itself when 0.1.2 stopped reporting the
framework `${ew.customSqlSegment}` placeholder, because an upload that no longer
lists a finding marks it fixed.
### MCP server
```json
{
"mcpServers": {
"spring-review": {
"command": "node",
"args": ["/path/to/spring-review/dist/cli.js", "mcp"]
}
}
}
```
Three tools: `review_diff` (patch text, or a range plus a `cwd`), `review_file`,
`list_rules`. `skills/spring-review/SKILL.md` is the same knowledge aimed at agents
that read skills instead of speaking MCP.
## Rules
`spring-review --list-rules` prints these with rationale, machine-readable.
| ID | Catches | Sev |
| --- | --- | --- |
| SPR001 | `@Transactional` self-invocation, by `this.m()` or a bare `m()` | error |
| SPR002 | `@Transactional` with a checked exception and no `rollbackFor` | error |
| SPR003 | `@Async`/`@Scheduled` that cannot apply: non-public, static, self-invoked, or `@Scheduled` with arguments | error |
| SPR004 | `new Thread` / `Executors.newXxx` inside a singleton bean | error |
| SPR005 | mutable instance state written from unguarded methods (`--experimental`) | warn |
| SPR006 | `@Cacheable` bypassed by a self-call, or keyed by every argument | warn |
| MYB001 | `${}` interpolation in statements, `<sql>` fragments and `@Select` SQL; the fix follows the position — `#{}` for a value, an identifier whitelist for a name, the server-side source for a whole fragment | error |
| MYB002 | N+1: mapper call in a loop or stream, or a `resultMap` nested select | error |
| MYB003 | leading-wildcard `LIKE`, as a literal, via `concat`, or via `<bind>` | warn |
| MYB004 | `SELECT *` | warn |
| MYB005 | `SELECT` with no `WHERE` and no `LIMIT` | error |
Three of those are less obvious than they look, and they are the reason the rule set
is small.
`${}` cannot simply be reported everywhere. MyBatis-Plus and MyBatis Generator put
their own text through `${ew.customSqlSegment}`, `${criterion.condition}` and
`order by ${orderByClause}`; those are framework contract and are silent, because
warning on generated code is how a rule set earns to be ignored wholesale. What
does get reported is `${}` in code someone wrote: a dynamic `ORDER BY ${sortField}`
drops to `warn` rather than `error`, because it cannot be turned into `#{}` —
column names are not bindable — and the useful answer is a server-side whitelist
that rejects anything unmapped. A placeholder written once in a shared
`<sql id="kwWhere">` block and `<include>`d by five statements is reported on the
block, once. That is the line to change, and it is where the injection is; the
statement bodies never contained it.
`SELECT` with no `LIMIT` is not always a full-table read. Under MyBatis-Plus the
interceptor adds pagination and the SQL stays bare, so the rule follows the mapper
XML's `namespace` to its interface and exempts statements whose parameters take an
`IPage`/`Page`. Without that step it would flag every paged query in a MyBatis-Plus
codebase, which is most of them. Text this file cannot see counts as unknown rather
than absent: an `<include refid="other.Mapper.commonWhere">` that resolves to nothing
is where the condition may be hiding.
A leading-wildcard `LIKE` rarely appears as `'%foo%'`, because a `#{}` inside
quotes stops being a parameter. The forms people write are `concat('%', #{kw},
'%')` and `<bind value="'%' + kw + '%'/>`, so a rule that matched only the literal
would be quietly useless. The quoted spelling does turn up in real code —
newbee-mall's goods search has `CONCAT('%','#{goodsName}','%')` in two statements —
and that one is not a slow query but a filter that cannot bind, so MYB003 calls it
an error and says to drop the quotes.
## Why not just ask the model
Three things a model does not do with a diff. It cannot give you a line number you
can click. It cannot promise the same answer twice, so it cannot gate a merge. And it
does not know that `ORDER BY ${sortField}` is a whitelist problem rather than a "use
`#{}`" one — every release of this tool up to v0.1.12 recommended `#{}` for all six
of a real framework's `${}` placeholders, which is a mistake a model would have
argued about. Rules at least fix theirs in one commit and pin it with a test.
So rules produce the findings: each has an id, the evidence, a line in HEAD, a
suggestion, a unit test. `--llm` talks to any OpenAI-compatible endpoint
(`SR_LLM_BASE_URL`, `SR_LLM_API_KEY`, `SR_LLM_MODEL`) to write the summary paragraph.
`tests/cli.test.ts` checks the JSON report is byte-identical with it on or off, and
that an unreachable endpoint yields the offline template rather than a failed build.
## Run against code that was not written for this tool
The demo project is a fixture: on its own it only proves the rules fire when told
to. So the rules were pointed at real repositories, in whole-directory mode, with
nothing planted for them.
| Codebase | Files | Findings | What they were |
|---|---|---|---|
| two Spring gateway modules (no MyBatis) | 195 | 3 | queries inside polling loops in `*IT.java` — real per-iteration round trips, intentional in a test |
| [abel533/MyBatis-Spring-Boot](https://github.com/abel533/MyBatis-Spring-Boot) | 24 | 2 | one `SELECT *`, one unbounded select |
| [macrozheng/mall](https://github.com/macrozheng/mall) | 638 | 29 | 15 `SELECT *`, 12 mapper calls inside batch loops, 1 unbounded select, 1 dead `@Scheduled` |
| [mybatis/mybatis-3](https://github.com/mybatis/mybatis-3) | 1837 | 590 | every one of them inside the framework's own test mappers: `SELECT *`, unbounded selects, `${}` feature tests |
| [newbee-ltd/newbee-mall](https://github.com/newbee-ltd/newbee-mall) | 98 | 4 | two leading-wildcard searches, and two goods-search conditions whose `#{}` sits inside quotes so it never binds |
| [yangzongzhuan/RuoYi-Vue](https://github.com/yangzongzhuan/RuoYi-Vue) | 295 | 50 | six `${}` injections of raw text, 7 unbounded `selectXxxAll`, 17 mapper calls inside service loops, 19 leading-wildcard searches, one `SELECT *` |
| [jeecgboot/JeecgBoot](https://github.com/jeecgboot/JeecgBoot) | 1099 | 243 | 128 mapper calls inside service loops, 55 `SELECT *`, 30 leading-wildcard searches, 26 `${}` on dynamic table/column names and injected filter fragments, and one `@Transactional` self-call that really does lose the transaction |
`Files` counts every `.java` and `.xml` the run opened. The files it refused to
analyze are named by path in that same run's `skipped:` block instead of being
quietly passed over — 226 of mybatis-3's 1837 (site-documentation `<document>` XML,
`pom.xml`, `<configuration>` files), 9 of RuoYi's 295, and 43 of JeecgBoot's 1099
(35 non-mapper XML plus the 8 generator templates above), which is why the reviewed
count in `--format json` is 1611, 286 and 1056.
The mall run found a real bug: `@Scheduled(cron = …)` on a **private** method in
`OrderTimeOutCancelTask`, which Spring will not invoke. That task does not run.
That pass produced four fixes, each pinned by a test afterwards. The first version
of MYB001 reported 84 findings on mall, all of them MyBatis Generator's own
`order by ${orderByClause}`; framework placeholders are silent now, because a rule
that fires 84 times on generated code gets the whole rule set ignored. Non-mapper
XML (`pom.xml`, `logback-spring.xml`) logged a skip per rule instead of per file. A
directory argument reported a clean run over zero files. And
`repository.findByName(name).map(e -> repository.save(e))` was read as an N+1 even
though the `Optional` runs once.
mybatis-3 was the first corpus with real MyBatis XML, and it cost four more. Its
site documentation is `<document>` XML whose `<source>` blocks quote mapper
examples — 64 findings, all of them on prose, including an injection error on
`<include refid="${include_target}"/>`, which is the documented way to choose a
fragment. Annotation SQL was scanned from the raw file, so the Javadoc of
`@Select` itself produced a `SELECT *` finding. Files with no SQL in them logged
one skip per rule again. And a `${}` that only picks an include target is a `warn`
now, not an injection `error`.
RuoYi cost two more, and both were about *what the tool says* rather than what it
counts. Its six `${}` are a data-scope aspect's WHERE fragment
(`${params.dataScope}`, five times across the user, role and dept mappers) and a
code generator's DDL (`<update id="createTable">${sql}</update>`) — positions
where `#{}` is structurally impossible, because a bound parameter can replace a
value but never a clause. The advice given there had been "改为预编译参数 `#{params}`": a Map handed to
the driver. MYB001 now reads where the placeholder sits and says the true thing —
`#{}` for `= ${kw}`, an identifier whitelist for `from ${table}`, `select ${id}`,
`col_${suffix}` and `order by ${sort}`, an OGNL warning for
`<if test="'${value}' == 'x'">`, and for a whole injected clause: the control
point is the server-side source, not the placeholder. Same for
`#{ids[${index}]}`, which picks *which parameter* to bind and was being reported as
injection; that is the one finding RuoYi and mybatis-3 combined removed.
The second: `SysConfigMapper.selectConfig` is `<include refid="selectConfigVo"/>`
plus `<include refid="sqlwhereSearch"/>`, and the second fragment is the `<where>`
block. Stripping tags to spaces to build the SQL text deleted the only evidence
that the statement was bounded, so MYB005 called a filtered query a full-table
read. `<where>`, `<set>` and `<trim prefix="WHERE">` now become the keyword
MyBatis actually emits, which also fixes every MyBatis Generator mapper whose
conditions arrive as `<include refid="Example_Where_Clause"/>`.
JeecgBoot cost four more, and a bug that no corpus would have found. Its
`SysDepartServiceImpl.deleteDepart()` calls `this.delete(id)` — which is
`@Transactional(rollbackFor = Exception.class)` — from a method that is not, so the
department delete and its child/relation cleanup run without the transaction the
author annotated; SPR001 reports it on the line. Two of the four fixes were about
what the tool reads rather than what it says: annotation arguments are captured from
a copy of the file with string literals blanked out (that is how a `)` inside a
`@Select` cannot close the annotation), and reading a rule's value out of that copy
made `@Cacheable(key = "#code+':'+#key")` look like a `@Cacheable` with no key at
all. And the repository ships the FreeMarker templates that *generate* its mappers,
under `code-template/…/mapper/xml/`, with a `<mapper>` root — 52 findings had been
sitting on template text, including `${r'$'}{key}`, whose only job is to write a
literal `${key}` into a file somebody will generate later. Those files are now
skipped by name, the same way a `<document>` of site documentation is.
The fourth: `@Cacheable` with several parameters and no `key` stopped being a
finding when every parameter is a plain value — `SimpleKey` of four `String`s *is*
the intended cache identity, and a warning on correct code is how a rule gets
muted. It still fires when an object joins the key.
The bug no corpus finds is the one a corpus run never touches: run the tool from a
module directory (`cd backend && spring-review`) instead of the repository root, and
git still answers with root-relative paths. v0.1.13 joined those onto `backend/`,
found no file, fell back to the patch text, and switched off every rule that needs
the whole file — 0 findings, exit 0, over a real `@Transactional` self-call. A
multi-module Maven tree is the normal shape for this tool's users, so that path is
now the tested one.
What is still not proven: a request parameter flowing straight into a `${}`.
RuoYi's values come from an aspect and a generator, and telling those apart from a
caller-controlled string needs the source of a value across files — which is
[#5](https://github.com/JingYu-create520/spring-review/issues/5), not this rule.
Every `${}` in all seven corpora was reported; none of them is known to have carried
a request parameter, and that distinction is not knowable from one file
([#5](https://github.com/JingYu-create520/spring-review/issues/5)).
## What it does not do
No compiler, no classpath. Structure comes from a bracket state machine over a copy
of the file with comments and string literals blanked out, so cross-file bean wiring,
custom meta-annotations like `@MyService`, and `@Bean`-registered classes are
invisible to it ([#2](https://github.com/JingYu-create520/spring-review/issues/2)),
and a `<sql>` fragment borrowed from another mapper file is not visible to the
statement that includes it
([#3](https://github.com/JingYu-create520/spring-review/issues/3)). Nor can it tell
whether the value behind a `${}` is reachable from a request
([#5](https://github.com/JingYu-create520/spring-review/issues/5)): every one is
reported, and `// spring-review:disable MYB001` at the site is the escape hatch.
When structure does not resolve, the rule stays quiet and says so under `skipped`.
That happens on Java it cannot follow, on `--patch` input for a file you do not have
locally, and on a patch whose context lines do not match the file it claims to
change; `${}` detection still works in those cases, because one line is enough to
judge.
It is not SonarQube and not Checkstyle. Formatting and code smell are not what it
looks at.
## Development
```bash
npm ci
npm run typecheck && npm test # 176 tests
npm run build # dist/cli.js, dist/index.js, dist/mcp/index.js
```
`examples/demo-project` is a small order/stock app with the same mistakes planted
without labels, and two files written correctly on purpose.
`tests/demo-project.test.ts` fails if a rule stops firing there or if a clean file
starts reporting, which is what keeps the numbers in this file honest.
Layout: `src/diff` (patch → lines), `src/analyze` (Java and mapper XML structure),
`src/rules` (rules, engine, suppression), `src/report`, `src/llm`, `src/mcp`.
## License
MIT. See [LICENSE](./LICENSE).
## Also by me
- [sql-index-advisor](https://github.com/JingYu-create520/sql-index-advisor) — MySQL/MyBatis index advice from slow logs and mapper XML
- [mcp-tool-gateway](https://github.com/JingYu-create520/mcp-tool-gateway) — RBAC, audit and human confirmation in front of MCP tool calls
- [agent-regression](https://github.com/JingYu-create520/agent-regression) — regression tests for agents, in CI
- [vredis](https://github.com/JingYu-create520/vredis) — a small vector database in Rust that speaks RESP2
- [mini-search](https://github.com/JingYu-create520/mini-search) — local-first hybrid search for Chinese, hand-written analyzer
- [jvm-incident-agent](https://github.com/JingYu-create520/jvm-incident-agent) — JVM incident analysis from thread dumps, GC logs and heap histograms
This server cannot be deployed
Maintenance
ActivityMaintained
ResponsivenessUnresponsive