Skip to content

Refactor apm-review-panel: VERDICT regime -> ADVISORY regime (drop binary APPROVE/REJECT, add doc-writer, restructure comment) #1091

@danielmeppiel

Description

@danielmeppiel

Problem

The current APM Review Panel skill (apm-review-panel) is structurally adversarial: it computes a binary APPROVE / REJECT verdict and writes a panel-approved / panel-rejected label. The schema forces every finding into one of two buckets — required[] (blocks merge, deterministically forces REJECT) or nits[] (skip-if-you-want). There is no middle ground.

In practice this produces two failure modes:

  1. Over-strict verdicts. A panelist reading carefully will lean required for any real-but-not-blocking issue, because nit reads as "ignore me" and the schema offers no third slot. One careful panelist with one borderline required finding flips the entire panel to REJECT.
  2. Adversarial comment shape. The comment top-loads Verdict: REJECT + Required before merge (N items) — a gating frame, not a collaborative one. PR authors (especially external contributors) read it as "the bots want me to fix N things" instead of "here is the panel's recommendation, the maintainer decides".

Concrete reference: PR #1084 (manual local panel run)

When PR #1084 (a 101+/1- bug-fix from external contributor @tillig unblocking GHES/GitLab/Bitbucket users) was reviewed manually using the same persona threads but a different output template, the comment (link):

  • top-loaded CEO arbitration prose (why this matters strategically)
  • followed with a principle alignment block (multi-host, pragmatic-as-npm)
  • carried a per-persona summary table (one line per persona, verdict + nit-count + one-liner)
  • rendered two mermaid diagrams (component flow + sequence) directly in the body, not buried in extras
  • listed 3 prioritized post-merge follow-ups (none blocking)
  • ended with ship recommendation prose ("merge as-is")
  • collapsed full per-persona findings at the bottom

This is the shape the current panel should produce. It actually helps the contributor and the human reviewer; it does not pretend to be a gate.

Proposal

Refactor apm-review-panel from a VERDICT regime to an ADVISORY regime (mapping onto genesis example 04, not example 05).

What changes

Aspect Today Proposed
Verdict APPROVE / REJECT, deterministic None. Comment carries CEO ship-recommendation prose; maintainer + author decide.
Severity model required[] (gates) + nits[] (skip) findings[] with severity: blocking | recommended | nit. blocking reserved for real correctness/security regressions with explicit rationale.
Labels written panel-approved XOR panel-rejected, plus panel-review removal Only panel-review removal (trigger idempotency). No verdict labels.
Comment top Verdict: REJECT + Required before merge (N) CEO framing paragraph + principle alignment + per-persona summary table
Mermaid diagrams Buried in python-architect.extras.diagrams; often missing Template-required: component diagram + sequence diagram in the body
Roster py-arch, cli-log, devx, sec, growth, [auth] + doc-writer (conditional like auth-expert: docs/, README, CHANGELOG, .apm/skills/, .github/agents/, instructions/, .github/workflows/*.md)
Companion workflow pr-panel-label-reset.yml strips verdict labels on every push Becomes obsolete (no verdict labels to strip) — delete or downscope

Why this is better

  • Helpful to contributors. The advisory frame matches the mental model of code review on an OSS repo: a panel of reviewers leaving recommendations, not a CI gate. External contributors are not punished by a bot verdict.
  • Helpful to maintainers. A summary table + mermaid diagrams + ship-recommendation prose lets the human reviewer scan in 10 seconds. Today's verdict comment is longer and less informative.
  • Honest about LLM panels. Binary deterministic verdicts give false confidence — they hide real disagreement under a sum(required) == 0 calculation. Advisory framing makes the panel's actual signal (recommendations, not decisions) explicit.
  • Aligned with APM principles. Pragmatic-as-npm: npm does not have a binary "your package quality is REJECTED" gate; it has scanners and recommendations. The panel should mirror that.
  • Doc-writer fills a real gap. Documentation drift is a frequent finding raised informally today; making it a first-class persona surfaces it consistently.

Reference

The good comment from PR #1084: #1084 (comment)

The PR refactoring this skill will land alongside this issue and explain WHY/WHAT/HOW with the same diagrams and trade-off analysis.

Out of scope

  • Changing the persona definitions themselves (each .agent.md continues to load and reason as today; only the orchestration contract and output shape change).
  • Changing the trigger surface (panel-review label + workflow_dispatch continue to work).
  • Auto-resolving severity disputes (the CEO arbitration paragraph already handles this; the schema just needs to express the dispute, not gate on it).

Metadata

Metadata

Assignees

No one assigned

    Labels

    area/ci-cdGitHub workflows, merge queue, gh-aw integrations, release pipeline.enhancementDeprecated: use type/feature. Kept for issue history; will be removed in milestone 0.10.0.priority/highShips in current or next milestonestatus/acceptedDirection approved, safe to start work.status/triagedInitial agentic triage complete; pending maintainer ratification (silence = approval).type/refactorInternal restructure, no behavior change.

    Type

    No type

    Projects

    Status

    In Progress

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions