From bf28178a08c791333376a231a60957870ed4b290 Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Wed, 3 Jun 2026 09:03:26 +0200 Subject: [PATCH] feat(dispatcher): fail-fast preflight when ast-grep binary is missing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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_results 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: explicit Step 0 in agents/ast-grep-runner.md and a parallel Step 4.0 in both commands/pr-review.md + commands/code-review.md that verify 'ast-grep' or 'sg' is in PATH. If absent, emit a precise error pointing at the install command (npm / apk / debian) and the maintainer Dockerfile commit (1de083f) that closes the container gap, then exit 1. Why exit 1 rather than continue: 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. The check is duplicated at the dispatcher level so the failure surfaces close to the user's invocation, not buried inside the runner agent's JSON envelope. --- agents/ast-grep-runner.md | 27 ++++++++++++++++++++++++++- commands/code-review.md | 11 +++++++++++ commands/pr-review.md | 11 +++++++++++ 3 files changed, 48 insertions(+), 1 deletion(-) diff --git a/agents/ast-grep-runner.md b/agents/ast-grep-runner.md index 9fe8f91..27e13c3 100644 --- a/agents/ast-grep-runner.md +++ b/agents/ast-grep-runner.md @@ -54,6 +54,31 @@ If `ast-grep` itself fails on a YAML (syntax error, missing file), include the e ## Process +### 0. Preflight: verify ast-grep binary (fail-fast) + +**Always run this first, before any scan or inventory step.** Without the binary, every subsequent `ast-grep scan` call returns empty `tool_result`, the LLM loops on the verification check, and the job hits `activeDeadlineSeconds` 30 min later with no review posted. Observed end-to-end on bborbe/coding#34 (admin-merged after the loop). + +```bash +if ! command -v ast-grep >/dev/null 2>&1 && ! command -v sg >/dev/null 2>&1; then + cat <<'EOF' >&2 +ERROR: ast-grep / sg binary not found in PATH. +This funnel cannot run without it. + +Install (host): npm install -g @ast-grep/cli +Install (alpine): apk --no-cache add ast-grep +Install (debian): curl -fsSL https://github.com/ast-grep/ast-grep/releases/latest/download/ast-grep-linux-x86_64.tar.gz | tar xz -C /usr/local/bin +Container fix: ensure pr-reviewer image has ast-grep — see + bborbe/maintainer agent/pr-reviewer/Dockerfile (commit 1de083f + added 'ast-grep' to the alpine apk line). +Runbook: [[Agent - Debug Missing PR Reviewer Review]] gate 5. +EOF + printf '{"stats":{"yamls_run":0,"findings_count":0,"elapsed_ms":0},"findings_by_owner":{},"errors":[{"kind":"missing-tool","tool":"ast-grep","detail":"binary not in PATH"}]}\n' + exit 1 +fi +``` + +**Why exit 1, not "continue with errors":** the dispatcher's per-Owner adjudication phase reads from `findings_by_owner` only. A missing-tool error in `errors[]` would be invisible to the LLM tier — the review would post as APPROVED with no comments, masking that the mechanical layer never ran. Exit 1 surfaces the gap to the dispatcher (which aborts the review with an actionable error) instead of producing a silently-empty review. + ### 1. Resolve rule inventory ```bash @@ -117,7 +142,7 @@ Write the structured output to stdout. The dispatcher reads stdout, slices `find | Symptom | Cause | Handling | |---|---|---| -| ast-grep binary missing | toolchain gap | emit `{"errors": [{"kind": "missing-tool", "tool": "ast-grep"}]}`, exit non-zero | +| ast-grep binary missing | toolchain gap (e.g. pr-reviewer image regression — see bborbe/coding#34) | preflight Step 0 catches this BEFORE any scan; emit the missing-tool JSON skeleton, exit 1, dispatcher surfaces actionable error rather than posting an empty review | | YAML rejected by ast-grep | rule syntax bug (PR #4 / #8 trap) | append to `errors[]`, continue | | Rule ID not in index | stale walker output | append to `errors[]`, continue | | Owner from index missing in `agents/` | stale rule entry | append to `errors[]` with `kind: missing-owner-agent`, drop finding | diff --git a/commands/code-review.md b/commands/code-review.md index 4d48446..dbcdb0b 100644 --- a/commands/code-review.md +++ b/commands/code-review.md @@ -74,6 +74,17 @@ Mirrors `commands/pr-review.md` Step 4 (PR #27). Mechanical pre-filter via `codi - "Missing LICENSE file" - "README missing license section" (check with Grep for `## License` in README.md) +#### 4.0: Toolchain preflight (fail-fast) + +Mirror of `commands/pr-review.md` Step 4.0. Verify `ast-grep` is in PATH before invoking the runner; the runner fail-fasts on the same check (`agents/ast-grep-runner.md` Step 0), but doing it here too keeps the failure surface close to the dispatcher: + +```bash +cd && (command -v ast-grep >/dev/null 2>&1 || command -v sg >/dev/null 2>&1) \ + || { echo "ERROR: ast-grep/sg not in PATH. Install via 'npm install -g @ast-grep/cli' (or 'apk add ast-grep' in alpine). pr-reviewer container fix: bborbe/maintainer agent/pr-reviewer/Dockerfile commit 1de083f." >&2; exit 1; } +``` + +If preflight fails, report the toolchain gap in Step 5 (Must Fix) and skip Step 4 entirely. + #### 4a: Mechanical funnel ``` diff --git a/commands/pr-review.md b/commands/pr-review.md index f0e9189..3e1970c 100644 --- a/commands/pr-review.md +++ b/commands/pr-review.md @@ -93,6 +93,17 @@ The dispatcher replaces the previous hardcoded "load conventions + invoke fixed **Short Mode**: No agents — skip to Step 5. +#### 4.0: Toolchain preflight (fail-fast) + +Before invoking the runner, verify `ast-grep` is available in PATH. The runner itself fail-fasts on the same check (`agents/ast-grep-runner.md` Step 0), but doing it here too keeps the failure surface close to the dispatcher so the user sees a single clear error rather than the runner's JSON envelope: + +```bash +cd && (command -v ast-grep >/dev/null 2>&1 || command -v sg >/dev/null 2>&1) \ + || { echo "ERROR: ast-grep/sg not in PATH. Install via 'npm install -g @ast-grep/cli' (or 'apk add ast-grep' in alpine). pr-reviewer container fix: bborbe/maintainer agent/pr-reviewer/Dockerfile commit 1de083f." >&2; exit 1; } +``` + +If preflight fails, report the toolchain gap in Step 5 (Must Fix) and skip Step 4 entirely — a review without the mechanical funnel would silently miss every MUST-tier YAML finding. + #### 4a: Mechanical funnel ```