From fc9adeb5d6b1dec95dee4ad3130c5ec2318245bc Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Sun, 2 Aug 2026 18:09:23 +0800 Subject: [PATCH 1/9] feat(cli): add a Java/JVM performance path rule to /review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The review's dimensions are domain-blind, and a Java diff's most expensive regressions are decided by the JVM, not by anything visible in the source: HotSpot chooses what to inline and what to compile by the callee's bytecode size (MaxTrivialSize 6 / MaxInlineSize 35 / FreqInlineSize 325 / HugeMethodLimit 8000), and a one-line change can flip it on a hot path. Like the GitHub Actions rule before it, the checklist attaches to *.java paths and reaches every code-reviewing agent whose territory contains one, scoped so non-Java diffs pay nothing. It carries: - the correctness traps dressed as perf/concurrency code (shared SimpleDateFormat, two-call ConcurrentHashMap compounds, DCL without volatile) at Critical; - the JVM-cost defects provable from source (per-call regex compiles, loop string +=, hot-path boxing, capturing lambdas in loops, unconditional log-message building, unpresized collections, legacy synchronized types, exceptions as control flow, per-call reflection) at Suggestion; - the JIT inlining thresholds with a two-tier verification discipline: measure with javap against base and head (never estimate bytecode from source), or run -XX:+PrintInlining / JMH when the code is runnable; unmeasured inlining claims are reported as mechanism at low confidence. Dogfooded against alibaba/fastjson2#3992 (BigDecimal parsing perf): the performance agent applied the threshold reasoning correctly — readBigDecimal was already far above FreqInlineSize before the diff and the change shrinks it, so no crossing was possible and no measurement was owed, stated with exactly that justification. Co-authored-by: Qwen-Coder --- .../commands/review/lib/path-rules.test.ts | 60 +++++++++++++++++++ .../cli/src/commands/review/lib/path-rules.ts | 51 +++++++++++++++- 2 files changed, 110 insertions(+), 1 deletion(-) diff --git a/packages/cli/src/commands/review/lib/path-rules.test.ts b/packages/cli/src/commands/review/lib/path-rules.test.ts index f76b852d0fc..d66c943c870 100644 --- a/packages/cli/src/commands/review/lib/path-rules.test.ts +++ b/packages/cli/src/commands/review/lib/path-rules.test.ts @@ -103,3 +103,63 @@ describe('pathRulesFor — scoped, or it is noise', () => { expect(out).toContain('$GITHUB_STEP_SUMMARY'); }); }); + +describe('pathRulesFor — the Java/JVM rule', () => { + it('attaches when a .java file changes, and names only that file', () => { + const out = pathRulesFor(['src/main/java/com/x/Main.java', 'src/pay.ts']); + expect(out).toContain('Java / JVM performance'); + expect(out).toContain('src/main/java/com/x/Main.java'); + expect(out).not.toContain('src/pay.ts'); + }); + + it.each([ + ['src/main/java/com/x/Main.java', true], + ['Main.java', true], + ['src/main/kotlin/Main.kt', false], + ['src/pay.ts', false], + ['docs/notes.java.md', false], + ])('%s → Java rule applies: %s', (path, applies) => { + expect(pathRulesFor([path]).includes('Java / JVM performance')).toBe( + applies, + ); + }); + + it('stacks with the workflow rule when a diff touches both', () => { + const out = pathRulesFor(['.github/workflows/ci.yml', 'src/Main.java']); + expect(out).toContain('GitHub Actions workflows'); + expect(out).toContain('Java / JVM performance'); + }); + + it('names the inline thresholds and both verification tiers', () => { + const out = pathRulesFor(['src/Main.java']); + // The table the whole JIT section hangs on: 325 is the user's case — a hot + // method that outgrows FreqInlineSize stops being inlined. + expect(out).toContain('FreqInlineSize'); + expect(out).toContain('325'); + expect(out).toContain('MaxInlineSize'); + // Static tier: compile and measure with javap; dynamic tier: PrintInlining. + expect(out).toContain('javap'); + expect(out).toContain('PrintInlining'); + }); + + it('refuses to estimate bytecode from source', () => { + // The one failure this checklist exists to prevent: an agent eyeballing a + // method and declaring it un-inlinable. The honest form is the mechanism, + // at low confidence, with the measurement named. + const out = pathRulesFor(['src/Main.java']); + expect(out).toMatch(/do not estimate bytecode from source/); + expect(out).toContain('Confidence: low'); + }); + + it('keeps the severity and scoping discipline of the skill', () => { + const out = pathRulesFor(['src/Main.java']); + expect(out).toContain('reviewing this diff, not auditing this file'); + expect(out).toContain('Favour precision over recall'); + // Inlining only matters where the call is hot; otherwise the rule is a + // lint sweep over every method in the file. + expect(out).toMatch(/cold method over 325 bytes is \*\*not\*\* a finding/); + // Slow is a cost, not a wrongness — perf findings are Suggestions, and the + // Criticals are reserved for the correctness traps. + expect(out).toMatch(/Performance findings are \*\*Suggestions\*\*/); + }); +}); diff --git a/packages/cli/src/commands/review/lib/path-rules.ts b/packages/cli/src/commands/review/lib/path-rules.ts index b51d46b54b2..e6dc68c548f 100644 --- a/packages/cli/src/commands/review/lib/path-rules.ts +++ b/packages/cli/src/commands/review/lib/path-rules.ts @@ -68,8 +68,57 @@ const GITHUB_ACTIONS: PathRule = { **Favour precision over recall here.** A false alarm on a workflow costs more reviewer trust than a missed minor nit, because a YAML finding is the easiest kind for an author to dismiss. Every finding needs the concrete trigger and the concrete outcome, like any other.`, }; +const JAVA: PathRule = { + title: 'Java / JVM performance', + matches: (p) => /\.java$/i.test(p), + checklist: `A Java change's most expensive regressions are decided by the JVM, not by anything a reader can see in the source: HotSpot chooses what to inline, what to scalar-replace, and what to compile at all — and a one-line change can flip each of those on a hot path. The general performance lens (N+1, repeated work, data structures) sees none of it. + +**You are reviewing this diff, not auditing this file.** A JVM-cost weakness the code already had, on a line this change does not touch, is out of scope — the same rule as everywhere else. What is in scope: a line this diff **adds or changes**, and a cheap path this diff **makes hot** (a new caller in a request loop, a call moved inside a loop). + +**Correctness traps dressed as concurrency or performance code (Critical):** + +- **A \`SimpleDateFormat\` shared across threads.** It is not thread-safe: concurrent \`parse\`/\`format\` corrupts its calendar state and returns silently wrong values or throws. If the diff adds a shared (static or instance field) \`SimpleDateFormat\`, or makes an existing one reachable from more than one thread, that is wrong, not slow. The fix is \`java.time.format.DateTimeFormatter\` (immutable) or a \`ThreadLocal\`. +- **A compound action on \`ConcurrentHashMap\` built from two calls.** \`get\`-then-\`put\`, \`containsKey\`-then-\`put\`, check-then-act of any shape: the map's thread safety covers each call, not the pair, and the race loses an update or runs the guarded work twice. Say what the race corrupts — that is the finding. Fix with \`putIfAbsent\`, \`computeIfAbsent\`, or \`merge\`. +- **Double-checked locking without \`volatile\` on the field.** The reading thread can observe a partially constructed object. If the diff adds or touches the idiom, the field must be \`volatile\` (or replaced with the holder idiom). + +**JVM-cost defects provable from the source (Suggestion — a cost, not a wrongness):** + +- **A regex compiled per call.** \`Pattern.compile(...)\` in a method body or loop, and the \`String\` conveniences that hide it — \`matches\`, \`replaceAll\`, \`replaceFirst\`, and \`split\` with a real regex (single-character splits take a fast path) — recompile the pattern on every invocation. On a per-request or per-record path that is real CPU. Fix: \`private static final Pattern\`. +- **\`+=\` on a \`String\` inside a loop.** Each iteration rebuilds and copies the whole accumulated prefix; the loop is O(n²) in the final length. Fix: one \`StringBuilder\` outside the loop. +- **Boxing on a hot path.** A \`Long\`/\`Integer\` loop accumulator, a \`Map\` over dense \`int\` keys, \`.boxed()\` in a counted stream: every box past the small-integer cache is an allocation plus a pointer chase per iteration. Fix: primitives, or the primitive streams/collections. +- **A capturing lambda or method reference inside a hot loop.** A non-capturing lambda is a singleton; one that closes over a local allocates per iteration. If the diff moves a capturing lambda into a loop, hoist it or restructure it to capture nothing. +- **A log message built whether or not it is logged.** \`log.debug("result: " + value)\` pays the concatenation with the level off, and even \`log.debug("{}", expensive())\` still pays \`expensive()\`. Fix: parameterized messages, and guard expensive arguments with \`isDebugEnabled()\` or a supplier-based logging API. +- **A collection sized after the fact.** A \`HashMap\`/\`ArrayList\` the code then fills with a known N pays repeated rehash/grow-copy chains; presize it (a \`HashMap\` needs \`N/0.75 + 1\` to avoid rehashing at all). +- **Legacy synchronized types in new code.** \`Vector\`, \`Hashtable\`, \`StringBuffer\` put a monitor on every call; the unsynchronized equivalents are the default for a reason. +- **An exception used as control flow.** Validating by catching \`NumberFormatException\`, looping until \`EOFException\`: \`fillInStackTrace\` costs more than the work being guarded. Validate instead. +- **Reflection rediscovered per call.** \`getMethod\`/\`getField\` on every invocation of a hot path; cache the \`Method\`/\`Field\` — or a \`MethodHandle\` — once. + +**JIT inlining — the regression no dimension can see, and you cannot prove from source:** + +HotSpot inlines by the **bytecode size of the callee**, against these product defaults (a project's own \`-XX:\` flags, or a different JVM, change them — check before citing): + +| callee bytecode size | inlinable | +| --- | --- | +| ≤ 6 (\`MaxTrivialSize\`) | always | +| ≤ 35 (\`MaxInlineSize\`) | even when cold | +| ≤ 325 (\`FreqInlineSize\`) | only at a hot call site | +| > 325 | never | +| ≥ 8000 (\`HugeMethodLimit\`) | never even JIT-compiled | + +Two more ways a diff can flip it: an already-compiled callee whose **native** size passes \`InlineSmallCode\` (~1000, product) stops being inlined to protect the code cache, and a call site that gains a **third** receiver implementation goes megamorphic — vtable dispatch, no inlining at any size. So the diff-shapes that matter: a small method on a hot path the change **grows** (added validation, logging, a branch), a new layer in a hot call chain, a new implementation registered for an interface invoked in a hot loop. A cold method over 325 bytes is **not** a finding — inlining only matters where the call is hot. + +**Measure it; do not estimate bytecode from source.** Two tiers, in order of cost: + +1. **Static — available whenever the project builds.** Compile (\`mvn -q -DskipTests compile\`, \`gradle classes\`, or \`javac\` the module), then \`javap -c -p \` and read the method's size: the offset of its last instruction plus that instruction's width. Compare against 35 and 325. For the **crossing** claim — "this diff pushed it over the threshold" — compile the base revision the same way and measure the same method on both sides; a method that was already over is pre-existing, not a finding. +2. **Dynamic — when the code is runnable.** Run the workload with \`-XX:+UnlockDiagnosticVMOptions -XX:+PrintInlining\` and grep for the method: \`callee is too large\` / \`hot method too big\` is the JVM itself declining to inline. JMH gives the before/after throughput; JITWatch visualizes the same decision logs. + +A finding that a method "can no longer be inlined" without one of these two tiers is a guess stated as fact. Report the **mechanism** instead, at \`Confidence: low\`: the threshold at risk, what the diff added to the method, and the measurement still to run. And if the diff *claims* this path got faster while the mechanism above says it cannot have, the finding is the unsubstantiated claim. + +**Favour precision over recall here.** A guessed JVM finding is the easiest kind for an author to dismiss, and one dismissal teaches them to skip the rest of the review. Every finding needs the concrete hot path and the concrete cost, like any other. Performance findings are **Suggestions**: slow is a cost, not incorrect behaviour — the Critical entries above are Critical because they are *wrong*.`, +}; + /** Every rule, in the order their checklists are appended. */ -export const PATH_RULES: PathRule[] = [GITHUB_ACTIONS]; +export const PATH_RULES: PathRule[] = [GITHUB_ACTIONS, JAVA]; /** * The checklists that govern `paths`, as a brief section — or `''` when none do. From 9413b96e6f5d85b9f62723376dabc9d4110dd07e Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Sun, 2 Aug 2026 19:15:27 +0800 Subject: [PATCH 2/9] feat(cli): name hot/cold splitting as the fix for a grown hot method MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An A/B experiment measured what the checklist adds on top of the model. Four crafted Java diffs (a hot method grown past FreqInlineSize, a map-backed cache, a per-element append loop, a precompiled regex parser) were each reviewed by a blind Agent-4 equivalent with and without the Java path rule, on qwen3.8-max-preview. Without the rule, the performance agent never once considered inlining across all four diffs, and filed a high-confidence finding that a constant long division costs 20-90 cycles per iteration — bytecode-true, but C2 strength-reduces constant division to a multiply-by-magic-number, so the cost does not survive the JIT. With the rule, it measured every diff with javap (base 80 bytes, head 338, crossing FreqInlineSize at 325), reported the crossing at low confidence with the tier-2 check named, dismissed the division with the correct mechanism, and proposed the fix as a hot/cold split with the exact bytecode range to extract. The fix shape is the part the model does not supply on its own, so the checklist now names it: move cold paths into a private helper, never @ForceInline (which bloats every caller), and state the extraction as a bytecode range and a resulting size. The same experiment cut three candidate additions — dense-key cache container choice, per-element-to-bulk loops, and regex-for-fixed-formats — because the control runs reached the same findings without them. Co-authored-by: Qwen-Coder --- .../cli/src/commands/review/lib/path-rules.test.ts | 12 ++++++++++++ packages/cli/src/commands/review/lib/path-rules.ts | 2 ++ 2 files changed, 14 insertions(+) diff --git a/packages/cli/src/commands/review/lib/path-rules.test.ts b/packages/cli/src/commands/review/lib/path-rules.test.ts index d66c943c870..c0e1e2478c4 100644 --- a/packages/cli/src/commands/review/lib/path-rules.test.ts +++ b/packages/cli/src/commands/review/lib/path-rules.test.ts @@ -151,6 +151,18 @@ describe('pathRulesFor — the Java/JVM rule', () => { expect(out).toContain('Confidence: low'); }); + it('names hot/cold splitting as the fix, and rules out @ForceInline', () => { + // A/B-measured: without the checklist the performance agent missed an + // 80→338-byte threshold crossing entirely; with it, the agent proposed + // extracting a named bytecode range into a helper. The fix shape is the + // part the model does not supply on its own — pin it, and pin the + // anti-pattern it must not suggest (@ForceInline bloats every caller). + const out = pathRulesFor(['src/Main.java']); + expect(out).toContain('hot/cold splitting'); + expect(out).toContain('@ForceInline'); + expect(out).toMatch(/bytecode range to extract/); + }); + it('keeps the severity and scoping discipline of the skill', () => { const out = pathRulesFor(['src/Main.java']); expect(out).toContain('reviewing this diff, not auditing this file'); diff --git a/packages/cli/src/commands/review/lib/path-rules.ts b/packages/cli/src/commands/review/lib/path-rules.ts index e6dc68c548f..90a445ccc42 100644 --- a/packages/cli/src/commands/review/lib/path-rules.ts +++ b/packages/cli/src/commands/review/lib/path-rules.ts @@ -114,6 +114,8 @@ Two more ways a diff can flip it: an already-compiled callee whose **native** si A finding that a method "can no longer be inlined" without one of these two tiers is a guess stated as fact. Report the **mechanism** instead, at \`Confidence: low\`: the threshold at risk, what the diff added to the method, and the measurement still to run. And if the diff *claims* this path got faster while the mechanism above says it cannot have, the finding is the unsubstantiated claim. +**When the finding IS a grown hot method, the fix to suggest is hot/cold splitting — not reverting the change, and not \`@ForceInline\`** (which copies the callee's full body into every caller, bloating their bytecode and their own inline budgets). Move the cold paths — error handling, rare branches, defensive validation, the \`switch\` arms that almost never fire — into a small private helper, leaving the common path under the threshold; the helper, now called from one site, is itself inlinable. Name the bytecode range to extract and the size it leaves behind, the way the measurement names them: "extract bytecodes 212–311 (~100 bytes) into \`applyRounding\`, leaving \`parseAmount\` at ~238 bytes" is a finding an author can act on; "consider splitting the method" is not. + **Favour precision over recall here.** A guessed JVM finding is the easiest kind for an author to dismiss, and one dismissal teaches them to skip the rest of the review. Every finding needs the concrete hot path and the concrete cost, like any other. Performance findings are **Suggestions**: slow is a cost, not incorrect behaviour — the Critical entries above are Critical because they are *wrong*.`, }; From bc862d89c0e3c1068462f94beb2cb8daffcef1fc Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Sun, 2 Aug 2026 20:00:01 +0800 Subject: [PATCH 3/9] fix(cli): address review on the Java/JVM path rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seven findings from the PR review, all verified before fixing: - The static measurement tier prescribed 'compile the base revision the same way', which reads as git checkout/stash in the one worktree nine agents share concurrently — or in the user's own checkout in local mode, where reviewsCode agents get no 'do not build the main checkout' guard (that guard is role-7-only). Rewritten as a non-mutating procedure: extract the base side with git show into a scratch dir, javac -d there, never checkout/stash/build in place. - A full mvn/gradle build runs the branch's contributor-controlled build logic; the checklist now says to prefer javac on the extracted file and treat any build it does run as untrusted code (Agent 7's brief already carried this caveat; the rule extended the capability to nine agents without it). - InlineSmallCode cited as ~1000 (the pre-JDK-11 value); measured 2500 on a live JVM. HugeMethodLimit is a develop flag gated by the product DontCompileHugeMethods, and the boundary is > 8000, not >= 8000. - 'Megamorphic -> no inlining at any size' overstated C2: a dominant receiver (TypeProfileMajorReceiverPercent, 90%) is still inlined behind a guard with an uncommon trap. - pathRulesFor listed every triggering path in the heading of every agent's brief; a 200-file Java PR put ~11 KB of paths there. Capped at ten plus a count, for both rules. - The flat 'performance findings are Suggestions' carried no escape hatch; added the one the workflow rule already needed — unbounded cost on attacker-reachable input is a DoS hole, graded Critical. - Nits: the split fast-path excludes regex metacharacters (split(".") does not take it); test and generated sources are out of scope for the hot-path items; the Java match rows fold into the shared governed-table test so both rules assert through PATH_RULES.matches. Co-authored-by: Qwen-Coder --- .../commands/review/lib/path-rules.test.ts | 80 ++++++++++++++++--- .../cli/src/commands/review/lib/path-rules.ts | 30 +++++-- 2 files changed, 91 insertions(+), 19 deletions(-) diff --git a/packages/cli/src/commands/review/lib/path-rules.test.ts b/packages/cli/src/commands/review/lib/path-rules.test.ts index c0e1e2478c4..c3499dee4d3 100644 --- a/packages/cli/src/commands/review/lib/path-rules.test.ts +++ b/packages/cli/src/commands/review/lib/path-rules.test.ts @@ -45,6 +45,10 @@ describe('pathRulesFor — scoped, or it is noise', () => { ['.github/ISSUE_TEMPLATE/bug.yml', false], ['deploy/workflows/ci.yml', false], ['src/github/workflows/ci.yml', false], + ['src/main/java/com/x/Main.java', true], + ['Main.java', true], + ['src/main/kotlin/Main.kt', false], + ['docs/notes.java.md', false], ])('%s → governed by a rule: %s', (path, governed) => { expect(PATH_RULES.some((r) => r.matches(path))).toBe(governed); }); @@ -112,18 +116,6 @@ describe('pathRulesFor — the Java/JVM rule', () => { expect(out).not.toContain('src/pay.ts'); }); - it.each([ - ['src/main/java/com/x/Main.java', true], - ['Main.java', true], - ['src/main/kotlin/Main.kt', false], - ['src/pay.ts', false], - ['docs/notes.java.md', false], - ])('%s → Java rule applies: %s', (path, applies) => { - expect(pathRulesFor([path]).includes('Java / JVM performance')).toBe( - applies, - ); - }); - it('stacks with the workflow rule when a diff touches both', () => { const out = pathRulesFor(['.github/workflows/ci.yml', 'src/Main.java']); expect(out).toContain('GitHub Actions workflows'); @@ -142,6 +134,19 @@ describe('pathRulesFor — the Java/JVM rule', () => { expect(out).toContain('PrintInlining'); }); + it('cites the thresholds a maintainer will check, correctly', () => { + // A checklist whose thesis is "don't guess the numbers" loses all trust the + // moment it cites a wrong one. These three were wrong in the first draft + // (InlineSmallCode quoted as the pre-JDK-11 value, HugeMethodLimit called a + // product flag with a ≥ boundary, megamorphic stated as unconditional) and a + // review measured them against a live JVM. Pin the corrected forms. + const out = pathRulesFor(['src/Main.java']); + expect(out).toContain('2500 on JDK 11+'); + expect(out).toContain('DontCompileHugeMethods'); + expect(out).toMatch(/> 8000/); + expect(out).toContain('TypeProfileMajorReceiverPercent'); + }); + it('refuses to estimate bytecode from source', () => { // The one failure this checklist exists to prevent: an agent eyeballing a // method and declaring it un-inlinable. The honest form is the mechanism, @@ -174,4 +179,55 @@ describe('pathRulesFor — the Java/JVM rule', () => { // Criticals are reserved for the correctness traps. expect(out).toMatch(/Performance findings are \*\*Suggestions\*\*/); }); + + it('prescribes a measurement that cannot mutate the shared tree', () => { + // The roster runs nine agents in ONE worktree concurrently, and a local + // review stands in the user's own checkout. "Compile the base revision the + // same way" reads as `git checkout`/`git stash` in that tree — corrupting + // files every other agent is reading. The procedure must be non-mutating + // (extract the base side with `git show`, build into a scratch dir), and it + // must say so, because an agent will do the natural thing unless told. + const out = pathRulesFor(['src/Main.java']); + expect(out).toContain('git show'); + expect(out).toMatch(/Never `git checkout`, `git stash`, or build in place/); + // And the build it does run is contributor-controlled, hence untrusted. + expect(out).toContain('untrusted code'); + }); + + it('keeps the DoS escape hatch the workflow rule already needed', () => { + // The flat "perf is a Suggestion" rule misfires on unbounded cost reachable + // by an attacker — that is a security hole, not a nit. GITHUB_ACTIONS walked + // back its own flat rule with a blast-radius carve-out; this one carries the + // matching escape hatch from the start. + const out = pathRulesFor(['src/Main.java']); + expect(out).toContain('cost is itself the wrongness'); + expect(out).toContain('denial-of-service'); + }); + + it('names the split fast-path exception precisely', () => { + // `split(".")` is single-character but "." is a regex metacharacter, so it + // does NOT take the fast path. The parenthetical must say so, or the rule + // teaches an agent to wave away a real per-call compile. + const out = pathRulesFor(['src/Main.java']); + expect(out).toContain('metacharacter'); + }); + + it('caps the triggering-path list in the heading', () => { + // A workflow matches one or two files; a large Java PR matches hundreds, and + // listing them all in the heading of every agent's brief is ~11 KB of a list + // the agent already has. Name the first ten and a count. + const many = Array.from( + { length: 12 }, + (_, i) => `src/main/java/com/x/F${i}.java`, + ); + const out = pathRulesFor(many); + expect(out).toContain('…and 2 more'); + expect(out).toContain('src/main/java/com/x/F9.java'); + expect(out).not.toContain('src/main/java/com/x/F10.java'); + // At or under the cap, every path is still named. + const few = many.slice(0, 10); + const outFew = pathRulesFor(few); + expect(outFew).not.toContain('…and'); + expect(outFew).toContain('src/main/java/com/x/F9.java'); + }); }); diff --git a/packages/cli/src/commands/review/lib/path-rules.ts b/packages/cli/src/commands/review/lib/path-rules.ts index 90a445ccc42..84fd58efd17 100644 --- a/packages/cli/src/commands/review/lib/path-rules.ts +++ b/packages/cli/src/commands/review/lib/path-rules.ts @@ -73,7 +73,7 @@ const JAVA: PathRule = { matches: (p) => /\.java$/i.test(p), checklist: `A Java change's most expensive regressions are decided by the JVM, not by anything a reader can see in the source: HotSpot chooses what to inline, what to scalar-replace, and what to compile at all — and a one-line change can flip each of those on a hot path. The general performance lens (N+1, repeated work, data structures) sees none of it. -**You are reviewing this diff, not auditing this file.** A JVM-cost weakness the code already had, on a line this change does not touch, is out of scope — the same rule as everywhere else. What is in scope: a line this diff **adds or changes**, and a cheap path this diff **makes hot** (a new caller in a request loop, a call moved inside a loop). +**You are reviewing this diff, not auditing this file.** A JVM-cost weakness the code already had, on a line this change does not touch, is out of scope — the same rule as everywhere else. What is in scope: a line this diff **adds or changes**, and a cheap path this diff **makes hot** (a new caller in a request loop, a call moved inside a loop). Test sources (\`src/test/**\`), \`package-info\`/\`module-info\`, and generated sources are out of scope for the hot-path items — nothing there is hot. **Correctness traps dressed as concurrency or performance code (Critical):** @@ -83,7 +83,7 @@ const JAVA: PathRule = { **JVM-cost defects provable from the source (Suggestion — a cost, not a wrongness):** -- **A regex compiled per call.** \`Pattern.compile(...)\` in a method body or loop, and the \`String\` conveniences that hide it — \`matches\`, \`replaceAll\`, \`replaceFirst\`, and \`split\` with a real regex (single-character splits take a fast path) — recompile the pattern on every invocation. On a per-request or per-record path that is real CPU. Fix: \`private static final Pattern\`. +- **A regex compiled per call.** \`Pattern.compile(...)\` in a method body or loop, and the \`String\` conveniences that hide it — \`matches\`, \`replaceAll\`, \`replaceFirst\`, and \`split\` with a real regex (a single-character \`split\` takes a fast path only when the character is not a regex metacharacter — \`split(".")\` does not) — recompile the pattern on every invocation. On a per-request or per-record path that is real CPU. Fix: \`private static final Pattern\`. - **\`+=\` on a \`String\` inside a loop.** Each iteration rebuilds and copies the whole accumulated prefix; the loop is O(n²) in the final length. Fix: one \`StringBuilder\` outside the loop. - **Boxing on a hot path.** A \`Long\`/\`Integer\` loop accumulator, a \`Map\` over dense \`int\` keys, \`.boxed()\` in a counted stream: every box past the small-integer cache is an allocation plus a pointer chase per iteration. Fix: primitives, or the primitive streams/collections. - **A capturing lambda or method reference inside a hot loop.** A non-capturing lambda is a singleton; one that closes over a local allocates per iteration. If the diff moves a capturing lambda into a loop, hoist it or restructure it to capture nothing. @@ -103,25 +103,41 @@ HotSpot inlines by the **bytecode size of the callee**, against these product de | ≤ 35 (\`MaxInlineSize\`) | even when cold | | ≤ 325 (\`FreqInlineSize\`) | only at a hot call site | | > 325 | never | -| ≥ 8000 (\`HugeMethodLimit\`) | never even JIT-compiled | +| > 8000 (\`HugeMethodLimit\`, a develop flag gated by the product \`DontCompileHugeMethods\`) | never even JIT-compiled | -Two more ways a diff can flip it: an already-compiled callee whose **native** size passes \`InlineSmallCode\` (~1000, product) stops being inlined to protect the code cache, and a call site that gains a **third** receiver implementation goes megamorphic — vtable dispatch, no inlining at any size. So the diff-shapes that matter: a small method on a hot path the change **grows** (added validation, logging, a branch), a new layer in a hot call chain, a new implementation registered for an interface invoked in a hot loop. A cold method over 325 bytes is **not** a finding — inlining only matters where the call is hot. +Two more ways a diff can flip it: an already-compiled callee whose **native** size passes \`InlineSmallCode\` (2500 on JDK 11+, 1000 on JDK 8) stops being inlined to protect the code cache, and a call site that gains a **third** receiver implementation goes megamorphic — vtable dispatch, and no inlining **unless one receiver still dominates the profile** (\`TypeProfileMajorReceiverPercent\`, 90% by default), in which case C2 inlines that receiver behind a guard with an uncommon trap for the rest. So the diff-shapes that matter: a small method on a hot path the change **grows** (added validation, logging, a branch), a new layer in a hot call chain, a new implementation registered for an interface invoked in a hot loop. A cold method over 325 bytes is **not** a finding — inlining only matters where the call is hot. **Measure it; do not estimate bytecode from source.** Two tiers, in order of cost: -1. **Static — available whenever the project builds.** Compile (\`mvn -q -DskipTests compile\`, \`gradle classes\`, or \`javac\` the module), then \`javap -c -p \` and read the method's size: the offset of its last instruction plus that instruction's width. Compare against 35 and 325. For the **crossing** claim — "this diff pushed it over the threshold" — compile the base revision the same way and measure the same method on both sides; a method that was already over is pre-existing, not a finding. +1. **Static — available whenever the project builds.** Extract the one class and \`javac\` it into a **scratch** output dir (\`javac -d /tmp/ …\`), then \`javap -c -p\` the class and read the method's size: the offset of its last instruction plus that instruction's width. Compare against 35 and 325. For the **crossing** claim — "this diff pushed it over the threshold" — get the base side **without touching the tree**: \`git show : > /tmp//X.java\`, compile that too, and measure both; a method already over on the base side is pre-existing, not a finding. **Never \`git checkout\`, \`git stash\`, or build in place.** In a PR review the worktree is shared with agents running concurrently, and in a local review it is the user's own checkout — mutating it corrupts work you cannot see. A full \`mvn\`/\`gradle\` build also executes the branch's contributor-controlled build logic, so prefer \`javac\` on the extracted file over a project build, and treat any build you do run as untrusted code. 2. **Dynamic — when the code is runnable.** Run the workload with \`-XX:+UnlockDiagnosticVMOptions -XX:+PrintInlining\` and grep for the method: \`callee is too large\` / \`hot method too big\` is the JVM itself declining to inline. JMH gives the before/after throughput; JITWatch visualizes the same decision logs. A finding that a method "can no longer be inlined" without one of these two tiers is a guess stated as fact. Report the **mechanism** instead, at \`Confidence: low\`: the threshold at risk, what the diff added to the method, and the measurement still to run. And if the diff *claims* this path got faster while the mechanism above says it cannot have, the finding is the unsubstantiated claim. **When the finding IS a grown hot method, the fix to suggest is hot/cold splitting — not reverting the change, and not \`@ForceInline\`** (which copies the callee's full body into every caller, bloating their bytecode and their own inline budgets). Move the cold paths — error handling, rare branches, defensive validation, the \`switch\` arms that almost never fire — into a small private helper, leaving the common path under the threshold; the helper, now called from one site, is itself inlinable. Name the bytecode range to extract and the size it leaves behind, the way the measurement names them: "extract bytecodes 212–311 (~100 bytes) into \`applyRounding\`, leaving \`parseAmount\` at ~238 bytes" is a finding an author can act on; "consider splitting the method" is not. -**Favour precision over recall here.** A guessed JVM finding is the easiest kind for an author to dismiss, and one dismissal teaches them to skip the rest of the review. Every finding needs the concrete hot path and the concrete cost, like any other. Performance findings are **Suggestions**: slow is a cost, not incorrect behaviour — the Critical entries above are Critical because they are *wrong*.`, +**Favour precision over recall here.** A guessed JVM finding is the easiest kind for an author to dismiss, and one dismissal teaches them to skip the rest of the review. Every finding needs the concrete hot path and the concrete cost, like any other. Performance findings are **Suggestions** — slow is a cost, not incorrect behaviour — **except where the cost is itself the wrongness**: unbounded allocation, quadratic work, or unbounded cache growth on attacker-reachable input is a denial-of-service hole, which the severity ladder grades Critical, not a Suggestion. Name the reachable input that triggers it; "this loop is slow" with no attacker-reachable trigger stays a Suggestion.`, }; /** Every rule, in the order their checklists are appended. */ export const PATH_RULES: PathRule[] = [GITHUB_ACTIONS, JAVA]; +/** + * The triggering paths named in a rule's heading — capped. A workflow rule + * matches one or two files; a Java rule matches every source file in a large + * PR, and listing hundreds of paths in the heading of every agent's brief is + * ~11 KB of a list the agent already has from its file table. Name the first + * few and a count; the checklist, not the path list, is what the heading is + * for. + */ +function describePaths(which: readonly string[]): string { + const CAP = 10; + if (which.length <= CAP) { + return which.join(', '); + } + return `${which.slice(0, CAP).join(', ')}, …and ${which.length - CAP} more`; +} + /** * The checklists that govern `paths`, as a brief section — or `''` when none do. * @@ -135,7 +151,7 @@ export function pathRulesFor(paths: readonly string[]): string { const parts = ['## Rules for the files in front of you', '']; for (const r of hit) { const which = paths.filter((p) => r.matches(p)); - parts.push(`### ${r.title} — ${which.join(', ')}`, '', r.checklist, ''); + parts.push(`### ${r.title} — ${describePaths(which)}`, '', r.checklist, ''); } return parts.join('\n').trimEnd(); } From cd9434732eaf20e5ee1e3b39b9af0513f21fbbd5 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Sun, 2 Aug 2026 22:04:06 +0800 Subject: [PATCH 4/9] fix(cli): second-round review on the Java/JVM path rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six findings plus nits and three inline coverage probes, all verified: - The 'safe' javac tier still executed contributor code: annotation processors on the classpath run at compile time. Add -proc:none (not optional), and make mvn/gradle a prohibition rather than a discouraged preference — on a stranger's PR branch the build logic is the attack surface. - /tmp/ was a placeholder agents converge on; two compiling different revisions of one class into the same dir measure each other's bytecode. Prescribe SCRATCH=$(mktemp -d), with a %TEMP% note for Windows. - 'Extract and javac' fails on any class with imports. Name the classpath path (-sourcepath at the module root, an existing target/classes, or mvn dependency:build-classpath which resolves without building) and a graceful fall-back to the mechanism tier instead of escalating to a project build. - @ForceInline was ruled out for bloating callers — true but secondary, and the annotation is JDK-internal, not general. Lead with the anti-pattern that actually bites app code (reaching for -XX:FreqInlineSize / -XX:CompileCommand=inline, runtime knobs a PR cannot ship) and note @ForceInline only as unavailable. - describePaths capped in diff order, so a test-heavy PR could name ten test files the rule scopes out and no production path. Stable-partition production first. - Header: a rule earns its place by naming an invisible defect AND pays a per-agent token cost; say so, before rule #3 arrives. - Nits: HashMap.newHashMap(n) (JDK 19+) for the presize arithmetic; the split fast path also covers the escaped two-char form. - Coverage probes (inline): the correctness-traps block and the nine Suggestion patterns had zero test coverage — deletion left all tests green. Pin the load-bearing strings of both, plus the new tier flags and the production-first ordering. --- .../commands/review/lib/path-rules.test.ts | 80 ++++++++++++++++--- .../cli/src/commands/review/lib/path-rules.ts | 24 ++++-- 2 files changed, 87 insertions(+), 17 deletions(-) diff --git a/packages/cli/src/commands/review/lib/path-rules.test.ts b/packages/cli/src/commands/review/lib/path-rules.test.ts index c3499dee4d3..ca7eaf6c2d3 100644 --- a/packages/cli/src/commands/review/lib/path-rules.test.ts +++ b/packages/cli/src/commands/review/lib/path-rules.test.ts @@ -180,18 +180,80 @@ describe('pathRulesFor — the Java/JVM rule', () => { expect(out).toMatch(/Performance findings are \*\*Suggestions\*\*/); }); - it('prescribes a measurement that cannot mutate the shared tree', () => { + it('prescribes a measurement that cannot mutate the tree or run contributor code', () => { // The roster runs nine agents in ONE worktree concurrently, and a local - // review stands in the user's own checkout. "Compile the base revision the - // same way" reads as `git checkout`/`git stash` in that tree — corrupting - // files every other agent is reading. The procedure must be non-mutating - // (extract the base side with `git show`, build into a scratch dir), and it - // must say so, because an agent will do the natural thing unless told. + // review stands in the user's own checkout. Three distinct hazards the + // procedure must close, each found by review: + // - a fixed scratch path (/tmp/scratch) collides between concurrent agents + // compiling different revisions of the same class → mktemp -d; + // - plain javac runs classpath annotation processors with the agent's + // privileges → -proc:none, and mvn/gradle (the branch's build logic) is a + // prohibition, not a discouraged preference; + // - "extract and javac" fails on any class with imports, so the tier names + // a classpath path (-sourcepath / target/classes / dependency:build-classpath, + // which resolves without building) and a graceful fall-back to the + // mechanism tier instead of escalating to a project build. const out = pathRulesFor(['src/Main.java']); + expect(out).toContain('mktemp -d'); + expect(out).toContain('-proc:none'); + expect(out).toContain('dependency:build-classpath'); expect(out).toContain('git show'); - expect(out).toMatch(/Never `git checkout`, `git stash`, or build in place/); - // And the build it does run is contributor-controlled, hence untrusted. - expect(out).toContain('untrusted code'); + expect(out).toMatch( + /never `git checkout`, `git stash`, build in place, or run `mvn`\/`gradle`/, + ); + }); + + it('pins the correctness traps that make this section Critical, not Suggestion', () => { + // Probe-confirmed in review: deleting the entire correctness-traps block left + // every test green, so a future edit could silently drop the only instruction + // that grades a shared SimpleDateFormat or a get-then-put race as *wrong*. + const out = pathRulesFor(['src/Main.java']); + expect(out).toContain('SimpleDateFormat'); + expect(out).toContain('ConcurrentHashMap'); + expect(out).toContain('computeIfAbsent'); + expect(out).toContain('volatile'); + }); + + it('pins the JVM-cost defect patterns an agent would otherwise skim past', () => { + // Same probe, Suggestion side: the nine source-provable patterns (regex, + // string +=, boxing, capturing lambda, log guard, presizing, legacy + // synchronized types, exceptions as control flow, per-call reflection) had + // zero coverage. Spot-pin the load-bearing ones so a mangled section fails. + const out = pathRulesFor(['src/Main.java']); + expect(out).toContain('Pattern.compile'); + expect(out).toContain('StringBuilder'); + expect(out).toContain('Boxing on a hot path'); + expect(out).toContain('newHashMap'); + }); + + it('steers the fix away from JVM tuning flags and internal annotations', () => { + // For a grown hot method the actionable fix is hot/cold splitting. The wrong + // suggestions an agent reaches for are runtime knobs the PR author cannot + // ship in a code change (-XX:FreqInlineSize, -XX:CompileCommand=inline) and + // the JDK-internal @ForceInline, which application code cannot use at all — + // none of these is general, so the checklist names them only to rule them out. + const out = pathRulesFor(['src/Main.java']); + expect(out).toContain('hot/cold splitting'); + expect(out).toContain('CompileCommand=inline'); + expect(out).toMatch(/runtime knobs the PR's author cannot ship/); + expect(out).toContain('@ForceInline` is not available to application code'); + }); + + it('names production paths before test paths in the heading', () => { + // The hot-path items the heading introduces do not apply under src/test, so a + // PR that is mostly test classes must not fill the ten named slots with files + // the rule scopes out. Production first, then tests. + const tests = Array.from( + { length: 30 }, + (_, i) => `src/test/java/com/x/T${i}Test.java`, + ); + const prod = ['src/main/java/com/x/A.java', 'src/main/java/com/x/B.java']; + const out = pathRulesFor([...tests, ...prod]); + expect(out).toContain('src/main/java/com/x/A.java'); + expect(out).toContain('src/main/java/com/x/B.java'); + // The first named slot is production, not a test file. + const heading = out.split('\n').find((l) => l.startsWith('### ')) ?? ''; + expect(heading.indexOf('A.java')).toBeLessThan(heading.indexOf('T0Test')); }); it('keeps the DoS escape hatch the workflow rule already needed', () => { diff --git a/packages/cli/src/commands/review/lib/path-rules.ts b/packages/cli/src/commands/review/lib/path-rules.ts index 84fd58efd17..b443c36cfa2 100644 --- a/packages/cli/src/commands/review/lib/path-rules.ts +++ b/packages/cli/src/commands/review/lib/path-rules.ts @@ -29,7 +29,9 @@ // its shape from ships opinions about `var`, `==`, and nested ternaries — which // collide head-on with this skill's Exclusion Criteria against formatter-fixable // nits, and would spend a reviewer's attention on the things a linter already owns. -// A path rule earns its place by naming a defect the dimensions cannot see. +// A path rule earns its place by naming a defect the dimensions cannot see — and it +// pays a cost: the checklist rides in the brief of every matching agent, so a rule +// that does not earn its tokens on the median matching diff is not a rule. export interface PathRule { /** Named in the brief, so an agent can say which rule it applied. */ @@ -83,12 +85,12 @@ const JAVA: PathRule = { **JVM-cost defects provable from the source (Suggestion — a cost, not a wrongness):** -- **A regex compiled per call.** \`Pattern.compile(...)\` in a method body or loop, and the \`String\` conveniences that hide it — \`matches\`, \`replaceAll\`, \`replaceFirst\`, and \`split\` with a real regex (a single-character \`split\` takes a fast path only when the character is not a regex metacharacter — \`split(".")\` does not) — recompile the pattern on every invocation. On a per-request or per-record path that is real CPU. Fix: \`private static final Pattern\`. +- **A regex compiled per call.** \`Pattern.compile(...)\` in a method body or loop, and the \`String\` conveniences that hide it — \`matches\`, \`replaceAll\`, \`replaceFirst\`, and \`split\` with a real regex (a one- or two-character \`split\` argument takes a fast path only when it is not a regex construct — \`split(".")\` does not, since \`.\` is a metacharacter; the escaped two-character form \`split("\\\\.")\` does) — recompile the pattern on every invocation. On a per-request or per-record path that is real CPU. Fix: \`private static final Pattern\`. - **\`+=\` on a \`String\` inside a loop.** Each iteration rebuilds and copies the whole accumulated prefix; the loop is O(n²) in the final length. Fix: one \`StringBuilder\` outside the loop. - **Boxing on a hot path.** A \`Long\`/\`Integer\` loop accumulator, a \`Map\` over dense \`int\` keys, \`.boxed()\` in a counted stream: every box past the small-integer cache is an allocation plus a pointer chase per iteration. Fix: primitives, or the primitive streams/collections. - **A capturing lambda or method reference inside a hot loop.** A non-capturing lambda is a singleton; one that closes over a local allocates per iteration. If the diff moves a capturing lambda into a loop, hoist it or restructure it to capture nothing. - **A log message built whether or not it is logged.** \`log.debug("result: " + value)\` pays the concatenation with the level off, and even \`log.debug("{}", expensive())\` still pays \`expensive()\`. Fix: parameterized messages, and guard expensive arguments with \`isDebugEnabled()\` or a supplier-based logging API. -- **A collection sized after the fact.** A \`HashMap\`/\`ArrayList\` the code then fills with a known N pays repeated rehash/grow-copy chains; presize it (a \`HashMap\` needs \`N/0.75 + 1\` to avoid rehashing at all). +- **A collection sized after the fact.** A \`HashMap\`/\`ArrayList\` the code then fills with a known N pays repeated rehash/grow-copy chains; presize it (a \`HashMap\` needs \`N/0.75 + 1\` to avoid rehashing at all — \`HashMap.newHashMap(n)\`, JDK 19+, does that arithmetic for you). - **Legacy synchronized types in new code.** \`Vector\`, \`Hashtable\`, \`StringBuffer\` put a monitor on every call; the unsynchronized equivalents are the default for a reason. - **An exception used as control flow.** Validating by catching \`NumberFormatException\`, looping until \`EOFException\`: \`fillInStackTrace\` costs more than the work being guarded. Validate instead. - **Reflection rediscovered per call.** \`getMethod\`/\`getField\` on every invocation of a hot path; cache the \`Method\`/\`Field\` — or a \`MethodHandle\` — once. @@ -109,12 +111,12 @@ Two more ways a diff can flip it: an already-compiled callee whose **native** si **Measure it; do not estimate bytecode from source.** Two tiers, in order of cost: -1. **Static — available whenever the project builds.** Extract the one class and \`javac\` it into a **scratch** output dir (\`javac -d /tmp/ …\`), then \`javap -c -p\` the class and read the method's size: the offset of its last instruction plus that instruction's width. Compare against 35 and 325. For the **crossing** claim — "this diff pushed it over the threshold" — get the base side **without touching the tree**: \`git show : > /tmp//X.java\`, compile that too, and measure both; a method already over on the base side is pre-existing, not a finding. **Never \`git checkout\`, \`git stash\`, or build in place.** In a PR review the worktree is shared with agents running concurrently, and in a local review it is the user's own checkout — mutating it corrupts work you cannot see. A full \`mvn\`/\`gradle\` build also executes the branch's contributor-controlled build logic, so prefer \`javac\` on the extracted file over a project build, and treat any build you do run as untrusted code. +1. **Static — available when the class compiles, which for a leaf class is always and for a connected one needs its dependencies.** Allocate a private scratch dir first — \`SCRATCH=$(mktemp -d)\`, never a fixed path like \`/tmp/scratch\`: other agents are compiling concurrently, and two of them writing different revisions of the same class into one directory read each other's \`.class\` files and measure the wrong bytecode (on Windows, \`%TEMP%\` plays the role of \`/tmp\`). Then \`javac -proc:none -nowarn -d "$SCRATCH" X.java\` — \`-proc:none\` is not optional: annotation processors on the classpath (Lombok, Dagger, one the PR itself added) run at compile time with your privileges, so a \`javac\` without it is itself an untrusted-code execution. Give the compiler what it needs to resolve imports: \`-sourcepath\` at the module source root, plus \`-cp\` against an already-built \`target/classes\` / \`build/classes\` if one exists; if neither is present, \`mvn dependency:build-classpath -Dmdep.outputFile="$SCRATCH/cp.txt"\` writes the dependency classpath **without compiling or running the project**, then \`javac -cp @"$SCRATCH/cp.txt"\`. Then \`javap -c -p\` the class and read the method's size: the offset of its last instruction plus that instruction's width. Compare against 35 and 325. For the **crossing** claim — "this diff pushed it over the threshold" — get the base side **without touching the tree**: \`git show : > "$SCRATCH/X.java"\`, compile that too, and measure both; a method already over on the base side is pre-existing, not a finding. If the class will not compile even with its dependencies, go straight to the mechanism-at-\`Confidence: low\` form below — **never \`git checkout\`, \`git stash\`, build in place, or run \`mvn\`/\`gradle\` to force a compile.** In a PR review the worktree is shared with agents running concurrently and the branch's build logic is contributor-controlled — the attack surface, not a cost; in a local review it is the user's own checkout. Mutating either corrupts work you cannot see. 2. **Dynamic — when the code is runnable.** Run the workload with \`-XX:+UnlockDiagnosticVMOptions -XX:+PrintInlining\` and grep for the method: \`callee is too large\` / \`hot method too big\` is the JVM itself declining to inline. JMH gives the before/after throughput; JITWatch visualizes the same decision logs. A finding that a method "can no longer be inlined" without one of these two tiers is a guess stated as fact. Report the **mechanism** instead, at \`Confidence: low\`: the threshold at risk, what the diff added to the method, and the measurement still to run. And if the diff *claims* this path got faster while the mechanism above says it cannot have, the finding is the unsubstantiated claim. -**When the finding IS a grown hot method, the fix to suggest is hot/cold splitting — not reverting the change, and not \`@ForceInline\`** (which copies the callee's full body into every caller, bloating their bytecode and their own inline budgets). Move the cold paths — error handling, rare branches, defensive validation, the \`switch\` arms that almost never fire — into a small private helper, leaving the common path under the threshold; the helper, now called from one site, is itself inlinable. Name the bytecode range to extract and the size it leaves behind, the way the measurement names them: "extract bytecodes 212–311 (~100 bytes) into \`applyRounding\`, leaving \`parseAmount\` at ~238 bytes" is a finding an author can act on; "consider splitting the method" is not. +**When the finding IS a grown hot method, the fix to suggest is hot/cold splitting — not reverting the change, and not a JVM tuning flag** (\`-XX:FreqInlineSize\`, \`-XX:CompileCommand=inline\`): those are runtime knobs the PR's author cannot ship in a code change, and raising an inline threshold to fit one method pays for it at every other call site. (The JDK-internal \`@ForceInline\` is not available to application code at all.) Move the cold paths — error handling, rare branches, defensive validation, the \`switch\` arms that almost never fire — into a small private helper, leaving the common path under the threshold; the helper, now called from one site, is itself inlinable. Name the bytecode range to extract and the size it leaves behind, the way the measurement names them: "extract bytecodes 212–311 (~100 bytes) into \`applyRounding\`, leaving \`parseAmount\` at ~238 bytes" is a finding an author can act on; "consider splitting the method" is not. **Favour precision over recall here.** A guessed JVM finding is the easiest kind for an author to dismiss, and one dismissal teaches them to skip the rest of the review. Every finding needs the concrete hot path and the concrete cost, like any other. Performance findings are **Suggestions** — slow is a cost, not incorrect behaviour — **except where the cost is itself the wrongness**: unbounded allocation, quadratic work, or unbounded cache growth on attacker-reachable input is a denial-of-service hole, which the severity ladder grades Critical, not a Suggestion. Name the reachable input that triggers it; "this loop is slow" with no attacker-reachable trigger stays a Suggestion.`, }; @@ -132,10 +134,16 @@ export const PATH_RULES: PathRule[] = [GITHUB_ACTIONS, JAVA]; */ function describePaths(which: readonly string[]): string { const CAP = 10; - if (which.length <= CAP) { - return which.join(', '); + // Production paths before test paths: the hot-path items this heading exists to + // introduce do not apply under src/test, so a PR that is mostly test classes + // must not fill the heading with files the rule explicitly scopes out. + const prod = which.filter((p) => !/src\/test\//i.test(p)); + const test = which.filter((p) => /src\/test\//i.test(p)); + const ordered = [...prod, ...test]; + if (ordered.length <= CAP) { + return ordered.join(', '); } - return `${which.slice(0, CAP).join(', ')}, …and ${which.length - CAP} more`; + return `${ordered.slice(0, CAP).join(', ')}, …and ${ordered.length - CAP} more`; } /** From 90a11f24b6c29573b86842096808be33df32a317 Mon Sep 17 00:00:00 2001 From: qwen-code-ci-bot Date: Sun, 2 Aug 2026 15:08:30 +0000 Subject: [PATCH 5/9] fix(cli): third-round review on the Java/JVM path rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Drop the mvn dependency:build-classpath recommendation — Maven extensions execute during any invocation, a strictly larger execution surface than the annotation processors -proc:none exists to close. Fall through to the mechanism tier when no pre-built classpath exists. Add --release at the project's target level: the same source compiles to different bytecode at different levels (61 vs 16 bytes for a five-+ concatenation), so measuring without it produces a threshold verdict on bytecode the shipped artifact does not contain. Name -proc:none as a fidelity hazard: on a Lombok/Dagger project the compiled class is missing generated members, so the static tier is void. Add clauses for new files (no base side to compare), the base-side target/classes caveat, and the Windows uniqueness primitive. Fix the vacuous ordering assertion (indexOf returns -1 outside the cap, and -1 < n passes). Add a GHA cap test and a --release/new-file test. --- .../commands/review/lib/path-rules.test.ts | 41 ++++++++++++++++--- .../cli/src/commands/review/lib/path-rules.ts | 2 +- 2 files changed, 36 insertions(+), 7 deletions(-) diff --git a/packages/cli/src/commands/review/lib/path-rules.test.ts b/packages/cli/src/commands/review/lib/path-rules.test.ts index ca7eaf6c2d3..f16ebb65504 100644 --- a/packages/cli/src/commands/review/lib/path-rules.test.ts +++ b/packages/cli/src/commands/review/lib/path-rules.test.ts @@ -106,6 +106,19 @@ describe('pathRulesFor — scoped, or it is noise', () => { // And a miss nobody can observe is part of the finding, not a separate nit. expect(out).toContain('$GITHUB_STEP_SUMMARY'); }); + + it('caps the path list for workflow files too', () => { + // The cap is rule-agnostic: a diff touching 15 workflows gets the same + // truncation as a large Java PR. + const many = Array.from( + { length: 15 }, + (_, i) => `.github/workflows/ci${i}.yml`, + ); + const out = pathRulesFor(many); + expect(out).toContain('…and 5 more'); + expect(out).toContain('.github/workflows/ci9.yml'); + expect(out).not.toContain('.github/workflows/ci10.yml'); + }); }); describe('pathRulesFor — the Java/JVM rule', () => { @@ -182,21 +195,22 @@ describe('pathRulesFor — the Java/JVM rule', () => { it('prescribes a measurement that cannot mutate the tree or run contributor code', () => { // The roster runs nine agents in ONE worktree concurrently, and a local - // review stands in the user's own checkout. Three distinct hazards the + // review stands in the user's own checkout. Four distinct hazards the // procedure must close, each found by review: // - a fixed scratch path (/tmp/scratch) collides between concurrent agents // compiling different revisions of the same class → mktemp -d; // - plain javac runs classpath annotation processors with the agent's // privileges → -proc:none, and mvn/gradle (the branch's build logic) is a // prohibition, not a discouraged preference; + // - -proc:none is also a fidelity hazard: on a Lombok/Dagger project the + // compiled class is missing generated members, so the static tier is void; // - "extract and javac" fails on any class with imports, so the tier names - // a classpath path (-sourcepath / target/classes / dependency:build-classpath, - // which resolves without building) and a graceful fall-back to the - // mechanism tier instead of escalating to a project build. + // a classpath path (-sourcepath / target/classes) and a graceful fall-back + // to the mechanism tier instead of escalating to a project build. const out = pathRulesFor(['src/Main.java']); expect(out).toContain('mktemp -d'); expect(out).toContain('-proc:none'); - expect(out).toContain('dependency:build-classpath'); + expect(out).toContain('static tier is **void**'); expect(out).toContain('git show'); expect(out).toMatch( /never `git checkout`, `git stash`, build in place, or run `mvn`\/`gradle`/, @@ -253,7 +267,11 @@ describe('pathRulesFor — the Java/JVM rule', () => { expect(out).toContain('src/main/java/com/x/B.java'); // The first named slot is production, not a test file. const heading = out.split('\n').find((l) => l.startsWith('### ')) ?? ''; - expect(heading.indexOf('A.java')).toBeLessThan(heading.indexOf('T0Test')); + const prodIdx = heading.indexOf('A.java'); + const testIdx = heading.indexOf('T0Test'); + expect(prodIdx).toBeGreaterThanOrEqual(0); + expect(testIdx).toBeGreaterThanOrEqual(0); + expect(prodIdx).toBeLessThan(testIdx); }); it('keeps the DoS escape hatch the workflow rule already needed', () => { @@ -292,4 +310,15 @@ describe('pathRulesFor — the Java/JVM rule', () => { expect(outFew).not.toContain('…and'); expect(outFew).toContain('src/main/java/com/x/F9.java'); }); + + it('names --release and the new-file clause in the static tier', () => { + // The same source compiles to different bytecode at different --release + // levels (61 vs 16 bytes for a five-+ concatenation), so measuring without + // the project's target level produces a verdict on bytecode the artifact + // does not contain. And a file the PR adds has no base side to compare. + const out = pathRulesFor(['src/Main.java']); + expect(out).toContain('--release'); + expect(out).toContain('maven.compiler.release'); + expect(out).toContain('no base side'); + }); }); diff --git a/packages/cli/src/commands/review/lib/path-rules.ts b/packages/cli/src/commands/review/lib/path-rules.ts index b443c36cfa2..16c524d270f 100644 --- a/packages/cli/src/commands/review/lib/path-rules.ts +++ b/packages/cli/src/commands/review/lib/path-rules.ts @@ -111,7 +111,7 @@ Two more ways a diff can flip it: an already-compiled callee whose **native** si **Measure it; do not estimate bytecode from source.** Two tiers, in order of cost: -1. **Static — available when the class compiles, which for a leaf class is always and for a connected one needs its dependencies.** Allocate a private scratch dir first — \`SCRATCH=$(mktemp -d)\`, never a fixed path like \`/tmp/scratch\`: other agents are compiling concurrently, and two of them writing different revisions of the same class into one directory read each other's \`.class\` files and measure the wrong bytecode (on Windows, \`%TEMP%\` plays the role of \`/tmp\`). Then \`javac -proc:none -nowarn -d "$SCRATCH" X.java\` — \`-proc:none\` is not optional: annotation processors on the classpath (Lombok, Dagger, one the PR itself added) run at compile time with your privileges, so a \`javac\` without it is itself an untrusted-code execution. Give the compiler what it needs to resolve imports: \`-sourcepath\` at the module source root, plus \`-cp\` against an already-built \`target/classes\` / \`build/classes\` if one exists; if neither is present, \`mvn dependency:build-classpath -Dmdep.outputFile="$SCRATCH/cp.txt"\` writes the dependency classpath **without compiling or running the project**, then \`javac -cp @"$SCRATCH/cp.txt"\`. Then \`javap -c -p\` the class and read the method's size: the offset of its last instruction plus that instruction's width. Compare against 35 and 325. For the **crossing** claim — "this diff pushed it over the threshold" — get the base side **without touching the tree**: \`git show : > "$SCRATCH/X.java"\`, compile that too, and measure both; a method already over on the base side is pre-existing, not a finding. If the class will not compile even with its dependencies, go straight to the mechanism-at-\`Confidence: low\` form below — **never \`git checkout\`, \`git stash\`, build in place, or run \`mvn\`/\`gradle\` to force a compile.** In a PR review the worktree is shared with agents running concurrently and the branch's build logic is contributor-controlled — the attack surface, not a cost; in a local review it is the user's own checkout. Mutating either corrupts work you cannot see. +1. **Static — available when the class compiles, which for a leaf class is always and for a connected one needs its dependencies.** Allocate a private scratch dir first — \`SCRATCH=$(mktemp -d)\`, never a fixed path like \`/tmp/scratch\`: other agents are compiling concurrently, and two of them writing different revisions of the same class into one directory read each other's \`.class\` files and measure the wrong bytecode (on Windows, \`mkdir %TEMP%\\review-%RANDOM%\` gives the same uniqueness). Then \`javac -proc:none -nowarn -d "$SCRATCH" X.java\` — \`-proc:none\` is not optional: annotation processors on the classpath (Lombok, Dagger, one the PR itself added) run at compile time with your privileges, so a \`javac\` without it is itself an untrusted-code execution. It is also a fidelity flag: on a project whose processor contributes code to the measured class (Lombok's generated members, Dagger's injected fields), the compiled class is missing those members and \`javap\` reports a size that is simply wrong — if the project runs a processor that touches this class, the static tier is **void**; go to \`Confidence: low\` with the mechanism. Give the compiler what it needs to resolve imports: \`-sourcepath\` at the module source root, plus \`-cp\` against an already-built \`target/classes\` / \`build/classes\` if one exists. Pass \`--release \` at the project's target level (read it from \`maven.compiler.release\`, \`\`, or Gradle's \`release\`/\`targetCompatibility\`): the same source compiles to different bytecode at different levels — a five-\`+\` concatenation is 61 bytes at release 8 and 16 at 11+ — and measuring the wrong level produces a threshold verdict on bytecode the shipped artifact does not contain; if the target level cannot be determined, say so in the finding. Then \`javap -c -p\` the class and read the method's size: the offset of its last instruction plus that instruction's width. Compare against 35 and 325. For the **crossing** claim — "this diff pushed it over the threshold" — get the base side **without touching the tree**: \`git show : > "$SCRATCH/X.java"\`, compile that too, and measure both; a method already over on the base side is pre-existing, not a finding. A file the PR **adds** has no base side — \`git show\` fails, and there is no crossing to claim; report the size without a before/after. The base side compiles against the same \`target/classes\` the head side used; on a PR that changes more than the measured file the conditions are not perfectly equivalent — note that caveat when it applies. If the class will not compile (no pre-built classpath, no source path that resolves its imports), go straight to the mechanism-at-\`Confidence: low\` form below — **never \`git checkout\`, \`git stash\`, build in place, or run \`mvn\`/\`gradle\` to force a compile.** In a PR review the worktree is shared with agents running concurrently and the branch's build logic is contributor-controlled — the attack surface, not a cost; in a local review it is the user's own checkout. Mutating either corrupts work you cannot see. 2. **Dynamic — when the code is runnable.** Run the workload with \`-XX:+UnlockDiagnosticVMOptions -XX:+PrintInlining\` and grep for the method: \`callee is too large\` / \`hot method too big\` is the JVM itself declining to inline. JMH gives the before/after throughput; JITWatch visualizes the same decision logs. A finding that a method "can no longer be inlined" without one of these two tiers is a guess stated as fact. Report the **mechanism** instead, at \`Confidence: low\`: the threshold at risk, what the diff added to the method, and the measurement still to run. And if the diff *claims* this path got faster while the mechanism above says it cannot have, the finding is the unsubstantiated claim. From 1a2692e0f0e0ca273d45e3d6ecdbd56ff774c5b9 Mon Sep 17 00:00:00 2001 From: qwen-code-ci-bot Date: Sun, 2 Aug 2026 16:10:15 +0000 Subject: [PATCH 6/9] fix(cli): fourth-round review on the Java/JVM path rule --- .../commands/review/lib/path-rules.test.ts | 31 +++++++++++++++++-- .../cli/src/commands/review/lib/path-rules.ts | 25 ++++++++++----- 2 files changed, 46 insertions(+), 10 deletions(-) diff --git a/packages/cli/src/commands/review/lib/path-rules.test.ts b/packages/cli/src/commands/review/lib/path-rules.test.ts index f16ebb65504..fb0c9fbb554 100644 --- a/packages/cli/src/commands/review/lib/path-rules.test.ts +++ b/packages/cli/src/commands/review/lib/path-rules.test.ts @@ -154,7 +154,8 @@ describe('pathRulesFor — the Java/JVM rule', () => { // product flag with a ≥ boundary, megamorphic stated as unconditional) and a // review measured them against a live JVM. Pin the corrected forms. const out = pathRulesFor(['src/Main.java']); - expect(out).toContain('2500 on JDK 11+'); + expect(out).toContain('2500 on JDK 17+'); + expect(out).toContain('2000 on JDK 8–11'); expect(out).toContain('DontCompileHugeMethods'); expect(out).toMatch(/> 8000/); expect(out).toContain('TypeProfileMajorReceiverPercent'); @@ -274,6 +275,32 @@ describe('pathRulesFor — the Java/JVM rule', () => { expect(prodIdx).toBeLessThan(testIdx); }); + it('deprioritizes generated, info-only, and non-Maven test sources too', () => { + // The checklist scopes out more than src/test: generated sources, + // package-info/module-info, and non-Maven test roots (integTest, + // androidTest, testFixtures) must not fill the named slots either. + const noise = [ + 'target/generated-sources/com/x/Stub.java', + 'build/generated/com/x/R.java', + 'src/main/java/com/x/generated/Proto.java', + 'src/main/java/com/x/package-info.java', + 'src/main/java/module-info.java', + 'src/integTest/java/com/x/IT.java', + 'src/androidTest/java/com/x/AT.java', + 'src/testFixtures/java/com/x/Fix.java', + ]; + const prod = ['src/main/java/com/x/Hot.java']; + const out = pathRulesFor([...noise, ...prod]); + const heading = out.split('\n').find((l) => l.startsWith('### ')) ?? ''; + // The single production path must be named first. + expect(heading.indexOf('Hot.java')).toBeGreaterThanOrEqual(0); + expect(heading.indexOf('Hot.java')).toBeLessThan( + heading.indexOf('Stub.java') === -1 + ? Infinity + : heading.indexOf('Stub.java'), + ); + }); + it('keeps the DoS escape hatch the workflow rule already needed', () => { // The flat "perf is a Suggestion" rule misfires on unbounded cost reachable // by an attacker — that is a security hole, not a nit. GITHUB_ACTIONS walked @@ -313,7 +340,7 @@ describe('pathRulesFor — the Java/JVM rule', () => { it('names --release and the new-file clause in the static tier', () => { // The same source compiles to different bytecode at different --release - // levels (61 vs 16 bytes for a five-+ concatenation), so measuring without + // levels (37 vs 14 bytes for a five-+ concatenation), so measuring without // the project's target level produces a verdict on bytecode the artifact // does not contain. And a file the PR adds has no base side to compare. const out = pathRulesFor(['src/Main.java']); diff --git a/packages/cli/src/commands/review/lib/path-rules.ts b/packages/cli/src/commands/review/lib/path-rules.ts index 16c524d270f..5f2e02147c9 100644 --- a/packages/cli/src/commands/review/lib/path-rules.ts +++ b/packages/cli/src/commands/review/lib/path-rules.ts @@ -107,11 +107,11 @@ HotSpot inlines by the **bytecode size of the callee**, against these product de | > 325 | never | | > 8000 (\`HugeMethodLimit\`, a develop flag gated by the product \`DontCompileHugeMethods\`) | never even JIT-compiled | -Two more ways a diff can flip it: an already-compiled callee whose **native** size passes \`InlineSmallCode\` (2500 on JDK 11+, 1000 on JDK 8) stops being inlined to protect the code cache, and a call site that gains a **third** receiver implementation goes megamorphic — vtable dispatch, and no inlining **unless one receiver still dominates the profile** (\`TypeProfileMajorReceiverPercent\`, 90% by default), in which case C2 inlines that receiver behind a guard with an uncommon trap for the rest. So the diff-shapes that matter: a small method on a hot path the change **grows** (added validation, logging, a branch), a new layer in a hot call chain, a new implementation registered for an interface invoked in a hot loop. A cold method over 325 bytes is **not** a finding — inlining only matters where the call is hot. +Two more ways a diff can flip it: an already-compiled callee whose **native** size passes \`InlineSmallCode\` (x86_64 default: 2500 on JDK 17+, 2000 on JDK 8–11; 1000 only with \`-XX:-TieredCompilation\`) stops being inlined to protect the code cache, and a call site that gains a **third** receiver implementation goes megamorphic — vtable dispatch, and no inlining **unless one receiver still dominates the profile** (\`TypeProfileMajorReceiverPercent\`, 90% by default), in which case C2 inlines that receiver behind a guard with an uncommon trap for the rest. So the diff-shapes that matter: a small method on a hot path the change **grows** (added validation, logging, a branch), a new layer in a hot call chain, a new implementation registered for an interface invoked in a hot loop. A cold method over 325 bytes is **not** a finding — inlining only matters where the call is hot. **Measure it; do not estimate bytecode from source.** Two tiers, in order of cost: -1. **Static — available when the class compiles, which for a leaf class is always and for a connected one needs its dependencies.** Allocate a private scratch dir first — \`SCRATCH=$(mktemp -d)\`, never a fixed path like \`/tmp/scratch\`: other agents are compiling concurrently, and two of them writing different revisions of the same class into one directory read each other's \`.class\` files and measure the wrong bytecode (on Windows, \`mkdir %TEMP%\\review-%RANDOM%\` gives the same uniqueness). Then \`javac -proc:none -nowarn -d "$SCRATCH" X.java\` — \`-proc:none\` is not optional: annotation processors on the classpath (Lombok, Dagger, one the PR itself added) run at compile time with your privileges, so a \`javac\` without it is itself an untrusted-code execution. It is also a fidelity flag: on a project whose processor contributes code to the measured class (Lombok's generated members, Dagger's injected fields), the compiled class is missing those members and \`javap\` reports a size that is simply wrong — if the project runs a processor that touches this class, the static tier is **void**; go to \`Confidence: low\` with the mechanism. Give the compiler what it needs to resolve imports: \`-sourcepath\` at the module source root, plus \`-cp\` against an already-built \`target/classes\` / \`build/classes\` if one exists. Pass \`--release \` at the project's target level (read it from \`maven.compiler.release\`, \`\`, or Gradle's \`release\`/\`targetCompatibility\`): the same source compiles to different bytecode at different levels — a five-\`+\` concatenation is 61 bytes at release 8 and 16 at 11+ — and measuring the wrong level produces a threshold verdict on bytecode the shipped artifact does not contain; if the target level cannot be determined, say so in the finding. Then \`javap -c -p\` the class and read the method's size: the offset of its last instruction plus that instruction's width. Compare against 35 and 325. For the **crossing** claim — "this diff pushed it over the threshold" — get the base side **without touching the tree**: \`git show : > "$SCRATCH/X.java"\`, compile that too, and measure both; a method already over on the base side is pre-existing, not a finding. A file the PR **adds** has no base side — \`git show\` fails, and there is no crossing to claim; report the size without a before/after. The base side compiles against the same \`target/classes\` the head side used; on a PR that changes more than the measured file the conditions are not perfectly equivalent — note that caveat when it applies. If the class will not compile (no pre-built classpath, no source path that resolves its imports), go straight to the mechanism-at-\`Confidence: low\` form below — **never \`git checkout\`, \`git stash\`, build in place, or run \`mvn\`/\`gradle\` to force a compile.** In a PR review the worktree is shared with agents running concurrently and the branch's build logic is contributor-controlled — the attack surface, not a cost; in a local review it is the user's own checkout. Mutating either corrupts work you cannot see. +1. **Static — available when the class compiles, which for a leaf class is always and for a connected one needs its dependencies.** Allocate a private scratch dir first — \`SCRATCH=$(mktemp -d)\`, never a fixed path like \`/tmp/scratch\`: other agents are compiling concurrently, and two of them writing different revisions of the same class into one directory read each other's \`.class\` files and measure the wrong bytecode (on Windows, \`mkdir %TEMP%\\review-%RANDOM%\` gives the same uniqueness). Then \`javac -proc:none -nowarn -d "$SCRATCH" X.java\` — \`-proc:none\` is not optional: annotation processors on the classpath (Lombok, Dagger, one the PR itself added) run at compile time with your privileges, so a \`javac\` without it is itself an untrusted-code execution. It is also a fidelity flag: on a project whose processor contributes code to the measured class (Lombok's generated members, Dagger's injected fields), the compiled class is missing those members and \`javap\` reports a size that is simply wrong — if the project runs a processor that touches this class, the static tier is **void**; go to \`Confidence: low\` with the mechanism. Give the compiler what it needs to resolve imports: \`-sourcepath\` at the module source root, plus \`-cp\` against an already-built \`target/classes\` / \`build/classes\` if one exists. Pass \`--release \` at the project's target level (read it from \`maven.compiler.release\`, \`\`, or Gradle's \`release\`/\`targetCompatibility\`): the same source compiles to different bytecode at different levels — a five-\`+\` concatenation is 37 bytes at release 8 and 14 at 9+ — and measuring the wrong level produces a threshold verdict on bytecode the shipped artifact does not contain; if the target level cannot be determined, say so in the finding. Then \`javap -c -p\` the class and read the method's size: the offset of its last instruction plus that instruction's width. Compare against 35 and 325. For the **crossing** claim — "this diff pushed it over the threshold" — get the base side **without touching the tree**: \`git show : > "$SCRATCH/X.java"\`, compile that too, and measure both; a method already over on the base side is pre-existing, not a finding. A file the PR **adds** has no base side — \`git show\` fails, and there is no crossing to claim; report the size without a before/after. The base side compiles against the same \`target/classes\` the head side used; on a PR that changes more than the measured file the conditions are not perfectly equivalent — note that caveat when it applies. If the class will not compile (no pre-built classpath, no source path that resolves its imports), go straight to the mechanism-at-\`Confidence: low\` form below — **never \`git checkout\`, \`git stash\`, build in place, or run \`mvn\`/\`gradle\` to force a compile.** In a PR review the worktree is shared with agents running concurrently and the branch's build logic is contributor-controlled — the attack surface, not a cost; in a local review it is the user's own checkout. Mutating either corrupts work you cannot see. 2. **Dynamic — when the code is runnable.** Run the workload with \`-XX:+UnlockDiagnosticVMOptions -XX:+PrintInlining\` and grep for the method: \`callee is too large\` / \`hot method too big\` is the JVM itself declining to inline. JMH gives the before/after throughput; JITWatch visualizes the same decision logs. A finding that a method "can no longer be inlined" without one of these two tiers is a guess stated as fact. Report the **mechanism** instead, at \`Confidence: low\`: the threshold at risk, what the diff added to the method, and the measurement still to run. And if the diff *claims* this path got faster while the mechanism above says it cannot have, the finding is the unsubstantiated claim. @@ -132,14 +132,23 @@ export const PATH_RULES: PathRule[] = [GITHUB_ACTIONS, JAVA]; * few and a count; the checklist, not the path list, is what the heading is * for. */ +function isOutOfScope(p: string): boolean { + return ( + /src\/(test|integTest|androidTest|testFixtures)\//i.test(p) || + /(?:^|\/)(?:package-info|module-info)\.java$/i.test(p) || + /(?:^|\/)generated(?:-sources)?\//i.test(p) + ); +} + function describePaths(which: readonly string[]): string { const CAP = 10; - // Production paths before test paths: the hot-path items this heading exists to - // introduce do not apply under src/test, so a PR that is mostly test classes - // must not fill the heading with files the rule explicitly scopes out. - const prod = which.filter((p) => !/src\/test\//i.test(p)); - const test = which.filter((p) => /src\/test\//i.test(p)); - const ordered = [...prod, ...test]; + // Production paths before out-of-scope paths: the hot-path items this heading + // exists to introduce do not apply to test, generated, or info-only sources, + // so a PR that is mostly such files must not fill the heading with paths the + // rule explicitly scopes out. + const prod = which.filter((p) => !isOutOfScope(p)); + const rest = which.filter((p) => isOutOfScope(p)); + const ordered = [...prod, ...rest]; if (ordered.length <= CAP) { return ordered.join(', '); } From 87b1ef75ab3d413a75730aad7ed6d6609272888f Mon Sep 17 00:00:00 2001 From: qwen-code-ci-bot Date: Sun, 2 Aug 2026 17:26:25 +0000 Subject: [PATCH 7/9] fix(cli): fifth-round review on the Java/JVM path rule --- .../commands/review/lib/path-rules.test.ts | 61 +++++++++++++++---- .../cli/src/commands/review/lib/path-rules.ts | 38 ++++++------ 2 files changed, 70 insertions(+), 29 deletions(-) diff --git a/packages/cli/src/commands/review/lib/path-rules.test.ts b/packages/cli/src/commands/review/lib/path-rules.test.ts index fb0c9fbb554..9b9b8a2c322 100644 --- a/packages/cli/src/commands/review/lib/path-rules.test.ts +++ b/packages/cli/src/commands/review/lib/path-rules.test.ts @@ -154,13 +154,27 @@ describe('pathRulesFor — the Java/JVM rule', () => { // product flag with a ≥ boundary, megamorphic stated as unconditional) and a // review measured them against a live JVM. Pin the corrected forms. const out = pathRulesFor(['src/Main.java']); - expect(out).toContain('2500 on JDK 17+'); - expect(out).toContain('2000 on JDK 8–11'); + expect(out).toContain('2500 on JDK 11+'); + expect(out).toContain('2000 on JDK 8'); expect(out).toContain('DontCompileHugeMethods'); expect(out).toMatch(/> 8000/); expect(out).toContain('TypeProfileMajorReceiverPercent'); }); + it('describes the inline table as size caps, not inlining outcomes', () => { + // A review reproduced C2 declining a 10-byte callee for `low call site + // frequency`: size is one gate among several, so the table reads as size + // caps and a "can no longer be inlined" claim needs the dynamic tier — a + // javap size diff alone proves only a threshold crossing, not an inlining + // change. The outcome words that contradicted that behaviour are gone. + const out = pathRulesFor(['src/Main.java']); + expect(out).toContain('size caps'); + expect(out).toMatch(/necessary but not sufficient/); + expect(out).toContain('low call site frequency'); + expect(out).toContain('needs the **dynamic** tier'); + expect(out).not.toContain('even when cold'); + }); + it('refuses to estimate bytecode from source', () => { // The one failure this checklist exists to prevent: an agent eyeballing a // method and declaring it un-inlinable. The honest form is the mechanism, @@ -276,28 +290,51 @@ describe('pathRulesFor — the Java/JVM rule', () => { }); it('deprioritizes generated, info-only, and non-Maven test sources too', () => { - // The checklist scopes out more than src/test: generated sources, - // package-info/module-info, and non-Maven test roots (integTest, - // androidTest, testFixtures) must not fill the named slots either. + // The checklist scopes out more than src/test: generated sources under the + // build output dirs, package-info/module-info, and non-Maven test roots + // (integrationTest, integTest, androidTest, testFixtures) must not fill the + // named slots either. A source package merely NAMED `generated` is production. const noise = [ 'target/generated-sources/com/x/Stub.java', 'build/generated/com/x/R.java', - 'src/main/java/com/x/generated/Proto.java', 'src/main/java/com/x/package-info.java', 'src/main/java/module-info.java', - 'src/integTest/java/com/x/IT.java', + 'src/integrationTest/java/com/x/IT.java', + 'src/integTest/java/com/x/IT2.java', 'src/androidTest/java/com/x/AT.java', 'src/testFixtures/java/com/x/Fix.java', ]; - const prod = ['src/main/java/com/x/Hot.java']; + const prod = [ + 'src/main/java/com/x/Hot.java', + 'src/main/java/com/x/generated/Proto.java', + ]; const out = pathRulesFor([...noise, ...prod]); const heading = out.split('\n').find((l) => l.startsWith('### ')) ?? ''; - // The single production path must be named first. + // Both production paths — including the one in a `generated` package — must + // be named before any out-of-scope path. + const stubIdx = heading.indexOf('Stub.java'); + expect(stubIdx).toBeGreaterThanOrEqual(0); + expect(heading.indexOf('Hot.java')).toBeGreaterThanOrEqual(0); + expect(heading.indexOf('Proto.java')).toBeGreaterThanOrEqual(0); + expect(heading.indexOf('Hot.java')).toBeLessThan(stubIdx); + expect(heading.indexOf('Proto.java')).toBeLessThan(stubIdx); + }); + + it('treats Gradle src/integrationTest as out of scope even past the cap', () => { + // Gradle's conventional directory for an `integrationTest` suite is + // src/integrationTest/java. With more than ten such paths plus one + // production path, the production path must still be named first — not + // truncated away by a heading full of test files. + const tests = Array.from( + { length: 12 }, + (_, i) => `src/integrationTest/java/com/x/T${i}.java`, + ); + const out = pathRulesFor([...tests, 'src/main/java/com/x/Hot.java']); + const heading = out.split('\n').find((l) => l.startsWith('### ')) ?? ''; + expect(heading).toContain('…and 3 more'); expect(heading.indexOf('Hot.java')).toBeGreaterThanOrEqual(0); expect(heading.indexOf('Hot.java')).toBeLessThan( - heading.indexOf('Stub.java') === -1 - ? Infinity - : heading.indexOf('Stub.java'), + heading.indexOf('T0.java'), ); }); diff --git a/packages/cli/src/commands/review/lib/path-rules.ts b/packages/cli/src/commands/review/lib/path-rules.ts index 5f2e02147c9..0b9d82cbe86 100644 --- a/packages/cli/src/commands/review/lib/path-rules.ts +++ b/packages/cli/src/commands/review/lib/path-rules.ts @@ -97,24 +97,26 @@ const JAVA: PathRule = { **JIT inlining — the regression no dimension can see, and you cannot prove from source:** -HotSpot inlines by the **bytecode size of the callee**, against these product defaults (a project's own \`-XX:\` flags, or a different JVM, change them — check before citing): +HotSpot's C2 compiler gates inlining on the **bytecode size of the callee** among other criteria. These are the size caps in its normal policy (a project's own \`-XX:\` flags, or a different JVM, change them — check before citing): -| callee bytecode size | inlinable | +| callee bytecode size | C2 normal-policy size cap | | --- | --- | -| ≤ 6 (\`MaxTrivialSize\`) | always | -| ≤ 35 (\`MaxInlineSize\`) | even when cold | -| ≤ 325 (\`FreqInlineSize\`) | only at a hot call site | -| > 325 | never | -| > 8000 (\`HugeMethodLimit\`, a develop flag gated by the product \`DontCompileHugeMethods\`) | never even JIT-compiled | +| ≤ 6 (\`MaxTrivialSize\`) | under the trivial cap — size alone does not block it | +| ≤ 35 (\`MaxInlineSize\`) | under the normal cap — size alone does not block it | +| ≤ 325 (\`FreqInlineSize\`) | under the hot cap — size blocks it unless the call site is hot | +| > 325 | over the hot cap — size blocks it at every call site | +| > 8000 (\`HugeMethodLimit\`, a develop flag gated by the product \`DontCompileHugeMethods\`) | over the huge-method limit — size blocks even JIT compilation | -Two more ways a diff can flip it: an already-compiled callee whose **native** size passes \`InlineSmallCode\` (x86_64 default: 2500 on JDK 17+, 2000 on JDK 8–11; 1000 only with \`-XX:-TieredCompilation\`) stops being inlined to protect the code cache, and a call site that gains a **third** receiver implementation goes megamorphic — vtable dispatch, and no inlining **unless one receiver still dominates the profile** (\`TypeProfileMajorReceiverPercent\`, 90% by default), in which case C2 inlines that receiver behind a guard with an uncommon trap for the rest. So the diff-shapes that matter: a small method on a hot path the change **grows** (added validation, logging, a branch), a new layer in a hot call chain, a new implementation registered for an interface invoked in a hot loop. A cold method over 325 bytes is **not** a finding — inlining only matters where the call is hot. +A size cap is a gate, not an outcome: passing one is necessary but not sufficient. Independent of size, C2 also declines a call site that was **never executed**, has **low call site frequency**, exceeds the inlining **depth or recursion limits**, or hits the **node-count** cutoff — so a method under a cap can still be left un-inlined, and a static size measurement proves only that a threshold was crossed, not that the method *was* inlined before and *is not* now. + +Two more ways a diff can flip it: an already-compiled callee whose **native** size passes \`InlineSmallCode\` (x86_64 default: 2500 on JDK 11+, 2000 on JDK 8; 1000 only with \`-XX:-TieredCompilation\`) stops being inlined to protect the code cache, and a call site that gains a **third** receiver implementation goes megamorphic — vtable dispatch, and no inlining **unless one receiver still dominates the profile** (\`TypeProfileMajorReceiverPercent\`, 90% by default), in which case C2 inlines that receiver behind a guard with an uncommon trap for the rest. So the diff-shapes that matter: a small method on a hot path the change **grows** (added validation, logging, a branch), a new layer in a hot call chain, a new implementation registered for an interface invoked in a hot loop. A cold method over 325 bytes is **not** a finding — inlining only matters where the call is hot. **Measure it; do not estimate bytecode from source.** Two tiers, in order of cost: 1. **Static — available when the class compiles, which for a leaf class is always and for a connected one needs its dependencies.** Allocate a private scratch dir first — \`SCRATCH=$(mktemp -d)\`, never a fixed path like \`/tmp/scratch\`: other agents are compiling concurrently, and two of them writing different revisions of the same class into one directory read each other's \`.class\` files and measure the wrong bytecode (on Windows, \`mkdir %TEMP%\\review-%RANDOM%\` gives the same uniqueness). Then \`javac -proc:none -nowarn -d "$SCRATCH" X.java\` — \`-proc:none\` is not optional: annotation processors on the classpath (Lombok, Dagger, one the PR itself added) run at compile time with your privileges, so a \`javac\` without it is itself an untrusted-code execution. It is also a fidelity flag: on a project whose processor contributes code to the measured class (Lombok's generated members, Dagger's injected fields), the compiled class is missing those members and \`javap\` reports a size that is simply wrong — if the project runs a processor that touches this class, the static tier is **void**; go to \`Confidence: low\` with the mechanism. Give the compiler what it needs to resolve imports: \`-sourcepath\` at the module source root, plus \`-cp\` against an already-built \`target/classes\` / \`build/classes\` if one exists. Pass \`--release \` at the project's target level (read it from \`maven.compiler.release\`, \`\`, or Gradle's \`release\`/\`targetCompatibility\`): the same source compiles to different bytecode at different levels — a five-\`+\` concatenation is 37 bytes at release 8 and 14 at 9+ — and measuring the wrong level produces a threshold verdict on bytecode the shipped artifact does not contain; if the target level cannot be determined, say so in the finding. Then \`javap -c -p\` the class and read the method's size: the offset of its last instruction plus that instruction's width. Compare against 35 and 325. For the **crossing** claim — "this diff pushed it over the threshold" — get the base side **without touching the tree**: \`git show : > "$SCRATCH/X.java"\`, compile that too, and measure both; a method already over on the base side is pre-existing, not a finding. A file the PR **adds** has no base side — \`git show\` fails, and there is no crossing to claim; report the size without a before/after. The base side compiles against the same \`target/classes\` the head side used; on a PR that changes more than the measured file the conditions are not perfectly equivalent — note that caveat when it applies. If the class will not compile (no pre-built classpath, no source path that resolves its imports), go straight to the mechanism-at-\`Confidence: low\` form below — **never \`git checkout\`, \`git stash\`, build in place, or run \`mvn\`/\`gradle\` to force a compile.** In a PR review the worktree is shared with agents running concurrently and the branch's build logic is contributor-controlled — the attack surface, not a cost; in a local review it is the user's own checkout. Mutating either corrupts work you cannot see. 2. **Dynamic — when the code is runnable.** Run the workload with \`-XX:+UnlockDiagnosticVMOptions -XX:+PrintInlining\` and grep for the method: \`callee is too large\` / \`hot method too big\` is the JVM itself declining to inline. JMH gives the before/after throughput; JITWatch visualizes the same decision logs. -A finding that a method "can no longer be inlined" without one of these two tiers is a guess stated as fact. Report the **mechanism** instead, at \`Confidence: low\`: the threshold at risk, what the diff added to the method, and the measurement still to run. And if the diff *claims* this path got faster while the mechanism above says it cannot have, the finding is the unsubstantiated claim. +A finding that a method "can no longer be inlined" needs the **dynamic** tier — only \`PrintInlining\` or equivalent runtime evidence shows the JVM actually declined to inline. The static tier proves a **size-gate crossing**, which is the mechanism: report it at \`Confidence: low\` — the threshold crossed, what the diff added to the method, and the runtime measurement still to run. A "can no longer be inlined" claim with neither tier is a guess stated as fact. And if the diff *claims* this path got faster while the mechanism above says it cannot have, the finding is the unsubstantiated claim. **When the finding IS a grown hot method, the fix to suggest is hot/cold splitting — not reverting the change, and not a JVM tuning flag** (\`-XX:FreqInlineSize\`, \`-XX:CompileCommand=inline\`): those are runtime knobs the PR's author cannot ship in a code change, and raising an inline threshold to fit one method pays for it at every other call site. (The JDK-internal \`@ForceInline\` is not available to application code at all.) Move the cold paths — error handling, rare branches, defensive validation, the \`switch\` arms that almost never fire — into a small private helper, leaving the common path under the threshold; the helper, now called from one site, is itself inlinable. Name the bytecode range to extract and the size it leaves behind, the way the measurement names them: "extract bytecodes 212–311 (~100 bytes) into \`applyRounding\`, leaving \`parseAmount\` at ~238 bytes" is a finding an author can act on; "consider splitting the method" is not. @@ -124,6 +126,16 @@ A finding that a method "can no longer be inlined" without one of these two tier /** Every rule, in the order their checklists are appended. */ export const PATH_RULES: PathRule[] = [GITHUB_ACTIONS, JAVA]; +function isOutOfScope(p: string): boolean { + return ( + /src\/(test|integrationTest|integTest|androidTest|testFixtures)\//i.test( + p, + ) || + /(?:^|\/)(?:package-info|module-info)\.java$/i.test(p) || + /(?:^|\/)(?:target|build)\/(?:generated-sources|generated)\//i.test(p) + ); +} + /** * The triggering paths named in a rule's heading — capped. A workflow rule * matches one or two files; a Java rule matches every source file in a large @@ -132,14 +144,6 @@ export const PATH_RULES: PathRule[] = [GITHUB_ACTIONS, JAVA]; * few and a count; the checklist, not the path list, is what the heading is * for. */ -function isOutOfScope(p: string): boolean { - return ( - /src\/(test|integTest|androidTest|testFixtures)\//i.test(p) || - /(?:^|\/)(?:package-info|module-info)\.java$/i.test(p) || - /(?:^|\/)generated(?:-sources)?\//i.test(p) - ); -} - function describePaths(which: readonly string[]): string { const CAP = 10; // Production paths before out-of-scope paths: the hot-path items this heading From 9ea864046d2e5f749dd21556ca93579528604138 Mon Sep 17 00:00:00 2001 From: qwen-code-ci-bot Date: Sun, 2 Aug 2026 18:31:13 +0000 Subject: [PATCH 8/9] fix(cli): sixth-round review on the Java/JVM path rule --- .../commands/review/lib/path-rules.test.ts | 80 ++++++++++++------- 1 file changed, 53 insertions(+), 27 deletions(-) diff --git a/packages/cli/src/commands/review/lib/path-rules.test.ts b/packages/cli/src/commands/review/lib/path-rules.test.ts index 9b9b8a2c322..dbe5f7eb63e 100644 --- a/packages/cli/src/commands/review/lib/path-rules.test.ts +++ b/packages/cli/src/commands/review/lib/path-rules.test.ts @@ -289,35 +289,61 @@ describe('pathRulesFor — the Java/JVM rule', () => { expect(prodIdx).toBeLessThan(testIdx); }); - it('deprioritizes generated, info-only, and non-Maven test sources too', () => { - // The checklist scopes out more than src/test: generated sources under the - // build output dirs, package-info/module-info, and non-Maven test roots - // (integrationTest, integTest, androidTest, testFixtures) must not fill the - // named slots either. A source package merely NAMED `generated` is production. - const noise = [ - 'target/generated-sources/com/x/Stub.java', - 'build/generated/com/x/R.java', - 'src/main/java/com/x/package-info.java', - 'src/main/java/module-info.java', - 'src/integrationTest/java/com/x/IT.java', - 'src/integTest/java/com/x/IT2.java', - 'src/androidTest/java/com/x/AT.java', - 'src/testFixtures/java/com/x/Fix.java', - ]; - const prod = [ - 'src/main/java/com/x/Hot.java', + it.each([ + [ + 'generated build output', + Array.from( + { length: 11 }, + (_, i) => `target/generated-sources/com/x/S${i}.java`, + ), + ], + [ + 'non-Maven test roots', + Array.from({ length: 11 }, (_, i) => { + const root = ['integTest', 'androidTest', 'testFixtures'][i % 3]; + return `src/${root}/java/com/x/N${i}.java`; + }), + ], + [ + 'info-only sources', + Array.from({ length: 11 }, (_, i) => + i < 6 + ? `src/main/java/com/x/p${i}/package-info.java` + : `src/main/java/com/x/m${i}/module-info.java`, + ), + ], + ])('deprioritizes %s past the cap, not just src/test', (_label, noise) => { + // The checklist scopes out more than src/test. Each family below, once it + // outnumbers the cap, must still not fill the named slots: the noise is + // pushed past CAP so truncation bites, and the production path is asserted + // to survive it. Drop the matching branch from isOutOfScope and the family + // is reclassified as production, fills the ten slots, and truncates Hot.java + // away — so the regression fails instead of shipping green. (integrationTest + // is pinned by the dedicated test below.) + const out = pathRulesFor([...noise, 'src/main/java/com/x/Hot.java']); + const heading = out.split('\n').find((l) => l.startsWith('### ')) ?? ''; + expect(heading).toContain('Hot.java'); + expect(heading).toContain('…and 2 more'); + expect(heading).not.toContain(noise[noise.length - 1]); + }); + + it('treats a source package merely named generated as production', () => { + // `src/main/java/com/x/generated/` is a source package that happens to be + // named `generated`; only build OUTPUT dirs (target/generated-sources, + // build/generated) are scoped out. Even with the cap full of real generated + // sources, the production path must keep its named slot — if the generated + // pattern over-matched, Proto.java would be scoped out and truncated away. + const noise = Array.from( + { length: 11 }, + (_, i) => `target/generated-sources/com/x/S${i}.java`, + ); + const out = pathRulesFor([ + ...noise, 'src/main/java/com/x/generated/Proto.java', - ]; - const out = pathRulesFor([...noise, ...prod]); + ]); const heading = out.split('\n').find((l) => l.startsWith('### ')) ?? ''; - // Both production paths — including the one in a `generated` package — must - // be named before any out-of-scope path. - const stubIdx = heading.indexOf('Stub.java'); - expect(stubIdx).toBeGreaterThanOrEqual(0); - expect(heading.indexOf('Hot.java')).toBeGreaterThanOrEqual(0); - expect(heading.indexOf('Proto.java')).toBeGreaterThanOrEqual(0); - expect(heading.indexOf('Hot.java')).toBeLessThan(stubIdx); - expect(heading.indexOf('Proto.java')).toBeLessThan(stubIdx); + expect(heading).toContain('Proto.java'); + expect(heading).toContain('…and 2 more'); }); it('treats Gradle src/integrationTest as out of scope even past the cap', () => { From 19824ffcfc196c7a17042022639742bfe85b7f20 Mon Sep 17 00:00:00 2001 From: qwen-code-ci-bot Date: Sun, 2 Aug 2026 20:19:10 +0000 Subject: [PATCH 9/9] fix(cli): seventh-round review on the Java/JVM path rule --- packages/cli/src/commands/review/lib/path-rules.test.ts | 8 ++++++-- packages/cli/src/commands/review/lib/path-rules.ts | 2 +- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/packages/cli/src/commands/review/lib/path-rules.test.ts b/packages/cli/src/commands/review/lib/path-rules.test.ts index dbe5f7eb63e..e0a2c3fe6dd 100644 --- a/packages/cli/src/commands/review/lib/path-rules.test.ts +++ b/packages/cli/src/commands/review/lib/path-rules.test.ts @@ -150,11 +150,11 @@ describe('pathRulesFor — the Java/JVM rule', () => { it('cites the thresholds a maintainer will check, correctly', () => { // A checklist whose thesis is "don't guess the numbers" loses all trust the // moment it cites a wrong one. These three were wrong in the first draft - // (InlineSmallCode quoted as the pre-JDK-11 value, HugeMethodLimit called a + // (InlineSmallCode quoted as the pre-JDK-17 value, HugeMethodLimit called a // product flag with a ≥ boundary, megamorphic stated as unconditional) and a // review measured them against a live JVM. Pin the corrected forms. const out = pathRulesFor(['src/Main.java']); - expect(out).toContain('2500 on JDK 11+'); + expect(out).toContain('2500 on JDK 17+'); expect(out).toContain('2000 on JDK 8'); expect(out).toContain('DontCompileHugeMethods'); expect(out).toMatch(/> 8000/); @@ -297,6 +297,10 @@ describe('pathRulesFor — the Java/JVM rule', () => { (_, i) => `target/generated-sources/com/x/S${i}.java`, ), ], + [ + 'Gradle generated output', + Array.from({ length: 11 }, (_, i) => `build/generated/com/x/S${i}.java`), + ], [ 'non-Maven test roots', Array.from({ length: 11 }, (_, i) => { diff --git a/packages/cli/src/commands/review/lib/path-rules.ts b/packages/cli/src/commands/review/lib/path-rules.ts index 0b9d82cbe86..0c597c02562 100644 --- a/packages/cli/src/commands/review/lib/path-rules.ts +++ b/packages/cli/src/commands/review/lib/path-rules.ts @@ -109,7 +109,7 @@ HotSpot's C2 compiler gates inlining on the **bytecode size of the callee** amon A size cap is a gate, not an outcome: passing one is necessary but not sufficient. Independent of size, C2 also declines a call site that was **never executed**, has **low call site frequency**, exceeds the inlining **depth or recursion limits**, or hits the **node-count** cutoff — so a method under a cap can still be left un-inlined, and a static size measurement proves only that a threshold was crossed, not that the method *was* inlined before and *is not* now. -Two more ways a diff can flip it: an already-compiled callee whose **native** size passes \`InlineSmallCode\` (x86_64 default: 2500 on JDK 11+, 2000 on JDK 8; 1000 only with \`-XX:-TieredCompilation\`) stops being inlined to protect the code cache, and a call site that gains a **third** receiver implementation goes megamorphic — vtable dispatch, and no inlining **unless one receiver still dominates the profile** (\`TypeProfileMajorReceiverPercent\`, 90% by default), in which case C2 inlines that receiver behind a guard with an uncommon trap for the rest. So the diff-shapes that matter: a small method on a hot path the change **grows** (added validation, logging, a branch), a new layer in a hot call chain, a new implementation registered for an interface invoked in a hot loop. A cold method over 325 bytes is **not** a finding — inlining only matters where the call is hot. +Two more ways a diff can flip it: an already-compiled callee whose **native** size passes \`InlineSmallCode\` (x86_64 default: 2500 on JDK 17+, 2000 on JDK 8–16; 1000 only with \`-XX:-TieredCompilation\`) stops being inlined to protect the code cache, and a call site that gains a **third** receiver implementation goes megamorphic — vtable dispatch, and no inlining **unless one receiver still dominates the profile** (\`TypeProfileMajorReceiverPercent\`, 90% by default), in which case C2 inlines that receiver behind a guard with an uncommon trap for the rest. So the diff-shapes that matter: a small method on a hot path the change **grows** (added validation, logging, a branch), a new layer in a hot call chain, a new implementation registered for an interface invoked in a hot loop. A cold method over 325 bytes is **not** a finding — inlining only matters where the call is hot. **Measure it; do not estimate bytecode from source.** Two tiers, in order of cost: