Skip to content

harden(apm-review-panel): one-comment discipline + Hybrid E auth routing + apm-primitives-architect persona#882

Merged
danielmeppiel merged 4 commits intomainfrom
harden/apm-review-panel
Apr 24, 2026
Merged

harden(apm-review-panel): one-comment discipline + Hybrid E auth routing + apm-primitives-architect persona#882
danielmeppiel merged 4 commits intomainfrom
harden/apm-review-panel

Conversation

@danielmeppiel
Copy link
Copy Markdown
Collaborator

@danielmeppiel danielmeppiel commented Apr 23, 2026

harden(apm-review-panel): one-comment-per-run discipline + Hybrid E auth routing + apm-primitives-architect persona

TL;DR

The apm-review-panel skill bundle was emitting one PR comment per panelist instead of one synthesized verdict, leaking persona prose into the orchestrator context, and gating Auth Expert participation on heuristics. This PR rewrites SKILL.md with a completeness gate plus single-emission output contract, extracts the verdict shape into assets/verdict-template.md (loaded only at synthesis), promotes Auth Expert to a Hybrid E (file fast-path + fallback self-check) conditional panelist, mandates a three-artifact return for the Python Architect, and adds a reusable apm-primitives-architect persona. Risk eliminated: drift between description, roster, template, and workflow producing partial, multi-comment, or persona-contaminated reviews.

Note

Roster invariant after this PR: 5 mandatory specialists + 1 conditional Auth Expert + 1 arbiter (CEO). Any divergence between SKILL.md, assets/verdict-template.md, and .github/workflows/pr-review-panel.md is now a single-source-of-truth violation.

Problem (WHY)

Recent panel runs surfaced these failure modes:

  • [x] Multiple comments per review (one per panelist + a trailing CEO summary) instead of one synthesized verdict.
  • [x] Orchestrator context contaminated with verbatim persona prose pasted from each *.agent.md before any specialist had been dispatched.
  • [!] Auth-touching PRs slipping past Auth Expert because the workflow had no explicit activation rule and the orchestrator's inline scan defaulted to "skip if not obvious".
  • [!] Workflow restating the verdict shape inline, so any change to comment structure required edits in two files in lockstep.
  • [!] Conditional/shape-shifting verdict body when a persona had no findings — broke diffability across reviews.

Why these matter (3 quoted anchors):

Approach (WHAT)

