apm-review-panel

Category: Tools & Productivity | Uploader: microsoft | Downloads: 0 | Version: v1.0（Latest）
Changelog: Source: GitHub https://github.com/microsoft/apm
Directory Structure

Current level: tree/main/.agents/skills/apm-review-panel/
📁 assets/
- 📄 ceo-return-schema.json 3.7 KB
- 📄 panelist-return-schema.json 10.4 KB
- 📄 recommendation-template.md 4.8 KB
📁 evals/
- 📁 fixtures/
  - 📄 01-ship-now-pr1084-shape.json 9.3 KB
  - 📄 01-ship-now-pr1084-shape.rendered.md 7.4 KB
  - 📄 02-needs-rework-shape.json 13.2 KB
  - 📄 02-needs-rework-shape.rendered.md 10.3 KB
- 📄 README.md 2.3 KB
- 📄 render_eval.py 9.1 KB
- 📄 trigger-evals.json 1.1 KB
📄 SKILL.md 20.4 KB
SKILL.md

---
name: apm-review-panel
description: >-
  Use this skill to run a multi-persona expert advisory review on a labelled
  pull request in microsoft/apm. The panel fans out to five mandatory
  specialists plus a test-coverage specialist (active on every PR that
  touches src/) plus two conditional specialists (auth, doc-writer),
  all running in their own agent threads, and a CEO
  synthesizer. The orchestrator is the sole writer to the PR: ONE
  recommendation comment, no verdict labels, no merge gating. The panel
  is advisory -- it surfaces findings, prioritizes follow-ups, and renders
  a ship-recommendation that the maintainer and author weigh. Activate
  when a non-trivial PR needs a cross-cutting recommendation
  (architecture, CLI logging, DevX UX, supply-chain security,
  growth/positioning, optionally auth, docs, and test coverage, with CEO
  arbitration).
---

# APM Review Panel - Fan-Out Advisory Review

The panel is FAN-OUT + SYNTHESIZER. Each persona runs in its own agent
thread (via the `task` tool) and returns JSON matching
`assets/panelist-return-schema.json`. The orchestrator schema-validates
each return, hands all returns to the apm-ceo synthesizer (also a task
thread, returns JSON matching `assets/ceo-return-schema.json`), then
renders ONE recommendation comment from `assets/recommendation-template.md`.

This skill is ADVISORY by design. It does not compute a binary verdict, it
does not apply verdict labels, and it does not gate merge. The panel
surfaces findings; the maintainer and the PR author decide ship.

## Architecture invariants

- **Advisory regime, not gate regime.** There is no `APPROVE` / `REJECT`,
  no `panel-approved` / `panel-rejected` label, no deterministic verdict
  computation. The CEO returns a `ship_recommendation.stance` (`ship_now`
  / `ship_with_followups` / `needs_discussion` / `needs_rework`); this is
  prose for the human reviewer, never auto-applied as a label or status
  check. This is the architectural fix for the previous regime's
  over-strictness: removing the binary gate removes the incentive for
  panelists to inflate `required[]` defensively.
- **Three severity buckets, none of them gate.** Findings carry
  `severity: blocking | recommended | nit`. `blocking` is the highest
  signal a panelist can send and renders prominently in the comment; it
  still does not block merge. `recommended` is the default for substantive
  feedback. `nit` is one-line polish. The orchestrator never reads
  severity to gate anything.
- **Single-writer interlock.** Only the orchestrator writes to the PR:
  exactly one `add-comment` and one `remove-labels` call. The
  `remove-labels` call always sweeps `panel-review` (trigger
  idempotency) AND defensively removes `panel-approved` /
  `panel-rejected` if present (legacy verdict labels from the
  pre-advisory regime; they have no meaning here and would mislead
  readers if left on a PR after a fresh advisory pass). NO `add-labels`
  call -- there are no verdict labels to apply. Panelist subagents and
  the CEO subagent return JSON only and MUST NOT call any `gh` write
  command, post comments, apply labels, or touch the PR state.
- **Single-emission discipline.** Exactly one comment per panel run,
  rendered from `assets/recommendation-template.md` after all subagents
  return.

## Agent roster

