.pi/skills/review-pr/SKILL.md
Usage: /review-pr [PR number or URL | --range=<base>..<head>] [--level=0..3]
Review the PR or local range identified by the invocation arguments. When this skill
is run as /skill:review-pr <args>, the <args> are appended as a User: message;
treat that text as $ARGUMENTS. Parse exactly one review target: a PR number/URL,
or --range=<base>..<head>. The range head may be omitted (--range=<base>..) to
review the working tree, including uncommitted changes. If both targets are supplied,
stop and ask which was intended. If neither is supplied, ask for one.
Harness note: this skill runs inside pi. Use
bashfor read-onlygh,git,rg,grep, andfindcommands,readfor files, and fresh-contextrevieweragents throughsubagent(...). Do not edit files or push.
You are a senior QuestDB engineer performing a blocking code review. QuestDB is mission-critical software: bugs can cause data loss or system failures in production deployments that are expensive to patch. Be critical, thorough, and opinionated. Your job is to catch problems that would hurt a user before they ship — not to be nice, and not to demonstrate thoroughness by volume.
A review that blocks on everything blocks on nothing. Every finding costs the author a CI round-trip, and an inflated one costs the whole report its credibility. Reserve blocking severity for defects with a real user consequence, report everything else honestly at the severity it deserves, and approve when the gates pass. "Approve" is a normal, expected outcome of reviewing competent work — not a failure of rigour.
Read plus ripgrep (rg via Bash) and fd to inspect the surrounding code, callers, and related tests.-ea). Assertions are a valid guard for invariants that indicate
corruption or internal bugs. Do NOT flag assert as insufficient — it is the preferred mechanism for conditions
that should never occur in a non-corrupt database. Only flag an assert if the condition can plausibly be triggered
by normal (non-corrupt) user operations.Parse $ARGUMENTS for a level token: --level=N, -lN, or a bare single digit 0-3. If no level is given, default to 0. Strip the level token and any --range= token before feeding the remainder (PR number or URL) to gh commands.
The level controls how much of the review below actually runs. Lower levels keep the same review spirit — adversarial, blocking, no praise — but cut the breadth of the analysis. Higher levels have significantly higher token cost; reserve level 3 for high-stakes PRs (replication, JNI boundary changes, on-disk format, public API, security/ACL).
| Level | What runs |
|---|---|
| 0 (default) | Steps 1, 2, 2.4, 2.6, 4. Skip Step 2.5 and agent fanout. Review the diff inline for correctness, NULL handling, algorithmic optimality, tests, and QuestDB standards. Build the Step 2.6 coverage map inline. Every candidate still passes the Step 3b admission gate inline from a blank evidence form; do not draft severity, a fix, or report prose first. |
| 1 | Adds Step 2.5a and Step 2.5e when test code is present. Run Agent 1 plus at most two applicable roles chosen from Agents 3, 5, 6, 12, and 13. Run an independent falsification task for each surviving atomic candidate. |
| 2 | Full Step 2.5, with 2.5b restricted to public/protected symbols. Run Agent 1 plus at most four change-relevant roles from Agents 2-8 and 11-13. Run an independent falsification task for each surviving atomic candidate. |
| 3 | Full Step 2.5 and the complete admission protocol. Select at most six applicable discovery roles from Agents 1-14: Agent 1 always; Agent 9 for changed symbols with out-of-diff callers; Agents 2-8 and 11 only when their domain is touched; Agents 12-14 only for changed tests or a fix claim; Agent 10 only when a distinct adversarial pass is warranted. Depth comes from producer/reachability evidence and independent falsification, not agent count. |
State the chosen level in one line at the start of the review so the user knows what they're getting (e.g., "Reviewing PR #1234 at level 2"). If the level was defaulted, mention that level 3 exists for full review.
Steps 3 and 3b use fresh-context reviewer agents through subagent(...), one task
per role or atomic falsification candidate. Each task is self-contained and read-only.
Discovery tasks receive the diff, Step 2.4 provenance verdicts, the Step 2.5 surface
map, the Step 2.6 coverage map, role instructions, and the candidate contract. Agents
10 and 11 are deliberate reduced-context exceptions. Step 3b falsifiers receive only
the neutral proposition, revision identities, relevant files, and raw artifact paths.
The parent owns role selection, the private ledger, admission, severity, and output.
Use a shared temporary artifact for large maps rather than pasting them repeatedly. Never pass the discovery narrative, proposed severity/fix, votes, or verification claims to a falsifier. Agents 10 and 11 receive only the diff and changed-file names, as their role descriptions require. The parent owns synthesis, deduplication, and the final report; children return candidates or falsification evidence only.
Every mode must end this step with $BASE set — the commit the change is measured against. $BASE is required by Step 2.4 and by every behavioral finding's same-trigger base check; a review that never established it cannot attribute anything.
Capture the PR identifier in $PR after stripping the level token, then fetch metadata, diff, comments, and the base revision:
PR='<PR number or URL from $ARGUMENTS, with any level token removed>'
gh pr view "$PR" --json number,title,body,labels,state
gh pr diff "$PR"
gh pr view "$PR" --comments
BASE=$(gh pr view "$PR" --json baseRefOid --jq .baseRefOid)
--range)When --range=<base>..<head> is given there is no PR, no description, and no
labels. Take the diff from Git instead:
BASE='<base from --range>'
HEAD='<head from --range, or empty for the working tree>'
git diff "$BASE"${HEAD:+"...$HEAD"} --stat
git diff "$BASE"${HEAD:+"...$HEAD"}
git diff "$BASE"${HEAD:+"...$HEAD"} --name-only
With <head> empty the diff includes uncommitted working-tree changes, which is
the normal case when reviewing a fix-pr result before it is pushed. Untracked
files do not appear in git diff — list them with git status --porcelain and
read any that are part of the change, especially new test files, or the coverage
map in Step 2.6 will silently miss them.
In range mode: skip Step 2 entirely (there is no title or description to check) and say so in the report. Every other step runs unchanged — the diff is still the entry point, callsite analysis still walks outward beyond the changed files, and findings are still classified by the same rubric. Restricting the review to the changed files would disable the out-of-diff breakage analysis that is the most valuable part of this skill.
Skipped in --range mode — a local range has no PR metadata. State that it was skipped and continue at Step 2.4.
Check against CLAUDE.md conventions:
type(scope): descriptionFixes #NNN is at the top of the bodyA changed submodule pointer is not automatically a change this PR makes. Before reviewing any content inside a submodule, classify the pointer move. This step is cheap, runs at every level including 0, and gates whether an entire repository's worth of diff is in scope. Skipping it is how a review attributes months of already-released upstream work to the PR in front of it.
List the pointer moves, then for each one resolve the submodule's default branch and test whether the new commit is already on it:
git diff "$BASE...HEAD" --submodule=short | grep -E '^(diff --git|[+-]Subproject commit)'
cd <submodule path>
git fetch origin --quiet
DEF=$(git symbolic-ref --short refs/remotes/origin/HEAD 2>/dev/null | sed 's|^origin/||') # e.g. main, master
git merge-base --is-ancestor <new-sha> "origin/$DEF" && echo UPSTREAM-SYNC || echo OFF-DEFAULT
git branch -r --contains <new-sha>
Classify each pointer move as exactly one of:
Record the verdict per submodule in one line each, and repeat it in the Step 4 report so the scope decision is auditable. Nested submodules are classified independently: an OFF-DEFAULT OSS pointer says nothing about the client pointer nested inside it, which is frequently an UPSTREAM-SYNC in the same PR.
Before launching review agents, produce a structured change surface map. This step is mandatory and must use ripgrep (rg) and fd via Bash — do not reason about callsites from memory. The output of this step is required input for every agent in Step 3.
For every modified or added function, method, trait, struct field, SQL operator/function, or public constant, write:
&self vs &mut self, final vs not), ordering/idempotency guarantees, allocation behavior, thread-safety"Refactored", "cleaned up", "improved", "simplified" are not acceptable deltas. State the actual behavioral difference. If nothing semantically changed, write "no behavioral change" — but only after checking, not as a default.
For every changed symbol that is public, protected, package-private, or exported (pub / pub(crate) in Rust), run rg across the entire repository to find every callsite, implementation, override, or reference outside the diff.
Produce a list grouped by file. For Java, also search for:
getMethod, getDeclaredField, Class.forName)FunctionFactory, OperatorRegistry)For Rust, also search for:
extern "C" boundariesA changed pub/protected/package-private symbol with zero recorded rg calls in the trace is a skill violation. The model is not allowed to assert "this is only used here" without showing the search.
For each changed symbol, walk this checklist and write one line per item, stating before vs after:
?/throws chains propagate themSend/Sync, thread-affinity, JFR/JNI thread attachment requirementsnull and sentinel-NULL (Numbers.LONG_NULL, Numbers.INT_NULL, etc.) are still distinguishedEnd this step with an explicit list of "places this change is visible from but the diff does not touch". This is the highest-priority input for the bug-hunting subagents in Step 3.
The list groups the callsites from 2.5b by execution context: hot data paths, SQL compilation, async runtime, JNI boundary, replication, materialized views, parallel execution workers, etc. Every entry on this list must be reviewed in Step 3.
Run this only when the PR adds or changes test code. It is the test-code counterpart to 2.5b and feeds Agents 12-14. Use real rg/fd searches via Bash — do not reason about helpers from memory.
@Before/@After, helper methods, fixtures, and assertion utilities the new tests could reuse (rg for extends Abstract.*Test, class .*TestUtils, assertMemoryLeak, assertQuery, assertSql, shared protected helpers in the base class). This list is the baseline Agent 13 uses to flag reinvented boilerplate — a "you stamped boilerplate instead of reusing helper X" finding requires X to appear in this inventory.This step runs at EVERY review level, for EVERY PR that touches production code — including (especially) PRs that add or change no test code at all. A PR with zero test changes does not skip test scrutiny; it concentrates it here. At level 0, derive the behavioral-change rows directly from the diff (2.5a is skipped); at level 1+ use the 2.5a semantic deltas.
Build a coverage table with one row per behavioral change: every changed symbol whose delta is not "no behavioral change", broken down further by every new or changed branch, error path, and NULL/boundary case inside it. For each row, record:
rg/fd searches across the test tree (search for the symbol name, the SQL function/operator name, the error message text, the config key). Citing a test without a recorded search command in the trace is a skill violation, same as 2.5b. "Existing tests probably cover it" is banned.COVERED, CRITICAL GAP, MODERATE GAP, ACCEPTED GAP, or EXEMPT.Rows with no effective test are marked UNTESTED, then classified by evidence rather than category:
A bug-fix label or zero test changes triggers this analysis; neither predetermines severity or verdict. Urgency cannot waive an actual defect or an admitted Critical gap. The coverage map is required internal evidence for Agent 5, Agents 12-14, and the Step 4 test gate. Publish only admitted gaps; keep COVERED, ACCEPTED, EXEMPT, and omitted rows private unless the user asks. At level 0, rows may be per-symbol to bound cost, but new error/exception and NULL/boundary paths still get separate rows.
Run this step with the subagent tool using fresh-context reviewer agents. Select only roles whose domain is materially touched, obey the level's discovery cap, and launch those roles as fresh-context, read-only reviewer tasks. Agent count is never evidence and unused roles are skipped.
Every selected agent receives:
The diff plus surface map can be large — write them to a shared file (e.g., under a temp/chain dir) and point each task at it via its reads/task text, rather than pasting the whole payload into every task. Agents 10 and 11 are deliberate exceptions and receive reduced context (see their entries).
producer: unknown; do not invent a deployment or state.unknown.Use the following as a role catalog. Select only the roles allowed by the chosen level and change surface; do not launch the whole catalog.
Agent 1 — Correctness & bugs: NULL handling, edge cases, logic errors, off-by-one, operator precedence, error paths. Cross-reference every changed symbol against its callsite inventory and verify the new behavior is correct at each callsite. When the diff touches the QWP ingress / role-gating path, an in-place switch or failover, a questdb submodule bump that carries client or ingress changes, or the questdb-ent/e2e failover/switch suites, also verify the "Store-and-forward & pool startup invariants" checklist — a change that lets a running SF drainer surface transport errors to the producer, imposes a reconnect time budget on it, or hard-fails it on a transient outage is a Critical (data-loss) finding.
Agent 2 — Concurrency: Race conditions, shared mutable state, missing volatile, lock ordering, thread-safety of data structures. Use the implicit contract list (lock order, thread-affinity) and check every callsite from 2.5b for violations of the new contract.
Agent 3 — Performance & algorithmic optimality: This agent enforces the principle that QuestDB code must use the best known algorithm for each task — not merely "avoid quadratic."
For every new or changed loop, traversal, data structure, or computation:
Algorithm optimality: State the time complexity. Then ask: does a better algorithm exist? O(n) where O(1) is achievable (hash lookup vs linear scan, direct indexing vs search) is a finding. O(n log n) where O(n) suffices is a finding. The bar is not "avoid quadratic" — the bar is "use the best known approach."
Multi-pass vs single-pass: If the code makes multiple passes over the same data (parsing, validation, transformation), determine whether they can be fused into a single pass. Multiple passes over the same input is a finding unless each pass has a structural dependency on the output of the previous one.
Redundant computation: Flag values that are recomputed on every call but could be computed once and cached. Flag repeated lookups of the same key. Flag re-parsing of already-parsed data.
Data structure choice: For each collection or map, ask whether the chosen data structure is optimal. Linear search through a list where a hash set gives O(1) membership test. Sorted array where a heap gives better insert/extract-min. ArrayList where a direct-indexed array suffices.
Unnecessary copies and conversions: Copying data that could be referenced in place. Converting between representations (String ↔ CharSequence, byte[] ↔ DirectByteCharSequence) when the original form would work.
Zero-GC violations: java.util.* collections vs io.questdb.std, string creation/concatenation on hot paths,
capturing lambdas, autoboxing. Even a single GC allocation on a per-row data path is a finding.
SIMD and vectorization: Where the code processes arrays or columns element-by-element, check whether a SIMD/vectorized alternative exists in QuestDB's native layer or could be added.
Compile-time vs data-path: GC allocations during SQL compilation are acceptable. Algorithmic inefficiency during compilation is still a finding — slow compilation means slow first-query latency — but its severity depends on whether the cost scales: a multi-pass parse or O(n^2) plan enumeration is serious; a bounded fixed cost paid once per compilation is a minor-impact finding. Report both, distinguished by item 9.
Magnitude (required on every performance finding): state what the cost multiplies by — rows scanned, values converted, pages read, partitions opened — or, if it does not scale with data, the fixed bound that caps it (column count, config-key count, once per query compilation, once at startup). Say plainly whether the cost is on a data path the user waits for, or off it. The parent uses this to assign severity: scaling-with-data costs block the merge, bounded off-path costs do not. A finding with no magnitude cannot be classified and will be dropped.
For changed symbols now reachable from new contexts (per 2.5d), check whether any of those new contexts is a hot path that amplifies an otherwise-acceptable cost.
Agent 4 — Resource management: Leaks on all code paths (especially errors), try-with-resources, native memory, pool management. Walk every callsite from 2.5b that constructs, owns, or transfers ownership of changed types and verify cleanup on all paths. When the diff adds or changes a native allocation site, also apply the "Per-query memory tracker integration" checklist below: confirm large, unbounded, data-scaled allocators are wired into the per-query MemoryTracker and bounded / process-lived ones are deliberately left out, that malloc and its matching free charge the same tracker, and that newly wired sites have breach / success / leak-loop tests.
Agent 5 — Test review & coverage: Coverage gaps, error path tests, NULL tests, boundary conditions, regression tests, test quality, assertMemoryLeak() usage. Cross-reference 2.5d: every cross-context exposure should have a test that exercises the changed symbol from that context. For each missing cross-context test, add an UNTESTED Step 2.6 row; do not predetermine its severity or publication. Consume the Step 2.6 coverage map: re-verify every claimed test and failure link (read the assertion, don't trust the map), and hunt for behavioral changes the map missed. Then run a mutation spot-check: pick the 3-5 most dangerous changed lines (boundary comparisons, error handling, null checks, off-by-one candidates) and ask, per line, "which test fails if this line is wrong — inverted condition, off-by-one, dropped null check?" When no assertion would catch a mutation, add an UNTESTED map row even if a test nominally executes the line; classify it under Step 2.6 and publish it only after Step 3b admission. Enforce the "SQL test assertions (builder API — strict)" checklist on every added/modified test line: any new assertSql(...)/assertPlanNoLeakCheck(...)/getPlan(...)/TestUtils.assertSql(...) is Critical; any new .returnsOnce(...) on a deterministic (non-RNG, non-time-varying) query is Critical; a lone assertQuery(...) wrapped in assertMemoryLeak(...) is a finding. Test efficacy (whether tests actually exercise the change and could fail) and test-code quality are handled by Agents 12-14 — here, focus only on whether coverage exists for every new or changed path.
Agent 6 — Code quality & standards: Code smell, member ordering, naming conventions, modern Java features, dead code, third-party dependencies. Also check for unclosed LOG statements: QuestDB logging uses a builder pattern (LOG.info().$("msg").$()) and every chain MUST end with .$() or .I$(). A missing close holds a ring buffer slot forever, causing other log producer threads to busy-wait in nextBully(), and the log consumer logging_0 thread cannot progress either. Also watch for .put() instead of .$() in LOG chains — .put() returns Utf16Sink, not LogRecord, breaking the chain. Also flag throw-capable expressions inside LOG chains (LOG.info().$(func()).$()): arguments are evaluated after the ring slot is acquired, so a throwing func() unwinds past the terminator and leaks the slot; the call must be hoisted into a local before the chain starts.
Agent 7 — PR metadata & conventions: Title format, description quality, commit messages, labels, SQL style in tests.
Agent 8 — Rust safety (only if PR contains .rs files): Check for any code that can panic at runtime — unwrap(),
expect(), array indexing without bounds checks, panic!(), unreachable!(), todo!(), integer overflow in release
mode, slice::from_raw_parts with invalid inputs. In mission-critical software a panic in Rust code called via JNI/FFI
will abort the entire JVM process with no recovery. Every fallible operation must use Result/Option with proper
error propagation. Flag every potential panic site.
Agent 9 — Cross-context caller impact: Walk the callsite inventory from 2.5b. For every callsite, fetch the surrounding code (the calling function plus its callers up two levels) and answer:
This agent's output is structured per callsite, not per failure mode. Each callsite gets a verdict: SAFE / CANDIDATE / INSUFFICIENT_EVIDENCE. A CANDIDATE is only an atomic hypothesis for Step 3b; it has no severity yet.
Select this role whenever changed symbols have meaningful out-of-diff callers. It counts toward the level's discovery cap; small diffs to widely used symbols usually justify it.
Agent 10 — Fresh-context adversarial: Dispatched separately from agents 1-9 to escape checklist anchoring. This agent operates under different rules from the rest:
Read and ripgrep (rg/fd via Bash) to explore the repository however it wants.The point is to escape the structured frame, not to create privileged findings. A unique hypothesis is not high signal by itself, and overlap is not corroboration unless it supplies an independent evidence type.
Select this role only when a distinct adversarial pass is warranted; it counts toward the level's discovery cap.
Agent 11 — Adversarial performance: Dispatched separately from Agent 3 to escape checklist anchoring. This agent operates under different rules:
Read and ripgrep (rg/fd via Bash) freely. Read callers to understand actual input sizes and access patterns — an O(n) scan that
runs once at startup is different from one that runs per row.Select this role only when the diff changes loops, algorithms, data structures, allocation behavior, or a plausible hot path; it counts toward the level's discovery cap.
Test-code agents (Agents 12-14) — eligible only when the diff adds or changes test code or claims a bug fix. A production change with no test code is still handled by the Step 2.6 gate. Select only the applicable test roles within the level's discovery cap. Each receives the diff, the change surface map, and the test surface inventory from 2.5e. Tests are not second-class code — apply the same adversarial rigor here as to production.
Agent 12 — Test efficacy & correctness (adversarial): Prove each test actually exercises the production change and could fail if that change regressed.
assertTrue(true), assertFalse(false), assertEquals(x, x), asserting a literal against the same literal, asserting on a value the test itself just hard-coded, or a @Test body with no assertion and no expected=/assertThrows.AssertionError thrown on a spawned thread where it is swallowed instead of failing the test, Thread.sleep-based synchronization that is timing-dependent and flaky.@Before that leaks on a failing path, missing assertMemoryLeak() wrapping.Agent 13 — Test-code quality & maintainability: Review the test as code.
setAccessible(true), getDeclaredField/getDeclaredMethod, Field.set, Class.forName, and similar when a public API, an existing test helper, or a constructor reaches the same state. Reflection in tests is a last resort; if a neater non-reflective path exists, the reflection is a finding — name the alternative.rg/fd for existing helpers, base test classes, and fixtures (e.g., extends Abstract.*Test, TestUtils, *TestUtils, shared assert*, shared @Before) using the 2.5e inventory. If a helper already exists that the new test reimplements inline, flag it and name the helper. Duplicated blocks across new test methods that should be a single helper or a parameterized test are findings.@Test methods, javadoc that merely restates the test name, and stacked/duplicated javadoc ("javadoc piled on javadoc"). Test intent belongs in a precise test name plus, at most, a one-line comment.testFoo that actually tests bar), System.out.println debugging, @Ignore without a referenced ticket, magic numbers >= 5 digits without _ separators.io.questdb.std-over-java.util do NOT apply to test code — do not flag java.util collections or allocations in tests. Member ordering, is/has boolean naming, and SQL style DO apply.Agent 14 — Regression-test efficacy verification: For any PR that claims to fix a bug, verify the regression test would actually fail without the production change. Reason about reverting the production hunk and confirm the new or changed test's assertions would then fail. If the test still passes with the fix reverted, it is not a regression test — flag it. State, per test, which production line the test depends on and what its assertion would do if that line were reverted. Run only when the PR is a fix; skip for pure features or refactors.
Combine agent outputs into a private candidate ledger. Split compound narratives into atomic propositions, deduplicate by proposition plus evidence, and record dependencies. Do not draft report prose, severity, or a suggested fix. A candidate is not a finding.
Use this state machine with no shortcuts:
HYPOTHESIS → FALSIFYING → PROVEN → ADMITTED
Any missing proof, unresolved contradiction, failed reproduction, unsupported producer, or dependence on an omitted premise ends at OMITTED. There is no DOWNGRADED state for an unproven behavioral claim, and “could not disprove” never means PROVEN.
At levels 1-3, launch one fresh-context falsifier per atomic candidate. The falsifier receives only (a) the neutral proposition, (b) target repository, base/head revision identities (commit SHAs, or a captured diff hash for an uncommitted working tree) and relevant file names, and (c) raw evidence/artifact paths. Do not send the discovery narrative, proposed severity, suggested fix, author identity, other agents' votes, or statements that the claim was verified. At level 0, the parent applies the same protocol inline from a blank evidence form before writing any report prose.
The falsifier's first task is to construct the strongest disproof: find a missing state producer, unsupported deployment, impossible version/format pairing, omitted caller or event source, retry, guard, lock, validation, downstream offset, or identical/better base behavior. Only if the candidate survives does it assemble affirmative proof.
A behavioral candidate is admitted only when every field below is backed by cited evidence:
$BASE, or N/A — genuinely new surface with proof.For static findings fully proved by source — compile errors, direct standards violations, or malformed LOG chains — mark producer/head/base/runtime fields N/A — static and cite the complete source proof. For a coverage gap, recorded searches may statically prove only that an effective test is absent; they never make the supported-state producer, reachability, affected population, credible regression consequence, or user impact N/A. A Critical coverage gap must prove those fields under Step 2.6. N/A is forbidden whenever a load-bearing premise concerns runtime shape, reachability, or impact.
Special burdens:
If required execution is impossible, record the validation limitation in the private ledger and omit the candidate from the public findings. Never fall back from failed or unavailable execution to confident prose.
After a candidate satisfies this admission schema, apply the domain-specific checks below:
Read the actual source code at the exact lines cited. Do not rely on the agent's description alone.
Trace the full code path: follow callers, inheritance hierarchies, and runtime types. A method called on a base-class reference may dispatch to a subclass override (e.g., PartitionDescriptor.clear() vs OwnedMemoryPartitionDescriptor.clear()).
Check both sides of JNI/FFI boundaries: if a finding involves Java↔Rust interaction, read both the Java caller and the Rust JNI function. Verify ownership transfer, error propagation, and cleanup on both sides.
For resource leak claims: trace every allocation to its corresponding free/close on ALL code paths (happy path,
error path, finally blocks). Check for polymorphic close()/clear() overrides. Before claiming a leak between
allocation and cleanup registration, verify that the intervening code can actually throw.
For Rust panic claims: verify whether the panic site is actually reachable. Trace control flow backwards — a preceding guard or early return may make it unreachable.
For Rust panic claims via JNI: trace the Java caller to check whether it can actually pass parameters that trigger the panic. If every caller validates inputs before the JNI call, the panic is unreachable — drop it.
For Rust numeric overflow claims: check whether the overflow is reachable at realistic scale. QuestDB handles billions to a few trillion rows, thousands of tables, and thousands of columns — not billions of columns or quintillions of rows. If overflow requires values beyond that scale, drop it.
For performance claims: verify the finding is technically accurate (correct complexity analysis, correct identification of the hot/cold path) and then establish its magnitude. State what the cost multiplies by — rows scanned, values converted, pages read, partitions opened — or, if it does not scale with data, state the fixed bound (column count, config-key count, once per query compilation). A performance claim with neither a multiplier nor a bound is not verified. Do not drop a technically correct finding because today's tables are small — data grows. Do move it from Critical to Moderate when the cost is structurally bounded and off the data path: that is the whole difference between IO amplification that hits every row and a few hundred nanoseconds spent once per SQL compilation. Sub-optimal algorithm choice is always reportable; whether it blocks is decided by the magnitude.
For cross-context findings (Agent 9): re-read the callsite in full, including its callers up two levels, and confirm the broken behavior is reachable from production code paths. Cross-context findings are high-value but also the easiest to overstate — verify carefully.
For test-efficacy candidates (Agents 12, 14): re-read the cited assertion in full context and confirm it can fail for the claimed regression. For “would pass without the fix” claims, use a scratch git worktree (never the primary working tree): run the new test at the reviewed revision, then revert the production hunks (git checkout <base> -- <files>) and run it again. Admission requires green-on-head and red-without-fix artifacts. If the environment cannot build or run the test, omit the candidate and record the validation limitation privately; do not fall back to confident reasoning. The same rule applies to every dynamic Critical candidate: execute the claimed trigger and attach the observed output, or omit it.
For coverage-gap candidates (UNTESTED rows from 2.6): verify the recorded test search and failure-link analysis, then try to falsify the risk with existing indirect assertions, guards, type/compile guarantees, constrained inputs, downstream validation, or operational controls. Establish supported reachability, an affected population, a credible regression mode, and its material consequence before assigning Critical. Evaluate the least fragile meaningful test and concrete alternatives. Reject bare "simple", "urgent", "hard to test", or "covered indirectly" claims; test-feasibility evidence counts only when it names the proposed observation seam, why it is invasive/unstable, and why cheaper stable alternatives do not work. A Critical gap may be counterfactual about whether the code is currently wrong, but never about reachability or impact. Test difficulty does not downgrade an independently proved functional defect.
For test-code-quality findings (Agent 13): confirm a flagged reflective access really has a non-reflective alternative (some QuestDB internals genuinely require reflection in tests) before reporting it. Confirm a "reinvented helper" finding by actually locating the helper with rg and checking its signature fits the test's need.
For "swallowed exception → silent wrong results / leak / corrupt state" claims: a catch block is defensive coding, not evidence that anything throws. Before reporting, name all three of:
(a) the concrete exception type and the exact statement that raises it — quote the throwing line, don't infer it from the presence of a try;
(b) proof that this type is actually caught by the specific catch clause cited — catch (SqlException | CairoException) does NOT catch OutOfMemoryError, IllegalArgumentException, NullPointerException, or any other unlisted Error/RuntimeException. An Error that escapes the catch means the query fails loudly, which inverts the finding;
(c) that the throwing statement is reachable with the arguments the callsite actually passes (constants, pre-reserved capacity, and guarded early returns frequently make it unreachable).
If any of (a)-(c) cannot be established, omit the candidate. Do not relabel the unproven mechanism as a latent invariant or hardening finding; it may remain private supporting analysis only.
Also check for the non-throwing sibling: a void method that silently drops or frees its argument on an early return (if (x) { free(arg); return; }) breaks the same invariant with no exception at all, is usually far more reachable than the throw, and is not fixed by reordering statements around the call. Report that path instead of, or in addition to, the throw.
Verify the conjunction, not just the links. A multi-step candidate ("A publishes early → B can throw → C swallows → D reads stale → wrong rows") is only as true as its weakest step. Identify the single load-bearing step — usually “this supported state can actually occur” — and try to falsify it first. Per-line support for each isolated link does not prove their conjunction. Reading code is not verification when the load-bearing step is a runtime-shape claim — “the plan contains factory X”, “the guard does not fire”, “this branch is taken”, or any claim about races, ordering, retries, restarts, or filesystem state. Such a step requires an attached execution artifact produced or independently re-run by the falsifier at the cited revision. An agent's prose is not an artifact. Votes do not count as corroboration; even independent evidence types must still satisfy every admission field.
Derive a fix only after admission, then verify it compiles and closes the window. A plausible fix is never evidence that the finding is real. Once admitted, check that every referenced variable is in scope and non-null, that ownership transfers do not create a double-free or leak, and that the fix closes every admitted path.
Determine net user impact, then classify. Step 4 assigns severity only after this determination. A behavioral candidate missing it is OMITTED and never reaches Step 4.
(a) Net user impact — answer all five, in order:
A coverage-gap row is counterfactual only about whether an unobserved regression currently exists. Its producer, reachable path, population, credible regression consequence, magnitude, offsets, change risk, and stable-test feasibility must be evidenced under Step 2.6. Coverage absence affects confidence; it does not manufacture impact. Static code-quality findings are assessed directly from changed lines.
A behavioral net determination missing a supported population or same-trigger base delta is not a determination. A coverage-gap Critical missing material reachable impact or test-feasibility evidence is not Critical; classify it Moderate, accept it with evidence, or omit it as the admission schema warrants.
(b) Classify ledger entries as:
Enumerated candidates are admitted per item. Never sample N instances and publish the unverified remainder. Every rendered item needs its own producer/trigger and evidence; otherwise omit that item.
Keep omitted candidates and their disproofs in the private ledger. Do not publish a Downgraded, retracted, rejected, or “possible issue” section, and do not report candidate counts. OMITTED pre-existing/not-attributed is the one exception: an entry whose producer, reachability, and observation are all proved leaves the ledger as a Step 4 adjacent issue draft. OMITTED false and OMITTED unverified entries never do.
Fresh falsifiers may run in parallel, but each receives only its neutral proposition and raw evidence contract. The parent independently checks every returned admission form before writing Step 4.
Review the diff for:
QuestDB is a performance-first database. On data paths the standard is not "avoid regressions" — it is "use the best known algorithm", and a violation blocks the merge. Off data paths (SQL compilation, DDL, startup, metadata operations) the standard is the same, but a bounded violation is a Moderate finding, not a blocker. Every new loop, traversal, data structure choice, and computation must be justified as optimal or near-optimal — and every finding must say which of the two categories it lands in, per the magnitude rule in Step 4.
java.util.* collections (HashMap, ArrayList, etc.) instead of QuestDB's own zero-GC collections in io.questdb.stdint → Integer, long → Long, etc.) allocate silently. Watch for primitives passed to generic methods, stored in java.util.* collections, or returned from methods with wrapper return types.is... / has... prefix.$() or .I$() — a missing close holds a ring buffer slot forever and stalls the logging_0 consumer.put() instead of .$() in LOG chains — .put() returns Utf16Sink, not LogRecord, breaking the chainLOG.info().$(func()).$() leaks the slot if func() throws — hoist into a local first (var a = func(); LOG.info().$(a).$();)QuestDB caps how much native memory a single bounded workload (user SQL query, materialized view refresh, WAL apply batch) may allocate through a per-query MemoryTracker. The tracker is bound on SqlExecutionContext (getMemoryTracker() / setMemoryTracker(...)) and threaded into the tracker-aware Unsafe.malloc / realloc / free / getNativeAllocator(tag, tracker) overloads (and the Rust QdbAllocator). A null tracker degrades to global-RSS-only accounting. Apply this checklist whenever the diff adds or changes a native allocation site, a factory/cursor that owns growing native buffers, or a pooled memory class (Map, RecordChain, RecordArray, sort/tree chains, GroupByAllocator, join-key maps, etc.).
The tracker is for large, potentially unbounded allocations only — that is the whole decision rule. Do not treat "wire everything" as the safe default; over-wiring is itself a finding.
LATEST BY rowid lists and maps, set-operation maps, encoded and top-K ORDER BY ... LIMIT N sort buffers (parallel and single-threaded), secondary / markout-horizon cross-join buffers, window-join and horizon-join aggregation maps, window partition maps and RANGE-frame ring buffers, SAMPLE BY fill, parquet decode buffers. These are the runaway vectors the limit exists to catch. An unbounded site that passes null (or omits the tracker overload entirely) is a coverage-gap candidate: record the runaway query path and classify it through Step 2.6. It is Critical only when that path independently proves the required material reachable impact.string_agg, fixed-size heaps (e.g. the single-column long top-K heap), ROWS-frame window buffers, table reader / writer columns, symbol tables, connection buffers, memory-mapped pages. Wiring one of these is a finding in its own right: it adds two atomic counter updates per malloc/free on both the Java and Rust paths for no protective benefit, and tracker-aware pooled classes give up cross-query backing retention (they free native backing on cursor close and re-allocate on next use), so charging a bounded or retained allocator to the tracker trades away a pool optimization for nothing.For each new or changed allocation site, verify:
null (or vice versa) desyncs the counter and trips the live recordPerQueryMemAlloc balance assert. Trace every free / close path — error paths and toTop() / clear() / cursor-close reuse included — and confirm the identical tracker is used on both ends.*MemoryTrackerTest proving (a) a breach throws the per-query out-of-memory message, (b) an under-limit run succeeds, and (c) a getCursor()-to-close leak loop stays balanced. Record a missing tracker test or an unpinned factory-class routing guard as an UNTESTED Step 2.6 row; classify and publish it only through the normal proportionality and admission gates.Apply this whenever the diff touches the QWP ingress path (upgrade/role
gating, in-place demote / lifecycle switch, connection handling on role
change), replication failover, a questdb submodule bump that carries
client (java-questdb-client) or ingress changes, or tests that drive a
producer through a failover/switch window (e.g. the questdb-ent/e2e
failover/switch suites). These are the CLIENT's store-and-forward
guarantees (the client code lives in the nested questdb/java-questdb-client
submodule); server-side changes and tests in this repo must be reviewed
against them. A violation here is a Critical finding: the whole point of
store-and-forward is that a running producer never loses data and never
hard-fails on a transient outage.
Drainer (steady state — once the pool is running).
Sender producer calls, flush(), the pooled handle). The ONLY
error a running drainer may surface to the caller is SF out of space (the
on-disk / backing buffer is full and can accept no more rows). Flag any other
failure class (connect-refused, DNS, unreachable/black-hole, TLS/cert, auth,
role-reject, upgrade/protocol timeout, reset) that can escape the drainer
onto a producer or borrow call.reconnect_max_duration_millis-style budget, no
deadline, no "give up and latch terminal after N ms". A budget that latches
the sender terminal on a long outage is a Critical violation: it drops a
producer that store-and-forward promised to keep alive. Flag any bounded
reconnect loop, deadlineNanos / while (now < deadline), or terminal
SenderError reachable from the running drainer's reconnect path.BackgroundDrainer) MAY quarantine its slot (.failed sentinel,
human-in-the-loop) on conditions that are terminal by design: auth failure,
a non-421 upgrade reject, and a genuine cluster-wide durable-ack capability
gap that exhausted its documented settle budget (16 consecutive
capability-gap sweeps, or a wall-clock budget anchored at the FIRST
capability-gap error of the episode — whichever is hit first). These are
NOT violations of the no-budget rule above. The settle budget applies ONLY
to consecutive capability-gap attempts: transient classes (role reject,
transport error) must never increment it or burn its wall clock — a
transient state consuming the terminal budget (shared attempt counter,
entry-anchored deadline) IS a Critical violation of this checklist.WRITE_ERROR, INTERNAL_ERROR, UNKNOWN — and any
future status byte) is RETRIABLE: recycle the wire and replay from
ackedFsn+1. It must NEVER drop the batch and NEVER latch a terminal /
quarantine a slot on first sight. Only rejections deterministic under
byte-identical replay (SCHEMA_MISMATCH, PARSE_ERROR, SECURITY_ERROR
on a writable node) may go TERMINAL. A client that advances the ack
watermark past a NACKed frame is silently losing data — Critical. A frame
repeatedly rejected with no ack progress must escalate through the
poison-frame detector (bounded consecutive strikes at the same head FSN),
not through a WS close-code list — close codes carry no policy semantics.
UNKNOWN must fail OPEN (retry), never closed (terminal): a status byte
from a newer server must degrade to retry, not to a dead sender.Pool startup — two modes; the mode decides who sees connectivity errors.
lazy_connect=true: build() MUST succeed with no server present. The
producing Sender must work immediately (writes buffer via SF), and once the
server comes up the read side must also connect and read (reads are deferred,
not disabled).lazy_connect=false (default): build() / the initial connect MUST expose
connectivity problems to the caller — DNS errors, connect-refused /
unreachable, TLS/cert, authentication/authorization, and connect/upgrade
timeouts must all surface as a thrown exception at startup, not be swallowed.Server-side & test application (this repo).
expr::TYPE cast syntax preferred over CAST()assertQuery(...) builder for SQL assertions (see "SQL test assertions" below) and execute() for DDLQuestDB has migrated SQL test assertions to the fluent AbstractCairoTest.assertQuery(query) builder. These rules are blocking — treat violations as Critical findings, not style nits. Apply them to every test line the diff adds or modifies (a residual pattern that the PR merely moves or reindents is not a finding; a newly written or edited one is).
assertSql(...) has been REMOVED — there is no query-result assertSql(...)/TestUtils.assertSql(...) to fall back to. Any new or changed test code that asserts query results with assertSql(...) / TestUtils.assertSql(...) is a Critical finding (it will not even compile against the current base class); the author must use the builder instead:
assertQuery(sql).returns(expected) — chain .timestamp(...), .expectSize(), .noRandomAccess(), .sizeMayVary(), .ddl(...), .mutateWith(...), .withEngine(...), .withContext(...) as needed.assertQuery(sql).assertsPlan(plan) / .assertsPlanContaining(...) / .assertsPlanNotContaining(...), or fold the plan into a data assertion via .withPlan(...) / .withPlanContaining(...) / .withPlanNotContaining(...).
Do not accept "the surrounding file already uses assertSql" — there is no such helper anymore, so the diff's lines must use the new API. Flag assertPlanNoLeakCheck(...), getPlan(...), assertPlanDoesNotContain(...), and direct TestUtils.assertSql(...) in new/changed test code for the same reason. The one assertSql that legitimately survives is the live-ServerMain wrapper TestServerMain.assertSql(sql, expected): it is a convenience for the running-server context, internally drives the builder via returnsOnce() (single pass, because a live server's state mutates between reads), and is NOT the banned query-result helper — do not flag it..returnsOnce(...) is a correctness smell — flag every newly added use. returnsOnce runs the query through a SINGLE cursor pass and deliberately SKIPS the second read, the calculateSize() pass, the variable-column check, and the factory-property assertions (supportsRandomAccess, expectSize) that .returns(...) performs. Those skipped checks catch real bugs: cursors that don't reset correctly on toTop(), size() that disagrees between passes, random-access records that return wrong values via recordAt(). returnsOnce is only justified when the query's output is genuinely unstable across two reads with no underlying data change — e.g. an unseeded rnd_* in the projection, now()/sysdate()/systimestamp()-style time-varying output, or inherently non-deterministic row order. For a .returnsOnce(...) on a deterministic query this is a Critical finding: demand .returns(...). Require the author to state why the query is unstable; "it was simpler" is not a reason — the shortcut leaves real bugs untested.
Anti-pattern: a lone assertQuery(...) wrapped in assertMemoryLeak(() -> { ... }). The builder runs its OWN memory-leak check by default (it wraps internally unless .noLeakCheck() is set). When an assertMemoryLeak(...) lambda's only meaningful statement is a single assertQuery(...) chain, the outer wrapper is redundant and almost always forces a .noLeakCheck() on the builder — which disables the builder's leak check and replaces it with a hand-rolled one, defeating the point. Flag it: drop the assertMemoryLeak wrapper and the .noLeakCheck(), letting the builder leak-check itself. The wrapper is only legitimate when the lambda genuinely holds multiple statements (DDL + inserts + several assertions) that must share one leak-check scope; a single builder call does not.
This check decides on two rg searches in this repo — run them instead of reasoning about them.
SecurityContext.authorize*() call on its execution path. Cite the callsite — the AlterOperation.apply() dispatch for ALTER, the op or compiler class for everything else. Absence is a proven finding, not a speculative one: the evidence is the search that finds the new operation and the search that finds no authorize* call covering it. Classify it with the standard rubric — a state-mutating operation no security context can refuse is a privilege bypass, which the severity table already lists as Critical.authorize*() method must be implemented wherever it is abstract: AllowAllSecurityContext and ReadOnlySecurityContext (DenyAllSecurityContext extends the latter), plus any test SecurityContext implementations the compiler does not already catch. If the PR instead adds the method with a permissive default body, every implementation that does not override it — including enterprise ones this checkout cannot see — silently grants the permission. The interface does use default deliberately in places, so ask for the rationale; treat a missing one as the finding, not the default itself.Permission.java registration, PermissionParser GRANT/REVOKE parsing, EntSecurityContextBase / AdminSecurityContext implementations, and replica deniedOnReplica() gating all live in a separate repository and cannot be verified from this checkout. Note them once as an enterprise follow-up when the PR adds an authorize*() method; do not raise them as findings and do not let them affect the verdict.rg -l Fuzz) covering the changed surface. If one exists and was neither extended nor mentioned as run against the change, add an UNTESTED Step 2.6 row; classify and publish it only through the normal proportionality and admission gates.UNTESTED Step 2.6 row; classify and publish it only through the normal proportionality and admission gates.assertMemoryLeak() for anything that allocates native memory.rg) and fd via Bash to find existing test files for the changed classes and verify they cover the new behavior.assertTrue(true), assertFalse(false), assertEquals(x, x), asserting a literal against the same literal, or a @Test body with no assertion and no expected=/assertThrows.setAccessible(true), getDeclaredField/getDeclaredMethod, Field.set, Class.forName when a public API, existing helper, or constructor would reach the same state. Name the non-reflective path.@Test methods, no javadoc that restates the test name, no stacked/duplicated javadoc. Prefer a precise test name and at most a one-line comment.io.questdb.std-over-java.util rules do NOT apply to tests — do not flag them there. Member ordering, is/has naming, and SQL style DO apply.System.out.println, no commented-out code, no @Ignore without a referenced ticket.TODO, FIXME, HACK, XXX, and WORKAROUND comments. For each one found:
Present only ADMITTED findings. Omitted candidates, disproofs, retractions, agent counts, candidate counts, and the private ledger never appear in the public review. Do not publish a hypothesis and retract it later; finish falsification first. It is valid to report no findings. The single exception is the Adjacent findings section below, which carries proved pre-existing bugs as issue drafts — not findings against this PR, and weightless in every gate.
Proportionality. Keep the report actionable in one sitting. If a normal-sized PR yields more than about seven total findings, re-run the admission gate on every item and remove dependent, duplicate, not-attributed, and low-value prose. Removing a not-attributed item means moving it to Adjacent findings, not discarding it. Review depth is demonstrated by evidence, not report length.
Every finding — at every severity — opens with three one-line summaries, before any prose:
Write these lines last from the completed admission form, never first from a hunch. Then give only the minimal producer → path → symptom trace, base comparison, and suggested fix.
Problem: Symbol column read twice per scanned row.
Net impact: ~2x column IO on every filtered scan.
Evidence: benchmark.sh output at abc123; base 8ms, head 16ms.
Problem: WAL segment leaks a file descriptor on the error path.
Net impact: Ingestion stalls after ~1k failed commits.
Evidence: WalLeakTest red at abc123, green at base def456.
Structure as:
Severity is a function of what the user loses, not of which checklist the finding came from. Classify by the worst user-visible consequence on a reachable path. Do not classify up "to be safe": an inflated Critical costs exactly what a real one costs and teaches the author to skim the report.
"The user" means a QuestDB database user or a production operator — someone running queries, ingesting data, or operating a deployment. It does not mean a QuestDB developer, a CI job, or the release process. A finding whose only affected population is the team — a slower build, a broken local setup, an awkward merge — is never Critical, whatever its symptom. Developer-experience problems are Moderate at most, and most are Minor.
The Critical test — name the symptom. A finding is Critical only if you can complete this sentence with something a user, operator, or on-call engineer would actually observe: "Because of this, the user sees ___." The valid completions are:
Every completion needs a trigger. A symptom sentence must name the concrete query shape, ingest pattern, API call, config value, or operation sequence a user/operator can run: "user does X → sees Y". For a coverage gap use "user does X; if this changed path regressed as Y, the user would see Z". "Could theoretically return wrong results" is not evidence.
If a behavioral candidate cannot name and execute a supported trigger with one of the consequences above, omit it; do not preserve the mechanism as Moderate. Concrete static standards, maintainability, and coverage findings may still be Moderate or Minor when fully established directly from changed source.
Magnitude rule for performance and IO. Cost blocks only when it is user-observable. Ask two questions: does the cost scale with data (per row, per value, per page, per partition, per scanned block), and is it on a path the user waits for or repeats?
Config-divergence rule. "The same statement is accepted under config A and rejected under config B" (a flag-dependent plan shape changing what a guard sees, a validation only some execution mode runs) is a finding in its own right — an inconsistency an operator can observe across nodes — and is classified on the consequence of the divergence itself. It does not inherit the severity of the worst case reachable through the more permissive configuration; that worst case is a separate finding that must pass the symptom test, the trigger requirement, and the base-behavior check on its own.
Out of scope — these are not findings. Three classes get reported constantly and are worth nothing. Drop them before they reach the report:
Moderate. Admitted, attributable defects with bounded or developer-facing impact: a concrete changed-line standards violation, proved weak test, missing internal-path coverage, documentation defect, or bounded off-data-path cost. An unreachable runtime theory, unchanged residual hardening opportunity, or proposition that only supports another candidate is not Moderate; omit it.
Minor. Cosmetics: member ordering, naming, formatting, comment wording, import order.
Do not inflate and do not deflate. Filing a real user-visible defect as Moderate is a review failure; so is filing a bounded compile-time micro-cost as Critical. Where two readings are defensible, pick the one you can evidence.
Blocking issues introduced or exposed by this PR, ordered worst user impact first. Each must include:
N/A — new surface and prove that base cannot express the trigger. Base rejection is the absence of a wrong-result defect, not a worse defect outcome.Pre-existing/not-attributed observations are never Critical; a fully proved one belongs under Adjacent findings instead.
Non-blocking admitted issues worth fixing. Every item must still include the three summary lines and its decisive evidence. Dynamic behavioral speculation is not allowed here.
Concrete cosmetics on changed lines. Non-blocking, optional.
Bugs that already exist on the merge base, found in code this review visited (changed files, callers from the callsite inventory, cross-context exposures), which this PR does not introduce, break, or worsen. They are not findings against this PR: they never appear under Critical/Moderate/Minor, never influence the verdict, and are never proposed as changes to this PR. Discarding them instead is pure waste — the investigation is already paid for, and nobody re-finds them later.
They are held to the same evidence bar as a published finding. An adjacent draft comes only from a candidate that reached OMITTED pre-existing/not-attributed with its producer, reachability, and observation proved. A candidate that ended OMITTED false or OMITTED unverified stays in the private ledger; this section is not a home for speculation that failed falsification.
Report each as a ready-to-file issue draft, so it can move to GitHub without re-investigation:
Offer to file them; do not file anything without being asked. Their count and severity sit outside the finding-proportionality budget and outside every gate in the Summary. If one is severe enough that shipping this PR without it is genuinely unsafe — because this PR moves code onto a path where the pre-existing bug now fires — then it is not adjacent: it is out-of-diff-breakage, it belongs under Critical, and you state that argument explicitly.
State the test-gate result and the number of admitted coverage gaps only. Render admitted gap rows with their recorded search and failure link. Do not expose counts for omitted candidates or private UNTESTED rows; keep the full Step 2.6 matrix private unless the user asks to see it.
Verdict, exactly one of:
Correctness gate (hard rule): the verdict cannot be "approve" while any ADMITTED Critical finding remains open, including an admitted Critical coverage gap. Omitted hypotheses never affect the verdict.
Before finalizing, rerun the admission audit from evidence fields rather than from report prose:
If any field fails, omit the candidate and rerun the verdict. If the admitted Critical list is empty and the test gate passes, approve plainly; zero findings is expected for correct changes.
Test gate (hard rule): the gate fails only while an ADMITTED Critical coverage gap remains open. Zero test changes, a bug-fix label, or missing regression coverage triggers the Step 2.6 analysis but never automatically forces request changes. Moderate gaps may accompany approve with comments; accepted gaps do not affect the verdict. Any independently admitted functional Critical still fails the correctness gate regardless of test effort or urgency.
State the test-gate result and admitted coverage-gap count. Do not publish total UNTESTED or omitted-candidate counts from the private map.
Highlight any regressions or tradeoffs
Never make the verdict conditional on splitting the PR. Pre-existing and not-attributed observations never affect the verdict, whether they were omitted or delivered as adjacent issue drafts.
Do not state agent counts, candidate counts, rejected/false-positive counts, or retraction history.
State the Step 2.4 submodule provenance verdicts, one line per changed pointer (e.g., "questdb: OFF-DEFAULT — in scope; java-questdb-client: UPSTREAM-SYNC — out of scope"). If a pointer moved and no verdict is stated, the scope of the review is unknown and the report is incomplete.
State only the admitted split: in-diff / out-of-diff-breakage. At levels 0-1, describe the limited callsite analysis rather than implying a clean bill of health.
State the severity distribution. If the report is long or severity-heavy, re-run admission; do not compensate by preserving weak items at a lower severity.