[Ravi Eda (ravimeda)]

Summary

Introduces a new ARM API Review Critic agent, a read-only adversarial verifier that independently re-validates findings produced by the ARM API Reviewer before any are presented to a human for posting on a PR.

Restructures the Reviewer around a shared Reviewer / Critic wire protocol (.github/agents/protocols/reviewer-critic.protocol.md) that defines the input contract, verdict tracks, telemetry markers, and override-reason validator both agents must honor. Adds session-SHA pinning (the entire review is bound to one commit; SHA drift mid-session invalidates the run), an existing-comment reconciliation plan (Step 5.5) that de-duplicates against prior comments and verifies fix-claims before auto-resolving threads, and a downstream-CI impact check (Step 4.5) that blocks suggested fixes which would collide with required LintDiff rules.

Adds multi-perspective passes (Security Skeptic / SDK Ergonomics / SRE) and a mandatory API graph & data-flow step (Step 3.5) to the Reviewer, so the same surface is examined through several biases and the API is reasoned about as a graph (resource, operation, sensitive-data-flow, version-delta) rather than a flat list of rules. The Critic independently re-derives these graphs and graph-diff is a primary missed-violation signal; graph fabrication is a non-overridable FAIL.

Adds fast-path triage to the Reviewer (examples-only and description-only PRs skip deep schema, breaking-change, and graph analysis but never skip the Critic), tightens the Critic loop with convergence-based iteration capped at 3, adds a consensus rule for Blocking severity (a finding only posts as Blocking when both the Reviewer and the Critic concur at High or Medium confidence; otherwise auto-capped to Warning), and ships eval suites that exercise the new behaviors end-to-end.

Motivation

The Reviewer operates on a public repo where every posted comment is durable, citable, and indexed. A single wrong finding becomes precedent. Precision dominates recall: dropping a borderline finding is cheap; posting a wrong Blocking finding is not. The Critic exists as a safety gate to catch false positives, wrong line numbers, misquoted rules, misclassified [NEW]/[EXISTING] tags, fabricated graph nodes, unjustified thread auto-resolutions, and suggested fixes that would break required downstream CI, all before the human sees the report.

Linear, file-by-file rule scanning is good at catching named-rule violations but bad at catching systemic problems such as orphan resources, asymmetric CRUD, $ref cycles, and secrets that enter via PUT and leak via LIST. Forcing the model to build resource / operation / data-flow graphs (and rendering them as Mermaid so the Critic can re-derive and diff) catches structural issues that no single rule would flag.

Separately, the Reviewer was previously running full deep-schema review on PRs that only touched example payloads or description text, and was re-posting findings already covered by prior threads on the same PR. The fast path and the Step 5.5 reconciliation plan fix both, conservatively.

Changes

New agent