| Agent | Role | Always active? |
|-------|------|----------------|
| [Python Architect](../../agents/python-architect.agent.md) | Architectural Reviewer + supplies mermaid diagrams | Yes |
| [CLI Logging Expert](../../agents/cli-logging-expert.agent.md) | Output UX Reviewer | Yes |
| [DevX UX Expert](../../agents/devx-ux-expert.agent.md) | Package-Manager UX | Yes |
| [Supply Chain Security Expert](../../agents/supply-chain-security-expert.agent.md) | Threat-Model Reviewer | Yes |
| [OSS Growth Hacker](../../agents/oss-growth-hacker.agent.md) | Adoption Strategist | Yes |
| [Auth Expert](../../agents/auth-expert.agent.md) | Auth / Token Reviewer | Conditional (see below) |
| [Doc Writer](../../agents/doc-writer.agent.md) | Documentation Reviewer | Conditional (see below) |
| [Test Coverage Expert](../../agents/test-coverage-expert.agent.md) | Test-Presence Reviewer (paired with DevX UX) | Yes (skipped only on docs-only PRs -- see below) |
| [APM CEO](../../agents/apm-ceo.agent.md) | Strategic Arbiter / Synthesizer | Yes |

## Topology

```
   apm-review-panel SKILL (orchestrator thread)
                      |
   FAN-OUT via task tool (panelists in parallel)
                      |
   +-----+-------+-------+-----+-----+------+-----------+----------+
   v     v       v       v     v     v      v           v          v (cond.)
  py    cli     dx-ux   sec   grw   auth   doc-writer  test-cov
   |     |       |       |     |     |      |           |
   |   each returns JSON per panelist-return-schema.json
   +-----+-------+-------+-----+-----+------+-----------+----------+
                      |
                      v   <-- S4 schema-validate
                      v   <-- on malformed: re-spawn that persona
                      v
   task: apm-ceo synthesizer
   - aggregates findings across panelists
   - resolves dissent
   - emits headline + arbitration prose + principle alignment
   - emits curated recommended_followups (prioritized)
   - emits ship_recommendation (stance + prose)
   - returns ceo-return-schema.json
                      |
                      v   <-- S4 schema-validate
                      v
   orchestrator (sole writer)
            |               |
            v               v
        add-comment    remove-labels
        (max:2)        [panel-review,
                        panel-approved,
                        panel-rejected]
                       (trigger reset +
                        legacy verdict sweep)
```

## Conditional panelists

Two personas are conditional (auth, doc-writer). A third
(test-coverage) is mandatory on every PR that touches `src/` and only
skipped on documentation-only PRs -- see its section below for why.
The orchestrator ALWAYS spawns ALL three tasks to keep the schema
return shape uniform; the prompt instructs the subagent to set
`active: false` with an `inactive_reason` if the condition does not
hold.

### Auth Expert

Activate when the PR changes any of:
- `src/apm_cli/core/auth.py`
- `src/apm_cli/core/token_manager.py`
- `src/apm_cli/core/azure_cli.py`
- `src/apm_cli/deps/github_downloader.py`
- `src/apm_cli/marketplace/client.py`
- `src/apm_cli/utils/github_host.py`
- `src/apm_cli/install/validation.py`
- `src/apm_cli/install/pipeline.py`
- `src/apm_cli/deps/registry_proxy.py`

Fallback self-check (when no fast-path file matched): "Does this PR
change authentication behavior, token management, credential resolution,
host classification used by `AuthResolver`, git or HTTP authorization
headers, or remote-host fallback semantics? If unsure, answer YES."

### Doc Writer

Activate when the PR changes any of:
- `README.md`
- `CHANGELOG.md`
- `MANIFESTO.md`
- `docs/src/content/docs/**`
- `.apm/skills/**/*.md`
- `.apm/agents/**/*.md`
- `.github/skills/**/*.md`
- `.github/agents/**/*.md`
- `.github/instructions/**/*.md`
- `.github/workflows/*.md` (gh-aw natural-language workflows)
- `packages/apm-guide/**`

Fallback self-check (when no fast-path file matched): "Does this PR
change user-facing documentation, agent or skill prose, instruction
files, CHANGELOG entries, README claims, or any natural-language
artifact a reader will rely on? If unsure, answer YES."

