Code review instructions

These instructions apply to AI tools when they review pull requests in this repository, and when they answer questions about this codebase. They are guidance, not a checklist. Use judgment, prefer fewer high-signal comments over many low-signal ones, and skip points that don't apply to the diff in front of you. For human contributor guidance see README.md; for general agent rules see AGENTS.md.

Repository at a glance

• Multi-language monorepo: Python (worker + UI API), TypeScript (UI), Rust (coordinator). Top-level modules are documented in AGENTS.md § Architecture — osprey_worker/, osprey_rpc/, osprey_ui/, osprey_coordinator/, proto/osprey/rpc/, example_plugins/, example_rules/.
• Python version pinned in .python-version, managed with uv. Linted and type-checked in CI by ruff + mypy via pre-commit. Unused-dep enforcement via fawltydeps.
• TypeScript in osprey_ui/ (React + Ant Design + Highcharts; Node version in .github/workflows/code-quality.yml). Formatted in CI by prettier (npm run format:check).
• Rust in osprey_coordinator/ (stable toolchain, tokio + tonic + etcd + rdkafka). cargo fmt and cargo build gate CI; cargo clippy -- -D warnings and cargo test run advisory (continue-on-error: true).
• Generated code: osprey_rpc/src/osprey/rpc/**/*_pb2*.py, *_pb2*.pyi, and osprey_coordinator/src/proto/ are produced by ./gen-protos.sh from proto/osprey/rpc/**/*.proto. Hand-edits drift from the schema — regenerate instead. Generated files are excluded from ruff and mypy for a reason.
• Plugin system: pluggy with @hookimpl_osprey hooks (register_udfs, register_output_sinks, register_labels_service_or_provider). Reference patterns live in example_plugins/src/register_plugins.py.
• Data model conventions: Pydantic for models, SQLAlchemy for persistence (versions pinned in pyproject.toml).

Scope of review — focus on quality and security

Lint and formatting are enforced in CI, so please skip:

• formatting, whitespace, indentation, quote style, or import ordering
• ruff / mypy / prettier / cargo fmt rule violations
• typos in comments or doc grammar nits
• missing docstrings on internal helpers
• subjective style preferences not codified in a project rule

If a finding would be caught by uv run ruff check, uv run mypy ., npm run format:check (in osprey_ui/), or cargo fmt --check (in osprey_coordinator/), it's redundant.

Security (cross-cutting)

Security findings are the highest-value comments you can leave. When you spot one, name the risk concretely and suggest a fix. Areas to watch across the whole codebase:

• Hard-coded secrets. API keys, tokens, passwords, OAuth secrets, JWT signing keys, DB connection strings (Postgres, Druid, Kafka SASL, MinIO, Bigtable), or webhook secrets in source or committed config. docker-compose.yaml uses environment variables intentionally — flag any secret that leaks into committed files.
• Injection. String-built SQL, shell commands, file paths, HTML, or LLM prompts are usually a smell. Look for SQLAlchemy bound parameters (or text() with bindparams), subprocess arg arrays (no shell=True), context-aware HTML encoding, and a clear separation between trusted system prompts and untrusted user content.
• Sensitive logging. Secrets, JWTs, full Authorization headers, full request/response bodies, raw Kafka payloads, or PII in logs, traces, metric labels, or error responses are risky. Error responses from the UI API (port 5004) shouldn't leak Python stack traces.
• Weak crypto. MD5 or SHA-1 used for security, ECB mode, reused IVs, random.random() / random.choice() for tokens or IDs (prefer secrets in Python, OsRng / getrandom in Rust), or hand-rolled crypto are worth questioning. JWTs should reject the none algorithm, use strong secrets, and have short access-token expirations.
• Unsafe deserialization or evaluation. eval, exec, pickle.loads, yaml.load without SafeLoader, and marshal.loads on untrusted input are risky. Same for eval / the Function constructor / setTimeout with string arguments in TypeScript.
• Removing security controls. If a diff disables authentication, authorization, CSRF, CORS, rate limiting, or the default 127.0.0.1 bindings in docker-compose.yaml (see docs/DEVELOPMENT.md §6), ask whether it's intentional and justified.
• Untrusted protobuf toolchain. Per AGENTS.md § Security, only regenerate bindings via ./gen-protos.sh. Flag generated-file diffs that don't match a corresponding .proto change.

Path-specific concerns (UI API views, output sinks, plugin adaptor, Rust coordinator, raw .proto changes, dependency manifests, restricted infra) are scoped in .coderabbit.yaml under reviews.path_instructions. When reviewing those areas, apply the same general principles — authorization checks on UI-API-supplied IDs, parameterized queries, XSS care on the client, panic-safety in Rust, license/CVE attention on dependency bumps.

Code quality (cross-cutting)

Use judgment — these patterns tend to cause bugs or maintenance pain regardless of where they appear:

• Generated files. *_pb2*.py, *_pb2*.pyi, and osprey_coordinator/src/proto/ are produced by ./gen-protos.sh; hand-edits drift from the .proto schema.
• Error handling. Silently swallowed exceptions (except Exception: pass with no log or rethrow), Rust let _ = result; discarding an error, unhandled promise rejections in TS, and missing await on a coroutine whose result matters tend to cause production surprises.
• Async correctness (Python). Calling sync I/O inside an async def blocks the event loop. asyncio.gather(...) without return_exceptions=True or explicit handling can mask failures.
• Async correctness (Rust). Holding a std::sync::Mutex guard across .await deadlocks the tokio runtime; long-lived tokio::sync guards can starve other tasks.
• Type safety. New # type: ignore / Any / cast(...) in Python, any / as unknown as / non-null ! / @ts-ignore in TypeScript, or unwrap() / expect() / unsafe on user-driven paths in Rust are worth questioning. Narrowly-scoped # type: ignore[<code>] and @ts-expect-error with a justifying comment are preferred when an escape hatch is genuinely needed (matches AGENTS.md § Security).
• Tests. New behavior generally warrants a test; bug fixes generally warrant a regression test (AGENTS.md § "Code review"). Integration tests run via ./run-tests.sh.
• Duplicated logic. If a helper already exists in the same package, prefer it over a parallel implementation.
• Dead code. Commented-out blocks and TODOs without an issue link are worth a nudge.

What not to flag

These categories of comments tend to add noise without surfacing real risk — please skip them:

• "Consider adding a None check" on a value already typed as non-None by mypy or non-null/undefined by TypeScript.
• "Consider adding error handling" on a wrapper that already propagates errors via async/await or ? in Rust.
• "This could be a constant" on a string literal used in a single place.
• "Add a docstring" on an internal helper.
• Rhetorical questions like "have you considered…" without a concrete risk attached.
• Defensive-coding suggestions on values whose types already prevent the failure mode.
• "Add a test" on a config-only, doc-only, or comment-only change.
• Suggestions to rename a symbol "for clarity" without a concrete ambiguity.
• Style nits in generated protobuf files (*_pb2*.py, *_pb2*.pyi, osprey_coordinator/src/proto/) — they're excluded from ruff and mypy for a reason.

Tone

Be specific and concise. When you flag something, name the concrete risk ("possible SQL injection via string-built text()", "missing authorization check — possible IDOR", "secret logged at info level", "unwrap() on RPC input — panics will crash the worker") and, where helpful, show the fix in code. Skip nits, and stay quiet when nothing in this list applies.