# Fix Principle Source
1 Description rewritten as imperative Use this skill to... and stating exact cardinality up front "agents pattern-match well against concrete structures" Agent Skills
2 Roster table marks every persona as "Always active?" with one explicit conditional row "Every agent operates within explicit boundaries" PROSE — Safety Boundaries
3 Verdict shape extracted into assets/verdict-template.md, loaded only at synthesis "store them in assets/ and reference them from SKILL.md so they only load when needed." Agent Skills
4 Pre-arbitration completeness gate (every persona return present, Python Architect's three artifacts present) before CEO arbitration "do the work, run a validator (a script, a reference checklist, or a self-check), fix any issues, and repeat until validation passes." Agent Skills
5 Output contract: exactly ONE comment per run, no progress comments, no persona spillover "Favor small, chainable primitives over monolithic frameworks." PROSE — Orchestrated Composition
6 Workflow Step 2 collapsed to "Load the skill and follow it"; Step 3 limited to workflow-only guardrails same PROSE — Orchestrated Composition
7 Python Architect declares mandatory three-artifact return (classDiagram + flowchart + Design patterns) "Specificity increases as scope narrows." PROSE — Explicit Hierarchy
H Auth Expert routing = Hybrid E: 8-file fast-path list + fallback self-check; never *auth* glob heuristics same PROSE — Explicit Hierarchy
P New apm-primitives-architect.agent.md persona owns "how APM packages and orchestrates knowledge" "Add what the agent lacks, omit what it knows" Agent Skills

Implementation (HOW)

  • .apm/skills/apm-review-panel/SKILL.md — full rewrite. Imperative description with cardinality; explicit Conditional panelist: Auth Expert (Hybrid E); Execution checklist (1..7); Pre-arbitration completeness gate; non-negotiable Output contract; Gotchas section capturing the four traps that produced the observed failure modes.
  • .apm/skills/apm-review-panel/assets/verdict-template.md — new asset. Concrete markdown skeleton with fixed top-level headings (Disposition, Per-persona findings × 6, CEO arbitration, Required actions, Optional follow-ups). HTML comment header tells the orchestrator: ASCII only, do not split, only Auth Expert may render Not activated — <reason>, Python Architect block must contain both mermaid diagrams + Design patterns subsection.
  • .apm/agents/python-architect.agent.md — adds PR review output contract with three numbered required artifacts (classDiagram, flowchart, Design patterns with Used in this PR: and Pragmatic suggestion: lines, both allowed none — ...). Surgical scope: pre-existing em dashes elsewhere in the file untouched.
  • .github/workflows/pr-review-panel.md — Step 1 (load SKILL.md) deleted (it implicitly invited persona pre-loading). Old Step 4 (inline verdict template) deleted. New Step 3 holds workflow-only guardrails: one safe-output comment, never call GitHub API directly, ASCII only.
  • .apm/agents/apm-primitives-architect.agent.md — new reusable persona scoped to bundle structure (not domain), cites PROSE + Agent Skills. Available for future skill-bundle work.
  • apm.lock.yaml, CHANGELOG.md, .github/ mirrors — regenerated via apm install --target copilot; byte-identical mirrors.

Diagrams

Legend: orchestrator execution flow showing Hybrid E auth routing and the pre-arbitration completeness gate.

flowchart TD
    A[Workflow gathers PR context via gh CLI] --> B[Load apm-review-panel SKILL.md]
    B --> C{Auth fast-path file touched?}
    C -- yes --> D[Mark Auth Expert ACTIVE]
    C -- no  --> E{Self-check: changes auth behavior?}
    E -- yes --> D
    E -- no  --> F[Record: Auth Expert inactive reason]
    D --> G[Dispatch mandatory specialists + auth-expert]
    F --> H[Dispatch mandatory specialists only]
    G --> I[Collect findings into working notes]
    H --> I
    I --> J{Completeness gate}
    J -- missing persona return --> K[Re-invoke missing persona]
    K --> I
    J -- "Python Architect missing 3 artifacts" --> L[Re-invoke python-architect]
    L --> I
    J -- pass --> M[CEO arbitration over collected findings]
    M --> N[Load assets/verdict-template.md]
    N --> O[Fill template once]
    O --> P[Emit ONE safe-outputs.add-comment]
Loading

Legend: verdict comment lifecycle — no comment is emitted until the gate passes and the template is filled exactly once.

stateDiagram-v2
    [*] --> CollectingFindings
    CollectingFindings --> CollectingFindings: persona return recorded
    CollectingFindings --> GateCheck: all dispatches complete
    GateCheck --> CollectingFindings: any persona return missing or empty
    GateCheck --> GateCheck: Python Architect missing classDiagram or flowchart or Design patterns
    GateCheck --> CeoArbitration: gate passes
    CeoArbitration --> TemplateLoad: arbitration recorded
    TemplateLoad --> TemplateFill: assets/verdict-template.md loaded
    TemplateFill --> Emit
    Emit --> [*]: exactly ONE safe-outputs.add-comment

    note right of CollectingFindings
        No comment is ever emitted
        from this state. Working
        notes only.
    end note
    note right of GateCheck
        Backstop: workflow caps
        safe-outputs.add-comment
        max at 1.
    end note
Loading

Legend: bundle artifact relationships after the rewrite — single owners for description, roster, verdict shape, and workflow boundary.

classDiagram
    class SKILL_md {
      +description: imperative
      +roster: 5 mandatory + 1 conditional + 1 arbiter
      +execution_checklist: 7 steps
      +completeness_gate
      +output_contract: one comment per run
      +gotchas
    }
    class verdict_template_md {
      +Disposition
      +PerPersonaFindings: 6 fixed headings
      +CEO_arbitration
      +RequiredActions
      +OptionalFollowups
    }
    class python_architect_agent {
      +PR_review_output_contract
      +classDiagram_required
      +flowchart_required
      +design_patterns_required
    }
    class auth_expert_agent {
      +activation: hybrid_E
    }
    class apm_primitives_architect_agent {
      +scope: bundle structure not domain
      +cites: PROSE + AgentSkills
    }
    class workflow_pr_review_panel_md {
      +step1: gather PR context
      +step2: defer to skill
      +step3: workflow guardrails only
    }
    SKILL_md --> verdict_template_md : loads at synthesis only
    SKILL_md --> python_architect_agent : dispatches and verifies 3 artifacts
    SKILL_md --> auth_expert_agent : conditionally dispatches
    workflow_pr_review_panel_md --> SKILL_md : loads and follows
    apm_primitives_architect_agent ..> SKILL_md : audits structure
    apm_primitives_architect_agent ..> verdict_template_md : audits structure
Loading

Trade-offs

  • Auth Expert always rendered, never conditionally omitted. Chose: keep heading + render Not activated — <reason>. Rejected: omit heading when inactive. Rationale: stable, diffable comment shape; conditional headings invite drift — the very failure mode we're fixing.
  • No panel-contract.yaml invariant manifest. Chose: keep the contract in SKILL.md + the template's HTML comment header. Rejected: a new YAML manifest + validator. Rationale: "Favor small, chainable primitives over monolithic frameworks." — the manifest only earns its place if we add a second panel skill.
  • Hybrid E activation, not pure path-glob and not pure self-check. Pure *auth* glob is simultaneously too noisy and too narrow; pure self-check is what produced the observed false negatives. Hybrid E gives a deterministic shortcut for 8 known auth-critical files plus an explicit fallback for the long tail (host classification, dependency parsing, clone URL construction) — "Specificity increases as scope narrows."
  • Pre-existing em dashes in unmodified portions of python-architect.agent.md left in place. Surgical scope; opening unrelated edits invites review noise. A separate ASCII-purity sweep across .apm/agents/*.agent.md is the right venue.

Benefits

  1. One comment per PR review run (vs N comments) — single artifact reviewers can cite, react to, and diff across pushes.
  2. Auth Expert false-negative rate reduced via Hybrid E — 8 auth-critical files always trigger; fallback covers indirect inputs (host classification, dependency parsing, clone URL construction).
  3. Persona pre-load contamination eliminated — orchestrator stays on the SKILL contract instead of holding 6 persona files in context simultaneously.
  4. Completeness gate prevents partial verdicts — missing persona returns trigger re-invocation; CEO arbitration runs only after gate passes.
  5. Verdict template structurally invariant — no conditional headings means stable diffability and lower drift risk between description, roster, and template.

Validation

apm audit --ci:

[+] lockfile-exists        No dependencies declared -- lockfile not required
[+] ref-consistency        All dependency refs match lockfile
[+] deployed-files-present All deployed files present on disk
[+] no-orphaned-packages   No orphaned packages in lockfile
[+] config-consistency     No MCP configs to check
[+] content-integrity      No critical hidden Unicode characters detected
[*] All 6 check(s) passed
Mirror parity and ASCII purity (per file)

.apm/.github/ parity (regenerated via apm install --target copilot):

  • SKILL.md — byte-identical
  • assets/verdict-template.md — byte-identical
  • apm-primitives-architect.agent.md — byte-identical
  • apm.lock.yaml — new entries for apm-primitives-architect and assets/verdict-template.md; updated hash for python-architect.agent.md

ASCII purity (printable U+0020–U+007E + \n/\t):

  • .apm/skills/apm-review-panel/SKILL.md — OK
  • .apm/skills/apm-review-panel/assets/verdict-template.md — OK
  • .apm/agents/apm-primitives-architect.agent.md — OK
  • .apm/agents/python-architect.agent.md — 13 pre-existing em dashes outside the modified region (out of surgical scope; see Trade-offs). The new "PR review output contract" section is ASCII.

How to test

  • Open a small non-trivial PR against microsoft/apm and apply the panel-review label. Watch pr-review-panel run.
  • Confirm exactly one comment is posted on the PR (not one per persona).
  • Verify the heading set matches assets/verdict-template.md verbatim: ## APM Review Panel Verdict, ### Per-persona findings with all six persona bold-prefixed lines, ### CEO arbitration, ### Required actions before merge, ### Optional follow-ups.
  • Verify the Python Architect block contains one mermaid classDiagram, one mermaid flowchart, and a **Design patterns** subsection with Used in this PR: and Pragmatic suggestion: lines (either may legitimately be none — ...).
  • For an auth-touching PR (e.g. src/apm_cli/core/auth.py, src/apm_cli/core/token_manager.py, src/apm_cli/utils/github_host.py), confirm Auth Expert section contains real findings; for a doc-only PR, confirm it reads Not activated — <one sentence citing touched files> with the surrounding heading still rendered.

…ing + apm-primitives-architect

Rewrites the apm-review-panel skill bundle so the orchestrator
collects every panelist's findings before posting a single
synthesized verdict comment. Extracts the verdict shape into
assets/verdict-template.md (loaded only at synthesis time), promotes
Auth Expert to a Hybrid E (file fast-path + fallback self-check)
conditional panelist, codifies a mandatory three-artifact return for
the Python Architect (mermaid classDiagram + flowchart + design
patterns subsection), and adds a reusable apm-primitives-architect
persona for future bundle work.

Pre-arbitration completeness gate validates 5 mandatory persona
returns + Auth Expert XOR (exactly one of findings or inactive
reason); CEO arbitration may run only after the gate passes. Workflow
file Step 2 collapsed to one-liner deferring to skill execution
checklist, eliminating the contract-duplication drift vector.

Validation: apm audit --ci passes all 6 baseline checks; ASCII
purity verified across all 4 modified/created bundle files; .apm/
<-> .github/ mirror parity confirmed.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings April 23, 2026 21:19
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens the apm-review-panel skill bundle so panel runs emit exactly one synthesized PR comment, avoids persona pre-loading in the orchestrator thread, and makes Auth Expert participation explicit via a Hybrid E routing rule. It also introduces a new reusable apm-primitives-architect persona and extracts the panel verdict skeleton into an assets/ template to reduce contract drift.

Changes:

  • Rewrote apm-review-panel SKILL to add an execution checklist, pre-arbitration completeness gate, and explicit conditional Auth Expert routing.
  • Added a canonical assets/verdict-template.md and updated the pr-review-panel workflow to defer verdict shape/routing to the skill.
  • Added apm-primitives-architect agent and strengthened python-architect with a mandatory PR-review output contract (diagrams + patterns).
Show a summary per file
File Description
apm.lock.yaml Adds the new agent to local deployed file tracking and updates hashes.
CHANGELOG.md Adds Unreleased entries describing the new agent and review-panel hardening.
.github/workflows/pr-review-panel.md Removes inline verdict/template duplication and defers orchestration behavior to the skill; keeps single-emission guardrails.
.github/skills/apm-review-panel/SKILL.md Full rewrite to codify routing, conditional Auth Expert activation, completeness gate, dispatch contract, and single-comment output contract.
.github/skills/apm-review-panel/assets/verdict-template.md Adds a canonical single-comment verdict template loaded only at synthesis time.
.github/agents/python-architect.agent.md Adds a strict PR-review output contract requiring two mermaid diagrams and a Design patterns subsection.
.github/agents/apm-primitives-architect.agent.md Adds a new persona focused on structuring and critiquing APM agent primitives and skill bundles.
.apm/skills/apm-review-panel/SKILL.md Mirrors the .github/ skill rewrite as the hand-authored source bundle.
.apm/skills/apm-review-panel/assets/verdict-template.md Mirrors the canonical verdict template in the .apm/ source bundle.
.apm/agents/python-architect.agent.md Mirrors the Python Architect PR-review output contract changes.
.apm/agents/apm-primitives-architect.agent.md Mirrors the new apm-primitives-architect persona in the .apm/ source bundle.

Copilot's findings

Comments suppressed due to low confidence (1)

CHANGELOG.md:18

  • These new Changed bullets are missing the required trailing PR reference "(#PR_NUMBER)". Please add the PR number to each line (and consider tightening the wording so each bullet stays one concise line).
- Hardened `apm-review-panel` skill: roster reconciled to five mandatory specialists + one conditional Auth Expert + one arbiter; added Hybrid E activation (file fast-path + fallback self-check) for Auth Expert; added pre-arbitration completeness gate; codified Dispatch contract; verdict template extracted to `assets/verdict-template.md` with structurally invariant heading set; collapsed `pr-review-panel.md` Step 2 to defer to skill execution checklist (no contract duplication).
- `python-architect` agent now has a mandatory PR-review output contract: every review must include a mermaid `classDiagram`, a mermaid `flowchart` of the runtime path, and an explicit "Design patterns" subsection naming patterns used and pragmatically applicable patterns that would improve modularity / readability / maintainability.
  • Files reviewed: 11/11 changed files
  • Comments generated: 4

write "Not activated -- <reason>" as that persona's body. Do not omit
the persona heading. All other persona headings always have findings.
- The Python Architect block MUST contain the two mermaid diagrams and
the Design patterns subsection declared in
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The template header hardcodes the Python Architect contract path as .apm/agents/python-architect.agent.md, but this template is duplicated under both .github/ and .apm/ and may be loaded from either layout. To avoid confusion/drift for the orchestrator, prefer a layout-agnostic reference (e.g., a relative path from the skill root) or mention both .github/... and .apm/... locations.

Suggested change
the Design patterns subsection declared in
the Design patterns subsection declared in
`.github/agents/python-architect.agent.md` or

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes must be a rel path, and the canonical source is always .apm, .github is just the place where the primitives deploy, so do not point to that one. the skill must be self-contained within its apm-review-panel folder

Copy link
Copy Markdown
Collaborator Author

@danielmeppiel danielmeppiel Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in c8e19be. Dropped the absolute path; the verdict template now references the python-architect persona's PR review output contract by name. Enforcement still lives in SKILL.md's completeness gate (which re-invokes the persona if the three artifacts are missing), so the skill is now genuinely file-self-contained while the contract remains load-bearing.

The deeper architectural point — that any cross-primitive reference should be a declared dependency, not implicit prose — is captured in follow-up issue #896.

Comment on lines +18 to +21
- The Python Architect block MUST contain the two mermaid diagrams and
the Design patterns subsection declared in
`.apm/agents/python-architect.agent.md`. If those are missing, the
synthesis is incomplete -- re-invoke the Python Architect before emitting.
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The template header hardcodes the Python Architect contract path as .apm/agents/python-architect.agent.md, but this template is duplicated under both .github/ and .apm/ and may be loaded from either layout. To avoid confusion/drift for the orchestrator, prefer a layout-agnostic reference (e.g., a relative path from the skill root) or mention both .github/... and .apm/... locations.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes must be a rel path, and the canonical source is always .apm, .github is just the place where the primitives deploy, so do not point to that one. the skill must be self-contained within its apm-review-panel folder

Copy link
Copy Markdown
Collaborator Author

@danielmeppiel danielmeppiel Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in c8e19be. Dropped the absolute path; the verdict template now references the python-architect persona's PR review output contract by name. Enforcement still lives in SKILL.md's completeness gate (which re-invokes the persona if the three artifacts are missing), so the skill is now genuinely file-self-contained while the contract remains load-bearing.

The deeper architectural point — that any cross-primitive reference should be a declared dependency, not implicit prose — is captured in follow-up issue #896.

Comment thread apm.lock.yaml
Comment on lines 4 to +7
local_deployed_files:
- .github/agents/agentic-workflows.agent.md
- .github/agents/apm-ceo.agent.md
- .github/agents/apm-primitives-architect.agent.md
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR description claims apm.lock.yaml includes a new explicit entry for assets/verdict-template.md, but the lockfile still tracks the skill bundle as a directory (.github/skills/apm-review-panel) rather than enumerating its asset files. Either update the PR description to match the actual lockfile behavior, or (if you intended per-asset tracking) add the file paths + hashes to local_deployed_files / local_deployed_file_hashes.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true gap in governance to fix

Copy link
Copy Markdown
Collaborator Author

@danielmeppiel danielmeppiel Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed governance gap, deferred to follow-up architectural issue #896. The lockfile tracks skill bundles as opaque directories today, so asset-level edits inside .github/skills/apm-review-panel/assets/ are invisible to the integrity check. Fixing this properly requires modeling skill bundles as first-class APM packages with per-bundle apm.yml and per-asset hashes in the lockfile — out of scope for this PR. PR description trimmed accordingly.

Comment thread CHANGELOG.md Outdated
Comment on lines +13 to +17
- `apm-primitives-architect` agent: reusable persona for designing and critiquing `.apm/` skill bundles and agent files; anchors on PROSE constraints and Agent Skills best practices.

### Changed

- Hardened `apm-review-panel` skill: roster reconciled to five mandatory specialists + one conditional Auth Expert + one arbiter; added Hybrid E activation (file fast-path + fallback self-check) for Auth Expert; added pre-arbitration completeness gate; codified Dispatch contract; verdict template extracted to `assets/verdict-template.md` with structurally invariant heading set; collapsed `pr-review-panel.md` Step 2 to defer to skill execution checklist (no contract duplication).
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CHANGELOG entries added in this PR do not follow the repo changelog format: each bullet should end with the PR number in parentheses (e.g. "(#123)"). Please update this new Added entry to include the PR number (and keep it a single concise line).

This issue also appears on line 17 of the same file.

Suggested change
- `apm-primitives-architect` agent: reusable persona for designing and critiquing `.apm/` skill bundles and agent files; anchors on PROSE constraints and Agent Skills best practices.
### Changed
- Hardened `apm-review-panel` skill: roster reconciled to five mandatory specialists + one conditional Auth Expert + one arbiter; added Hybrid E activation (file fast-path + fallback self-check) for Auth Expert; added pre-arbitration completeness gate; codified Dispatch contract; verdict template extracted to `assets/verdict-template.md` with structurally invariant heading set; collapsed `pr-review-panel.md` Step 2 to defer to skill execution checklist (no contract duplication).
- `apm-primitives-architect` agent: reusable persona for designing and critiquing `.apm/` skill bundles and agent files; anchors on PROSE constraints and Agent Skills best practices. (#PR_NUMBER)
### Changed
- Hardened `apm-review-panel` skill: roster reconciled to five mandatory specialists + one conditional Auth Expert + one arbiter; added Hybrid E activation (file fast-path + fallback self-check) for Auth Expert; added pre-arbitration completeness gate; codified Dispatch contract; verdict template extracted to `assets/verdict-template.md` with structurally invariant heading set; collapsed `pr-review-panel.md` Step 2 to defer to skill execution checklist (no contract duplication). (#PR_NUMBER)

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes and also, there should be only 1 entry per PR and be short and to the point

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in c8e19be. Collapsed to one bullet per section, each ending with (#882), per repo conventions.

danielmeppiel added a commit that referenced this pull request Apr 23, 2026
…n, tighten ceilings

Same-PR hardening in response to maintainer review of #882
body produced by the v1 skill. Three findings, three fixes:

1. v1 over-applied the source-code ASCII rule to PR-comment output,
   producing flat 400-line walls with no GFM features. PR comments
   are rendered by GitHub's Primer engine, not Windows cp1252
   terminals -- the encoding rule does not govern them. SKILL.md
   now explicitly distinguishes: bundle source files MUST stay ASCII
   (subject to .github/instructions/encoding.instructions.md); the
   PR body OUTPUT is UTF-8 GFM. Encourages alerts (> [!NOTE]),
   <details> collapsibles, task lists, aligned tables, em dashes.

2. v1 had no deterministic mermaid validation. PR #882's third
   diagram (classDiagram) shipped with a parser error: a semicolon
   in a link label broke the lexer. Added a mandatory step in the
   execution checklist that extracts every mermaid block via awk
   and validates each one with mmdc (npx --yes -p
   @mermaid-js/mermaid-cli). The skill MUST NOT save the draft
   until every block validates. Added a Common mermaid pitfalls
   subsection covering semicolons in classDiagram, note-end-note
   in stateDiagram-v2, parens in flowchart labels.

3. v1 produced 220-400 line bodies. Tightened section caps (max 3
   anchored quotes in Problem, max 5 trade-offs, max 5 alignment-
   matrix rows, max 5 numbered How-to-test steps). Total target
   150-220 lines. Long content lives in <details> so the body
   stays scannable in the 60-second budget reviewers actually have.

Bundle source files remain ASCII-pure; apm audit --ci 6/6 pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
danielmeppiel and others added 2 commits April 24, 2026 00:14
Replace generic mermaid skeletons that anchored the persona to trivial
output with description-of-good guidance and a problem-space worked
example.

- Class diagram: now problem-space context (not just touched classes),
  with patterns annotated visually via mermaid stereotypes and notes,
  and a :::touched cssClass distinguishing modified surface.
- Flow diagram: real function names / file paths / exit codes from the
  diff are mandatory; generic placeholder labels are explicitly refused.
- Side-effect markers ([I/O], [NET], [FS], [LOCK], [EXEC]) replace the
  prose 'annotate side effects' guidance with a scannable convention.
- Major architectural changes (new ABC / protocol / registry,
  restructured hierarchy, new gate/fork/async boundary, pattern shift)
  may produce up to 4 mermaid blocks (Before/After x class+flow); 4 is
  the cap, never the default.
- Patterns claimed in section 3 must be visible as stereotypes or notes
  in the section-1 class diagram -- prose-only patterns are refused.

Worked example uses AuthResolver chain (Strategy + Chain of
Responsibility) as a bar-setter; explicit cite-don't-template note
prevents copy-paste anchoring.

.github/agents/python-architect.agent.md regenerated via
apm install --target copilot.

Refs: #882

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
danielmeppiel added a commit that referenced this pull request Apr 23, 2026
…, drop model frontmatter

Address 6 review comments on PR #884 (3 maintainer, 3 Copilot bot).

M1 (template L37, danielmeppiel): the 'Why these matter' line previously
anchored every failure to a verbatim PROSE / Agent Skills quote. That
hard-codes a citation type that only fits APM's primitives PRs. Per the
Reduced Scope principle and the templates-as-assets rule (templates
should accept the anchor type the PR naturally affords, not enforce
one), replace with a placeholder that signals 'anchor each claim to the
most credible source available -- doc URL, file path, prior PR, CLI
excerpt, or omitted'.

M2 (template L48, danielmeppiel): the Approach table over-specified
columns ('Principle' / 'Source' as verbatim quote + URL) and forced
4 columns even for trivial fixes. Same Reduced Scope principle: collapse
to a 2-column 'Fix (and why, if non-obvious)' table by default, with
explicit guidance that 3 columns is opt-in and the anchor column should
be dropped when most rows have none. Reduces reviewer cognitive load.

M3 (template L72, danielmeppiel): the literal flowchart example
(A[Start] -> B{Decision} -> C/D -> E[End]) is the same failure mode the
python-architect.agent.md fix from PR #882 addressed -- agents follow
verbatim and produce the trivial diagram. Per Calibrated Control,
replace the example block with an imperative placeholder: real names
from the diff, real branches, real side-effect markers; explicit
guidance to pick diagram type (flowchart / sequenceDiagram /
stateDiagram-v2 / classDiagram / erDiagram) by change shape; explicit
permission to omit the entire Diagrams section when no relationship is
genuinely non-trivial. Show-don't-tell is reserved for the shape we
actually want copied.

C1 (rubric L141-143, Copilot): the trailer-line checklist item used
inline backticks across a hard line break, which Markdown renders as
literal backticks. Collapsed to single-line inline code per the bot's
suggestion.

C2 (SKILL.md L13, Copilot): removed 'model: claude-opus-4.7' from
frontmatter. Verified parser at src/apm_cli/primitives/parser.py only
consumes name/description from SKILL.md frontmatter (the model field is
silently dropped). Cross-checked .apm/skills/*: zero other SKILL.md
files use model:. The 7 .apm/agents/*.agent.md files do use model:, but
that field is consumed by Claude Code's subagent dispatch (an agent.md
becomes an invokable subagent that selects its own model); SKILL.md
files are loaded as content into the active context, with no equivalent
model-selection hook. Keeping it on SKILL.md would set a non-standard
convention.

C3 (CHANGELOG L13, Copilot): rewrote the entry to reflect current
shape: 10 sections (the alignment matrix was removed at 72654f0),
single bullet, ending with (#884) per Keep-a-Changelog convention.
Dropped the 'Self-applied' clause that no longer reflects the current
PR body.

Mirror regenerated via 'apm install --target copilot'; .github/skills/
pr-description-skill matches .apm/skills/pr-description-skill exactly.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Drop hardcoded `.apm/agents/python-architect.agent.md` path from
  verdict template; reference the persona's PR review output contract
  by name. Skill stays self-contained; enforcement still lives in the
  SKILL.md completeness gate.
- Collapse CHANGELOG entries to one bullet per section, each ending
  with (#882), per repo changelog conventions.

Per-asset lockfile hash tracking and skill-as-package dependency
declaration are deferred to a follow-up architectural issue.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@danielmeppiel
Copy link
Copy Markdown
Collaborator Author

Addressed all four reviewer findings in c8e19be:

@danielmeppiel
Copy link
Copy Markdown
Collaborator Author

Note on follow-up

The "asset edit not visible in lockfile" finding raised on this PR is the exemplar incident behind Epic #898. The Epic owns the deterministic fix (per-asset lockfile entries, apm audit --drift, dependency-closure policy rule). #896 is folded into the Epic as a sub-issue.

This PR remains scoped to the four review fixes already landed in c8e19be. No additional changes requested here.

@danielmeppiel danielmeppiel merged commit c74444c into main Apr 24, 2026
49 checks passed
@danielmeppiel danielmeppiel deleted the harden/apm-review-panel branch April 24, 2026 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants