# Ror Pr Review > Review a ReadonlyREST pull request with the depth of a senior Scala/Elasticsearch reviewer — not a rubber-stamp. Use when invoked as an automated PR reviewer, when the user asks for a PR review on this repo, or whenever the task is "review PR - Skill: `sscarduzio/ror-pr-review` (Agent Skill) - Install (CLI): `npx skillmds@latest add sscarduzio/ror-pr-review` - Raw SKILL.md: https://api.skillmd.com/api/skills/sscarduzio/ror-pr-review/raw - Safety review: pending - Works with: Claude Code, Claude.ai, OpenAI Codex - Category: Productivity - Author: sscarduzio (https://skillmd.com/u/sscarduzio) - Updated: 2026-09-17 - Page: https://skillmd.com/skills/sscarduzio/ror-pr-review --- # ReadonlyREST PR Review The goal is the level of insight a careful senior Scala/Elasticsearch reviewer would produce — not a rubber-stamp "No issues found." ## Mandatory methodology 1. **Read CLAUDE.md first.** Then read the PR description and any prior review comments — they often contain context, previous findings, and the contributor's responses you must not duplicate or contradict without reason. 2. **Pull the full diff and full context.** Use `gh pr diff ` for the diff; `gh pr view --json title,body,commits,reviews` for metadata; `gh api repos/.../issues//comments` for prior discussion. Do **not** rely on a truncated diff — if the diff is large, process it in chunks but cover every file. 3. **Read the modified files in full** (not just the hunks). Bugs usually live in the lines around the diff, not inside it. Use `Read` on each changed file before commenting on it. 4. **Cross-reference call sites.** When a function/type is touched, `grep -rn` for every caller and assess whether the change is safe for each. A behavioral change that compiles is not necessarily a behavioral change that's *correct*. The Scala compiler's variance and implicit-resolution machinery hides a lot of subtle behavior — always verify the runtime semantics. 5. **Check the tests.** For every behavioral change, ask: does a test cover this? If a test claims to cover it, does the test actually verify the new behavior, or does it pass for an incidental reason (false-confidence test)? Property-based tests with overly-narrow generators are a common variant. 6. **Check CI.** Use `gh pr checks ` and `gh run view --log-failed` for any failing job. A failing integration test on a single ES module is often a hint that the code change broke a real flow on that ES version. 7. **Empirically verify claims.** If the diff includes a regex, parser, encoder, or security check — run it. `node -e "/regex/.test('input')"` is fair game for regexes; `scala-cli` if available for Scala snippets. Do not approve a security-sensitive regex without trying bypasses. ## What to actively hunt for Hunt for these categories on **every** PR, not just security-flagged ones: - **Cross-ES-version drift.** This is the #1 source of bugs in this repo. The plugin supports ES 6.7 → 9.2 via 30+ `es{version}x/` adapter modules. When a change touches one module, check whether the same fix needs to apply to siblings. The `MultiGetEsRequestContext` / `MultiSearchEsRequestContext` files in particular often need identical changes across all es modules. Use `git diff --stat` to check; if the diff touches only `es814x` but the changed code exists verbatim in `es815x`, flag it. - **Internal Scala runtime APIs.** Any usage of `scala.runtime.ScalaRunTime._*`, `_`-prefixed runtime methods, or other implementation-detail APIs is forbidden — they change silently between Scala versions. Public alternatives exist (`scala.util.hashing.MurmurHash3.productHash` for hashCode, etc.). Recent real example: PR #1247 used `ScalaRunTime._hashCode(this)` for cached case-class hashCode; fixed by extracting an `EagerHashCode` trait using `MurmurHash3.productHash`. - **Hot-path allocation patterns.** This codebase processes ACL evaluation per-request. Watch for: `foldLeft` over an immutable `Set` (allocates O(n) intermediate Sets — use `partition` or `mutable.Set.Builder` instead); repeated `.filter(...).map(...)` chains where a single pass would do; computing `hashCode` lazily on objects appearing in high-frequency `Set`/`Map` operations. - **Dead-code branches.** A new branch that can never fire because an earlier branch always matches first. Common pattern: a fallback strategy preempted by an unconditional default in the same caller. - **False-confidence tests.** Tests that appear to assert the new behavior but actually pass via an incidental code path. Stress-test by mentally running the test with the *fix* reverted — does the test still pass? If yes, the test is decorative. Property-based tests with overly-narrow generators are a common offender. - **Behavioral changes without test coverage.** Look for code that changes the shape of a response, the conditions under which a side-effect fires, or the result of a function — without a corresponding test addition. - **Silent removals.** A condition, branch, or feature flag deleted with no replacement. Search for the deleted identifier across the codebase to confirm intent. - **Wrong enum style.** Sealed traits with implicit case-object children but **no** `enumeratum.Enum[T]` mixin and no `findValues` — they break JSON encoders/decoders that rely on enumeratum's introspection. Always `enumeratum`. - **Raw `Future` instead of `monix.eval.Task`.** All async work in this codebase uses `Task`. Adding a `Future`-based code path forces an awkward conversion at the boundary and breaks structured concurrency. - **`println` / `System.out` / `System.err`.** Forbidden — use structured logging. - **`-Xfatal-warnings` violations.** Compilation will fail anyway, but flag obvious cases: unused imports/params/locals/privates, deprecated API usage, exhaustiveness warnings on `match`. - **Missing GNU GPL v3 license header.** Every new `.scala` file must carry it. The pre-commit hook normally catches this, but PRs from external contributors may slip through. - **Internal-Elasticsearch-API misuse.** Anything in `org.elasticsearch.*` outside the public APIs may break between minor versions. Particularly check usages added in `es{version}x/` modules. - **Shadow/shading boundaries.** Direct imports of un-shaded dependency packages (e.g. `com.google.gson`, `org.apache.logging`) should go through the relocated `tech.beshu.ror.*` prefix. The Shadow plugin should hide all third-party deps. - **Unrelated changes bundled.** A PR titled "fix LDAP timeout" should not also change FLS evaluation or settings parsing. Note bundled drift in the review. - **Domain-invariant violations.** For changes touching `kibana_access`, the fields/FLS rule, audit, or `/_readonlyrest` APIs, check the design invariants in the `ror-internals` skill (decision-tree ordering, hidden `lucene` FLS engine, 5KB audit-event cap, endpoint→action-name mapping). ## Project-convention checklist (apply mechanically) - `Future` instead of `monix.eval.Task` in new code - `println` / `System.out` / `System.err` instead of structured logging - Sealed trait + case objects without `enumeratum.Enum[T]` / `EnumEntry` - `scala.runtime.ScalaRunTime._*` or other `_`-prefixed internal Scala APIs - Missing GNU GPL v3 license header on new `.scala` files - `-Xfatal-warnings` violations (unused imports/params/locals/privates) - Identical change applied to one `es{version}x` module but missed in siblings where the same code path exists - Direct imports of un-shaded dependency packages (should go through `tech.beshu.ror` prefix) - Comment defects (`docs/dev/code-style.md`): narrating the change ("no longer set here", "moved from X") instead of the current state; explaining a function at its call site instead of at its definition; restating self-describing code - `Future.successful(...)` wrappers around already-synchronous values inside `Task` flows (anti-pattern: just lift directly with `Task.now`/`Task.pure`) ## Output format **BE CONCISE.** The review is read by a busy maintainer. Every finding = what to do + why. Nothing else. - **Findings only**, ordered by severity (`🚨 Critical` → `⚠️ Warning` → `Nit`). Each finding, max 4 lines: - `file:line` — imperative one-liner: what to change. - Why, in 1–2 sentences, with the single strongest piece of evidence (a grep hit, a failing input, a call site) — not the full investigation. - A code snippet ONLY when the fix isn't obvious from the one-liner. - NO "What I checked" narrative, NO "What looks good" section, NO methodology recap, NO restating the PR description. Do the investigation; don't serialize it. - **Open questions**: only if the answer would change a finding; max 2. - Genuinely no issues? One line: `No issues found — verified .` That line IS the required backing; never a bare "No issues found". - Write the review itself in the repo style: Simplified Technical English plus Zinsser (`docs/dev/writing-style.md`). One idea per sentence, active voice, no metaphor. - **Hard cap: ~30 lines / ~350 words total.** With many findings, keep every Critical/Warning full and compress Nits to one line each. ## When the PR is small / docs-only Same rules, shorter: the no-issues one-liner (with what you verified) is a complete review for a docs-only diff you read in full.