Azure / Azure/azure-rest-api-specs

ARM API Reviewer agent: harden gh CLI fallbacks against PowerShell quoting failures and add a shell-retry loop-breaker

Open
#44,356 0 comments 1 reaction 2 assignees Claimed by @ravimeda 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

### Background

Two ARM API Reviewer sessions got stuck in a loop at the **data-fetch stage** (not in review logic): PR #44324 (public) during Step 5.5 comment fetch, and PR #29150 in `azure-rest-api-specs-pr` during a Step 3 file/diff fetch. In each case the agent shelled out to `gh` with a command that is malformed for **PowerShell** or for `gh`'s own arg parser, the command failed, and the agent retried quoting/escaping permutations of the *same* broken command instead of switching approach. That retry-without-changing-approach is the loop. (The repeated "Warning from Post-Tool Use hook" lines are the post-tool hook firing on every failed terminal call.)

Observed failures:

1. **Inline `gh ... --jq` filter mangled by PowerShell** (PR #44324):
- `gh api ".../pulls/44324/comments" --paginate --jq '.[] | {id: .id, body: ..., path: .path, ...}'` → `gh : accepts 1 arg(s), received 3`. The single-quoted jq object filter is word-split before it reaches `gh`, so `gh api` sees 3 positional args instead of 1.
- The agent "fixed" it with double quotes + escaped `\"`: `gh api ... --jq ".[] | (.id | tostring) + \" | \" + .user.login + ..."` → `CommandNotFoundException`. Under double quotes PowerShell treats the bare `|` in the jq filter as a **pipeline operator** and tries to run `.user.login` as a command.

2. **`gh pr diff -- ` is not valid** (PR #29150):
- `gh pr diff 29150 --repo Azure/azure-rest-api-specs-pr -- "specification/.../readme.md"` → `accepts at most 1 arg(s), received 2`. Unlike `git diff`, `gh pr diff` has no git-style `-- ` filter, so the file path becomes an illegal second positional arg.

**Root cause.** The agent went straight to fragile inline `gh --jq` / `gh pr diff` shell one-liners instead of the **preferred GitHub MCP tools** (`get_review_comments`, `get_file_contents`, `get_pull_request`, `list_pull_request_files`), and it has no rule that a **shell parse error is a client-side failure** (not a GitHub failure) that must trigger a change of approach. So it brute-forced quoting variants, which the base instructions explicitly forbid.

### Proposed solution

Surgical additions to [.github/agents/arm-api-reviewer.agent.md](https://github.com/Azure/azure-rest-api-specs/blob/main/.github/agents/arm-api-reviewer.agent.md) (Step 5.5 fetch guidance + Failure Modes table); no change to the Critic protocol:

1. **PowerShell-safe `gh` recipe.** In "Fetching Files from GitHub" / Failure Modes, add:
- Prefer the GitHub MCP tools first; shell out to `gh` only when the MCP tool is missing or errors.
- When a `--jq` filter or GraphQL `query` is unavoidable, **write it to a temp file** and reference it (`--jq (Get-Content -Raw filter.jq)`, `-F query=@query.graphql`) rather than inlining `|`, `{}`, spaces, and `\"`. Or drop `--jq` and pipe raw JSON through `ConvertFrom-Json`.
- Never build a jq/GraphQL expression inside a **double-quoted** PowerShell string — a bare `|` becomes a pipeline operator.

2. **Shell-retry loop-breaker.** Add a Failure Modes row: a shell parse error (`CommandNotFoundException`, `accepts N arg(s)`, unbalanced-quote) is a **client-side** failure, not a GitHub failure. On the **second** consecutive shell-syntax failure for the same objective, stop retrying the shell and fall back to the MCP tool (or a temp-file-based invocation). Cap `gh`-syntax retries at 2.

3. **`gh pr diff` caveat.** Note that `gh pr diff` has no `-- `; to inspect one file use `get_file_contents` (preferred) or `gh api repos///contents/?ref=`.

### Acceptance criteria

- The Reviewer fetches comments/files via the MCP tools first and only uses `gh` as a documented fallback.
- When `gh --jq` / `gh api graphql` is used, the filter/query is passed via a temp file, never inlined in a double-quoted PowerShell string.
- Two consecutive shell-syntax failures on the same objective force a tool switch instead of further quoting retries (no more loop).
- `gh pr diff -- ` is never emitted; single-file inspection uses `get_file_contents` or the `contents` API.

### References

- Triggering sessions: PR #44324 (public), PR #29150 in `azure-rest-api-specs-pr`.
- Agent definition: [.github/agents/arm-api-reviewer.agent.md](https://github.com/Azure/azure-rest-api-specs/blob/main/.github/agents/arm-api-reviewer.agent.md) — Step 5.5, Failure Modes & Recovery.
- Related: [#44098](https://github.com/Azure/azure-rest-api-specs/issues/44098) (Reviewer ↔ Critic protocol hardening).

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.