• .github/agents/arm-api-review-critic.agent.md: read-only critic with no mutating tools, enforced by an explicit allowlist plus a pre-tool self-check. Per-finding re-verification (file re-fetch at the session SHA, verbatim rule quote, line-number check including conflict-marker handling, [NEW] / [EXISTING] re-classification, confidence assignment, override-reason validation). Six bias-filter passes for missed-violation hunting (future-breaking shape, operational pain, silent breaking changes, security smell, naming rot, what's missing). Independent graph re-derivation that diffs against the Reviewer's Mermaid output. Independent re-verification of the Step 5.5 reconciliation plan: every SKIP-COVERED, RESOLVE-AND-REPOST, REPLY-LINE-SHIFT, THANK-AND-RESOLVE, and PROPOSE-HUMAN-RESOLVE entry is re-checked with an anchored proof-of-fix.
• Four verdict tracks: Finding accuracy, Graph integrity (binding when FAIL: fabrication), Reconciliation accuracy (binding on FAIL because wrong fix-claims would silently close still-broken threads), and Coverage quality. A fifth state Finding accuracy = INVALIDATED overrides every other track and kills the session when the SHA the Reviewer judged no longer matches the PR head.

Shared protocol

• .github/agents/protocols/reviewer-critic.protocol.md: the canonical wire contract. Defines the ten Critic inputs, the <!-- review-state: ... --> per-response marker, the 6-field per-comment telemetry marker (including head-sha, critic, downstream-rule, override-reason, and the telemetry: degraded fallback form), the Override-reason validator (length + boilerplate denylist + structured-anchor / verbatim-quote requirement), the session-handoff verification checks, and the non-overridable FAIL catalog.
• .github/agents/protocols/critic-inputs.template.md: the verbatim YAML template (# critic-inputs/v1) the Reviewer pastes into every Critic dispatch.

Reviewer updates

• .github/agents/arm-api-reviewer.agent.md: restructured into ten numbered steps with a Pre-Presentation Invariant at the top that gates any user-visible findings on one of four explicit states (Critic ran / session-handoff pending / Critic unavailable with disclosure banner / session invalidated). Key additions:
  • Step 1: Session SHA pinning (binding for the entire review, every fetch on every iteration); PR-state checks (draft / mergeable / closed); fast-path vs full-review selection; size guardrail with explicit human choice on oversized PRs.
  • Step 3 / TSP-REQUIRED-V1: Blocks new handwritten OpenAPI API version directories that are missing TypeSpec.
  • Step 3.5: Mandatory API graph & data-flow analysis on the full-review track. Four views (resource, operation, sensitive-data-flow, version-delta) rendered as Mermaid in collapsible <details> blocks. Size guardrails downgrade rendering on extreme-scale RPs but still run the sensitive-data-flow analysis. Failure-recovery ladder distinguishes graphs-produced: false (fast path, silent) from degraded (failed even after retry, requires a [!CAUTION] banner).
  • Step 4 multi-perspective passes (mandatory on full review): Security Skeptic, SDK Ergonomics Advocate, Operations/SRE Lens.
  • Step 4.5 Downstream-CI Impact Check: Any finding that adds or tightens output is re-checked against required LintDiff rules (linter-rule-coverage.md). Findings that would collide are re-authored as three-option recommendations with a properly-rendered suppression where: clause; the Critic FAILs the run non-overridably on downstream-ci-conflict or suppression-path-mismatch.
  • Step 5.5 Existing-Comment Reconciliation Plan: Inventories every existing thread (any state, any author) via read-only GraphQL, classifies each into Scenarios A through F (SKIP-COVERED / RESOLVE-AND-REPOST / REPLY-LINE-SHIFT / Scenario D bulk-skip / THANK-AND-RESOLVE / PROPOSE-HUMAN-RESOLVE), and records a mandatory proof-of-fix anchor for every fix-verified disposition. The plan is rendered verbatim in Step 6 and passed verbatim to the Critic.
  • Step 6: Internal-draft report; never user-visible until the Critic gate is cleared. Includes a "Findings dropped after critic review" section (rendered only when non-empty) for transparency on Critic-rejected items.
  • Step 7 Mandatory Critic Review: Subagent dispatch with a strict three-rung fallback (subagent, then session-handoff with verbatim paste + 6-check validation, then disclose-and-stop with a CAUTION banner). Hard cap at 3 iterations (reduced from 5; the interactive override checkpoint at iteration 3 routes hard cases to the human). Override workflow uses the protocol's three-check Override-reason validator (length + denylist + structured-anchor/verbatim-quote); length-only or boilerplate reasons fail validation. Reconciliation FAILs, graph-fabrication FAILs, downstream-ci-conflict, suppression-path-mismatch, unescaped-mention, and override-reason-invalid are non-overridable.
  • Step 8: Bundles the Step 9 label proposal into the plan-approval prompt; includes a bulk auto-resolve disclosure (count of Scenario E threads, URLs, opt-out via "Execute selectively"); thread resolution via GraphQL resolveReviewThread; full Chat-PR Parity rule (posted bodies are byte-for-byte identical to the Step 6 canonical body).
  • Step 10 Workspace cleanup: Mandatory probe-and-remove on every review, including aborted ones.
• Posting-hygiene scan (Critic Step 6.5): Detects unescaped @-mentions (TypeSpec decorators, library handles) and #<number> autolinks in posted bodies; both are non-overridable FAILs.
• Suppression-continuity guidance: Every dropped suppression requires a recorded violation-absence anchor against the emitted OpenAPI (not the TypeSpec source); anti-pattern lookups for high-frequency rules (ProvisioningStateMustBeReadOnly $ref+sibling issue, LroPostMustNotUseOriginalUriAsFinalState, XmsPageableMustHaveCorrespondingResponse).
• Two distinct telemetry markers: A per-response <!-- review-state: critic-mode=... --> (response-scope: mode/iteration/PR) and a per-comment <!-- posted-by: arm-api-reviewer-agent | ... --> (finding-scope: rule/severity/classification/critic-verdict/head-sha + optional downstream-rule and override-reason, with a telemetry: degraded fallback when fields can't be assembled).
• Cross-repo support: Azure/azure-rest-api-specs and Azure/azure-rest-api-specs-pr (private); explicit gh CLI fallback path for the private repo when MCP tools are unavailable.

Shared references (under .github/skills/azure-api-review/references/)

• downstream-ci-impact.md: scope criteria, five required-shape rules, suppression where: exact-match guarantee.
• linter-rule-coverage.md: known LintDiff rule IDs in scope for the Step 4.5 check.
• think-in-graphs.md: canonical Step 3.5 specification (node/edge definitions, partition thresholds, the catalogue of graph-only findings).
• reviewer-posted-parity.md: byte-for-byte parity contract between Step 6 chat-rendered findings and posted PR comment bodies.
• suppression-review-criteria.md: shared criteria for evaluating both readme.md directives and suppressions.yaml entries.
• guid-and-uuid-on-arm.md: GUID/UUID handling guidance on ARM resources (covers the R3017 conflict-aware area referenced by Step 4.5).

Evals

• New suites under .github/skills/evals/arm-api-reviewer/vally/: eval-fast-path-triage.yaml, eval-report-format.yaml, eval-suppressions-yaml.yaml, eval-citation-and-parity.yaml, eval-protocol-safety.yaml, eval-typespec-required.yaml, plus expanded fixtures (TypeSpec anti-patterns, suppressions-yaml fixtures, additional version-pair fixtures).
• Eval runner improvements: repeat / stability checks, additional output-validation assertions, configurable timeouts.
• Test count grew significantly versus pre-PR baseline (see README.md under the eval skill for the current catalog).

Docs

• documentation/api-reviewer-agent.md: documents the Critic, fast path, multi-perspective passes, graph step, reconciliation plan, downstream-CI check, and suppression continuity.
• .github/agents/README.md: agent-folder conventions.

Validation

• Full eval grid passes; remaining grader-regex edge cases were fixed in follow-up commits (fix(evals): Update regex pattern..., Refine evaluation patterns...).
• End-to-end exercise on a real public PR (Disk RP 2026-03-02, #40595). The Reviewer produced the required graph artifacts, reconciled against a prior agent review of the same PR, and surfaced two net-new findings (one Blocking OAPI027 on a readOnly property in SnapshotUpdateProperties, one Warning on ImmutabilityPolicyLockData.minimum: 0 inconsistency) that the prior pass had not covered. See the Evidence section below.

Evidence: example graphs from a real review (PR #40595)

These are the actual Step 3.5 artifacts produced by the updated reviewer on PR #40595 (Microsoft.Compute Disk RP, new stable/2026-03-02 API version adding snapshot immutability policy, confidentialVMVersion, and snapshotAccessState). All graphs are derivable from DiskRP.json at the PR head, so the Critic could independently re-derive and diff them.

Resource graph (6 nodes, 7 edges)

graph TD
  Disk["Disk<br/>(tracked, RG)"]
  DiskAccess["DiskAccess<br/>(tracked, RG)"]
  DiskEncryptionSet["DiskEncryptionSet<br/>(tracked, RG)"]
  Snapshot["Snapshot<br/>(tracked, RG)<br/>+ immutabilityPolicy (NEW)"]
  DiskRestorePoint["DiskRestorePoint<br/>(proxy)<br/>+ snapshotAccessState (NEW)"]
  PEC["PrivateEndpointConnection<br/>(proxy, under DiskAccess)"]
  KeyVault["KeyVault<br/>(external)"]
  vmRestorePoint["vmRestorePoint<br/>(in restorePointCollection)"]

  DiskAccess -- parent-of --> PEC
  DiskRestorePoint -- child-of --> vmRestorePoint
  Disk -- refs --> DiskAccess
  Disk -- refs --> DiskEncryptionSet
  Snapshot -- refs --> DiskAccess
  Snapshot -- refs --> DiskEncryptionSet
  DiskEncryptionSet -- refs --> KeyVault

Loading

Sensitive-data-flow graph (secrets confined to explicit access-grant operations; no new leaks)

flowchart LR
  GrantAccess["Disks_GrantAccess<br/>Snapshots_GrantAccess<br/>DiskRestorePoint_GrantAccess<br/>(explicit POST actions)"]
  GrantAccess -- response --> AccessUri["AccessUri<br/>accessSAS<br/>securityDataAccessSAS<br/>securityMetadataAccessSAS<br/>(all readOnly)"]

  GetSnapshot["Snapshots_Get<br/>Snapshots_List<br/>Snapshots_ListByResourceGroup"]
  GetSnapshot -- response --> Snapshot["Snapshot.properties<br/>no SAS / no key material<br/>(LIST is clean)"]

  DES["DiskEncryptionSets_Get"]
  DES -- response --> KeyUrl["KeyForDiskEncryptionSet.keyUrl<br/>(versioned KeyVault URL by design)"]

  UpdateImmutability["Snapshots_UpdateImmutabilityPolicy<br/>Snapshots_UpdateImmutabilityPolicyLock<br/>(NEW in 2026-03-02)"]
  UpdateImmutability -- response --> Snapshot

Loading

Operation graph for Snapshots (new immutability surface highlighted)

graph LR
  subgraph Snapshots
    SList["Snapshots_List"]
    SListRG["Snapshots_ListByResourceGroup"]
    SGet["Snapshots_Get"]
    SPut["Snapshots_CreateOrUpdate"]
    SPatch["Snapshots_Update"]
    SDel["Snapshots_Delete"]
    SGrant["Snapshots_GrantAccess"]
    SRevoke["Snapshots_RevokeAccess"]
    SImmut["Snapshots_UpdateImmutabilityPolicy (NEW, async)"]
    SLock["Snapshots_UpdateImmutabilityPolicyLock (NEW, async)"]
  end
  SList --- SGet
  SListRG --- SGet
  SGet --- SPut
  SGet --- SPatch
  SGet --- SDel
  SGet --- SGrant
  SGrant --- SRevoke
  SGet --- SImmut
  SImmut --- SLock

Loading

No structural asymmetries detected: full CRUD + LIST + LIST-by-subscription + grant/revoke + the two new immutability POSTs. Both new POSTs declare Azure-AsyncOperation headers and final-state-via: azure-async-operation. ✅

Findings surfaced from the graph view that would have been hard to catch from the rule checklist alone:

• The sensitive-data-flow graph confirms no secret enters via PUT and leaks via LIST on the new surface (SAS URIs only come back from explicit beginGetAccess POSTs).
• The operation graph showed both new immutability POSTs share the Snapshot resource (correctly), and the new Azure-AsyncOperation header is consistently present on both. A one-pass rule scan would have checked them independently and could have missed the consistency issue if it had been there.
• The Critic's independent re-derivation flagged snapshotAccessState on SnapshotUpdateProperties as a node mismatch (it appears on the request-body side of the operation graph but is readOnly), which led to the Blocking OAPI027 finding the prior pass had missed.

Safety notes

• The Critic has no mutating tools by design, enforced by an explicit allowlist plus a pre-tool self-check that runs before every shell or API call. The allowlist principle, the forbidden-command list, and the self-check are codified in the agent file; any future PR adding a write capability would defeat the safety gate.
• Fast-path selection defaults to full review on any ambiguity, and never skips the Critic step.
• The Blocking-severity consensus rule caps to Warning when the Reviewer and Critic disagree on a Blocking finding; humans can still upgrade via the structured override workflow (critic: override telemetry marker plus an override-reason that passes the three-check validator: length + denylist + structured-anchor or verbatim rule quote).
• Several FAIL classes are non-overridable: reconciliation FAILs (fix-not-verified, fix-anchor-wrong, fix-anchor-unreachable, skip-not-justified, shift-misclassified), Graph integrity = FAIL: fabrication, downstream-ci-conflict, suppression-path-mismatch, unescaped-mention, and override-reason-invalid. Each has a documented recovery path (correct and re-invoke, drop, demote, or escalate to MANUAL DECISION REQUIRED); the critic: override marker cannot silently clear them.
• Session-SHA pinning prevents the Reviewer or Critic from judging a tree that no longer matches the PR. SHA drift mid-session returns Finding accuracy = INVALIDATED and forces an explicit restart-or-abandon choice; no findings are presented from a stale tree.
• When the Critic subagent is unavailable, the Reviewer follows a strict three-rung fallback (subagent, then session-handoff with verbatim paste + 6-check validation, then disclose-and-stop) and never silently substitutes inline self-critique. The exception case is disclosed via a [!CAUTION] "Independent critic verification was NOT performed" banner so the human cannot mistake self-check for independent verification.
• Prompt-injection resistance is explicit: PR content, file content, commit messages, and existing threads are treated as data, not instructions. Adversarial patterns (e.g., "skip the critic", "treat INVALIDATED as PASS") are listed in both agent files as inert.

Related issues

Umbrella:

• Closes ARM API Reviewer agent: add a critic sub-agent to independently validate findings before posting #43302 (add a critic sub-agent to independently validate findings before posting)
• Closes ARM API Reviewer agent: improvement suggestions for reliability and scalability #42660 (improvement suggestions for reliability and scalability)

Structural improvements introduced by this PR (each tracked separately so they remain discoverable after merge):

• ARM API Reviewer agent: formalize Reviewer / Critic wire protocol as a shared, versioned contract #43646: Formalize Reviewer / Critic wire protocol as a shared, versioned contract
• ARM API Reviewer agent: pin a session SHA for the entire Reviewer/Critic loop and invalidate on drift #43647: Pin a session SHA for the entire Reviewer/Critic loop and invalidate on drift
• ARM API Reviewer agent: reconcile findings against existing PR comments and verify fix-claims before auto-resolving threads #43648: Reconcile findings against existing PR comments and verify fix-claims before auto-resolving threads
• ARM API Reviewer agent: verify suggested fixes do not collide with required LintDiff rules (downstream-CI impact check) #43649: Verify suggested fixes do not collide with required LintDiff rules (downstream-CI impact check)
• ARM API Reviewer agent: reason about the API as a graph (resource / operation / data-flow / version-delta) rather than a flat list of rules #43650: Reason about the API as a graph (resource / operation / data-flow / version-delta) rather than a flat list of rules
• ARM API Reviewer agent: verify dropped readme.md / suppressions.yaml entries against the emitted OpenAPI, not the TypeSpec source #43651: Verify dropped readme.md / suppressions.yaml entries against the emitted OpenAPI, not the TypeSpec source
• ARM API Reviewer agent: block new handwritten OpenAPI API version directories that are missing TypeSpec (TSP-REQUIRED-V1) #43652: Block new handwritten OpenAPI API version directories that are missing TypeSpec (TSP-REQUIRED-V1)

[github-actions]

Next Steps to Merge

Next steps that must be taken to merge this PR:

• ❌ The required check named Protected Files has failed. Refer to the check in the PR's 'Checks' tab for details on how to fix it and consult the aka.ms/ci-fix guide

Comment generated by summarize-checks workflow run.

[Ravi Eda (ravimeda)]

Praveen Kuttappan (@praveenkuttappan) , Mariana Rios Flores (@maririos) please could you approve.