When the doc-writer is active and the PR includes documentation changes,
the persona reviews them for: (a) consistency with the existing voice
and structure, (b) accuracy against the code being changed, (c)
completeness for the typical reader (no orphan claims, no missing
prerequisites), (d) discoverability (cross-links, sidebar order if
Starlight content). When the doc-writer is active because of code
changes that SHOULD have updated docs but did not, the persona surfaces
that gap as a finding.

### Test Coverage Expert

**Active by default on every PR that touches `src/**/*.py`.** The only
condition that flips this persona to `active: false` is a
documentation-only PR -- the diff contains zero `src/**/*.py` files.
In that case set `inactive_reason: "documentation-only PR -- no
runtime code paths to defend"`.

The activation rule is intentionally narrow: under the advisory regime,
test outcomes are LOAD-BEARING for CEO arbitration (passed / failed /
missing test evidence outranks opinion-only findings -- see
`apm-ceo.agent.md` and `panelist-return-schema.json` evidence block).
A persona whose findings carry that weight cannot be silently skipped
on a heuristic. Better to spawn it on a pure refactor and have it
return a single `nit`-severity "no behavior surface touched -- no
coverage finding" line than to skip it and leave the CEO without
evidence to weigh. (Earlier revisions of this skill paired test-coverage
with auth and doc-writer as conditional for symmetry; that symmetry
broke when test evidence became load-bearing.)

The test-coverage-expert is paired with the devx-ux-expert lens and
defends the user-promise contracts the DevX persona enumerates (CLI
surface, error wording, install idempotency, lockfile determinism, auth
resolution). It MUST verify "no test exists" claims with `view`/`grep`
on the test tree before emitting a finding -- false-positive coverage
findings destroy trust in the field. It does NOT compute coverage
percentages, does NOT flag tests for pure refactors, and does NOT
duplicate python-architect on test-code design.

## Routing matrix (CEO synthesis emphasis only)

These routes describe WHICH specialist's findings the CEO weights more
heavily for a given PR type. They do NOT change which personas run --
every mandatory persona always runs. Routing is a CEO synthesis hint.

- **Architecture-heavy PR** -> CEO weights Python Architect on
  abstraction calls; CLI Logging on consistency.
- **CLI UX PR** -> CEO weights DevX UX on command surface; CLI Logging
  on output paths; Growth Hacker on first-run conversion.
- **Security PR** -> CEO biases toward Supply Chain Security on default
  behavior; DevX UX flags ergonomics regression from any mitigation.
- **Auth PR** (auth-expert active) -> CEO weights Auth Expert on
  AuthResolver / token precedence; Supply Chain on token-scoping.
- **Docs / release / comms PR** (doc-writer active) -> CEO weights Doc
  Writer on accuracy and voice; Growth Hacker on hook and story angle.
- **Behavior-change PR** (test-coverage active) -> CEO weights Test
  Coverage Expert on regression-trap presence; DevX UX on which user
  promises the change touches. A blocking-severity coverage finding on
  a critical-promise surface (auth, lockfile, install, marketplace,
  hooks) is the highest signal in this routing.
- **Full panel** (default) -> CEO synthesizes equally; calls out any
  dissent in `dissent_notes`.

## Execution checklist

Work through these steps in order. Do not skip ahead. Do not emit any
output to the PR before step 6.

1. **Read PR context** (the orchestrating workflow already fetched it
   via `gh pr view` / `gh pr diff`). Identify changed files for the
   conditional panelist routing decisions (auth-expert and doc-writer).

2. **Resolve the conditional panelists** using the rules above. Decide
   for EACH conditional persona: spawn active OR spawn with
   `active: false` + an `inactive_reason`. Either way, all three
   conditional personas ARE spawned -- the schema requires uniform
   return shape.

