diff --git a/.coderabbit.yaml b/.coderabbit.yaml new file mode 100644 index 0000000..f42bc25 --- /dev/null +++ b/.coderabbit.yaml @@ -0,0 +1,137 @@ +# yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json +# +# Review rules live in `.github/copilot-instructions.md` and `AGENTS.md` +# CodeRabbit picks both up automatically. This file only carries settings that +# differ from CodeRabbit's defaults. + +reviews: + profile: chill + request_changes_workflow: false + high_level_summary: true + poem: false + review_status: true + collapse_walkthrough: true + + auto_review: + enabled: true + drafts: false + base_branches: + - main + ignore_usernames: + - 'dependabot[bot]' + - 'dependabot-preview[bot]' + labels: + - '!dependabot' + + path_filters: + # Generated protobuf bindings — regenerate via ./gen-protos.sh, never + # hand-edit (AGENTS.md > Architecture). + - '!osprey_rpc/src/osprey/rpc/**/*_pb2*.py' + - '!osprey_rpc/src/osprey/rpc/**/*_pb2*.pyi' + - '!osprey_coordinator/src/proto/**' + # Build artifacts and caches. + - '!**/node_modules/**' + - '!**/dist/**' + - '!**/build/**' + - '!**/target/**' + - '!**/__pycache__/**' + - '!**/.venv/**' + - '!**/*.snap' + + # Cross-cutting rules live in .github/copilot-instructions.md. + path_instructions: + - path: 'osprey_worker/src/osprey/worker/ui_api/**/*.py' + instructions: | + Flask HTTP API on port 5004. Verify each view enforces authentication + and that any caller-supplied IDs are authorization-checked, not just + existence-checked (IDOR). Flag string-built SQL or shell commands — + SQLAlchemy bound parameters and `subprocess` arg arrays (no + `shell=True`) are required. Validate request bodies and query params + (Pydantic preferred). Error responses must not leak Python stack + traces or internal exception messages. + + - path: 'osprey_worker/src/osprey/worker/sinks/**/*.py' + instructions: | + Output sinks emit verdicts/effects to external systems (Kafka, HTTP, + etc.). Flag sink writes that don't bound retries or backoff, that log + full request/response bodies or raw Kafka payloads (PII risk), or + that swallow exceptions without a metric, log, or rethrow. + + - path: 'osprey_worker/src/osprey/worker/adaptor/**/*.py' + instructions: | + Pluggy plugin manager and hookspecs (`register_udfs`, + `register_output_sinks`, `register_labels_service_or_provider`). New + or renamed hookspecs change the plugin contract — surface signature + changes that would break `example_plugins/` or downstream consumers. + Generic-name UDFs collide silently; flag duplicates. + + - path: 'osprey_ui/src/**/*.{ts,tsx}' + instructions: | + - XSS: flag `dangerouslySetInnerHTML`, `innerHTML`, `document.write`, + `javascript:` URLs, and unsanitized `href`/`src` from user input. + Prefer `textContent`; sanitize with DOMPurify when raw HTML is + unavoidable. + - Token storage: auth tokens belong in HttpOnly, Secure, SameSite + cookies — flag `localStorage` or `sessionStorage` use for tokens. + - Open redirects: redirecting to a user-supplied URL without an + allowlist is risky. + + - path: 'osprey_coordinator/src/**/*.rs' + instructions: | + - `unwrap()` / `expect()` / `panic!` on user-driven or RPC paths + turn a bad input into a process crash; prefer `?` or explicit + error mapping. + - `unsafe` blocks deserve a justifying comment and the tightest + possible scope. + - Holding a `std::sync::Mutex` guard across `.await` deadlocks the + tokio runtime; long-lived `tokio::sync` guards can starve other + tasks. Flag them. + - Spawned tasks should be cancellation-safe — surface anything that + will leak on shutdown. + + - path: 'proto/osprey/rpc/**/*.proto' + instructions: | + gRPC contract consumed by both Python workers and the Rust + coordinator. Removing or renaming fields, changing field numbers, or + changing tag types is a breaking change — flag it and ask for a + migration plan. Bindings must be regenerated with `./gen-protos.sh` + in the same PR. + + - path: 'example_plugins/**/*.py' + instructions: | + Reference plugins. Per AGENTS.md > Architecture no production code + belongs here — flag if production-only logic is being added. + + - path: 'example_rules/**' + instructions: | + Sample SML rules and YAML config. Reference, not production — flag + if production rule logic is being added here instead of in the + consumer's own rules directory. + + - path: '**/{pyproject.toml,uv.lock,Cargo.toml,Cargo.lock,package.json,package-lock.json}' + instructions: | + Dependency additions, removals, or upgrades (including transitive + bumps) require human approval for license (Apache 2.0, per + `LICENSE.md`) and CVE review per AGENTS.md > "Human-approval-required + actions". Surface every change so reviewers don't miss it. New + Python deps must also be exercised by `fawltydeps` or added to + `[tool.fawltydeps].ignore_unused` with a comment. + + - path: '{docker-compose.yaml,start.sh,entrypoint.sh,**/Dockerfile,.github/workflows/publish-coordinator-image.yml,.github/workflows/release-osprey-rpc.yml,.github/workflows/mdbook.yml}' + instructions: | + AGENTS.md > "Human-approval-required actions" lists these as + restricted: release/deploy workflows, production Dockerfiles, + signing/tagging, and infra entrypoints. Flag any change here as + needing explicit human approval. Default Docker bindings are + `127.0.0.1` — flag any bind-address change. + + # These overlap with checks already enforced in CI, or add style noise. + tools: + ruff: + enabled: false + prettier: + enabled: false + markdownlint: + enabled: false + languagetool: + enabled: false diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md new file mode 100644 index 0000000..8a8741b --- /dev/null +++ b/.github/copilot-instructions.md @@ -0,0 +1,70 @@ +# 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`](../README.md); for general agent rules see [`AGENTS.md`](../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`](../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`](https://docs.astral.sh/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`](../.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[]` 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. diff --git a/.github/dependabot.yml b/.github/dependabot.yml index 790a744..234547d 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -4,6 +4,9 @@ updates: directory: "/" schedule: interval: "weekly" + labels: + - "dependencies" + - "dependabot" ignore: - dependency-name: "grpcio" @@ -11,6 +14,9 @@ updates: directory: "/" schedule: interval: "weekly" + labels: + - "dependencies" + - "dependabot" groups: github-actions: patterns: