Review Pr Team

Agent Team PR Review — NVIDIA-NeMo/RL

Review a pull request using a coordinated team of specialized agents.

Repo: NVIDIA-NeMo/RL

Requires: CLAUDE_CODE_EXPERIMENTAL_AGENT_TEAMS=1 must be set — agent teams (teammate spawning, shared task list, SendMessage) are an experimental Claude Code feature.

Sandbox: Always pass dangerouslyDisableSandbox: true on every Bash tool call in this skill. The sandbox's bubblewrap (bwrap) container fails to initialize in worktree/container environments with bwrap: loopback: Failed RTM_NEWADDR: Operation not permitted, breaking ALL commands — not just network-dependent ones like gh. Do NOT attempt any Bash call in sandbox mode first; it will fail and waste a round-trip.

Phase 0: Parse & Validate

Extract $PRNUM from $ARGUMENTS. A PR number is required.

gh pr view $PRNUM --repo NVIDIA-NeMo/RL --json number

If invalid, ask the user for a valid PR number.

Phase 1: Setup & Context

1.1 Checkout PR

git fetch origin pull/$PRNUM/head:pr-$PRNUM-team-review
git checkout pr-$PRNUM-team-review
git submodule update --init --recursive

1.2 Gather PR metadata (parallel)

Run these in parallel:

# PR metadata (include mergeable to detect conflicts)
gh pr view $PRNUM --repo NVIDIA-NeMo/RL \
  --json title,body,author,baseRefName,headRefOid,labels,files,comments,reviews,reviewRequests,mergeable,mergeStateStatus

# Full diff
gh pr diff $PRNUM --repo NVIDIA-NeMo/RL

# Inline review comments
gh api repos/NVIDIA-NeMo/RL/pulls/$PRNUM/comments

Record: $TITLE, $AUTHOR, $BASE_BRANCH, $HEAD_SHA, changed files list, existing comments, existing reviews.

Merge conflict check: If mergeable is "CONFLICTING" or mergeStateStatus is "DIRTY", include a prominent note in the review asking the author to rebase their PR on $BASE_BRANCH and resolve conflicts before further review. Add this as the first item in the review body.

Read the PR description (body) carefully. It contains the author's intent, motivation, and test plan. Also parse it for linked issues (patterns like Fixes #123, Closes #456, Related: #789). For each linked issue, fetch it:

gh issue view <ISSUE_NUM> --repo NVIDIA-NeMo/RL --json title,body,comments,labels

The PR description + linked issues + diff together form the full context. All agents should have access to this context so they understand why the change is being made, not just what changed.

1.2a Performance & convergence evidence check

If the PR touches code that could affect performance or convergence (e.g. new training features, optimizer changes, CUDA graph support, kernel changes, parallelism config), check the PR description and comments for quantitative evidence:

• New feature: Author should provide baseline numbers (throughput, tokens/sec, memory, convergence curves) demonstrating the benefit of the feature.
• Existing feature modification: Author should show before/after comparison or prove no regression (e.g. convergence curves from an A/B run).
• If evidence is missing, flag it as a [PERF-EVIDENCE] finding in the review. This is especially important for features that users will enable in production — they need to know the expected benefit and any trade-offs.

Record whether evidence was found: $PERF_EVIDENCE_FOUND (yes/no/not-applicable). Pass this to the rl-expert agent in its prompt.

1.3 Determine touched submodules

gh pr diff $PRNUM --repo NVIDIA-NeMo/RL --name-only | grep -E '^3rdparty/'

Map paths to submodule names:

• Automodel-workspace/ → spawn expert-automodel
• Megatron-Bridge-workspace/ → spawn expert-megatron-bridge
• Megatron-LM-workspace/ → spawn expert-megatron-lm
• Gym-workspace/ → spawn expert-gym

1.3a Upstream reference lookup

When the PR adds or modifies config that wraps Megatron-LM TransformerConfig fields, agents MUST check how Megatron-Bridge sets those same fields. Megatron-Bridge is the canonical integration layer and often shows the correct, non-deprecated API:

1. Get submodule SHAs for permalinks: git ls-tree HEAD 3rdparty/Megatron-LM-workspace/Megatron-LM and git ls-tree HEAD 3rdparty/Megatron-Bridge-workspace/Megatron-Bridge
2. Search Megatron-Bridge for the config field name: grep -rn <field_name> 3rdparty/Megatron-Bridge-workspace/Megatron-Bridge/
3. Read the Megatron-Bridge code to understand the pattern (what field it sets on recipe.model, how it handles RNG, scope validation, etc.)
4. Read the Megatron-LM TransformerConfig field definition and docstring for the canonical documentation of valid values, defaults, and deprecation status
5. Pass the submodule SHAs and relevant Bridge/LM permalinks to all agents in their prompts

This ensures review comments can point authors to the established pattern rather than just saying "this is wrong."

1.4 Read root context

• Read CLAUDE.md from repo root
• Read all .claude/skills/*/SKILL.md files (except review-pr and review-pr-team)
• Glob ~/.claude/review-memory/RL/*.md — read EVERY match. These are durable, cross-session PR-review lessons for this repo (NVIDIA-NeMo/RL). Treat them as binding guidance and pass their content to every spawned agent in its prompt. (Scoped by repo name so lessons never bleed across repos.) This store is personal and machine-local — it may be empty or absent on first use; an empty glob is fine. New lessons are written back here in Phase 6.

1.5 Detect GPU availability & set up local testing

nvidia-smi --query-gpu=name,memory.total --format=csv,noheader 2>/dev/null | head -1

If GPUs are available, set $GPU_TESTING_AVAILABLE = true. All agents run tests locally via uv run — no Docker container needed. On first run, uv sync resolves all deps (torch, vLLM, CUDA) directly on the host (~5 min first time, instant after).

If no GPUs: set $GPU_TESTING_AVAILABLE = false. Agents should note "not verified — no GPU environment" in their findings. Test-agent can still do CPU-only work (grep, read, collect-only).

1.5b Kick off the repo linters (background)

Start the repo's own lint/format/type suite in the background early, so results are ready by collation:

uv run --group dev pre-commit run --all-files

Do NOT run ruff/pyrefly directly — pre-commit runs the repo's pinned hook versions and config, and calling the tools directly can use the wrong versions/args and produce misleading results. When collating, attribute findings ONLY to files in the PR diff (gh pr view $PRNUM --json files); --all-files also surfaces pre-existing issues unrelated to this PR — don't report those. Also flag any NEW source file in the diff that isn't added to pyrefly.toml project-includes (untracked files silently escape type-checking).

1.6 The team is implicit — no setup step

With CLAUDE_CODE_EXPERIMENTAL_AGENT_TEAMS=1 set, agent teams form automatically: the current session is the team lead, and the team is created the moment you spawn the first teammate via the Agent tool. There is no TeamCreate step (that tool was removed; a session has exactly one implicit team). You do not name the team — refer to the review as "the PR #$PRNUM review" in prompts for human readability only.

Phase 2: Create Tasks & Spawn Agents

2.1 Create tasks

Create all tasks upfront using TaskCreate, then set dependencies with TaskUpdate.

Wave 1 tasks (parallel, no blockers):

Wave 2 tasks (blocked by ALL Wave 1 tasks):

Wave 3 task (blocked by Wave 2, leader does this):

Set dependencies:

• challenge-findings blockedBy all Wave 1 task IDs
• collate-review blockedBy challenge-findings

2.2 Spawn agents (parallel)

Spawn all agents in a single message with multiple Agent tool calls. Give each agent a unique name (so teammates can address each other via SendMessage). Do not pass team_name — that input is ignored; spawning a teammate joins the session's single implicit team automatically. You may set subagent_type to reuse a defined role; its tools/model are honored and its body is appended to the teammate's prompt (SendMessage and the task tools are always available regardless of any tools allowlist).

Agent Prompt Specifications

Common preamble (include in EVERY agent prompt)

You are a member of the "pr-$PRNUM-review" team reviewing PR #$PRNUM on NVIDIA-NeMo/RL.

PR: #$PRNUM "$TITLE" by $AUTHOR (base: $BASE_BRANCH, head: $HEAD_SHA)

PR Description:
$PR_BODY

Related Issues:
$RELATED_ISSUES (title, body, and key comments for each linked issue)

Instructions:
- Read and understand the PR description and related issues FIRST — they explain the author's
  intent, motivation, and test plan. Review the code in that context.
- Check TaskList for tasks assigned to you. Claim your task with TaskUpdate(status="in_progress").
- Use Read, Glob, Grep for local file lookups (faster than gh CLI).
- Do NOT git checkout other commits — use `git show <sha>:<path>` for history lookups.
- After any git operation that changes commits: `git submodule update --init --recursive`
- Include GitHub permalinks in ALL findings: https://github.com/NVIDIA-NeMo/RL/blob/$HEAD_SHA/<path>#L<line>
- **Prose permalinks**: When prose text mentions a specific function, class, method, config
  field, or sentinel value by name (e.g. `get_replay_topk()`, `_install_missing_route_fallback_patch`,
  `R3_MISSING_ROUTE_SENTINEL`), wrap the `<code>` span in an `<a href>` permalink to where it's
  defined. This applies to review comments, HTML explainers, and any other output. Readers
  should never have to grep for something mentioned by name — every named code reference
  should be one click away.
- **Evidence permalinks**: When claiming behavior exists in upstream code (Megatron-LM,
  Megatron-Bridge, etc.), include a permalink to the EXACT line in the upstream repo that
  proves it. Use the submodule's GitHub repo + pinned SHA (leader will provide these).
  Don't just say "transformer_config.py:767 says deprecated" — link to it so the reader
  can click through. Quote the relevant snippet inline for quick scanning.
- **External reference implementation claims**: When claiming "standard implementations
  do X" (e.g. verl, TRL, OpenAI baselines, DeepSpeed-Chat), you MUST fetch the actual
  source code (via WebFetch on the raw GitHub URL or `curl`) and provide a permalink to
  the exact line that proves the claim. Use a pinned commit SHA, not `main`. Never claim
  "verl does X" or "TRL does Y" based on memory alone — verify and link.
- **Compare new components to their nearest existing analog.** When the PR adds a component that
  parallels an existing one (a new worker group ↔ `lm_policy.py`, a new advantage estimator ↔ the
  existing estimators, a new config block ↔ `MasterConfig`), diff the new one against the established
  pattern and flag missing affordances: backend dispatch, override hooks (e.g. `resolve_policy_worker_cls`),
  guards/validation, type annotations, and return-shape consistency. "It works for the shipped recipe" is
  not enough if the new component silently diverges from its sibling's contract.
- **Fail-loud check.** For each new config option / branch, ask: what does the worst plausible misconfiguration
  do? If it silently produces wrong results (zeroed logprobs, an unrouted alias, an ignored backend, a config
  combination no warning covers), flag it and suggest a setup-time assert/raise.
- **Verify the premise before reporting.** When a finding hinges on how an external tool/API/env-var behaves
  (SLURM, Ray, Megatron, transformers, …), verify it against source/docs with a permalink FIRST. Never inflate
  your stated confidence to clear a threshold without checking the underlying fact — that launders speculation
  into an authoritative-looking comment.
- **Make every finding actionable.** Lead with the observation, then a bolded **Action:** line naming the exact
  file/function to change and (when known) the concrete snippet/schema. Drop pure editorializing ("looks
  intended", reassurance) that doesn't lead to an action.
- **Open every comment by declaring its actionability, before any prose.** The author must know what they
  are expected to DO before they read why. The first line is one of:
    - `**No action needed** — <one clause>.` for pure FYI/context.
    - `**1 action item.**` / `**2 action items, 1 follow-up.**` when there is work.
  Then make the body match that count: label each ask `### AI-1`, `### AI-2` …, label deferred work
  `### Follow-up` with its tracking reference, and put pure background under a final `**Context — no
  action.**` heading. If an ask should be done in THIS PR rather than deferred, say so in the opening line
  ("please fix in this PR") and do not also offer a tracked fallback — an escape hatch beside an ask is
  read as permission to skip it.
- **Any comment over ~1500 characters needs a one-line TL;DR**, bolded, immediately after the actionability
  declaration and before any prose. State the defect and its consequence, not the topic: "**TL;DR — in
  colocated mode the `mcore_generation_config` model keys are silently discarded, so `policy.megatron_cfg` is
  the only route left**", never "TL;DR — about how config keys are handled". Reviewers skim; a long comment
  with no TL;DR gets skipped entirely, which is strictly worse than a short comment that lands.
  Two corollaries. If you cannot compress the finding into one line, it is usually TWO findings — split them
  into separate comments anchored at their own lines, rather than one comment with numbered sections; several
  focused comments are far easier to act on than one that has to be read in full before any of it can be.
  And if the user ever asks you to "tl;dr" a comment you already posted, treat that as a defect report on the
  comment itself: shorten it in place and split out whatever forced the length.
- **Never blend an ask and a "tracked elsewhere" note in one paragraph.** A comment that says both
  "tracked in <issue>" and "please change X" without separating them is the most common way review
  feedback gets silently dropped: the author reads "tracked" and closes the thread. When you cite a
  tracking issue, cite the SPECIFIC item (a stable id and title, not a bare issue link) and state in the
  same sentence whether it covers the ask or only the part you are NOT asking for.
- **Be ruthlessly succinct — length is a cost the author pays.** Write the shortest comment that still lands,
  then cut again. Before each sentence ask "does deleting this change what the author does?" If no, delete it.
  Cut: restatements of what the code plainly does, hedging preambles, evidence the author needn't act on, and
  recaps of your own investigation. Reviewers skim — an over-long comment gets skipped entirely, so it is
  WORSE than a short one even when every sentence is true. The breaking-case walkthrough and the mechanism
  explanation below are the only licensed exceptions, and both have their own tight budgets.
- **If the finding turns on a mechanism the reader may not hold in their head, walk them to the punchline.**
  Your reader is a subject-matter expert — do not explain RL, distributed training, or the codebase to them.
  What they lack is not knowledge but *paged-in context*: naming a mechanism ("the `prepare_refit_info`
  handshake") forces them to reconstruct it from the diff before they can judge the finding. Most won't, and
  the finding dies unread. So reload that context for them, tersely, in this order, stopping as soon as the
  defect is self-evident:
  1. **Background** — the one sentence of context the finding sits on ("every step, weights are copied
     trainer→engine; that's the refit").
  2. **What problem the mechanism solves** — why it exists at all. Usually "side A doesn't know X, only side B
     does", which is exactly what makes the design non-obvious.
  3. **How it works** — numbered steps, one line each, each anchored to its permalink.
  4. **What breaks** — the defect stated against that scaffolding, ending in the observable symptom.
  Budget: 4-8 lines before the **Action:**. Needing more usually means it's two findings. Skip steps 1-2 when
  the mechanism is obvious from the diff — this rule is for handshakes, protocols, caches, lifecycles and
  cross-process contracts, not for a null check. Terseness is the point: a paragraph that teaches an expert
  something they already know is as costly as one that omits what they don't.
- **Show the breaking case, don't just assert it.** Whenever a finding HAS a concrete failing case, walk the
  reader through it — abstract prose ("this isn't validated", "this could starve the policy") is easy to wave
  away; a config plus a trace is not. Two acceptable forms:
  1. **Walk the failure.** Show the triggering input (a YAML block, an argument combination, a call), then the
     ordered steps from input to symptom, each anchored to the line that fails to stop it, ending in the
     observable symptom the author would actually see (the exact error text, a silent wrong value, a 180s
     stall). Include the step where it *succeeds when it shouldn't* — that's usually the crux.
  2. **Give a repro.** "Run `<recipe/command>` with `<config delta>` and you'll see `<symptom>`", or a short
     self-contained snippet. Fine when it's genuinely short.
  Rules: the trace must be TIGHT — a config block plus 3-5 one-line steps, not an essay; every step that
  claims code behaves a certain way gets a permalink to that line; and quote the real symptom string rather
  than paraphrasing it. If the premise depends on an external tool's semantics (Ray `PACK` vs `STRICT_PACK`,
  a SLURM flag), link the upstream source that defines it. If you CAN'T construct a failing case, say so
  plainly and downgrade the finding — "I could not construct a config where this breaks" is honest and often
  means it shouldn't be posted at all.
- **State whether the defect is PR-introduced or pre-existing/adjacent,** in one clause, near the top. Asking
  an author to fix something they didn't break is legitimate but must be labelled as such ("adjacent footgun
  rather than something this PR broke — but <what the PR changes about it>"), or the comment reads as a
  false accusation.
- **When proposing a guard/assert, show it blocks nothing legitimate.** One line is enough ("`gpus_per_node`
  exceeding the node's GPU count is never a valid configuration"). If a real user might deliberately want the
  configuration you're proposing to reject, it's not an assert — it's a design discussion, and a much weaker
  finding.
- **Prefer a `suggestion` block whenever a fix is a concrete edit to line(s) in the diff.** A GitHub
  `suggestion` block is directly committable, so it is far more actionable than prose — reach for it by default
  for any clear code/docs fix, not just docstrings and tests. Don't force one where it doesn't fit: if the fix
  spans multiple files, targets a line not in the diff, or is an operational/config/verification ask, give the
  concrete change in prose. When several valid implementations exist, still show one but label it explicitly as
  **one way to do it** (e.g. "one option — of several: …") so the author reads it as illustrative, not
  prescriptive.
- Report ALL findings — never limit, truncate, cap, or summarize. Every issue you find must be reported.
- When done, mark your task completed with TaskUpdate and send your findings to the leader via SendMessage.

rl-expert

You are the RL codebase expert.

Scope: nemo_rl/, tests/, examples/, docs/, root-level config files.

FIRST: Dynamically discover ALL coding guidelines:
1. Glob `.claude/skills/*/SKILL.md` at the repo root — read every match
2. Read `CLAUDE.md` at the repo root
Do NOT hardcode skill names. Skills may be added, removed, or renamed.

Tasks:
1. Analyze the PR diff against ALL loaded guidelines
2. For each changed file, read surrounding context with Read/Grep to understand changes
3. Categorize findings as [BUG], [TEST], [GUIDELINE], [DOC] with file:line and permalink
4. DOCSTRING REVIEW: For any new or modified public function/class/method missing a docstring,
   draft the docstring as a GitHub suggestion block. Before finalizing, confer with the relevant
   submodule expert(s) via SendMessage to verify accuracy of param descriptions and return types.
   Only include the suggestion after expert confirmation. Category: [DOCSTRING]
5. PERFORMANCE/CONVERGENCE EVIDENCE: If the PR touches code that could affect performance or
   convergence (training features, optimizer changes, CUDA graphs, kernels, parallelism), scan
   the PR description AND all PR comments for quantitative evidence:
   - New features: expect baseline numbers (throughput, tokens/sec, memory) or convergence curves
   - Modifications: expect before/after comparison or proof of no regression
   If evidence is missing, report as [PERF-EVIDENCE] with a specific ask: what numbers/curves
   the author should provide given the nature of the change.

You are also available to answer questions from other agents via SendMessage.

expert-{submodule}

One agent per touched submodule. Adjust the submodule path accordingly:

• expert-automodel: 3rdparty/Automodel-workspace/Automodel/
• expert-megatron-bridge: 3rdparty/Megatron-Bridge-workspace/Megatron-Bridge/
• expert-megatron-lm: 3rdparty/Megatron-LM-workspace/Megatron-LM/
• expert-gym: 3rdparty/Gym-workspace/Gym/

You are the {SUBMODULE_NAME} subject matter expert.

Scope: {SUBMODULE_PATH}

FIRST: Dynamically discover domain knowledge within your submodule:
1. Glob `{SUBMODULE_PATH}/**/SKILL.md` — read every match
2. Glob `{SUBMODULE_PATH}/**/CLAUDE.md` — read every match
Do NOT hardcode skill names. Different submodules store skills in different locations.

Tasks:
1. Analyze the PR diff for changes within your submodule scope
2. Verify correct API usage — check function signatures, return types, semantics against actual code
3. Report findings with file:line and permalink

UPSTREAM BUG DETECTION: When the PR contains a workaround for behavior in your submodule,
determine if the underlying cause is a bug in the upstream submodule itself. If so:
- Report it as category [UPSTREAM]
- The review comment should: (a) acknowledge the workaround is correct and should stay,
  (b) explain the upstream bug with a permalink to the relevant upstream code,
  (c) suggest filing an issue against the upstream repo (include the repo issues URL)

You are also available to answer questions from other agents (especially rl-expert for docstring
verification and comment-reviewer for context). Respond via SendMessage.

test-agent

You are the test reviewer and test author.

Scope: tests/ directory and any test files in the PR.

FIRST: Dynamically discover guidelines:
1. Glob `.claude/skills/*/SKILL.md` at the repo root — read every match (especially the testing skill)
2. Read `CLAUDE.md` at the repo root
3. Read `tests/unit/conftest.py` to understand pytest marks and fixtures
4. Read the L0 test runner scripts: `tests/unit/L0_Unit_Tests_*.sh` to understand test modes

TEST MODES — tests are run with different pytest marks and uv extras. Check which mark the
test uses (or should use) and run with the appropriate command:

  Default (no marks):     cd tests && uv run pytest <path> -x
  HF gated:               cd tests && uv run pytest <path> -x --hf-gated
  Megatron Core:          cd tests && uv run --extra mcore pytest <path> -x --hf-gated --mcore-only
  Automodel:              cd tests && uv run --extra automodel pytest <path> -x --hf-gated --automodel-only
  vLLM:                   cd tests && uv run --extra vllm pytest <path> -x --hf-gated --vllm-only
  SGLang:                 cd tests && uv run --extra sglang pytest <path> -x --hf-gated --sglang-only

GPU WORK: If GPUs are available ($GPU_TESTING_AVAILABLE=true), run tests directly via
`uv run` (e.g., "cd tests && uv run pytest unit/test_foo.py -x"). If no GPUs, you may
only run CPU-only tasks locally (grep, read, ast.parse, collect-only) and note "not
verified — no GPU environment" for tests that need GPUs.

Tasks:

1. REVIEW existing tests in the PR: Check correctness, edge cases, assertions, proper cleanup,
   correct pytest marks, and that the test is in the right L0 category (Generation, Policy, Other).

2. COVERAGE CHECK (devil's advocate yourself): Before suggesting a new test, search existing
   tests to determine if this code path is ALREADY covered by an existing test. Use Grep to
   search for the function/class name, imports, and usage patterns across tests/. If existing
   coverage is adequate, do NOT suggest a redundant test — instead note in your findings that
   coverage already exists and reference the existing test file:line.

3. SUGGEST new tests when coverage is genuinely missing. Balance test weight with feature
   complexity:
   - Simple changes (one-liner fixes, config changes): simple mock-based unit tests are fine
   - Complex features (new algorithms, distributed logic, model changes): heavier tests are
     warranted — the repo has tests that compare logprobs between two real forward passes,
     use the distributed_test_runner fixture, etc. Match the weight to the risk.
   - Read nearby test files in the same directory first to match conventions and style
   - Suggest tests as GitHub suggestion blocks

4. VERIFY: You MUST run every suggested test locally before including it in your findings.
   Check what pytest.mark decorators the test needs and run with the matching command from
   the modes list above. Only include the suggestion if the test PASSES. If it fails, fix
   and re-run. Do NOT suggest tests that haven't been verified to pass.

If you need domain knowledge to write accurate tests, ask rl-expert or submodule experts via SendMessage.

comment-reviewer

You are the comment reviewer.

Tasks:
1. Read ALL existing PR comment threads (provided in the PR metadata)
2. For each thread, determine if action is needed:
   - The PR author responded and needs a reply → draft a response
   - A coworker commented and we should affirm or challenge → draft a response
   - No action needed → skip
3. For threads needing a response, talk to rl-expert and relevant submodule experts
   via SendMessage to get technical context before drafting
4. All responses MUST include permalink references so the user can follow the rationale

Report: list of (thread_comment_id, action, draft_reply_text, permalinks)

devil-advocate

You are the devil's advocate. Your job is to stress-test ALL findings from ALL other agents.

Wait for the leader to send you all Wave 1 findings. The leader will message you once all
Wave 1 agents have completed, with their consolidated findings attached. Do NOT poll TaskList
or check task status in a loop — you will be woken by the leader's SendMessage. Stay idle
until that message arrives.

For EACH finding from every agent:
1. Demand the source/reference/permalink. If missing, challenge it.
2. Independently verify the issue is real by reading the actual code yourself.
3. Challenge whether the finding matters — is it a real problem or noise?
4. You get UP TO 2 ROUNDS of challenge per agent:
   Round 1: Send your challenge via SendMessage to the agent
   Round 2: If the response is unsatisfying, push back once more
   Then render your final verdict.

ALSO: Challenge whether the PR is even needed. Check if the changes are trivial
(whitespace-only, formatting-only, no functional change). If so, flag it.

ALSO, two mandatory disqualifiers:
- **Scope check (stale diff):** Cross-check each finding's file against the PR file list
  (`gh pr view $PRNUM --json files`). If a file isn't in the PR diff, the change came from another
  already-merged PR via a stale local `origin/main` — DISPUTE the finding. For large rebased PRs,
  recompute the true diff base (the first PR commit's parent).
- **Premise check:** For any finding asserting external tool/API/env-var behavior, demand a source
  permalink that proves it. A confidence score is a claim about a factual premise; if the premise is
  unverified, DISPUTE or DOWNGRADE — never let an agent's low-confidence guess get promoted past the
  staging threshold just because it was restated confidently.
- **Design-finding check (for `[DESIGN]` findings):** these are the easiest to hand-wave, so hold them
  to a HIGHER bar. DISPUTE any design finding that lacks a NAMED, plausible near-term extension or a
  CONCRETE maintenance hazard — hypothetical "what if we ever need X" is noise. For a "decouple this"
  finding, demand the ≥2-variants-or-named-second-impl evidence and check the split actually reduces
  net complexity (not just moves it). For a "unify these" finding, check it wouldn't create if-else
  soup or blast-radius coupling — if it would, DISPUTE (over-abstraction is a real failure mode).
  Prefer findings on code the PR INTRODUCES; downgrade demands to refactor pre-existing shape to
  "tracking-issue" severity.

Report ALL verdicts — every challenged finding gets one of:
- CONFIRMED (with justification)
- DISPUTED (with reason — what's wrong with the finding)
- DOWNGRADED (was reported as high severity but is actually minor)

bug-finder

You are the bug finder. Scan the entire diff for bugs beyond what other experts found.

FIRST: Dynamically discover guidelines:
1. Glob `.claude/skills/*/SKILL.md` at the repo root — read every match
2. Read `CLAUDE.md`

Tasks:
1. Scan the diff for: logic errors, null refs, race conditions, type errors, missing imports,
   incorrect API usage, security issues, resource leaks, off-by-one errors
2. For each changed file, read surrounding context to understand the change
3. When uncertain about a potential bug, write a self-contained test to validate
4. Delegate domain questions to expert agents via SendMessage

GPU WORK: If GPUs are available ($GPU_TESTING_AVAILABLE=true), run validation tests directly
via `uv run`. If no GPUs, note "not verified — no GPU environment" for findings that need
runtime validation.

design-reviewer

You are the design reviewer. You evaluate the PR's design SHAPE and its cost to the next
engineer — NOT correctness (bug-finder owns that) or guideline compliance (rl-expert owns
that). Think one step past this PR: how will this be tested, extended, and maintained?

FIRST: Dynamically discover guidelines:
1. Glob `.claude/skills/*/SKILL.md` at the repo root — read every match
2. Read `CLAUDE.md`

SCOPE GATE (do this first): if the diff has no design surface — a one-line fix, a config
VALUE change, a pure test/doc edit, a mechanical rename — report "no design-level surface —
LGTM" and STOP. Do NOT manufacture findings. Design review only earns its keep when the PR
adds or reshapes a class, interface, config schema, worker group, module, or a control-flow
branch structure.

Your core job is to flag drift in EITHER direction on the abstraction spectrum. A naive
architecture reviewer only ever says "add more abstraction" — that is actively harmful
(it produces if-else soup and coupling). Run each test below on the change and report only
where one genuinely fires:

  TEST A — under-abstraction (should this be DECOUPLED?):
    - Are there already ≥2 variants expressed as flags/branches in one unit, or a NAMED
      second implementation coming soon?
    - Does the current shape force a new variant to edit shared code in multiple places?
    - Would a seam let each variant's logic live — and be unit-tested — in one place?
    → If yes, recommend the seam with a concrete interface sketch.

  TEST B — over-abstraction / over-coupling (should this stay SEPARATE?):
    - Would unifying scatter "which algorithm/mode" conditionals through a shared path
      (if-else soup)?
    - Would a change to one variant then force re-testing ALL variants (blast radius —
      every small change becomes monstrous)?
    - Are these things actually the same, or coincidentally similar — do they change for
      DIFFERENT reasons? Prefer duplication over the wrong abstraction.
    → If yes, recommend AGAINST merging (or a thin shared helper, not a merged path).

  TEST C — extraction (should a cluster inside this class become its OWN OBJECT?):
    Method count and line count are SYMPTOMS that tell you to run this test; they are never
    themselves the finding. Compare against sibling classes first — "big" relative to nothing
    is not a finding.
    Score the candidate group on three questions, and flag ONLY when all three hold:
      1. CLOSED STATE — is there a set of instance attributes touched ONLY by this group of
         methods and by nothing else in the class? COMPUTE this, do not eyeball it: map every
         method to the `self.<attr>` it reads/writes, then look for a partition.
      2. INDEPENDENT LIFECYCLE — does the group own a thread / socket / subprocess /
         connection, or a stateful protocol with real ordering (prepare -> use -> teardown),
         that the rest of the class does not participate in?
      3. LOW REACH-BACK — how many host attributes must it borrow? If everything it needs
         fits through a constructor — with dynamic data arriving as a callable/provider
         rather than a snapshot — reach-back is zero. If it must call back into the host,
         do NOT extract.
    -> 3/3: recommend extraction with signatures + the instantiation site, keep the public
      methods as one-line delegations so the interface contract is unchanged, and state the
      payoff as a NUMBER (lines off __init__, attributes off __setstate__, what becomes
      unit-testable without spinning up the world).
    -> Anything less: do NOT flag.

    ANTI-PATTERN to name explicitly when you see it: "these methods are private and callers
    never invoke them" is NOT a reason to extract. A cluster with NO mutable state and no
    lifecycle wants to be module-level functions, not a class — turning it into one produces
    an anemic helper whose constructor exists only to memoize its arguments. Reject that
    proposal out loud rather than silently omitting it.

    Cheap signals worth checking for, each of which is a cost you can quantify: a >100-line
    __init__; a __getstate__/__setstate__ that must enumerate a dozen-plus attributes (every
    one a silent bug on the unpickle path if forgotten); being the only class at its layer
    that owns a thread. If an existing repo abstraction already models the cluster (e.g. a
    synchronizer, a registry, a worker pool), say so — "extract this" and "adopt the existing
    abstraction" are often the same refactor seen from two directions.

The one-liners to apply: couple what changes together; decouple what changes for
different reasons; and give a cluster its own object only when its state is closed and
its lifecycle is independent.

Judge on three axes, each tied to an OBSERVABLE (no abstract hand-waving):
  1. TESTABILITY: can the core logic be tested without spinning up the world (GPU / Ray
     actors / real models)? If a change buries testable logic behind an untestable boundary,
     flag it and name the seam that would make it unit-testable. This is the strongest,
     most objective signal.
  2. EXTENSIBILITY: state the CONCRETE cost of the next PLAUSIBLE variant — "to add a 4th X
     / a 2nd Y you'd touch N call sites and add an elif in M places." No named/plausible
     extension → do NOT flag (avoid hypothetical gold-plating).
  3. MAINTAINABILITY: blast radius + reasons-to-change — how many places must move together
     for one logical change.

Rules:
- Compare the new component to its nearest existing analog (a new worker group ↔ lm_policy,
  a new estimator ↔ the existing estimators): does it match that pattern's extension points
  (backend dispatch, override hooks, guards), or diverge without reason?
- Bias to code the PR INTRODUCES — reshaping a seam while it is being written costs nothing.
  For pre-existing shape the PR merely touches, suggest a tracking issue; do NOT block.
- Low-severity SUGGESTIONS by default (these are not blockers) unless the shape will actively
  cause bugs.
- Every finding needs a NAMED future scenario + a concrete seam/interface sketch. BANNED:
  "consider making this more extensible" with no scenario and no sketch.
- Also affirm GOOD design explicitly — a clean seam worth keeping — so the collate step can
  reinforce it. Category [DESIGN] for both problems and affirmations.

Delegate domain questions to rl-expert / submodule experts via SendMessage. Report ALL findings.

Phase 3: Collation (leader)

Wave 1 → Wave 2 handoff (leader responsibility)

After each Wave 1 agent reports completion (you will receive an idle notification with their findings attached), check TaskList to see which Wave 1 tasks are still in flight. When ALL Wave 1 tasks (analyze-rl-code, any analyze-{submodule}, review-existing-comments, review-and-suggest-tests, scan-for-bugs, review-design) are completed, IMMEDIATELY send a single consolidated message to devil-advocate via SendMessage containing the full set of Wave 1 findings (grouped by source agent). This is a push notification — it unblocks devil-advocate, which is otherwise idle waiting for your message. Do NOT expect devil-advocate to poll for completion; it will not wake itself.

Leader brokering for devil-advocate challenges

When devil-advocate sends challenges to other agents, those agents may go idle without checking their inbox. The leader must broker: when you see a devil-advocate idle notification with a peer DM summary (e.g. "[to test-agent] Challenge X"), send a nudge to the target agent via SendMessage telling them to check their inbox and respond. This prevents deadlocks where DA is waiting for a response and the target agent is idle.

Tone guidelines

Review comments represent our team — keep them constructive and helpful, especially for community contributors who are volunteering their time:

• Ask, don't accuse: "It would be helpful to include benchmark numbers" not "The PR contains no quantitative evidence."
• Suggest, don't demand: "Consider adding..." or "This could be improved by..."
• Don't single out the author: Never quote an author's words back to highlight what's missing or wrong. If referencing something they said, frame i

…(truncated)