3. **Fan out panelist tasks.** Spawn the following tasks in PARALLEL
   via the `task` tool, one task per persona:
   - `python-architect` (also asked to supply `extras.diagrams`:
     `class_diagram` (mermaid `classDiagram`), `component` (mermaid
     `flowchart TD`), and OPTIONAL `sequence` (mermaid
     `sequenceDiagram`) blocks per the persona's section 1/2/3 contract)
   - `cli-logging-expert`
   - `devx-ux-expert`
   - `supply-chain-security-expert`
   - `oss-growth-hacker`
   - `auth-expert` (always - active per step 2)
   - `doc-writer` (always - active per step 2)
   - `test-coverage-expert` (always - active per step 2)

   Each task prompt MUST:
   - Reference its persona file by relative path so the subagent loads
     its own scope, lens, and anti-patterns.
   - Include the PR number, title, body, and diff (passed inline).
   - Cite `assets/panelist-return-schema.json` and require the subagent
     to emit JSON matching that schema as its FINAL message.
   - State the calibrated severity contract: "Use `severity: blocking`
     ONLY for correctness regressions, security/auth bypasses, or
     architectural faults that compound, with explicit rationale.
     Default substantive feedback to `recommended`. Use `nit` for
     one-line polish. The panel is advisory; nothing you return blocks
     merge -- pick the severity that honestly matches your signal
     strength."
   - Restate the output contract: NO `gh` write commands, NO posting
     comments, NO label changes, NO touching PR state. JSON return only.

4. **S4 schema gate.** When each panelist task returns, parse the JSON
   and validate against `assets/panelist-return-schema.json`. On
   validation failure:
   - Re-spawn that ONE panelist with an explicit error message pointing
     at the violated rule.
   - Maximum two re-spawn attempts per panelist. If still malformed,
     synthesize a placeholder
     `{persona: "<slug>", active: true, summary: "Schema failure -- see
     extras.", findings: [], extras: {schema_failure: "<reason>"}}`
     and surface the failure in the CEO arbitration prompt.

5. **Spawn the CEO synthesizer task.** Pass the full set of validated
   panelist JSON returns to a `task` invocation that loads
   `../../agents/apm-ceo.agent.md`. The prompt MUST:
   - Provide all panelist returns as structured input.
   - Ask for: headline, arbitration prose, principle alignment (only
     applicable principles), curated recommended_followups (prioritized
     by signal, NOT a re-listing of every finding), ship_recommendation
     (stance + prose).
   - Cite `assets/ceo-return-schema.json` and require JSON return.
   - Restate the contract: the panel is advisory. The CEO does NOT pick
     a verdict label. The `ship_recommendation.stance` is prose for the
     human reviewer, not a gate. NO `gh` write commands.

   Validate the CEO return against `assets/ceo-return-schema.json`. On
   failure, re-spawn once with the violation cited.

6. **Resolve the notification audience.** The advisory comment must
   surface in the inboxes of the people who will act on it. Run:

   ```
   gh pr view <PR_NUMBER> --json author,reviewRequests
   ```

   Build `notify_audience` as the deduplicated list:
   - the PR author's `@login` (always included);
   - every requested reviewer's `@login` (these are the
     CODEOWNERS-resolved reviewers GitHub auto-requested for the
     touched paths, plus any explicitly-requested human reviewers);
   - every requested team's `@org/team-slug` (CODEOWNERS team
     entries).

   Filter out:
   - bot logins (login ending in `[bot]` or matching
     `dependabot|github-actions|copilot-pull-request-reviewer`);
   - the orchestrator's own identity (avoid self-ping).

   Cap the final list at 6 handles to avoid notification noise (PR
   author + up to 5 reviewers/teams). If the cap trims, prefer team
   handles over individual logins. Pass the resulting list to the
   template renderer as `notify_audience`.

   This step replaces the maintainer-notification signal that the
   pre-advisory verdict labels carried. It is the only mechanism by
   which a fresh panel pass announces itself.

7. **Render the comment.** Load `assets/recommendation-template.md`,
   fill the placeholders from the panelist + CEO JSON, and emit it as
   exactly ONE comment.

   Filling rules:
   - The per-persona summary table renders ONLY active panelists, one
     row per persona, with finding counts by severity and the persona's
     `summary` field.
   - The mermaid diagrams come from `python-architect.extras.diagrams`.
     If absent, render the placeholder lines from the template (do NOT
     invent diagrams).
   - The recommended follow-ups list renders the CEO's curated subset,
     not every finding. Full per-persona findings collapse at the bottom.
   - NEVER render the words "Verdict", "APPROVE", "REJECT", "blocked",
     "merge gate", or any equivalent. The panel is advisory.

