Azure / Azure/azure-rest-api-specs

ARM API Reviewer agent: harden shell fallbacks, immutable SHA binding, trusted thread ownership, overflow handling, and marker degradation

Open
#45,948 0 comments 0 reactions 0 assignees View on GitHub
ARM API Reviewer agent
Dominant language
TypeSpec
Stars
3.1k
Forks
5.9k
Avg merge
3d 2h
Merged PRs (30d)
424

Description

## Problem

Several independent robustness gaps were found across the interactive agent and workflow definitions, all sharing the theme of "the happy path works, but the agent has no safe fallback when something is even slightly non-standard":

1. **Shell fallback fragility.** The agent's own documented recovery recipes told it to run `gh api ... --jq '...'` and `gh api graphql` one-liners built via double-quoted PowerShell string interpolation. Under PowerShell, a bare `|` inside such a string is treated as a pipeline operator, breaking the command exactly as reported in #44356 ("`^Gpi-version`" comment corruption from shell string interpolation instead of JSON serialization). The agent's recovery guidance was itself instructing the failure it was supposed to fix.
2. **Mutable base-SHA binding.** Previous-version fetches for breaking-change comparison used the `main` branch name or an ambiguous `{base-sha-or-ref}` shorthand rather than a fixed commit. A PR's comparison baseline could silently shift underneath a review if `main` moved between comparison and posting.
3. **Weak thread-ownership authorization.** Autonomous thread resolution treated a `posted-by: arm-api-reviewer-agent` marker substring alone as sufficient authorization to resolve or mutate a thread, without confirming the author was actually `github-actions[bot]` — a marker string is attribution, not authentication, and could in principle be present in human-authored text.
4. **No overflow disclosure in interactive mode.** When findings exceeded posting caps, the workflow already collapsed the overflow into a "key themes" summary, but the interactive agent's review-body template had no equivalent slot, so overflow information could be silently dropped in that context.
5. **Marker degradation on non-standard runs.** Failure-mode recovery paths (e.g., "Comment-post returned no visible output," "Inline-anchor 422") could leave findings whose only provenance was a hidden marker with no visible attribution text, making it unclear to a human reader that an automated review had occurred at all.

## Evidence

- #44356 — the shell-quoting corruption report that is direct evidence for gap 1.
- #45013 — visible-attribution-preamble gaps, directly evidencing gap 5 and adjacent to gap 4.
- #45800 — truncated-scope disclosure gap, part of the same "silent degradation" theme addressed alongside gap 4.
- Local snapshot digest review during this hardening effort found the digest did not include a `repository-head` component, meaning a HEAD-only drift with no file changes would not invalidate a cached local snapshot — a related SHA-binding integrity gap folded into gap 2's fix.
- PR #45841.

## Proposed Solution (implemented in PR #45841)

1. Add a "Shell fallback discipline" section to the agent: MCP tools first; any unavoidable `--jq`/GraphQL expression goes through a temp file or `ConvertFrom-Json`, never inline in a double-quoted PowerShell string; cap shell-syntax retries at 2 before switching approach. Rewrite the three existing recipes that violated this (`--jq` one-liner, `gh api graphql` fallback, `resolveReviewThread` fallback) to match.
2. Bind all previous-version/base-SHA fetches to the full 40-character immutable commit SHA, never a branch name or shorthand. Extend the local snapshot digest to include a `repository-head` line with the full 40-character HEAD SHA so HEAD-only drift invalidates the snapshot even without file changes.
3. Require both a valid marker **and** author `github-actions[bot]` for any autonomous thread mutation/resolution; a marker alone remains sufficient for attribution/telemetry but not for authorization.
4. Add an overflow-disclosure template slot to the interactive review-body template, matching the workflow's existing "N additional findings were identified but not posted inline" wording.
5. Add a post-condition after the last posting action: re-fetch reviews/comments and assert at least one visible body contains the review marker heading; if none does, post the preamble as a top-level comment before the run ends, so no run can complete with only a hidden marker as its record.

## Labels

`ARM API Reviewer agent`

Contributor guide

Open the contributing guide

Research direction

Start by reviewing PR #45841 and the interactive agent, workflow definitions, review-body template, recovery paths, and local snapshot digest described in the issue. Done means fallback recipes avoid fragile PowerShell interpolation, base references use immutable SHAs, thread mutations verify bot ownership, overflow and visible attribution are disclosed, and missing visible markers trigger a preamble comment.

Written by the indexing model from the issue text.

Assessment

Tech stack
git, github-actions, powershell
Domain
ci-cd, security, tooling
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.