diff --git a/.claude/skills/team-review/SKILL.md b/.claude/skills/team-review/SKILL.md new file mode 100644 index 0000000000..5d43f6117a --- /dev/null +++ b/.claude/skills/team-review/SKILL.md @@ -0,0 +1,117 @@ +--- +name: team-review +description: Adversarial 4-expert review (storage, perf, distsys, ecosystem) of a PR, branch, or ref range, with clean-room validation of every finding. Experts work alone, no peer debate. Expensive, one run spawns ~10 subagents. +argument-hint: "[PR number | branch | ref range]" +disable-model-invocation: true +--- + +# Apache Iggy Team Review + +`` = `$ARGUMENTS`: a PR number, a branch, or a ref range. Empty means `origin/master..HEAD`. Mission critical code. + +You = **moderator**. You never open the diff or a source file: you route paths, merge claims, synthesize. Every token you load rides along every later turn. Reviewers and validators are one-shot agents that deliver by writing a file; nobody chats. + +## Charter (paste VERBATIM into every expert, validator, and tiebreak prompt) + +> You think big brain. You speak caveman. Separate things. +> +> **Thinking, unchanged.** Read the diff, then every changed file in full from the local checkout, then whatever call sites you need. Trace call chains. Verify invariants. Prove findings, don't guess. Cite exact `file:line`. Running tests or builds needs a stated justification: reading and tracing settles most claims, and parallel cargo runs block on one target-dir lock. +> +> **Output style.** Drop articles, filler, pleasantries, hedging. Fragments OK. Keep EXACT: `file:line`, error quotes, code, technical terms, severity and confidence labels. +> +> - Finding, one line each: `[sev] file:line - problem. Fix: action. (origin, conf:H|M|L)` +> - `sev`: `critical` = correctness/safety/data-loss/security, blocks merge; `warning` = real defect, perf hit, API issue; `nit` = style/naming; `simplify` = complexity/dead-code reduction, format `[simplify] file:line - what's complex. Simpler: alternative. Saves: ~N lines / removes indirection. (origin, conf)`. +> - `origin`: `intro` (PR introduced), `pre-surfaced` (existed, exposed by PR), `pre-untouched` (existed, not touched). +> - Never flag em dashes or other punctuation style as a finding. +> - Simplification mandate: less code > more code. Per changed file ask whether ~30% smaller keeps correctness: dead fields/params/branches/imports, duplication of an existing helper (cite it), single-impl traits, premature generics, checks for impossible states. Do not propose simplifications that change semantics or break public API. If nothing qualifies, write `Simplifications: none`. +> +> Caveman = output compression, not analysis compression. Dig deep. Write short. + +## Step 1: Identify the target (no reading) + +- Classify ``: matches `^#?(pr)?[0-9]+$` case-insensitively -> PR, the digits are ``. Anything else -> ref range or bare branch. Empty -> review `origin/master..HEAD`. +- ``: `` lowercased, chars outside `[a-z0-9-]` replaced by `-`, repeats collapsed, trimmed, max 40 chars (`PR3123` -> `pr3123`, `origin/master..HEAD` -> `origin-master-head`). Empty -> `date +%s`. +- `` = `/review-`. `mkdir -p` it. +- PR: `gh pr view --json title,body,headRefOid > /pr.json`, `gh pr diff > /diff.patch`, `gh pr diff --name-only > /files.txt`. `` = first 8 of `headRefOid`. +- Ref range or bare branch: `git diff $(git merge-base origin/master HEAD)..HEAD > /diff.patch`, same with `--name-only`, `` = `git rev-parse --short=8 HEAD`. No `pr.json` on this path. +- Guard: `git rev-parse HEAD` must equal the reviewed head. Experts read the local checkout; if it differs, stop and ask the user to check out the reviewed head. +- ``: 1-3 word `snake_case` summary, `[a-z0-9_]`, <= 24 chars. From the PR title; no PR -> from `git log -1 --format=%s`. +- Report path: `/report.md`. + +Do not `cat` any of the files you just wrote. `wc -l /diff.patch` is the only look you take. + +## Step 2: Round 1, four one-shot experts (one message, parallel) + +Spawn 4 `Agent` calls in a single message: `subagent_type: general-purpose`, `name: -` (bare role names collide with concurrent sessions: one shared agent namespace), no `model` (inherits). Prompt = role block + Charter + this brief, with ``, ``, `` filled in: + +> Target: `` at ``. Diff: `/diff.patch`. Changed files: `/files.txt`. PR title and body: `/pr.json` (drop this sentence when there is no PR). Classify each finding's origin; check existing codebase conventions before calling a deviation `intro`. +> Deliverable = the file `/.md`, written with the Write tool BEFORE you end your turn: findings in Charter format, then `Simplifications: ...`, then `Verdict: APPROVE | REQUEST CHANGES - reason`. A previous worker finished reading and then idled without delivering; the Write call IS the delivery, your final message is just the path. Budget 3/4 reading, 1/4 writing; partial beats unshipped. +> You work alone: no teammates, no SendMessage, no questions back. + +Role blocks: + +- **storage**: Senior storage/DB engineer, 15 years of WAL, B-trees, LSM, crash recovery, fsync semantics. Paranoid about data loss; demands proof data survives power loss, partial writes, bit rot. Focus: data-structure invariants, state machines, ownership/lifetimes, resource leaks, error paths, crash recovery, write atomicity. Simplify: redundant state, dead error variants, unreachable transitions, duplicated lifecycle logic. +- **perf**: Performance engineer / kernel dev. Flamegraphs, cache lines, io_uring, allocators. Hostile to clones, heap allocs in hot paths, blocking in async, but honest about hot vs cold: never rate a cold-path clone critical. Focus: allocation hot paths, lock contention, syscall overhead, buffer management, zero-copy. Simplify: trait dispatch where a direct call suffices, redundant buffering, manual loops with an idiomatic equal-perf form. +- **distsys**: Distributed-systems architect, formal methods. TLA+, linearizability, "message arrives twice / out of order / never". For every finding trace the actual call path; theoretical concerns without a reachable path are not findings. Focus: safety invariants, TOCTOU, unsafe soundness, overflow, panics in libs, deadlocks, comment/code contradictions, protocol and ser/de compat. Simplify: predicates enforced twice, unreachable branches, control flow that hides an invariant. +- **ecosystem**: SDK and API ecosystem lead across the client languages. Focus: public API ergonomics, breaking changes, type safety at boundaries, naming consistency, error message clarity, input validation, doc gaps. Simplify: API surface bloat, single-impl traits, wrapper types adding no safety, builders for 1-2 fields, unused re-exports. + +Collect: wait for the completion notifications, then `ls /*.md`. A role with no file gets one `SendMessage` nudge to `-` ("Write `/.md` now, then stop."); still missing after that, respawn the role once with the same prompt. Never open a subagent transcript via `TaskOutput` (it is the whole JSONL). + +## Step 3: Merge into neutral claims (moderator) + +Read the 4 role files. Write `/claims.md`, one line per claim: `C [sev] file:line - claim. Fix: action. (origin)`. Strip role names, confidence, and argument. Same anchor + same defect from several roles = one claim at the highest severity; keep a private raised-by map for the report. Simplify items are claims too. + +No claims at all: skip Steps 4 and 5, go to Step 6 with empty sections and `Verdict: APPROVE`. The report file still gets written. + +## Step 4: Clean-room validation (one message, parallel) + +Shard claims ~5 per validator. Spawn one `Agent` per shard plus one sweep validator, all in one message: `subagent_type: general-purpose`, `model: opus`, `name: validator--` / `sweep-`. Each gets ONLY: its claims verbatim, `/files.txt`, `/diff.patch`, the target identity, the Charter. Not the role files, not raised-by, not your reasoning; the missing context is what removes the anchoring bias. + +Validator mandate (adversarial): for each claim open the cited `file:line`, trace call sites, then rate `C: PASS | FIX: | REMOVE: `; judge whether the severity is calibrated; re-check the anchor. Deliverable `/validate-.md` via Write, same idle rule as Step 2. + +Sweep mandate: all claims + the diff. Two questions only: which real defects in the diff are missing from the list, and which listed items wrongly clear a bug. Deliverable `/sweep.md`, additions in Charter format tagged `(sweep)`. + +Apply: drop REMOVE, apply FIX (wording, line, severity), fold sweep additions in as `(sweep, unvalidated)`. A `critical` sweep addition gets one extra validator before it may block the verdict. + +## Step 5: Contested items (only when triggered) + +Contested = a validator REMOVEs or downgrades a `critical` or `warning`, or a sweep addition contradicts a PASS. Per item spawn one `Agent` (`model: opus`) with the claim, the validator's verdict text, the expert's original line, and the paths; it writes `UPHELD | OVERTURNED - reason (cite path)` to `/contested-.md`. Cap 5 per run; past the cap you adjudicate and mark `(moderator call)`. + +## Step 6: Synthesize, write, done + +Output in caveman style: + +```text +## Review: [change desc] + +### Confirmed (expert + clean-room validator) +- [sev] file:line - problem. Fix: action. (raised: role[, role]; validated: PASS|FIX) + +### Contested +- file:line - problem. + Expert: position. Validator: counter. **Tiebreak**: UPHELD|OVERTURNED - why. + +### Retracted (validator REMOVE) +- finding - why. + +### Pre-existing (origin pre-*, not blocking) +- file:line - follows pattern in [ref]. + +### Simplification opportunities (non-blocking) +- file:line - current shape. Simpler: alternative. Saves: ~N lines / removes indirection. + +### Verdict: APPROVE | REQUEST CHANGES +Confirmed critical + warning only. Simplifications informational. Reason: one line. + +Counts: critical N, warning N, nit N, simplify N (Confirmed + Simplification sections) +``` + +Then write `/report.md` with: + +1. H1 `# Iggy Team Review - ()`. +2. Metadata, one line each: target ``, reviewed commit, ISO timestamp, roles, validator count, contested count. +3. The report above, verbatim. +4. Appendix `## Raw findings per expert`: each role file verbatim in a fenced block. +5. `## Validation record`: counts of PASS / FIX / REMOVE, sweep additions, contested outcomes. + +Last user-facing line: `Findings written: /report.md`. No cleanup: one-shot agents end themselves, `` stays in the scratchpad. diff --git a/AGENTS.md b/AGENTS.md index 5e74d5a6ff..ada37f1457 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -6,11 +6,10 @@ Transports: QUIC, WebSocket, TCP (custom binary), HTTP (REST). SDKs: Rust, .NET, Java, Python, Go, C++, Node.js. A connectors subsystem ingests from / egresses to external systems via dlopened plugins. -> Skills under `.claude/skills/` are currently scoped to the -> **connectors** subsystem (`core/connectors/`). Load +> Skills live under `.claude/skills/`. Load > [connectors-overview](.claude/skills/connectors-overview/SKILL.md) -> first for any change there. Other subsystems follow the repo-wide -> principles in this file. +> first for any change under `core/connectors/`. Other subsystems +> follow the repo-wide principles in this file. ## Contents @@ -121,8 +120,8 @@ iggy/ ## Skills -Connectors-scoped. Each `SKILL.md` has YAML frontmatter (name, -description). Load `connectors-overview` first as router. +Each `SKILL.md` has YAML frontmatter (name, description). For +connectors work, load `connectors-overview` first as router. - [connectors-overview](.claude/skills/connectors-overview/SKILL.md) - router + universal connector rules - [connector-runtime](.claude/skills/connector-runtime/SKILL.md) - FFI host, lifecycle, state, metrics @@ -132,6 +131,12 @@ description). Load `connectors-overview` first as router. - [connector-transform](.claude/skills/connector-transform/SKILL.md) - transform authoring - [connector-testing](.claude/skills/connector-testing/SKILL.md) - unit + integration test patterns +Repo-wide, user-invoked only. `disable-model-invocation: true` keeps it +out of the agent's context; do not replicate its steps. When a +non-trivial change passes verification, suggest `/team-review `. + +- [team-review](.claude/skills/team-review/SKILL.md) - adversarial 4-expert PR/branch review, ~10 subagents per run + ## Repo-wide principles 1. **Apache 2.0 header on every new source file:** Follow the comment style configured in `licenserc.toml`; common examples: `.rs` uses `// ...`, `Cargo.toml` and shell use `# ...`.