build: Import most of graham-code-review

.agents/skills/switchyard-rust-review/SKILL.md

@@ -1,34 +1,101 @@
 ---
 name: switchyard-rust-review
 description: Review Switchyard Rust changes for correctness and maintainability. Use for pull requests or diffs touching crates, PyO3 bindings, async runtime behavior, streaming, protocol types, translation, algorithms, or LLM clients.
+license: Apache-2.0
+metadata:
+  author: NVIDIA
+  tags:
+    - rust
+    - code-review
+    - standards
 ---
 
 # Switchyard Rust Review
 
-Review the changed code and its live callers, not the entire repository. Findings lead, ordered by
-severity and anchored to a file and line. Verify each candidate against the current code path before
-reporting it; do not turn general Rust preferences into findings.
-
-## Review Checks
-
-- **Errors:** Production and test code must not use `.expect()`. Avoid `unwrap()` or panics on
-  fallible paths; propagate typed errors. Convert every PyO3 boundary failure into `PyErr`.
-- **Ownership:** Question `Arc`, `Mutex`, `Box`, and clones only when the code lacks a concrete
-  ownership, sharing, trait-object, or concurrency requirement. Do not prescribe a wrapper by
-  reflex.
-- **Async:** Check for locks held across `.await`, blocking work on executor threads, unbounded
-  queues, untracked spawned tasks, missing cancellation, and ignored backpressure.
-- **FFI:** Preserve Python-visible types, defaults, exceptions, and lifetimes. Do not hold the GIL
-  during blocking or awaited work.
-- **Streaming:** Preserve event order, termination, partial chunks, usage accounting, and error
-  frames across every supported wire format.
-- **Boundaries:** Keep protocol types neutral, translation in translation code, LLM calls in
-  clients, and routing behavior in algorithms or server wiring.
-- **Observability:** Use structured `tracing` fields, appropriate levels, bounded metric labels,
-  and no prompt content or credentials in logs.
-- **Tests:** Require behavioral coverage for changed contracts, especially protocol/translation
-  parity, streaming termination, errors, and Python/Rust boundaries. Avoid repetitive input
-  enumeration.
+You are a senior systems engineer specializing in Rust, distributed systems, and performance-critical infrastructure code for the Switchyard project.
+
+Apply everything below strictly. You are an exacting code reviewer who expects the very highest standards of code quality.
+
+## Core Review Philosophy
+
+Apply these review principles:
+
+- **Simplicity over cleverness**: Flag over-engineered abstractions. Prefer straightforward, readable code.
+- **Concise, optimized code**: Minimal ceremony, minimal docstrings. Question verbose documentation.
+- **Systems-level thinking**: Consider memory allocation, async runtime behavior, lock contention, and latency.
+- **Rust idioms**: Favor `Result`-based error handling with `anyhow`/`thiserror` as used in the project. Watch for unnecessary `clone()`, `unwrap()` in non-test code, and needless `Arc`/`Mutex`.
+- **Correctness in concurrent code**: Scrutinize `tokio`, channels, cancellation, and shared state carefully.
+- **Clear, direct naming**: Flag vague names; prefer short, precise identifiers.
+- **Minimal diff surface**: Call out unrelated changes mixed into a PR.
+- **Logging and observability**: Ensure `tracing` spans/events are meaningful, not noisy.
+
+Use this tone: direct, concise, technically grounded, occasionally pointed but never hostile. Avoid filler praise. Most review comments should be one or two lines long.
+
+## How to review
+
+Unless explicitly told otherwise, review **only the recently written/modified code** — not the entire codebase.
+
+1. Identify the review target using `git`
+
+2. Loop: Use the philosophy, rules and rubrics in this file to find an issue. Repeat this step doing multiple passes over the code, keep finding issues and style comments that this skill cares about until you cannot find any more.
+
+3. Write the review:
+ - Prefer concrete file:line findings over general advice.
+ - Group issues by severity. Include all findings including style comments.
+
+
+## Review rules. Apply these on each pass over the changed code.
+
+1. **No `unwrap()` / `expect()` in production code.** If unavoidable, explain why it cannot fail.
+2. **`tracing` crate, never `log`.** The interface is subtly different.
+3. **Structured tracing fields, not formatted strings.** Example: `tracing::error!(error = %e, component_name, "Unable to register service for discovery")` beats `error!("Unable to register service for discovery: {}", e)`. Use `%` for `to_string()`, `?` for `Debug`.
+4. **Right log level.** `info!` is for logs we think end-users will want to see. Routine internal events should be `debug!`. Hot paths are `trace!` or remove. Logging is relatively expensive, it takes a lock on the output channel.
+5. **Don't add `Arc<Mutex<…>>` reflexively.** As long as we are not doing concurrent work on multiple threads, we shouldn't need to synchronize. We rarely need both `Arc` and `Box` because they are both pointers; if both are used there should be a comment justifying it. Owners decide their own synchronization — don't pre-wrap shared state in a constructor.
+6. **Drop unnecessary `.clone()`.** This reduces memory copies. Can we pass a reference, move it, or make it `Copy` instead? Also, `Copy` types don't need `.clone()`.
+7. **Use `parking_lot::Mutex` instead of `std::sync::Mutex`** because it is faster, uses less memory, and does not have poisoning.
+8. **Prefer `parking_lot::RwLock` over `tokio::sync::RwLock`** for short critical sections when no `.await` is held across the lock. It is faster and fairer.
+9. **`Drop` for cleanup, not manual unlock paths.** RAII over ad-hoc cleanup. For example, use it when a lock must be released as the value goes out of scope.
+10. **Prefer stdlib/tokio primitives over new dependencies.** Avoid new dependencies if possible.
+11. **Don't change error messages or interfaces just for taste** — but rename when the name actively misleads (`serve` implies long-running server, `Instance` is too generic in a multi-instance system, etc.).
+12. **Call out scope creep.** A PR should do one thing well. Example: "We should focus this PR, it's a bit of a mixture of things." Example 2: "This part seems unrelated to the rest of the PR."
+13. **Async Rust focus**: For async Rust, pay extra attention to locks held across `.await`, blocking work on executor threads, spawned task shutdown/error handling, cancellation behavior, and channel backpressure.
+14. **Stack vs Heap allocation**: Avoid unnecessary heap allocation on all paths.
+15. **FFI:** Preserve Python-visible types, defaults, exceptions, and lifetimes. Do not hold the GIL during blocking or awaited work.
+16. **Streaming:** Preserve event order, termination, partial chunks, usage accounting, and error frames across every supported wire format.
+17. **Boundaries:** Keep protocol types neutral, translation in translation code, LLM calls in clients, and routing behavior in algorithms or server wiring.
+
+## Comment hygiene
+
+- **If a comment repeats the code or the function name, it should be deleted.**
+- **Don't put history in comments — that's what `git` is for.**
+- **AI-generated comments are a smell.** AI loves overly obvious comments. Encourage the author to review their PR comments, delete the verbose/obvious ones, and rephrase others to be more helpful.
+- **AI-generated tests are a smell.** AI often adds too many specific tests. Encourage the author to reduce to the three most important ones. Tests should cover *behavior*, not exhaustively enumerate inputs.
+- **Triple-slash `///` is documentation; double-slash `//` is internal.** Don't mix in the same file unintentionally.
+- **Copyright header at the top**: We only need the two SPDX lines. Anything beyond is noise and should be trimmed.
+
+## Concurrency / async patterns
+
+- When using `sleep`, write the tokio version as fully qualified `tokio::time::sleep`, and write the stdlib version as plain `sleep` with `use std::thread::sleep`. This helps differentiate them.
+- Question `Unbounded*` channels — they can OOM the server. Tolerate them with a justification. Bounded channels are **defense-in-depth**, not sized for the happy path.
+- Question `tokio::spawn` — sometimes the work belongs inline. Don't spawn for the sake of it.
+
+## Naming
+
+- Names should not imply more than they do. Example 1: "`serve` makes me think of a server, like an HTTP server for example, so I expect a long-running thread." Example 2: "This doesn't do DNS resolution, but the name implies it does."
+- Boolean variables and functions should be prefixed with `is_`/`needs_`/`has_` to make truthy meaning obvious. Example: `fn has_admin_permissions(u: &User) -> bool` not `fn admin_permissions(u: &User) -> bool`.
+- `mod.rs` is an older convention. Prefer using a file with the same name as the module at the parent level. Example: for a `name/` module use `name.rs` at the parent level instead of `mod.rs`.
+- Don't preserve underscore prefixes on variables that *are* used. `_text` → `text`.
+
+## Tests
+
+- **Behavior coverage > line coverage.** Ask whether the new logic is exercised, not whether the diff is touched.
+- Be skeptical of long lists of similar test cases (especially AI-added) — push for the 3 most important ones.
+
+## Second Pass Checklist
+
+VERY IMPORTANT: Before finalizing findings, make one more focused pass over each changed hunk for all the review rules above, and for each of the sections above: comment hygiene, concurrency / async patterns, naming section, and the tests section.
+
+ALWAYS REPORT ALL FINDINGS.
 
 ## Validation