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..e0a2c3fe6dd 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); }); @@ -102,4 +106,313 @@ 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', () => { + 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('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('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-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 17+'); + 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, + // 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('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'); + 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\*\*/); + }); + + 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. 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) 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('static tier is **void**'); + expect(out).toContain('git show'); + 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('### ')) ?? ''; + const prodIdx = heading.indexOf('A.java'); + const testIdx = heading.indexOf('T0Test'); + expect(prodIdx).toBeGreaterThanOrEqual(0); + expect(testIdx).toBeGreaterThanOrEqual(0); + expect(prodIdx).toBeLessThan(testIdx); + }); + + it.each([ + [ + 'generated build output', + Array.from( + { length: 11 }, + (_, 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) => { + 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 heading = out.split('\n').find((l) => l.startsWith('### ')) ?? ''; + expect(heading).toContain('Proto.java'); + expect(heading).toContain('…and 2 more'); + }); + + 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('T0.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 + // 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'); + }); + + it('names --release and the new-file clause in the static tier', () => { + // The same source compiles to different bytecode at different --release + // 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']); + 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 b51d46b54b2..0c597c02562 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. */ @@ -68,8 +70,94 @@ 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). 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):** + +- **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 (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 — \`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. + +**JIT inlining — the regression no dimension can see, and you cannot prove from source:** + +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 | C2 normal-policy size cap | +| --- | --- | +| ≤ 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 | + +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 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: + +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" 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. + +**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]; +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 + * 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; + // 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(', '); + } + return `${ordered.slice(0, CAP).join(', ')}, …and ${ordered.length - CAP} more`; +} /** * The checklists that govern `paths`, as a brief section — or `''` when none do. @@ -84,7 +172,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(); }