team-review

# Apache Iggy Team Review `<TARGET>` is the PR number, branch, or ref range supplied after the skill name. With no target, review `HEAD` from its merge base with `origin/master`. Mission critical code. Read [execution instructions](references/execution.md) before starting. Use the section for your client to translate the tool calls below. Include those instructions in worker prompts. Keep this skill's Charter, roles, rounds, and report. 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 deep. You write plain. Keep the two apart. > > **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, do not 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, simple english** When you write technical text (documentation, READMEs, runbooks, procedures, error > messages, release notes, reports), write plain English in the spirit of ASD-STE100 Simplified Technical English, so > that a smart reader outside the field understands it on one read. Obey these rules: > > CLASSIFY FIRST. Procedural text tells the reader what to do: imperative mood, maximum 20 words per sentence, one > instruction per sentence. Descriptive text explains: simple tenses, maximum 25 words per sentence, one topic per > paragraph, maximum six sentences per paragraph. Never mix the two in one passage. > > PLAIN WORDS, for replies and for explanations written for readers outside the field. Use the common word when one > exists ("use", not "utilize"). Define a concept term at its first use, in under ten words, at most one per sentence: > "idempotent (safe to run twice)". Do not define product names, standard names (Postgres, S3, HTTP), or the tool the > document is about. Address the reader as "you". Lead with the point. Procedures and reference documents follow the > rules above alone. > > VERBS. Use only: infinitive, imperative, simple present, simple past, simple future, past participle as adjective. No > present perfect ("has completed" -> "completed"). No "-ing" verb forms ("making it easy" -> new sentence). Active > voice; passive only in descriptions when the agent is unknown. Approved modals: can, will, must. Banned: should, > would, may, might, could. For "should": write "must" if required, delete if optional. > > SENTENCES. Keep complete grammar: no contractions, keep articles, keep "that" ("make sure that the file exists"). Put > conditions before commands, with a comma: "If the test fails, read the log." No semicolons: write two sentences. No > em-dashes: an em-dash hides the logic between two statements. Name the relation ("because", "but", "for example", > "that is") or write two sentences. Use a vertical list for more than two items or steps. > > WORDS. One word, one meaning, for the whole document: use "make sure that" for check/verify/confirm, and > "configuration" for config/settings. Noun chains of maximum three words. Break longer ones with prepositions ("the > timeout value for the connection pool"). Delete words that carry no fact: simply, seamlessly, robust, powerful, > comprehensive, leverage, delve, pivotal, "in order to", "it is worth noting". Do not open or close with chat filler: > "in conclusion", "in summary", "let's dive in", "that being said", "I hope this helps". > > AVOID THE AI DRIFTS. Guard against these by direction: inflated significance ("crucial", "a testament to"), "not just > X, it is Y" reframes, decorative triplets, vague attribution ("studies show"), "it is important to note" asides, and > formatting habits (no emoji as structure, no boldface as decoration). State the fact. The fact carries itself. > Replace: utilize -> use, prior to -> before, in the event that -> if, e.g. -> for example. American spelling. > > WARNINGS. Command or condition first, then the risk: "Do not run this against production. The command deletes rows." > > NEVER TOUCH. Code blocks, identifiers, CLI commands, file paths, quoted error messages, product names. Each counts as > one word toward sentence limits. Facts too: when the source does not give a number or a cause, keep the general > statement. Do not invent specifics. > > SELF-CHECK before returning: scan for contractions, "has been", "should", ", making", semicolons, em-dashes, and the > deleted-word list above. Count words in your three longest sentences and split any over the limit. Collapse synonym > rotation. > > REPLIES TO THE USER. The same rules apply to the chat reply, at the descriptive limits (25 words per sentence, simple > tenses, active voice, no contractions). Start with the answer or the result. If a concept term is necessary, define it > in a few words. Do not restate the request. Keep the whole reply to 5 sentences or fewer, code and lists excluded. Do > not add openers ("Certainly", "You're absolutely right") or closers ("I hope this helps"). Do not shorten quoted > errors, security warnings, or confirmations before a destructive action. > > Four review rules take precedence: > > - Write one sentence for the problem, 25 words or fewer. Write one sentence for the fix, imperative, 20 words or > fewer. > - Name the actor in the problem sentence: "The writer drops the flush error", not "The flush error is dropped". > - NEVER TOUCH covers these too: `file:line`, code, identifiers, file paths, quoted errors, technical terms, severity > labels, confidence labels. Keep them EXACT. > - These writing rules govern your own output only. Never review the code, the comments, or the commit messages against > them. > > **Finding format.** > > - 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`. > > Deep analysis, plain words. Dig deep. Write short and clear. ## Step 1: Identify the target (no reading) Prepare the target and artifacts as the execution instructions specify. Use the resolved starting commit and reviewed head for every round. Do not read the diff or source files in the moderator context. Do not `cat` any of the files you just wrote. `wc -l <DIR>/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: <role>-<TOPIC>` (bare role names collide with concurrent sessions: one shared agent namespace), no `model` (inherits). Prompt = role block + Charter + this brief, with `<DIR>`, `<TARGET>`, `<SHORTCOMMIT>` filled in: > Target: `<TARGET>` at `<SHORTCOMMIT>`. Diff: `<DIR>/diff.patch`. Changed files: `<DIR>/files.txt`. PR title and body: > `<DIR>/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 `<DIR>/<role>.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 <DIR>/*.md`. A role with no file gets one `SendMessage` nudge to `<role>-<TOPIC>` ("Write `<DIR>/<role>.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 `<DIR>/claims.md`, one line per claim: `C<N> [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-<k>-<TOPIC>` / `sweep-<TOPIC>`. Each gets ONLY: its claims verbatim, `<DIR>/files.txt`, `<DIR>/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<N>: PASS | FIX: <correction, correct line, correct severity> | REMOVE: <why false or unverifiable>`; judge whether the severity is calibrated; re-check the anchor. Deliverable `<DIR>/validate-<k>.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 `<DIR>/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 `<DIR>/contested-<N>.md`. Cap 5 per run; past the cap you adjudicate and mark `(moderator call)`. ## Step 6: Synthesize, write, done Output in the simple English of the Charter. The Charter binds you too: ```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) Verification: <actual commands and outcomes, or none ran>. ``` Then write `<DIR>/report.md` with: 1. H1 `# Iggy Team Review - <change desc> (<SHORTCOMMIT>)`. 2. Metadata, one line each: target `<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: <DIR>/report.md`. No cleanup: one-shot agents end themselves, `<DIR>` keeps its artifacts.