8. **Sweep labels** via `safe-outputs.remove-labels`. The list MUST be
   `[panel-review, panel-approved, panel-rejected]` -- always all three,
   regardless of which are currently on the PR. `panel-review` is the
   re-run idempotency reset; the other two are LEGACY VERDICT LABELS
   from the pre-advisory regime that have no meaning under the advisory
   contract and would mislead readers if left on a freshly-reviewed PR.
   `safe-outputs.remove-labels` is idempotent on missing labels, so
   sweeping all three on every run is safe and self-healing. NO
   verdict labels are applied.

## Output contract (non-negotiable)

- Exactly ONE comment per panel run, rendered from
  `assets/recommendation-template.md`. The `safe-outputs.add-comment.max:
  2` is a fail-soft ceiling; the discipline lives here.
- Exactly ONE `remove-labels` call sweeping
  `[panel-review, panel-approved, panel-rejected]`.
- NO `add-labels` call. The advisory regime has no verdict to encode.
- Subagents (panelists + CEO) NEVER write to PR state, NEVER call `gh
  pr comment`, NEVER call `gh pr edit --add-label`. They return JSON.
  The orchestrator is the sole writer.
- Never invent new top-level template sections or drop existing ones.

## Gotchas

- **Roster invariant.** The frontmatter description, the roster table,
  the conditional rules, the recommendation template, and the JSON
  schema MUST agree on the persona set. If you change one, change all
  in the same edit.
- **Calibrated severity discipline.** The advisory regime relies on
  panelists honestly distinguishing `blocking` from `recommended`. If a
  panelist marks everything `blocking`, the comment becomes noisy and
  the maintainer learns to ignore the field. The panelist prompts state
  the contract explicitly; the CEO arbitration prose is the safety
  valve when a panelist over-flags.
- **Mermaid diagrams are template-required.** The python-architect
  persona is asked to supply `extras.diagrams.class_diagram`,
  `extras.diagrams.component`, and the OPTIONAL
  `extras.diagrams.sequence`. The template renders nothing when they
  are missing -- it does NOT invent diagrams. Real diagrams are
  what makes the comment scannable for the human reviewer.
- **Mermaid `classDiagram` `:::cssClass` shorthand gotcha.** GitHub's
  mermaid renderer rejects `:::cssClass` appended to relationship
  lines (e.g. `A *-- B:::touched`); use standalone
  `class Name:::cssClass` declarations instead. Authority:
  `python-architect.agent.md:146-154`.
- **Doc-writer detects DRIFT, not just edits.** When the PR changes
  user-facing code that SHOULD have updated docs but did not, doc-writer
  surfaces that as a finding. The conditional rule above is necessary
  but not sufficient -- doc-writer reasons about doc consistency given
  the diff, not just whether doc files were touched.
- **False-negative auth gotcha.** Auth regressions can be introduced
  from non-auth files that change the inputs to auth -- host
  classification, dependency parsing, clone URL construction, HTTP
  authorization headers, or call sites that bypass `AuthResolver`. If
  a diff changes how a remote host, org, token source, or fallback path
  is selected and you are not certain it is auth-neutral, activate
  auth-expert as `active: true`.
- **Test-coverage probe is mandatory.** The test-coverage-expert MUST
  verify "no test exists for X" via `view`/`grep` on the `tests/` tree
  before emitting a finding. A false-positive coverage finding (test
  exists but persona claimed it does not) destroys maintainer trust in
  the field. The persona scope file enforces this; the orchestrator
  passes the diff and trusts the persona to probe.
- **Subagent write enforcement is contract-based, not sandbox-based.**
  Tool permissions are workflow-scoped, not subagent-scoped, so every
  spawned task technically inherits the same `gh` toolset. The
  "subagents must not write" rule is enforced by the prompt contract in
  each `.agent.md` plus the `safe-outputs.add-comment.max: 2`
  fail-soft. If a subagent ever tries to post a comment, the cap
  catches it.
- **No verdict-label reset workflow.** The previous regime had a
  companion workflow `pr-panel-label-reset.yml` that stripped verdict
  labels on every push. The advisory regime has no verdict labels to
  strip; that workflow is removed.
Comments 0
Latest | Hot
Please login before commenting.
Loading comments...
apm-review-panel

Directory Structure

SKILL.md

Report

Notice
