Menu
Review a GitHub pull request against QuestDB client library coding standards
Install with Codex or Claude Copy this prompt, paste it into Codex, Claude, or another assistant, and let it review the skill page and install it for you.
Based on SOC occupation classification
| name | review-pr |
| description | Review a GitHub pull request against QuestDB client library coding standards |
Review the pull request $ARGUMENTS.
You are a senior QuestDB engineer performing a blocking code review. The QuestDB client library is mission-critical software — bugs can cause data loss, silent data corruption, or crashes in customer applications across Rust, C, C++, Python, and downstream language bindings. There is zero tolerance for correctness issues, resource leaks, undefined behavior, or unsound FFI. Be critical, thorough, and opinionated. Your job is to catch problems before they ship, not to be nice.
usize::MAX, NUL-byte injection through an API that already rejects it, panics behind validation guards), it is not a real finding — drop it. Focus on bugs that real workloads can trigger, not theoretical edge cases.debug_assert! and assert! are valid guards for invariants that indicate library-internal bugs. Do NOT flag them as insufficient — they are the preferred mechanism for conditions that should never occur given the FFI contracts. Only flag an assert!/debug_assert! if the condition can plausibly be triggered by callers honoring the documented contract.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 (FFI ABI changes, ILP wire format, authentication/TLS, public C/C++ headers, new unsafe blocks, sender or buffer state-machine changes).
| Level | What runs |
|---|---|
| 0 (default) | Steps 1, 2, 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, FFI safety, panics, tests, and coding standards on the diff itself. |
| 1 | Adds Step 2.5a (semantic delta only — skip 2.5b/2.5c/2.5d). In Step 3, launch only Agent 1 (correctness), Agent 2 (Rust safety), and Agent 7 (tests) in parallel. Skip all other agents. Skip Step 3b — verify findings inline as you draft the report. |
| 2 | Full Step 2.5, but in 2.5b restrict the callsite inventory to pub/pub(crate) Rust symbols plus every #[no_mangle]/extern "C" export. In Step 3, launch Agents 1-8. Skip Agent 9 (cross-context) and Agent 10 (adversarial fresh-context). Step 3b uses a single batched verification agent for all findings instead of one per finding. |
| 3 | Every step below as written, all 10 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 #141 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:
Fixes #NNN or a link to the issue is presentBefore 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, public constant, or FFI export, write:
questdb::ingress::Sender::flush, line_sender_buffer_column_f64)&self vs &mut self), ordering/idempotency guarantees, allocation behavior, thread-safety, FFI ownership semantics (caller-owned vs callee-owned)"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 pub, pub(crate), #[no_mangle], extern "C", exported in the C header, or referenced from the C++ wrapper, run Grep across the entire repository to find every callsite, implementation, override, or reference outside the diff.
Produce a list grouped by file. Search at minimum:
grep -r 'symbol_name' questdb-rs/ questdb-rs-ffi/grep -rn 'impl.*TraitName' questdb-rs/grep -rn 'symbol_name' include/questdb/ingress/grep -rn 'symbol_name' include/questdb/ingress/*.hppgrep -rn 'symbol_name' cpp_test/grep -rn 'symbol_name' system_test/grep -rn 'symbol_name' examples/grep -rn 'symbol_name' questdb-rs/src/A changed pub/pub(crate)/#[no_mangle] 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:
Result::Err variants returned and which ? chains propagate themSend / Sync bounds and whether the value can cross threadsconst-ness, length parameters, NUL-termination expectationsEnd 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.
Group the callsites from 2.5b by execution context. Typical contexts in this codebase:
extern "C" function in questdb-rs-ffi/src/lib.rs that calls (transitively) into the changed codeBuffer::column_*, Buffer::symbol, Buffer::at* and their callersSender::flush*, HTTP/TCP/ILP transportsSenderBuilder, conf string parsers in questdb-rs/src/ingress/conf.rsquestdb-rs/src/ingress/tls.rs, basic auth, token authinclude/questdb/ingress/*.hppcpp_test/test_line_sender.cppsystem_test/test.py, system_test/fixture.py, ctypes shims in system_test/questdb_line_sender.pyexamples/ programs that link the C/C++ APIEvery entry on this list must be reviewed in Step 3.
This sub-step runs at every level, including levels 0 and 1 where the rest of Step 2.5 is skipped. A single Cargo.toml setting can flip the panic-safety story for the entire crate; agents must reason from the actual profile, not from defaults.
Read questdb-rs/Cargo.toml and questdb-rs-ffi/Cargo.toml and record, with file:line citations:
[profile.release], [profile.dev]). If panic = "abort" in either, every catch_unwind in that crate is a no-op for that profile and every reachable panic is a process abort. Agents 2, 3, and 4 (and the level-0 inline review) must not credit catch_unwind as a panic guard under panic = "abort". The only acceptable defense under abort-panic is proving no panic path exists.overflow-checks = false in release (the default), integer overflow wraps silently in release builds instead of panicking — bugs that look like panics in test builds disappear into wrong values in production. State which mode applies.[profile.*.package.*] overrides if present — a per-dependency profile can reintroduce unwinding for one crate even when the workspace defaults to abort.#[global_allocator] if defined anywhere in the workspace. A custom allocator changes the OOM behavior (some abort, some unwind, some return null).A review without this section is incomplete. State the panic mode in one line at the top of every Step 3 agent prompt so the agent reasons from the right premise.
Every agent receives:
Cargo.toml (panic strategy, allocator, feature defaults, MSRV, profile overrides), a new #[global_allocator], or a new panic_handler retroactively changes the safety story for every existing function in the crate — not just the diff. When Cargo.toml, build scripts, or workspace-level config files appear in the diff, the review covers the panic/allocation/overflow contract of the entire affected crate, not just the touched lines. The same applies when 2.5e records a profile fact (e.g. panic = "abort") that invalidates existing safety patterns in untouched code.test_line_sender.cpp the new behavior of line_sender_buffer_column_f64 causes Y" is worth more than five findings inside the diff.Launch the following agents in parallel.
Agent 1 — Correctness & bugs: NULL/None handling, edge cases, logic errors, off-by-one, operator precedence, error paths, integer overflow/truncation, buffer length math. Cross-reference every changed symbol against its callsite inventory and verify the new behavior is correct at each callsite.
Agent 2 — Rust safety, panics, and crash surface: In a client library, anything that aborts the Rust side aborts the host process with no recovery. Flag every reachable instance of:
unwrap(), expect(), array indexing without bounds checks, panic!(), unreachable!(), todo!(), integer overflow in release mode, slice::from_raw_parts with invalid inputs, Mutex::lock().unwrap() on a poisoned mutex.std::process::abort(), libc::abort(), std::intrinsics::abort().Vec::with_capacity, Box::new, String::reserve, or similar sized by an untrusted length parameter. Rust's default allocator aborts on OOM — a caller passing a 100 GB length parameter is a crash, not a recoverable error. Check whether the FFI function validates length bounds before allocating.Drop impls, deeply nested data structures decoded from untrusted input.Drop impl while another panic is unwinding aborts the process. Flag any Drop impl that can panic (calls unwrap, indexes, allocates).unsafe: verify safety invariants are documented and upheld, check pointer validity, aliasing rules, lifetime correctness, dangling pointers, use-after-free, double-free, data races.include/questdb/ingress/*.hpp) is reachable from pure-C callers via inline forwarders. Any path where the wrapper can throw (std::bad_alloc, std::system_error, user-defined throw) and reach a C caller is undefined behavior. Verify wrapper functions called from C are noexcept or only invoked from C++ contexts.MSG_NOSIGNAL or mask SIGPIPE.Panic strategy is the foundation. Before reasoning about any panic guard, look up the panic setting from Step 2.5e:
panic = "abort", catch_unwind is a no-op — it cannot catch anything because nothing unwinds. Every reachable panic is a process abort regardless of where the catch_unwind is placed. The only acceptable defense is proving no panic path exists: front-load every length check, replace unwrap/expect/indexing on wire-derived or caller-supplied values with Result-returning equivalents, validate before allocating, use checked_* arithmetic. A catch_unwind wrapper in this mode is misleading documentation, not a safety net — flag it if it gives the reader false confidence.panic = "unwind", every extern "C" function must wrap its body in catch_unwind AND every Drop impl on the unwind path must be panic-free (double-panic aborts the process). Fallible operations must use Result/Option with proper error propagation.State which panic mode applies in the agent's first sentence. Every panic-related finding must be evaluated under the actual mode, not the textbook one.
Agent 3 — FFI boundary safety: Check every #[no_mangle] / extern "C" function. Verify: NULL pointer checks on all pointer arguments, proper error propagation across the FFI boundary (no panics escaping into C), correct ownership transfer semantics (who allocates, who frees), buffer length validation, string encoding correctness (UTF-8 ↔ C strings, NUL handling), and that the C header (include/questdb/ingress/line_sender.h) and C++ wrapper (include/questdb/ingress/line_sender.hpp + the split line_sender_core.hpp / line_sender_array.hpp / line_sender_decimal.hpp) accurately reflect the Rust implementation. If cbindgen.toml is involved, verify generated output matches handwritten headers.
Agent 4 — Concurrency & thread safety: Race conditions, Send/Sync bounds, shared mutable state, lock ordering, correct use of Arc/Mutex/RwLock, tokio runtime safety (blocking vs async calls), thread-safety of data structures. For C/C++ API: verify documented thread-safety guarantees match the implementation. Cross-reference every callsite from 2.5b for violations of the new concurrency contract.
Agent 5 — Resource management & memory: Leaks on all code paths (especially errors), Drop implementations, native memory management, buffer lifecycle, socket/connection cleanup on error paths, TLS session teardown. For C API: verify every allocation has a documented deallocation path, and error paths don't leak. Walk every callsite from 2.5b that constructs, owns, or transfers ownership of changed types and verify cleanup on all paths (Ok branch, ? early return, panic-unwind boundary).
Agent 6 — Performance & allocations: Unnecessary allocations on hot paths (buffer building, column writes, flushing), excessive copying, inefficient serialization, unnecessary syscalls, buffer growth strategy. For each new loop on the data path, analyze how it scales with realistic message volumes (millions of rows per flush, hundreds of columns). Flag any O(n²) pattern. Setup-path allocations (sender construction, configuration parsing) are acceptable; data-path allocations are not.
Agent 7 — Test review & coverage: Coverage gaps, error path tests, NULL/edge-case tests, boundary conditions, regression tests, test quality. Check:
questdb-rs/src/**/tests.rs and #[cfg(test)] mod tests blockscpp_test/test_line_sender.cppsystem_test/test.pyexamples/ still build and runCross-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.
Agent 8 — Code quality & API design: Public API ergonomics and consistency, backward compatibility of C/C++ headers, naming conventions, dead code, documentation for public items (/// docs on public Rust, Doxygen-style comments on C/C++ headers), clippy and cargo fmt compliance.
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:
Drop impl) where the new behavior misbehaves even if the inputs are valid?extern "C" signatures: does the C header still match? Do the C++ wrapper and Python ctypes binding still pass the right types and lifetimes?flush called only when the sender is in a flushable state)?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 (Buffer::column_*, Sender::flush, FFI exports) 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.
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, unsafe blocks, FFI lifecycle conventions, and Rust control-flow guarantees. Every finding MUST be verified before it is reported.
For each finding in the draft report:
? operator early returns, panic unwind). Check for Drop impls. Before claiming a leak between allocation and cleanup registration, verify that the intervening code can actually fail.usize::MAX, billions of columns), drop it.// SAFETY: comment.Classify each finding as:
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:
unwrap()/expect() in library code (only in tests, or on infallible paths with a // SAFETY: / // INVARIANT: justification)unsafe blocks have documented safety invariantsSend/Sync bounds on public typespanic = "abort", catch_unwind is a no-op and every reachable panic is a fatal escape; the FFI function must prove no panic path exists. Under panic = "unwind", every extern "C" function must wrap its body in catch_unwind.Anything that aborts the Rust side aborts the host process. The first check is the panic strategy itself — everything else is downstream of it.
panic = "abort", the entire catch_unwind defense collapses — every panic across the entire crate is fatal. Verify the profile before crediting any panic guard. A finding that says "the panic at X is caught by catch_unwind at Y" is incorrect under abort-panic.std::process::abort(), libc::abort(), std::intrinsics::abort()Drop impls, deeply nested untrusted inputDrop / double-panic during unwind — every Drop impl must be panic-freenoexcept or proven not to throwMSG_NOSIGNAL or mask SIGPIPEMutex::lock().unwrap() panics if the mutex is poisoned by a prior panic; use lock() with explicit poison handlingline_sender.h) and C++ wrapper (line_sender.hpp and split headers) match Rust implementation signaturescbindgen.toml produces output consistent with handwritten headers when both existSend boundariesDrop implementations correct and completeclippy clean (no #[allow] attributes added without justification)cargo fmt appliedcpp_test/?system_test/?TODO, FIXME, HACK, XXX, and WORKAROUND comments. For each one found:
Present ONLY verified findings (false positives are excluded from Critical/Moderate/Minor). 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: