feat(dispatcher): fail-fast preflight when ast-grep binary is missing

[bborbe]

Summary

Closes the silent-failure gap surfaced by bborbe/coding#34: the pr-reviewer container regressed on the ast-grep install, the new dispatcher's mechanical funnel returned empty `tool_result` to every `sg --version` LLM check, the model looped 30 min until `activeDeadlineSeconds` killed the job, and the PR ended up admin-merged without a real review.

Fix

Three coordinated changes:

1. `agents/ast-grep-runner.md` Step 0 — explicit preflight check before any inventory or scan attempt. Emits a precise error pointing at install commands (npm / apk / debian) and the maintainer Dockerfile commit (`1de083f`) that closed the container gap, then exit 1.

2. `commands/pr-review.md` Step 4.0 — parallel preflight at the dispatcher level so the failure surfaces close to the user's invocation, not buried inside the runner's JSON envelope.

3. `commands/code-review.md` Step 4.0 — mirror of the above.

Why exit 1 rather than continue with errors[]

An empty `findings_by_owner` would let the LLM tier post APPROVED with no comments — silently masking that the mechanical layer never ran. Exit 1 surfaces the gap as Must Fix in the review report.

Why duplicate the check at runner + dispatcher

• Runner-level: defensive — runner is the single source of truth for "did the mechanical funnel actually run?"
• Dispatcher-level: UX — user-visible error surfaces close to the slash command invocation; runner's exit 1 carries minimal context

Test plan

• `make precommit` clean (131 rules, 28 mechanical YAMLs, no drift)
• Review of the three preflight blocks for syntax (bash, error message wording, exit codes)
• Bot review
• Manual smoke: temporarily `PATH=` mask ast-grep, re-run `/coding:code-review master`, confirm dispatcher aborts with the error message instead of looping

[ben-s-pull-request-reviewer]

Based on my analysis of the diff, let me compile the review findings.

Analysis Summary

Diff Overview

Three files changed:

• agents/ast-grep-runner.md — Added Step 0 preflight check + updated Failure Modes table
• commands/code-review.md — Added Step 4.0 toolchain preflight
• commands/pr-review.md — Added Step 4.0 toolchain preflight

Review Against Plan Concerns

✓ Concern 1 — Identical PATH-checking logic: Both ast-grep-runner.md Step 0 and both command files' Step 4.0 use command -v ast-grep >/dev/null 2>&1 || command -v sg >/dev/null 2>&1 — identical logic, both exit 1 on failure.

✓ Concern 2 — JSON skeleton before exit: ast-grep-runner.md line 75 emits the valid JSON skeleton via printf before exit 1 on line 76. The structure matches the documented contract (stats, findings_by_owner, errors).

✓ Concern 3 — Step 4.0 mirroring: code-review.md lines 82-84 and pr-review.md lines 101-103 have identical bash conditionals and error message wording. Minor formatting difference (&& vs \ line continuation) is cosmetic.

✓ Concern 4 — Preflight failure reporting: Both commands document "report in Step 5 (Must Fix) and skip Step 4 entirely" — lines 86 and 105 respectively.

✓ Concern 5 — Dockerfile commit reference: All three files reference bborbe/maintainer agent/pr-reviewer/Dockerfile (commit 1de083f consistently.

Note on Concern 6 — Smoke test: The agents/ast-grep-runner.md Smoke Test section (lines 150-167) documents manual smoke tests. The plan mentions "make precommit clean" but make precommit here runs link/JSON validation (no runtime tests in a plugin-only repo) — this is appropriate.

Precommit Check

Makefile has precommit: target (line 4: precommit: check-links check-json check-index check-coverage). Unable to execute in this environment.

Manual Review Finding

The duplicate preflight logic (dispatcher does it AND runner does it) is intentional per the design — the dispatcher check gives user-facing error immediately; the runner check is the authoritative fail-safe. This is correctly documented.

{
  "verdict": "approve",
  "summary": "Feature adds fail-fast preflight for the ast-grep binary to both the dispatcher commands (pr-review, code-review) and the ast-grep-runner agent itself. All three preflight implementations use identical PATH-checking logic and exit 1 with an actionable error message, preventing the silent-empty-review failure mode from bborbe/coding#34. The JSON skeleton emitted before exit 1 is valid and matches the documented contract. All concerns from the plan review are addressed.",
  "comments": [],
  "concerns_addressed": [
    "correctness: Preflight Step 0 and Step 4.0 use identical PATH-checking logic (command -v ast-grep || command -v sg) — verified",
    "correctness: Runner emits valid JSON skeleton to stdout before exit 1 (line 75) — verified",
    "correctness: code-review.md Step 4.0 mirrors pr-review.md Step 4.0 with identical bash conditional and error message — verified",
    "correctness: Preflight failure is documented to be reported in Step 5 Must Fix and skips Step 4 — verified in both commands",
    "security: Dockerfile commit 1de083f referenced consistently across all three changed files — verified",
    "tests: Smoke test steps documented in ast-grep-runner.md (lines 150-167); plugin-only repo does not have runtime tests — appropriate"
  ]
}

[ben-s-pull-request-reviewer]

{
  "verdict": "request-changes",
  "summary": "PR #36 correctly identifies the failure mode from #34 (ast-grep binary missing causing silent empty reviews) and adds fail-fast preflight to both command files and the ast-grep-runner agent. However, the preflight Bash commands in both `commands/code-review.md` and `commands/pr-review.md` use `cd`, `command -v`, `echo`, `exit`, and shell operators that are NOT declared in their `allowed-tools` frontmatter — the commands would be rejected at runtime before the preflight can execute. Additionally, a broken wiki-link reference to a non-existent agent exists in `agents/ast-grep-runner.md`.",
  "comments": [
    {
      "file": "commands/code-review.md",
      "line": 82,
      "severity": "critical",
      "message": "allowed-tools gap: preflight uses `cd`, `command -v`, `echo`, `exit`, and shell operators (`||`, `{`, `}`) none of which are declared. Only `Bash(git diff:+)`, `Bash(git log:+)`, `Bash(git status:+)`, `Bash(git ls-files:+)` are permitted. The preflight would be rejected at runtime before it can execute. Fix: add required tools to allowed-tools or delegate to a sub-agent that has `Bash` access."
    },
    {
      "file": "commands/pr-review.md",
      "line": 101,
      "severity": "critical",
      "message": "Same allowed-tools gap as code-review.md: `cd`, `command -v`, `echo`, `exit`, and `|| { }` operators are not in the declaration (`Bash(git *)`, `Bash(git fetch:+)`, `Bash(git worktree:+)`, `Bash(git branch:+)`, `Bash(rm -rf:+)` only). The preflight would fail at the permission layer before running. Fix: add required tools to allowed-tools or delegate to a sub-agent."
    },
    {
      "file": "agents/ast-grep-runner.md",
      "line": 73,
      "severity": "major",
      "message": "Broken reference: `[[Agent - Debug Missing PR Reviewer Review]]` points to a non-existent agent file in this codebase. This link will render as a dead wiki-link in Obsidian. Should be removed or replaced with a valid reference."
    },
    {
      "file": "commands/code-review.md",
      "line": 82,
      "severity": "major",
      "message": "Redundant preflight: Step 4.0 duplicates the exact same check already implemented in `agents/ast-grep-runner.md` Step 0. If the runner is invoked first (which it is, per Step 4a), it already fail-fasts with the same error. The inline preflight is redundant and creates maintenance risk — if one changes without the other, they diverge. Consider removing the inline preflight and relying solely on the runner's Step 0, or restructuring to delegate this check to a shared sub-agent."
    },
    {
      "file": "commands/pr-review.md",
      "line": 101,
      "severity": "major",
      "message": "Same redundancy issue as code-review.md: Step 4.0 duplicates the runner's Step 0 preflight. The runner is invoked at Step 4a, so the inline preflight adds no value — it just creates a second failure point that can drift out of sync."
    },
    {
      "file": "agents/ast-grep-runner.md",
      "line": 62,
      "severity": "nit",
      "message": "The preflight check `! command -v ast-grep >/dev/null 2>&1 && ! command -v sg >/dev/null 2>&1` uses `&&` (AND), so it only fails if NEITHER binary exists. This is correct (fail-fast if neither is available), but verify this matches the command files' `||` (OR) formulation — they check if at least one is missing and exit 1. The logic is subtly different: runner requires BOTH missing to trigger, commands trigger if EITHER is missing."
    }
  ],
  "concerns_addressed": [
    "correctness: exit 1 behavior verified — intentional fail-fast instead of silently posting empty review",
    "correctness: JSON error payload format matches dispatcher contract — structured envelope with stats, findings_by_owner, and errors[]",
    "correctness: Step 4.0 in pr-review.md and code-review.md mirrors runner Step 0 — implementations are mostly consistent (see critical allowed-tools gap)",
    "correctness: duplication across both command files confirmed intentional but creates maintenance risk (see major finding)",
    "tests: no automated test coverage added for preflight path — gap acknowledged in plan"
  ]
}