diff --git a/README.md b/README.md index b8b468d..9490920 100644 --- a/README.md +++ b/README.md @@ -122,6 +122,25 @@ python3 scripts/persona.py # --db data/turnstone.db default (Without a console deployment, persona CRUD is a direct SQLite write; with `turnstone-console` running, use its admin API/Personas tab.) +### Skills + +The 13 skills from the old opencode setup (`~/marvin/configs/opencode/skills`, +copied into this repo under `skills/`) are imported into turnstone's +DB-backed catalog via `scripts/import_skills.py`. The script runs +INSIDE the VM (uses turnstone's storage layer + SKILL.md parser + +risk scanner): + +```sh +scp -O -i configs/ssh/id_ed25519 -P 2222 -r skills scripts/import_skills.py \ + marvin@localhost:/tmp/ +ssh -i configs/ssh/id_ed25519 -p 2222 marvin@localhost \ + /opt/turnstone/bin/python /tmp/import_skills.py --dir /tmp/skills +``` + +Idempotent (`--update` re-imports + appends version rows). **Reboot the +VM after importing** — the running server holds a stale WAL snapshot +and won't see new rows until restart. + ## Notes / trade-offs - boco boots with `-snapshot`: **only the virtiofs shares persist.** diff --git a/scripts/import_skills.py b/scripts/import_skills.py new file mode 100644 index 0000000..6adeac4 --- /dev/null +++ b/scripts/import_skills.py @@ -0,0 +1,120 @@ +#!/usr/bin/env python3 +"""Import SKILL.md skill directories into the turnstone DB. + +Runs INSIDE the VM (needs the turnstone venv), using turnstone's own +storage layer and SKILL.md parser so rows match what the console admin +API / in-session `skills` tool would have written (risk scan included): + + /opt/turnstone/bin/python import_skills.py --dir /tmp/skills + +The DB (data/turnstone.db on the host) is shared into the guest at +/var/lib/turnstone/turnstone.db. Idempotent: existing skill names are +skipped unless --update (which re-imports and appends a version row). +""" +from __future__ import annotations + +import argparse +import json +import pathlib +import uuid + +from turnstone.core.skill_parser import parse_skill_md +from turnstone.core.storage._sqlite import SQLiteBackend + +DB_DEFAULT = "/var/lib/turnstone/turnstone.db" + + +def next_version(backend: SQLiteBackend, skill_id: str) -> int: + try: + rows = backend.list_skill_versions(skill_id) + return max((int(r["version"]) for r in rows), default=0) + 1 + except Exception: # noqa: BLE001 + return 1 + + +def import_one( + backend: SQLiteBackend, skill_dir: pathlib.Path, update: bool +) -> str: + raw = (skill_dir / "SKILL.md").read_text() + p = parse_skill_md(raw) + if p is None or not p.name or not p.description: + return f"SKIP {skill_dir.name}: unparseable frontmatter" + if not p.content.strip(): + return f"SKIP {skill_dir.name}: empty body" + + allowed = p.allowed_tools or [] + # Storage stores these four as canonical JSON strings (the console + # layer json.dumps them before calling in). + tags_str = json.dumps(p.tags or []) + allowed_str = json.dumps(allowed) + paths_str = json.dumps(p.paths or []) + arguments_str = json.dumps(p.arguments or []) + + existing = backend.get_skill_by_name(p.name) + if existing and not update: + return f"SKIP {p.name}: already exists" + + if existing: + skill_id = existing["template_id"] + backend.update_prompt_template( + skill_id, + description=p.description, + content=p.content, + allowed_tools=allowed_str, + skill_license=p.license or "", + compatibility=(p.compatibility or "")[:500], + paths=paths_str, + arguments=arguments_str, + tags=tags_str, + ) + version = next_version(backend, skill_id) + action = "UPDATE" + else: + skill_id = uuid.uuid4().hex + backend.create_prompt_template( + template_id=skill_id, + name=p.name, + category="general", + content=p.content, + created_by="marvin", + description=p.description, + tags=tags_str, + version=str(p.version or "1.0.0"), + author=p.author or "", + skill_license=p.license or "", + compatibility=(p.compatibility or "")[:500], + token_estimate=len(p.content) // 4, + allowed_tools=allowed_str, + kind="any", + paths=paths_str, + arguments=arguments_str, + ) + version = 1 + action = "IMPORT" + + backend.create_skill_version(skill_id, version, raw, changed_by="marvin") + row = backend.get_prompt_template(skill_id) + return f"{action} {p.name} v{version} risk={row.get('risk_level') or 'none'}" + + +def main() -> None: + ap = argparse.ArgumentParser(description=__doc__) + ap.add_argument("--dir", required=True, help="directory of skill folders") + ap.add_argument("--db", default=DB_DEFAULT) + ap.add_argument("--update", action="store_true") + args = ap.parse_args() + + backend = SQLiteBackend(args.db, create_tables=False) + root = pathlib.Path(args.dir) + dirs = sorted(d for d in root.iterdir() if (d / "SKILL.md").is_file()) + if not dirs: + raise SystemExit(f"no SKILL.md directories under {root}") + for d in dirs: + try: + print(import_one(backend, d, args.update)) + except Exception as e: # noqa: BLE001 + print(f"FAIL {d.name}: {e}") + + +if __name__ == "__main__": + main() diff --git a/skills/3d-modeling-openscad/SKILL.md b/skills/3d-modeling-openscad/SKILL.md new file mode 100644 index 0000000..8bb5263 --- /dev/null +++ b/skills/3d-modeling-openscad/SKILL.md @@ -0,0 +1,68 @@ +--- +name: 3d-modeling-openscad +description: Generate parametric 3D models using OpenSCAD script +license: WTFPL +compatibility: opencode +metadata: + audience: makers, engineers, designers + workflow: 3d-modeling +--- + +## What I do + +- Write `.scad` files using OpenSCAD syntax (primitives, transformations, CSG). +- Create parametric models using modules, functions, and variables. +- Implement complex geometries using Boolean operations (`union`, `difference`, `intersection`). +- Apply modifiers like `$fn` for smoothness or `center` for alignment. +- Create scripts for procedural 3D design. +- **Execute a Visual Verification Loop**: + - Render `.scad` to `.png` using the OpenSCAD CLI. + - Use the `read` tool to inspect the rendered image. + - Verify geometry against requirements and perform iterative refinements. + +## When to use me + +Use this when you need to create a 3D printable model through code. +Use this when you want to create a parametric part that can be easily resized. +Use this when you have a mathematical or algorithmic approach to 3D modeling. + +## Best Practices + +- **Ask for details**: Use the `questions` tool to ask for clarifying details. + - For example: What aspects of the model should be parameterized? + +## Boolean subtractions +**Avoid 3D boolean subtractions**. The `difference` and `intersection` operations are unreliable when used in 3D. Whenever possible, perform `difference` and `intersection` operations on 2D shapes (e.g., `circle`, `square`) before using `linear_extrude` to create 3D volumes. This approach is much more robust and avoids the precision errors and artifacts common with complex 3D Boolean operations (e.g., when creating a tube or a hollow cylinder). + +Example: Creating a Tube: +```openscad +inner_radius = 5; +outer_radius = 7; +height = 10; + +// This is the CORRECT way to create a tube: +linear_extrude(height) +difference() { + circle(r = outer_radius); + circle(r = inner_radius); +}; + +// WRONG way to create a tube, don't do this!!! `difference` on 3d objects should be avoided! +difference() { + cylinder(h = height, r = outer_radius); + cylinder(h = height, r = inner_radius); +}; +``` + +## Verification Workflow + +To ensure the generated `.scad` file is valid and matches the design intent, run: + +```bash +openscad .scad -o .render.png +``` + +**Purpose:** +1. **Syntax Check**: If the command fails, the script contains errors that must be fixed. +2. **Visual Validation**: Use the `read` tool on the resulting `.png` to inspect the 2D projection of the 3D model for geometric correctness. **It is critical to compare the rendered image against the user's original request/image to ensure all design requirements and dimensions are met.** +``` diff --git a/skills/code-review-dedupe/SKILL.md b/skills/code-review-dedupe/SKILL.md new file mode 100644 index 0000000..4a6e0fd --- /dev/null +++ b/skills/code-review-dedupe/SKILL.md @@ -0,0 +1,170 @@ +--- +name: code-review-dedupe +description: Dedupe stage of the code review pipeline. Takes verified findings, + drops refuted, applies severity overrides, merges overlapping findings, and + emits structured JSON for the synthesize stage. +--- + +You are the **dedupe stage** of a multi-stage code review pipeline. You +receive (1) the full finder output (finding bodies) and (2) the verifier's +verdicts. Your job: drop refuted, apply severity overrides, merge overlapping +findings, and emit a clean **JSON** object that the **synthesize** stage will +use to render the final markdown report. + +**You do not render markdown.** Synthesize re-reads source for each fix to +validate / refine, then produces the user-facing report. + +## Input format + +{ + "findings": [ + { "id": "bug-1", "finder": "bug", "severity": "...", "location": "...", + "claim": "...", "evidence": "...", "fix": "..." } + ], + "verdicts": [ + { "id": "bug-1", "verdict": "confirmed|refuted|uncertain", "reason": "...", + "uncertain_reason": "...", "severity_override": "..." } + ] +} + +`findings` ids are namespaced by finder (`bug-N`, `sec-N`, `perf-N`, `q-N`) +and unique. Verdicts are keyed by the same ids. + +## Processing steps + +### Step 1 — Filter by verdict + +* Drop every finding whose verdict is **refuted**. Do not include them in the + output. +* Keep every finding whose verdict is **confirmed**. +* Keep every finding whose verdict is **uncertain**, carrying its + `uncertain_reason`. + +### Step 2 — Apply severity overrides + +For each surviving finding, if the verdict includes `severity_override`, +replace the finder’s severity with it. The verifier’s judgment beats the +finder’s self-assessment. + +### Step 3 — Cluster overlapping findings + +Two or more findings are **overlapping** if they describe the same underlying +problem. Merge signals: + +* **Same location** (same file, overlapping line range) AND describe the same + defect. Example: `find-security` says “unescaped user input at line 42” and + `find-bug` says “missing escape call at line 42” — same issue. +* **Different locations but same root cause.** Example: three finders each + flag an N+1 pattern in the same loop; or a security finding and a quality + finding both point at the same untrusted-data entry point from different + angles. + +Two findings at the same location that describe **different** defects are NOT +overlapping — keep them separate. (E.g., a race condition and a missing +permission check on the same function.) + +For each cluster, build a single merged finding: + +* `ids`: list of original finding ids in the cluster (e.g. `["bug-1", "sec-1"]`). +* `dimensions`: union of finder names from the cluster (e.g. `["bug", "security"]`). Stable order: `bug`, `security`, `performance`, `quality`. +* `severity`: highest among cluster members (`critical > major > minor > nit`). +* `location`: from the cluster member with the most precise location. +* `claim`: the clearest / most specific version (longer claim usually wins; + prefer concrete over hedged). +* `evidence`: from whichever member had the most precise location. +* `fix`: best fix from the cluster. If two finders proposed complementary + fixes, synthesize a one-paragraph combined fix; do not lose information. +* `verdict`: `confirmed` if any cluster member is confirmed; otherwise + `uncertain`. +* `uncertain_reason`: present only when `verdict == "uncertain"`, copied from + the uncertain member (or the longest if multiple). + +### Step 4 — Sort + +Sort the deduped array by: + +1. Severity (critical → nit). +2. Within same severity, `confirmed` before `uncertain`. +3. Within same certainty, group by file (stable order within a file). + +The synthesize stage renders in array order — your sort is final. + +## Output format + +Return **JSON only** — no prose, no markdown fence. Single object: + + + "deduped": [ + { + "ids": ["bug-1", "sec-1"], + "dimensions": ["bug", "security"], + "severity": "critical", + "location": "path/to/file.py:123", + "claim": "One-sentence merged claim.", + "evidence": "Verbatim quote from source.", + "fix": "Best / synthesized fix.", + "verdict": "confirmed", + "uncertain_reason": null + } + ], + "stats": { + "input_findings": 12, + "refuted": 4, + "after_clustering": 6, + "by_severity": {"critical": 1, "major": 2, "minor": 2, "nit": 1}, + "uncertain": 1, + "finder_failures": [] + } +} + +Field rules: + +* `deduped`: array of merged findings, sorted as Step 4. Empty array is valid + (zero findings reach the user). +* `ids`: every original finder id that contributed to this cluster, + deduplicated. +* `dimensions`: every finder name in the cluster, in stable order, + deduplicated. +* `severity`: post-override; matches Step 2. +* `verdict`: `"confirmed"` or `"uncertain"`. Refuted are filtered out, never + present. +* `uncertain_reason`: `null` when verdict is confirmed; non-empty string when + uncertain. +* `stats.input_findings`: total findings you received from all finders + combined. +* `stats.refuted`: count dropped by Step 1. +* `stats.after_clustering`: length of `deduped` array (sanity check). +* `stats.by_severity`: histogram over post-override severity. Synthesize uses + this to derive its summary line. +* `stats.uncertain`: count of `verdict == "uncertain"` in `deduped`. +* `stats.finder_failures`: list of finder names whose output was malformed or + empty due to error (e.g. `["security"]` if the security finder errored). + Synthesize surfaces this as a pipeline note. + +## Anti-patterns + +* Re-including refuted findings → breaks verify’s contract; drop them. +* Failing to merge obvious duplicates (same line, same defect described two + ways) → noisy report; merge. +* Over-merging unrelated bugs that happen to share a line → user misses one; + keep separate when defects differ. +* Dropping `dimensions` on merged findings → synthesize loses the + cross-dimension signal; always emit the union. +* Inflating severity on merge — actually correct: keep the highest among + cluster members. Do NOT downgrade. +* Emitting markdown → that’s synthesize’s job; emit JSON only. +* Deduping by `claim` text similarity alone → use location + defect-shape, not + surface text. + +## Notes for downstream stages + +Synthesize will: + +1. Read the cited source for each finding and validate the `fix`. +2. Refine the fix when there’s a clearly better local approach (different + chokepoint, missing follow-on detail, fragile shape). +3. Render the final markdown using `stats.by_severity` to derive counts so + prose and counts can’t drift. +4. Mark each fix as `unchanged`, `refined`, or `unverifiable`. + +Your output is the contract synthesize relies on. Be precise. diff --git a/skills/code-review-find-bug/SKILL.md b/skills/code-review-find-bug/SKILL.md new file mode 100644 index 0000000..f98904c --- /dev/null +++ b/skills/code-review-find-bug/SKILL.md @@ -0,0 +1,166 @@ +--- +name: code-review-find-bug +description: Finder stage of the code review pipeline. Looks for correctness + issues only — logic bugs, edge cases, off-by-one, null/undefined handling, + concurrency, error handling, resource leaks. +version: 1.0.0 +category: engineering +--- + +You are the **bug finder** in a multi-stage code review pipeline. Your job: +find **correctness defects** in the supplied diff. Peer finders own security, +performance, and maintainability. + +## Calibrate for accuracy + recall + +A downstream **verify** stage re-reads source and drops hallucinated claims. +**Don't pre-filter aggressively.** A real bug you almost reported but dropped +is one the user never sees — verify can drop it cheaply, but only if you +propose it. + +- Report every defect you can substantiate from the cited source, including + ones whose blast radius depends on the caller — mark those with downgraded + severity rather than omit. +- Be precise about *what* breaks and *under what input*. Verify checks the + claim against the surrounding code; vague claims get refuted. +- Skip pure speculation ("might be racy if…") — but a concrete defect with a + named input shape is fair game even if rare. + +## In-lane: dimensions you own + +> **Codebase context (CUSTOMIZE):** name the language(s), frameworks, and any +> architectural concepts that affect what counts as a defect. Example: +> *"Python 3.11+ async (FastAPI, asyncio); React/TypeScript frontend; +> PostgreSQL via SQLAlchemy; Rust with unsafe."* +> The more specific, the better the findings. + +- **Logic errors**: inverted boolean, wrong operator (`<` vs `<=`), missing + branch, fallthrough, condition that's always true/false, short-circuit + confusion +- **Edge cases**: empty input, single element, off-by-one, integer overflow, + float comparison, NaN/Infinity, timezone-naive datetime, signed/unsigned + confusion +- **Null / missing / undefined**: dereferencing without a guard, missing-key + reads where a default was expected, mutable default arguments, language- + specific `null` vs `undefined` vs `None` confusion +- **Undefined/unsound behavior**: Use of invalidated/dangling pointers, invalid + transmutation, alignment issues, broken FFI, reading uninitialized values. + Usage usage of `unsafe` operations must be extensively scrutinized. +- **Concurrency**: + - Races: shared state mutated outside a lock; read-modify-write across a + suspension point + - Lock ordering inversions, deadlocks, lost updates + - Cancellation / cleanup: `finally` that swallows the cancellation signal; + cleanup paths that don't run when cancelled + - Fire-and-forget tasks with no reference held — silently dropped + - Blocking I/O on an event-loop / single-threaded path +- **Error handling**: too-broad catches that swallow real failures; `finally` + that masks the original raise; wrong exception type caught; async exception + swallowed by an unawaited task +- **Resource leaks**: file handles, sockets, DB connections, HTTP clients + not closed on the failure path; generator cleanup that won't run on early + return +- **Control flow**: unreachable code, early return that bypasses cleanup, + infinite loop, recursion without base case, yield inside a context manager + whose scope doesn't survive suspension +- **Contract drift**: caller assumes shape A, callee returns B; new field + added but old reader skips it; renamed key the caller still reads; schema + mismatch between producer and consumer +- **State bugs**: mutable default arg, shared mutable class attribute across + instances, stale cache, write-then-read across an await/yield +- **Type-defeated bugs**: type-checker suppression hiding a real mismatch; + any-typed values smuggling through a typed boundary + +### Lane-routing rules + +- A bug whose only consequence is a security vuln (logic error → auth bypass): + route to `find-security` (frame as security). +- A bug that hangs / leaks / loops on user input: report here as critical even + though perf would also flag it. Availability is a correctness concern. +- A bug that wastes work but produces correct output: route to + `find-performance`. +- Style / naming / unused imports: route to `find-quality`. + +If a security/perf-relevant bug exists and you're unsure who owns it, report +it. Dedupe will merge if a peer also reported it; nothing is lost. + +## Search protocol + +Walk the diff in this order: + +1. **Surface inventory.** List every changed function, control-flow change + (new `if`/`for`/`try`), and new boundary call (DB, HTTP, file, async). +2. **Per function — what input breaks it?** Empty list, None, very large + value, dict missing the expected key, string already encoded, datetime in a + non-UTC zone, negative number, bytes vs str. +3. **Per loop**: termination condition, mutation of the iterated collection, + side effects ordered correctly across `await`. +4. **Per `await`**: cancellation safety, exception propagation, cleanup that + runs on `CancelledError`. +5. **Per error path**: swallowed? wrong type? does `finally` mask the original? +6. **Per return / new field**: do all callers in the diff handle the shape? + Caller outside the diff → mark uncertain via lower severity. +7. **Per new schema / model class** (dataclass, pydantic, TypeScript + interface, protobuf, etc.): required vs optional consistent with how + callers read it. + +## Severity anchors + +- **critical** — defect produces wrong output, data loss, crash, or hang on + **realistic** input on a code path that runs in production. + - e.g. unguarded resource not closed on a raised exception; required-key + read against external JSON where the key is optional; mutating a + collection while iterating it. +- **major** — defect on plausible edge case (not hot path but realistic): + off-by-one on a length-bounded input; missing error path on a retry/timeout + branch; race that fires under load; broken contract between caller and + callee. +- **minor** — defect under contrived input only, or partially mitigated where + the mitigation could regress. +- **nit** — defensive-programming opportunity with no named failing input. + Usually drop. + +Calibration test: "Is there a concrete input that triggers the failure?" +Yes → at least major. No, but the path is logically wrong and **will** run → +major. No, and the path is theoretical → minor or drop. + +## Output format + +Return **JSON only** — no prose, no markdown fence. Single object: + +{ + "finder": "bug", + "findings": [ + { + "id": "bug-1", + "severity": "critical|major|minor|nit", + "location": "path/to/file.py:123", + "claim": "One sentence: what's wrong and the input shape that triggers it.", + "evidence": "Verbatim quote (≤3 lines) from the cited source. Whitespace-insensitive but character-exact.", + "fix": "Concrete change — specific call / pattern. Empty string only if obvious from the claim." + } + ] +} + +* `id`: `bug-N`, N starts at 1, monotonic. +* `location`: must include line number; multi-line uses `file:start-end`. +* `evidence`: verbatim. Verify compares character-for-character; paraphrase → + refuted. +* Empty list is fine: `{"finder": "bug", "findings": []}`. Don’t pad. + +## Anti-patterns in your output + +* Hedged claims (“might be”, “could potentially”) with no named input → drop + or sharpen +* Evidence that doesn’t match source character-for-character → re-quote + literally +* Defensive nits on internal-trust boundaries → drop (project norm: terse, + trust callers) +* Severity inflation to look impactful → calibrate honestly; verify can + override +* Cross-finder leakage (security/perf/style in your output) → drop or route + +Your findings feed `code-review-verify` (re-reads source, confirms / refutes / +marks uncertain) and then `code-review-dedupe` (merges overlap, produces +markdown). Confirmed and uncertain findings reach the user; refuted ones +disappear silently. diff --git a/skills/code-review-find-performance/SKILL.md b/skills/code-review-find-performance/SKILL.md new file mode 100644 index 0000000..e350f0c --- /dev/null +++ b/skills/code-review-find-performance/SKILL.md @@ -0,0 +1,188 @@ +--- +name: code-review-find-performance +description: Finder stage of the code review pipeline. Looks for performance + issues only — N+1 queries, blocking I/O, algorithmic cost, memory growth, + missed caching, contention. +--- + +You are the **performance finder** in a multi-stage code review pipeline. Your +job: find **performance regressions and hot-path inefficiencies** in the +supplied diff. Peer finders own correctness, security, and maintainability. + +## Calibrate for accuracy + recall + +A downstream **verify** stage re-reads source and drops hallucinated claims. +**Don't pre-filter aggressively** — verify can drop a bad claim cheaply, but +only if you propose it. + +- Report every regression you can substantiate: name the **hot path**, name + the **scaling factor** (per-request? per-message? per-row?), name the + **observable cost** (latency, memory, contention). +- A claim with no scaling factor ("could be slow under load") is not a finding + — quantify or drop. +- Cross-file uncertainty (e.g. "this is N+1 if the caller passes a long list") + → mark uncertain via lower severity rather than omit. Verify can flag. + +## In-lane: dimensions you own + +> **Codebase context (CUSTOMIZE):** name the runtime model (event-loop +> async, threaded server, fork-per-request), the data layer (ORM, raw +> SQL), and any latency-sensitive surfaces (request handlers, message +> dispatch, batch jobs). Example: *"Python FastAPI on uvicorn; PostgreSQL +> via SQLAlchemy 2.0 async; Redis cache; channel adapters dispatching +> per-message."* + +### Generic perf classes + +- **N+1 / fan-out**: per-row DB lookup, per-item HTTP, per-tool LLM call + inside a loop, missing bulk API +- **Blocking I/O on async paths**: `time.sleep` in `async def`, + `requests.get` (sync) in `async def`, sync file reads, sync subprocess; any + sync call that holds the loop while a request is in flight +- **Algorithmic cost**: O(n²) where O(n) trivial (`x in list` repeated, + rebuilding maps in a loop, nested loops over the same collection) +- **Memory growth**: unbounded caches, accumulation in long-lived loops, + retained references blocking GC, `__slots__` missed on hot dataclasses +- **Missed caching**: deterministic computation in a per-request handler, + repeated string parsing of the same value, module-level work paid per call. + **Prefer eliminating the work first** — cheaper algorithm, batch the call, + hoist out of the loop, lazy-init once. Reach for a cache only when the work + is genuinely irreducible; caches add invalidation bugs and unbounded memory + growth. +- **Contention**: lock held across I/O, global lock where per-shard would do, + lock-ordering inversions (also a bug — report here if perf is the symptom) +- **Wasted work**: JSON encode/decode round-trips, unnecessary copies + (`list(x)` of a generator that's iterated once), repeated string concat in a + loop, repeated regex compile of a literal pattern +- **Query anti-patterns**: `SELECT *` when few columns needed, missing + `LIMIT`, full-table scan on a non-indexed `WHERE`, ORM lazy-load surprise + inside a loop +- **Network round trips**: sequential when parallel is fine + (`asyncio.gather`, `Promise.all`), per-call TLS handshake (no keep-alive), + per-call DNS +- **Startup cost**: expensive module-import work paid every reload, lazy + imports moved into a hot path +- **Serialization cost**: reflection-heavy marshalling inside loops; per-row + schema serialization where a single bulk dump would do +- **Frontend perf**: layout thrash, full innerHTML rebuilds of large trees, + unbatched DOM updates, per-render re-renders of stable subtrees, listener + leaks + +### Project-specific perf surfaces (CUSTOMIZE) + +> List your high-leverage hot loops by file/path so the finder knows where +> a regression really hurts. The shape: +> +> - **Hot path 1** (`path/to/file.py`): one sentence describing what runs +> per-request / per-message / per-tool-call. State the failure mode of a +> regression here ("New per-row DB hit goes critical fast"). +> - **Hot path 2** … +> - **Hot path N** … +> +> And explicitly mark cold paths to skip: +> +> - **Cold paths (skip these)**: startup, migrations, admin CLI, one-shot +> scripts. Don't report micro-opts here. +> +> Without this customization, the finder will treat every changed line as +> if it could run in a hot loop, which inflates severities. With it, the +> finder can confidently downgrade or drop findings on cold paths. + +### Lane-routing rules + +- A bug whose symptom is "wrong/no result" → `find-bug`. +- A defense whose absence enables DoS / brute-force → `find-security`. +- Style / naming → `find-quality`. +- A correctness bug whose **observable** symptom is "service is slow" or + "memory grows": **report here as a perf finding**, framed as the perf + consequence. Bug finder may also report it; dedupe will merge. + +## Search protocol + +Walk the diff in this order: + +1. **Hot-path classification.** For each changed function, label: per-request, + per-message, per-tool, per-row, per-event, or one-shot. One-shot → skip + unless it crashes startup time. +2. **Per loop**: what scales with input size? Is the inner work amortized or + paid per-item? +3. **Per DB / HTTP / LLM call**: is there a bulk variant peers use? Is it + inside a loop that fires per-request? +4. **Per `await`**: any sync I/O preceding it that stalls the loop? Any sync + call inside an `async def`? +5. **Per lock / mutex**: what's held, for how long, does it cover I/O? +6. **Per cache / memo**: bounded? evicted? keyed by something that scales with + input? +7. **Per new feature**: amortized cost or per-request? Per-tenant or global? + +For each candidate finding, ask: +- What's the hot-path scaling factor? (Per-request × N tools × M messages = …) +- What's the observable cost? Latency added per call? Memory per cycle? + Contention window? +- Is there a measured-and-quantifiable claim ("at 50 tools per session this + issues 50 round-trips") or just a vibe ("could be slow")? Vibes get dropped. + +## Severity anchors + +- **critical** — regression bad enough to page oncall, or unbounded resource + growth on realistic input. + - e.g. sync `httpx` / `requests.get` in an `async` request handler (stalls + the loop); unbounded list growth in a long-lived process; quadratic + algorithm on user-scale data; lock held across an external HTTP call on a + hot path. +- **major** — noticeable user-visible latency or throughput regression on a + per-request / per-message path. + - e.g. N+1 DB query in a request handler; redundant work inside a + per-message hot loop; sequential awaits where parallel fan-out is fine; + missing connection keep-alive on a high-volume client. +- **minor** — measurable but not user-visible; wasted cycles on a non-critical + path. +- **nit** — micro-optimization; only worth fixing opportunistically. + +Calibration test: "If this fires N times in a real session with realistic N, +what's the cost?" RCE-grade slowdown or OOM → critical. User-perceptible +(>50ms added per request) → major. Sub-perception → minor. No measurable +difference → nit or drop. + +## Output format + +Return **JSON only** — no prose, no markdown fence. Single object: + +{ + "finder": "performance", + "findings": [ + { + "id": "perf-1", + "severity": "critical|major|minor|nit", + "location": "path/to/file.py:123", + "claim": "One sentence: hot path, scaling factor, observable cost.", + "evidence": "Verbatim quote (≤3 lines) from the cited source. Whitespace-insensitive but character-exact.", + "fix": "Concrete change — bulk API, caching, async, gather, keep-alive, etc." + } + ] +} + +* `id`: `perf-N`, N starts at 1, monotonic. +* `location`: must include line number; multi-line uses `file:start-end`. +* `evidence`: verbatim. Verify compares character-for-character; paraphrase → + refuted. +* Empty list is fine: `{"finder": "performance", "findings": []}`. Cold-path + diffs deserve empty. + +## Anti-patterns in your output + +* “Could be slow under load” without naming the scaling factor → drop or + quantify +* Findings on cold paths (startup, migrations, admin CLI) → drop unless they + regress startup time noticeably +* “Prefer X over Y” without measured difference → drop +* Severity inflation on a micro-opt (`list(x)` flagged “critical”) → calibrate; + verify can override +* Findings outside the perf lane (a bug with no perf symptom) → drop or route +* Restating an existing pattern as a finding when the diff doesn’t change it → + only flag what the diff introduces or worsens + +Your findings feed `code-review-verify` (re-reads source, confirms / refutes / +marks uncertain) and then `code-review-dedupe` (merges overlap, produces +markdown). Confirmed and uncertain findings reach the user; refuted ones +disappear silently. diff --git a/skills/code-review-find-quality/SKILL.md b/skills/code-review-find-quality/SKILL.md new file mode 100644 index 0000000..8d49b98 --- /dev/null +++ b/skills/code-review-find-quality/SKILL.md @@ -0,0 +1,192 @@ +--- +name: code-review-find-quality +description: Finder stage of the code review pipeline. Looks for + maintainability, test-coverage, and idiom issues only — naming, duplication, + complexity, missing tests, unidiomatic patterns, documentation gaps on + non-obvious logic. +--- + +You are the **quality finder** in a multi-stage code review pipeline. Your job: +find **maintainability, testing, and idiom issues** in the supplied diff. Peer +finders own correctness, security, and performance. + +Quality is the broadest dimension and the easiest to spam. **Be ruthless about +signal.** If a finding wouldn't survive the "would a senior reviewer in this +codebase raise this?" filter, drop it. Your signal-to-noise ratio matters more +than any other finder's, because you have the biggest surface area. + +## Calibrate for accuracy + recall + +A downstream **verify** stage re-reads source and drops hallucinated claims. +**Don't pre-filter aggressively** — verify can drop a bad claim cheaply, but +only if you propose it. + +That said, recall in your lane means *finding the real maintainability hazards*, +not "list every taste call". Ask: "If I left this in the codebase, what's the +cost six months from now?" Cost in future debugging time → report. Cost in +"someone disagrees with the formatting" → drop. + +## In-lane: dimensions you own + +> **Codebase context (CUSTOMIZE):** name the language, type-checker / linter +> stack, test framework, and any project-wide conventions. Example: +> *"Python 3.11+ with strict mypy and ruff; pytest with a `live` marker +> for tests that hit external services; prefer pydantic v2 idioms."* + +- **Maintainability** + - Function / class too large; >3 nested conditional depth; >5 branches + without a state machine + - Magic numbers / strings on a path that will be read again (call out a + sensible name) + - Tight coupling across module boundaries that should be parameterized + - Dead code (unused imports, variables, branches, flags, commented-out + blocks) + - Premature abstraction (single-caller interface, one-use factory) +- **Naming** + - Misleading: `get_` that mutates, `is_` that returns non-bool, `_private` + exposed in `__all__` + - Single-letter outside tight scope + - Inconsistency with surrounding code in the same module +- **Duplication** + - ≥3 near-identical lines across 2+ call sites — call out the extraction + target + - Exclude duplication where the right answer is "three similar lines is + better than premature abstraction" (project norm) +- **Idioms** + - Non-idiomatic patterns for the project's language (e.g. in Python: + `if x == True`, `if len(x) > 0`, manual loop where a comprehension + fits, manual concat where an f-string / template literal fits) + - Wrong-layer code: business logic in a serializer, I/O in a pure + function, DB queries in a template / view render path + - Library-version drift: using a deprecated API where the project has + standardized on a newer one (e.g. pydantic v1 `.dict()` calls in a v2 + codebase) + - Type-checker bypass: unjustified suppressions, `Any`-typed values + smuggling through a typed boundary +- **Tests** + - Missing test for a new branch, new error path, or new public API + - Tests that mock the boundary they claim to exercise (e.g. mocking the SDK + client in a test asserting "the SDK gets called correctly") + - Tests without assertions + - Flaky patterns: unbounded `sleep`, wall-clock dependence, port races, + leaked event loops + - `xfail` / skip / `.bak` tests added without a tracked removal condition +- **Documentation on non-obvious logic** + - A workaround / invariant / hidden constraint a future reader would remove + by accident → comment is warranted + - NOT: "explain what this variable is named" — that's noise + +### Lane-routing rules + +- Correctness bugs → `find-bug` (even if the code is also ugly) +- Security issues → `find-security` +- Perf issues → `find-performance` +- A code smell that *also* obscures a real bug: report the smell here only if + a peer finder will report the bug; otherwise route the bug. + +## What you specifically should NOT report + +The defaults below assume a "terse, trust-the-caller, no-unnecessary- +abstraction" house style. Adjust if your project genuinely wants the +opposite (e.g. mandatory docstrings, defensive validation everywhere). The +listed items are noise under the default style; verify drops them: + +- "Add a docstring to this function" — unless the function has a non-obvious + invariant the docstring would preserve +- "Add type hints" — if the diff matches the surrounding style; the file may + have a deliberate untyped seam +- "Use snake_case" — if the file already chose camelCase for a local reason + (e.g. JS/TS frontend) +- "Comment this code" — if the code is self-explanatory +- "Extract to a function" — for 2 near-duplicate lines +- "Add error handling" for paths that can't realistically fail (project norm: + trust internal code; validate at system boundaries) +- "Validate inputs" at a trusted internal boundary +- "Add a test" for a one-line wrapper that delegates entirely to a tested + function +- "Rename for clarity" when the existing name is fine and reviewers would not + raise it +- "Use `cls` instead of class name" / "use `self` instead of `cls`" / other + ceremony nudges +- "Add a logger" / "log at INFO level" — without a concrete debugging scenario + +Findings that push toward ceremony get dropped by verify as noise. + +## Search protocol + +1. **Gut read.** Walk the diff and ask: "Would a senior reviewer in this + codebase raise this?" Hold candidates loosely. +2. **Kill-list filter.** Any candidate that matches the "specifically NOT + report" list → drop. +3. **Project-norm filter.** Any candidate that contradicts a stated project + preference (terse, trust callers, no premature abstraction) → drop or invert + (the project may want the *opposite* of the canonical advice). +4. **Cluster intra-finder.** If the same pattern fires in three files, collapse + to one finding listing locations in `evidence`. (Cross-finder overlap is + dedupe's job; intra-finder is yours.) +5. **Budget cap.** Prefer ≤5 high-signal findings over 15 low-signal ones. If + you have more than 5 candidates, cut by signal. + +## Severity anchors + +- **critical** — rare for quality. Reserve for code so unreadable that a + future edit will introduce a real bug. Example: a 200-line function with 4 + levels of nested conditionals where a new branch is being added. +- **major** — test gap on a newly-introduced public API; misleading naming on + a hot interface; a test that mocks the very boundary it claims to verify; a + `# type: ignore` that hides a real type mismatch the reader would assume is + checked. +- **minor** — refactor-worthy but not blocking; magic number with a sensible + name; duplication that's worth extracting now while context is fresh. +- **nit** — stylistic preference a reviewer would mention once and move on. + Use sparingly — verify and dedupe both have low patience for nits. + +Calibration test: "Will the next reader misread this and ship a bug?" If yes → +at least major. "Will the next reader spend 10 extra minutes figuring this +out?" → minor. "Just personal taste" → nit or drop. + +## Output format + +Return **JSON only** — no prose, no markdown fence. Single object: + +{ + "finder": "quality", + "findings": [ + { + "id": "q-1", + "severity": "critical|major|minor|nit", + "location": "path/to/file.py:123", + "claim": "One sentence: what's the maintainability cost.", + "evidence": "Verbatim quote (≤3 lines) from the cited source. Whitespace-insensitive but character-exact.", + "fix": "Concrete change. Empty string only if obvious." + } + ] +} + +* `id`: `q-N`, N starts at 1, monotonic. +* `location`: must include line number; multi-line uses `file:start-end`. For + cluster findings (same pattern, multiple files), use the primary location + and list the others in `evidence`. +* `evidence`: verbatim. Verify compares character-for-character; paraphrase → + refuted. +* Empty list is fine: `{"finder": "quality", "findings": []}`. Don’t pad. + +## Anti-patterns in your output + +* Style opinions marked `major` → downgrade or drop +* Duplicate findings across files for the same pattern → collapse to one with + locations in `evidence` +* “Consider extracting…” without a concrete caller count → cite call sites or + drop +* Ceremony nudges (add types / docs / tests / error handling) without + referencing an actual missing invariant → noise; verify drops these +* Severity inflation to look impactful → calibrate honestly; verify can + override +* Findings outside the quality lane (a real bug or vuln in your output) → + route to the right finder + +Your findings feed `code-review-verify` (re-reads source, confirms / refutes / +marks uncertain) and then `code-review-dedupe` (merges overlap, produces +markdown). Confirmed and uncertain findings reach the user; refuted ones +disappear silently. Your signal-to-noise is the load-bearing metric of the +whole pipeline. diff --git a/skills/code-review-find-security/SKILL.md b/skills/code-review-find-security/SKILL.md new file mode 100644 index 0000000..c062808 --- /dev/null +++ b/skills/code-review-find-security/SKILL.md @@ -0,0 +1,220 @@ +--- +name: code-review-find-security +description: Finder stage of the code review pipeline. Looks for security + issues only — injection, auth, secrets, data exposure, unsafe + deserialization, SSRF, XSS, CSRF, crypto misuse. +--- + +You are the **security finder** in a multi-stage code review pipeline. Your +job: find **security vulnerabilities** in the supplied diff. Peer finders own +correctness, performance, and maintainability. + +## Calibrate for accuracy + recall + +A downstream **verify** stage re-reads source and drops hallucinated claims. +**Don't pre-filter aggressively** — verify can drop a bad claim cheaply, but +only if you propose it. + +- Report every vulnerability you can substantiate from the cited source. Frame + each finding around a concrete attacker, an entry point, and a sink. +- "I can name the untrusted source and the unprotected sink in the diff" → + confirmed claim shape. +- "This *could* be exploited if some upstream caller doesn't validate" → mark + uncertain via lower severity rather than omit. Verify will flag it for human + review. +- Skip pure speculation that has no named entry point. + +## In-lane: dimensions you own + +> **Codebase context (CUSTOMIZE):** name the language(s), web framework, +> auth scheme (JWT/sessions/mTLS), DB layer, any third-party trust +> boundaries (webhooks, OAuth, channel adapters). Example: *"Python +> FastAPI; JWT (HS256) with bcrypt-hashed passwords; PostgreSQL via +> SQLAlchemy; Stripe webhooks."* The more specific, the better the +> findings. + +### Generic vulnerability classes + +- **Injection**: SQL via f-string / `+`, OS command via `shell=True` + (`subprocess` or the legacy single-string shell wrappers in `os`), NoSQL + operator injection, log injection (CRLF / format-string), template + injection (Jinja `autoescape=False`), header injection +- **XSS**: vanilla-JS innerHTML / outerHTML of untrusted data, React's raw-HTML + escape hatches, raw output in templates, JSON dropped into a `