Menu
Review a GitHub pull request against QuestDB coding standards
Installer avec Codex ou Claude Copiez ce prompt, collez-le dans Codex, Claude ou un autre assistant, puis laissez-le vérifier la page du skill et l'installer pour vous.
Basé sur la classification professionnelle SOC
| name | review-pr |
| description | Review a GitHub pull request against QuestDB coding standards |
Review the pull request $ARGUMENTS.
You are a senior QuestDB engineer performing a blocking code review. QuestDB is mission-critical software deployed on spacecraft — bugs can cause data loss or system failures that cannot be patched after deployment. There is zero tolerance for correctness issues, resource leaks, or undefined behavior. Be critical, thorough, and opinionated. Your job is to catch problems before they ship, not to be nice.
-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 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.6, 4. Skip Step 2.5. Skip Step 3 — no agent spawn; review the diff inline in the main loop, using Read/Grep on demand to resolve ambiguities. Skip Step 3b — verify each finding inline as you write it. Single-pass review covering correctness, NULL handling, algorithmic optimality, tests, and QuestDB standards on the diff itself. The performance checklist (including algorithm optimality) is mandatory at every level. When the diff touches test code, also apply the test-efficacy and test-code-quality anti-pattern checks inline (vacuous assertions, reflection overuse, reinvented helpers, javadoc bloat). Step 2.6 (test coverage map) is mandatory here as at every level — build it inline before writing findings; derive the behavioral-change rows directly from the diff since 2.5a is skipped. |
| 1 | Adds Step 2.5a (semantic delta only — skip 2.5b/2.5c/2.5d) plus Step 2.5e when test code is present. In Step 3, launch Agent 1 (correctness), Agent 3 (performance), Agent 5 (tests), Agent 6 (code quality), and — when the diff touches test code — Agent 12 (test efficacy) and Agent 13 (test-code quality) in parallel. Skip all other agents. Skip Step 3b — verify findings inline as you draft the report. |
| 2 | Full Step 2.5 (including 2.5e when test code is present), but in 2.5b restrict the callsite inventory to public/protected symbols (skip package-private and pub(crate)). In Step 3, launch Agents 1-8 (Agent 8 only if .rs files are present), plus Agent 11 (adversarial performance), plus Agents 12 and 13 when the diff touches test code. Skip Agent 9 (cross-context), Agent 10 (adversarial fresh-context), and Agent 14 (regression-test efficacy verification). Step 3b uses a single batched verification agent for all findings instead of one per finding. |
| 3 | Every step below as written, all 14 agents, per-finding verification. The full mission-critical pass. |
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.
Capture the PR identifier in $PR (the part of $ARGUMENTS left after stripping the level token), then fetch metadata, diff, and review comments in a single bash call so $PR is in scope for all three gh invocations:
PR='<PR number or URL from $ARGUMENTS, with any --level=N / -lN / bare-digit level token removed>'
gh pr view "$PR" --json number,title,body,labels,state
gh pr diff "$PR"
gh pr view "$PR" --comments
Check against CLAUDE.md conventions:
type(scope): descriptionfix(sql): fix ... not fix(sql): DECIMAL ...)Fixes #NNN is at the top of the bodyBefore launching review agents, produce a structured change surface map. This step is mandatory and must use Grep/Glob — 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 Grep 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 Grep 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 agents 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 Grep/Glob searches — do not reason about helpers from memory.
@Before/@After, helper methods, fixtures, and assertion utilities the new tests could reuse (Grep 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:
assertMemoryLeak() (if native memory is allocated). An N-A mark requires a one-line reason ("no shared state → concurrency N/A"); an unexplained N-A counts as uncovered — "not applicable" is the easiest place to hide a gap.Rows with no test, or with a test that has no plausible failure link, are marked UNTESTED and carry a default severity:
The coverage map is required input for Agent 5, Agents 12-14, and the Step 4 verdict. Every UNTESTED row must surface as a finding in the Step 4 report under its severity section, and the full map must be rendered in the report's "Coverage map" section — a map that exists only as summary totals is unauditable and does not count. At level 0, rows may be kept per-symbol instead of per-branch to bound cost, but new error/exception paths and NULL/boundary handling introduced by the change must still get their own rows.
Every agent receives:
The fresh-context adversarial agents (Agent 10, Agent 11) are the exception — they must NOT receive the change surface map, the test coverage map, or checklists; give them only the diff and changed-file names, per their instructions below. The parent session owns synthesis, deduplication, and the final report — children only return findings.
FooReader.java the new behavior of Bar.x() causes Y" is worth more than five findings inside the diff.Launch the following agents in parallel.
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 java-questdb-client submodule bump, or store-and-forward test 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:
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.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. Missing tests for cross-context callsites is a high-priority finding. 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?" A dangerous line no assertion would catch is an UNTESTED finding even if a test nominally executes it. 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.
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 / BROKEN / NEEDS VERIFICATION. Every BROKEN entry is a P0 finding regardless of whether the file is in the diff.
This agent is not optional even when the diff is small. Small diffs to widely-used symbols have the largest blast radius.
Agent 10 — Fresh-context adversarial: Dispatched separately from agents 1-9 to escape checklist anchoring. This agent operates under different rules from the rest:
The point of this agent is to surface bugs the structured agents cannot see because they are reasoning inside the same frame. A finding here that none of agents 1-9 produced is high signal — it means the structured review missed it. A finding here that overlaps with agents 1-9 is corroboration.
Run this agent in parallel with agents 1-9. It is mandatory regardless of diff size.
Agent 11 — Adversarial performance: Dispatched separately from Agent 3 to escape checklist anchoring. This agent operates under different rules:
Run this agent in parallel with agents 1-10. It is mandatory regardless of diff size.
Test-code agents (Agents 12-14) — run only when the diff adds or changes test code. When a PR changes production behavior but adds or changes NO test code, do not treat this gate as letting the PR off the hook — the Step 2.6 coverage map already classifies every uncovered behavioral change, and a fix PR with no regression test is an automatic Critical finding without any agent run. Launch them in the same parallel batch as agents 1-11. Each receives the diff, the change surface map, and the test surface inventory from 2.5e. They are the test-code counterparts to the production agents: Agent 12 mirrors Agent 1 (correctness), Agent 13 mirrors Agent 6 (code quality), and Agent 14 verifies regression-test efficacy. 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 (or poll-with-timeout) synchronization that is timing-dependent and flaky. Sleep-based coordination is the classic whitebox-flake trap: demand deterministic coordination via a latch/barrier (SOCountDownLatch, CountDownLatch, CyclicBarrier) or a test callback/Runnable injected at the exact coordination point so the test blocks until the awaited state is reached rather than guessing a delay. This deterministic-hook requirement is the one sanctioned reason a test may reach into internals (see Agent 13's blackbox-over-whitebox bullet).@Before that leaks on a failing path, missing assertMemoryLeak() wrapping.Agent 13 — Test-code quality & maintainability: Review the test as code.
assertQuery builder, public/protected API return values, on-disk / wire / log output — over tests that couple to implementation detail. Flag whitebox tests that reach into internals: reflection onto private state, assertions on internal counters / field values / intermediate collections that a correct refactor would legitimately change, mocking an internal collaborator only to assert it was called, or depending on a specific number of internal iterations / allocations / passes. A passing whitebox test proves the implementation is unchanged, not that the behavior is correct; these tests decay into flakes and break on every refactor, so they are a maintenance liability, not an asset. When the same behavior is observable through the public surface, the finding is: rewrite as blackbox and assert on the named observable signal. The one sanctioned exception is thread coordination. A test that must order events across threads MUST NOT do it with Thread.sleep / wall-clock timing — that is the whitebox-flake trap and is itself a finding (see Agent 12). It must make the coordination deterministic via an explicit hook: a latch / barrier (SOCountDownLatch, CountDownLatch, CyclicBarrier), or a test callback / Runnable injected at the exact coordination point so the test blocks until the awaited state is reached instead of guessing at a delay. A test that reaches into internals ONLY to install such a deterministic hook (a callback, a latch trip) is acceptable — it is buying determinism, not asserting on implementation detail. Demand the callback/latch mechanism in place of any sleep-based or poll-with-timeout coordination.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. Reflection used to observe or assert private state is also a whitebox smell (see the blackbox bullet above); reflection used only to install a deterministic coordination hook is the sanctioned exception.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 all agent findings into a single deduplicated draft report. Do NOT present this draft to the user yet — it goes straight into verification.
The parallel review agents work from the diff plus the change surface map and frequently produce false positives — especially around memory ownership, polymorphic dispatch, Rust control-flow guarantees, and JNI lifecycle conventions. Every finding MUST be verified before it is reported.
For each finding in the draft report:
PartitionDescriptor.clear() vs OwnedMemoryPartitionDescriptor.clear()).close()/clear() overrides. Before claiming a leak between
allocation and cleanup registration, verify that the intervening code can actually throw.git worktree (never the primary working tree), run the new test on the PR branch (must pass), then revert the production hunks (git checkout <base> -- <files>) and run it again (must fail). A regression test that passes with the fix reverted is Critical — attach the run output as evidence. If the environment cannot build or run tests, state so explicitly and fall back to reasoning; remove the worktree afterwards.Move false positives to a separate "Downgraded" section at the end of the report. For each, give a one-line explanation of why it was dismissed. This lets the PR author verify the reasoning and catch verification mistakes.
Launch verification agents in parallel where findings are independent. Each verification agent should read surrounding source files, not just the diff.
Review the diff for:
QuestDB is a performance-first database. The standard is not "avoid regressions" — it is "use the best known algorithm." Every new loop, traversal, data structure choice, and computation must be justified as optimal or near-optimal.
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... prefixQuestDB 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 Critical coverage gap; flag it with the runaway query path that reaches it.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. Missing tracker tests for a newly wired site is a high-priority finding; so is a factory-class routing guard that no longer pins the test to the intended plan.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 java-questdb-client submodule bump, or
tests that drive a producer through a failover/switch window. These are the
CLIENT's store-and-forward guarantees (the client code lives in the
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.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) (and the enterprise EntGriffinServerMain.assertSql(...)): 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.
SecurityContext.authorize*() call in the execution path.authorize* methods in SecurityContext and whether all enterprise SecurityContext implementations (EntSecurityContextBase, AdminSecurityContext, AbstractReplicaSecurityContext, and test mocks) are updated.Permission.java (constant, name maps, and included in TABLE_PERMISSIONS/ALL_PERMISSIONS as appropriate).PermissionParser must be able to parse GRANT/REVOKE for the new permission name — especially if the name contains SQL keywords like ON, TO, or FROM that could conflict with parser grammar.deniedOnReplica()).Fuzz) covering the changed surface. If one exists and was neither extended nor mentioned as run against the change, flag it.assertMemoryLeak() for anything that allocates native memory.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.assertQuery, public/protected API, on-disk/wire/log output) over tests that couple to implementation detail. Whitebox tests — reflection onto private state, assertions on internal counters / intermediate collections / call sequences that a correct refactor would change, mocking internal collaborators to assert they were called, depending on a specific internal iteration/allocation count — decay into flakes and break on refactors; they are a liability, and a green whitebox test only proves the implementation is unchanged, not that the behavior is correct. When the behavior is observable through the public surface, flag the test and name the observable signal it should assert instead. Exception — thread coordination only: synchronizing threads with Thread.sleep or a poll-with-timeout is the flake trap and is a finding; require deterministic coordination via a latch/barrier (SOCountDownLatch, CountDownLatch, CyclicBarrier) or a test callback/Runnable injected at the coordination point so the test blocks until the awaited state is reached. Reaching into internals solely to install such a deterministic hook is acceptable.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. Reflection used to read/assert private state is also a whitebox smell (see above); reflection used only to install a deterministic coordination hook is the sanctioned exception.@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 verified findings (false positives are excluded). Structure as:
Issues that must be fixed before merge. Each must include:
Issues worth addressing but not blocking.
Style nits and suggestions.
Findings from the initial review that were dismissed after source code verification. For each, state:
Render the full Step 2.6 coverage map: one row per behavioral change with its test, failure link, dimension marks (including justified N-As), and TESTED / UNTESTED / EXEMPT verdict. EXEMPT rows must show the verified no-behavioral-change delta. This section is mandatory whenever the PR touches production code — it is the audit trail for the test gate below; a review without it is incomplete.
Validate and fix pasted QuestDB pull-request review findings one at a time, with claim verification, a failing regression test where feasible, robust and performant implementation, testing, and an independent review/fix loop. Use when the user pastes Critical, Moderate, or other actionable PR review items.
Validate and fix pasted QuestDB pull-request review findings one at a time, with claim verification, a failing regression test where feasible, robust and performant implementation, testing, and an independent review/fix loop. Use when Codex is asked to fix Critical, Moderate, or other actionable PR review items.
Validate and fix a pasted list of QuestDB pull-request review findings. Use when the user pastes Critical, Moderate, or other actionable review items and wants each claim reproduced where feasible, fixed for correctness and performance, tested, and independently reviewed in a serial review/fix loop.
Review GitHub pull requests against QuestDB coding standards using gh, local source inspection, review levels 0-3, and verified Critical, Moderate, Minor, and Downgraded findings. Use when Codex is asked to review a PR number or URL, run review-pr, inspect a GitHub PR, validate PR metadata, analyze diffs and callsites, or perform a QuestDB blocking code review.
Review a GitHub pull request against QuestDB coding standards. Use when asked to review a PR, given a PR number or URL, optionally with a review level 0..3. Performs an adversarial, blocking, mission-critical code review.
Approve the pull request for the currently checked-out branch. Posts the review from this conversation as an approving review and adds the "QUEUED FOR MERGE" label. Use after review-pr when you decide to approve.