feat: implement issue #402 — [Fleet Monitor] petry-projects/.github-private — pr-review.yml

prompts/single-review.md

@@ -12,6 +12,7 @@ is small or this is a re-review after a prior cascade review.
 
 - `$PR_URL` — the PR to review.
 - `$PR_HEAD_SHA` — the head commit SHA.
+- `$OUTPUT_FILE` — path where you **must** write the final verdict JSON.
 - `$DRY_RUN` — `true` or `false`.
 - `$AI_DELEGATION_ENABLED` — `true` or `false` (repo org has AI delegation configured).
 - `$REVIEW_CYCLE` — integer, number of prior review cycles.
@@ -35,8 +36,8 @@ You review **exactly one pull request**: `$PR_URL`. Nothing else.
 ## Context-gathering
 
 1. `gh pr view "$PR_URL" --json number,title,body,author,isDraft,baseRefName,headRefName,headRefOid,url,headRepository,headRepositoryOwner,labels,reviewDecision,mergeable,mergeStateStatus,statusCheckRollup,reviewRequests,reviews,comments,commits,closingIssuesReferences,additions,deletions,changedFiles,files`
-   - If `isDraft` → skip. Print `{"pr":"...","decision":"skip","reason":"draft"}` and exit.
-   - Verify `headRefOid == $PR_HEAD_SHA`. If not → skip with `"reason":"head-sha-changed"`.
+   - If `isDraft` → skip. Write the skip verdict to `$OUTPUT_FILE` using `jq` (e.g., `jq -n --arg pr "$PR_URL" '{pr: $pr, decision: "skip", reason: "draft"}' > "$OUTPUT_FILE"`) and exit.
+   - Verify `headRefOid == $PR_HEAD_SHA`. If not → write the skip verdict to `$OUTPUT_FILE` using `jq` (e.g., `jq -n --arg pr "$PR_URL" '{pr: $pr, decision: "skip", reason: "head-sha-changed"}' > "$OUTPUT_FILE"`) and exit.
 2. `gh pr diff "$PR_URL"` — read the diff.
    - **Incremental mode**: also get the diff since the prior review. Derive
      `<owner>` and `<repo>` from the `headRepository` field in the PR metadata
@@ -98,25 +99,79 @@ If all prior findings are resolved AND no new issues → approve.
 
 ## Output
 
-Compose a complete review verdict with the review body, then output as JSON:
-
-```json
-{
-  "pr": "<PR_URL>",
-  "sha": "<PR_HEAD_SHA>",
-  "risk": "LOW|MEDIUM|HIGH",
-  "decision": "approve|escalate",
-  "mode": "small|incremental|triage-approved",
-  "summary": "2-4 sentence summary of review",
-  "body": "<!-- pr-review-agent v1 sha=<PR_HEAD_SHA> decision=<approved|escalated> risk=<LOW|MEDIUM|HIGH> -->\n\n## Automated review — <APPROVED ✓|NEEDS HUMAN REVIEW>\n\n**Risk:** <risk>\n**Reviewed commit:** `<SHA>`\n**Review mode:** <mode> (single reviewer)\n\n### Summary\n<summary>\n\n### Linked issue analysis\n<analysis>\n\n### Findings\n<findings>\n\n### CI status\n<status>\n\n---\n_Reviewed automatically by the PR-review agent ($ENGINE_SINGLE_LABEL). Reply if you need a human review._",
-  "escalate_to_ai": false
-}
+Compose the review body as a markdown string, then write the verdict JSON to
+`$OUTPUT_FILE` using `jq` so all strings (especially the body) are properly
+escaped. Use the pattern below exactly:
+
+0. Initialize verdict variables from your analysis:
+
+```bash
+DECISION="approve"     # or "escalate"
+RISK="LOW"             # "LOW", "MEDIUM", or "HIGH"
+MODE="$REVIEW_MODE"
+SUMMARY="..."
+ISSUE_ANALYSIS="..."
+FINDINGS="..."
+CI_STATUS="..."
+# Marker vocabulary: "approved"/"escalated" (review-cycle.sh expects these exact strings)
+DECISION_MARKER=$([ "$DECISION" = "approve" ] && echo "approved" || echo "escalated")
 ```
 
-The `body` field must be the complete markdown text that will be posted to GitHub.
+1. Build the review body in a temp file:
 
-**Important:** The bash script will parse this JSON and post the review, so ensure:
-1. `decision` is either `approve` or `escalate`
-2. `body` is properly escaped JSON string with embedded newlines (\n)
-3. The review body includes the HTML marker comment on line 1
-4. Output ONLY valid JSON to stdout (no other text)
+```bash
+HEADING=$([ "$DECISION" = "approve" ] && echo "APPROVED ✓" || echo "NEEDS HUMAN REVIEW")
+cat > /tmp/single-review-body.txt << BODYEOF
+<!-- pr-review-agent v1 sha=${PR_HEAD_SHA} decision=${DECISION_MARKER} risk=${RISK} -->
+
+## Automated review — ${HEADING}
+
+**Risk:** ${RISK}
+**Reviewed commit:** \`${PR_HEAD_SHA}\`
+**Review mode:** ${MODE} (single reviewer)
+
+### Summary
+${SUMMARY}
+
+### Linked issue analysis
+${ISSUE_ANALYSIS}
+
+### Findings
+${FINDINGS}
+
+### CI status
+${CI_STATUS}
+
+---
+_Reviewed automatically by the PR-review agent (${ENGINE_SINGLE_LABEL}). Reply if you need a human review._
+BODYEOF
+```
+
+2. Write the verdict JSON to `$OUTPUT_FILE` using jq so all strings are
+   properly escaped:
+
+```bash
+BODY=$(cat /tmp/single-review-body.txt)
+ESCALATE_TO_AI=$([ "$AI_DELEGATION_ENABLED" = "true" ] && [ "$DECISION" = "escalate" ] && [ "$RISK" != "HIGH" ] && echo "true" || echo "false")
+jq -n \
+  --arg pr        "$PR_URL" \
+  --arg sha       "$PR_HEAD_SHA" \
+  --arg risk      "$RISK" \
+  --arg decision  "$DECISION" \
+  --arg mode      "$MODE" \
+  --arg summary   "$SUMMARY" \
+  --arg body      "$BODY" \
+  --argjson escalate_to_ai "$ESCALATE_TO_AI" \
+  '{pr: $pr, sha: $sha, risk: $risk, decision: $decision, mode: $mode, summary: $summary, body: $body, escalate_to_ai: $escalate_to_ai}' \
+  > "$OUTPUT_FILE"
+```
+
+3. Verify the output is valid (send to stderr so Copilot's tee does not corrupt `$OUTPUT_FILE`):
+
+```bash
+jq -r '.decision' "$OUTPUT_FILE" >&2
+echo "Verdict written to $OUTPUT_FILE" >&2
+```
+
+**IMPORTANT:** Do NOT print the JSON to stdout. Write it to `$OUTPUT_FILE`
+only. The bash script reads it from there.

scripts/engine.sh

@@ -626,19 +626,14 @@ run_agentic() {
       ;;
     copilot)
       # Full tool support via GitHub Copilot CLI agentic mode (--yolo).
-      # Stream directly to stdout; tee to OUTPUT_FILE when set.
-      if [ -n "${OUTPUT_FILE:-}" ]; then
-        if [ -n "$_tok_tmp" ]; then
-          copilot_chat "$prompt_file" "$DEEP_TIMEOUT_SEC" --yolo | tee "$OUTPUT_FILE" "$_tok_tmp" || rc=${PIPESTATUS[0]}
-        else
-          copilot_chat "$prompt_file" "$DEEP_TIMEOUT_SEC" --yolo | tee "$OUTPUT_FILE" || rc=${PIPESTATUS[0]}
-        fi
+      # Do NOT tee stdout to OUTPUT_FILE: the prompt instructs the model to
+      # write the verdict JSON directly to $OUTPUT_FILE via the Bash tool.
+      # Teeing stdout (which includes assistant text and tool transcripts)
+      # would overwrite that file and corrupt the JSON.
+      if [ -n "$_tok_tmp" ]; then
+        copilot_chat "$prompt_file" "$DEEP_TIMEOUT_SEC" --yolo | tee "$_tok_tmp" || rc=${PIPESTATUS[0]}
       else
-        if [ -n "$_tok_tmp" ]; then
-          copilot_chat "$prompt_file" "$DEEP_TIMEOUT_SEC" --yolo | tee "$_tok_tmp" || rc=${PIPESTATUS[0]}
-        else
-          copilot_chat "$prompt_file" "$DEEP_TIMEOUT_SEC" --yolo || rc=$?
-        fi
+        copilot_chat "$prompt_file" "$DEEP_TIMEOUT_SEC" --yolo || rc=$?
       fi
       ;;
   esac
@@ -739,18 +734,12 @@ run_duck() {
     copilot)
       unset CLAUDE_CODE_OAUTH_TOKEN 2>/dev/null || true
       unset GOOGLE_API_KEY 2>/dev/null || true
-      if [ -n "${OUTPUT_FILE:-}" ]; then
-        if [ -n "$_tok_tmp" ]; then
-          copilot_chat "$prompt_file" "$DUCK_TIMEOUT_SEC" --yolo | tee "$OUTPUT_FILE" "$_tok_tmp" || rc=${PIPESTATUS[0]}
-        else
-          copilot_chat "$prompt_file" "$DUCK_TIMEOUT_SEC" --yolo | tee "$OUTPUT_FILE" || rc=${PIPESTATUS[0]}
-        fi
+      # Do NOT tee stdout to OUTPUT_FILE — same rationale as run_agentic copilot
+      # branch: the prompt writes verdict JSON directly via the Bash tool.
+      if [ -n "$_tok_tmp" ]; then
+        copilot_chat "$prompt_file" "$DUCK_TIMEOUT_SEC" --yolo | tee "$_tok_tmp" || rc=${PIPESTATUS[0]}
       else
-        if [ -n "$_tok_tmp" ]; then
-          copilot_chat "$prompt_file" "$DUCK_TIMEOUT_SEC" --yolo | tee "$_tok_tmp" || rc=${PIPESTATUS[0]}
-        else
-          copilot_chat "$prompt_file" "$DUCK_TIMEOUT_SEC" --yolo || rc=$?
-        fi
+        copilot_chat "$prompt_file" "$DUCK_TIMEOUT_SEC" --yolo || rc=$?
       fi
       ;;
     *)

tests/test_cli_error_routing.bats

@@ -121,3 +121,112 @@ route_error() {
   [ "$status" -eq 0 ]
   [ "$output" = "pass-through" ]
 }
+
+# ---------------------------------------------------------------------------
+# Regression: single-review.md must instruct the model to write to $OUTPUT_FILE
+# (not to stdout).  When TOKEN_LOG_FILE is set, engine.sh adds --output-format
+# json, which causes the model's Bash tool echoes to NOT appear in the `result`
+# field — only text the model emits as its final message does.  If the prompt
+# says "output to stdout", the model's Bash echo goes to tool output (not
+# result), result stays "", and extract_verdict_json never sees the review JSON.
+# Fix: the prompt must use `jq > "$OUTPUT_FILE"` like cascade-action.md does.
+# ---------------------------------------------------------------------------
+
+@test "single-review prompt: instructs model to write to OUTPUT_FILE (not stdout)" {
+  local prompt_file
+  prompt_file="$(dirname "$BATS_TEST_FILENAME")/../prompts/single-review.md"
+
+  # The Output section must reference OUTPUT_FILE so the model writes there.
+  run grep -c 'OUTPUT_FILE' "$prompt_file"
+  [ "$status" -eq 0 ]
+  [ "$output" -ge 1 ]
+}
+
+@test "single-review prompt: does NOT instruct model to output to stdout" {
+  local prompt_file
+  prompt_file="$(dirname "$BATS_TEST_FILENAME")/../prompts/single-review.md"
+
+  # The old (broken) instruction said "Output ONLY valid JSON to stdout".
+  # After the fix, this phrase must be absent.
+  run grep -c 'Output ONLY valid JSON to stdout' "$prompt_file"
+  [ "$output" -eq 0 ]
+}
+
+@test "single-review prompt: instructs model NOT to print JSON to stdout" {
+  local prompt_file
+  prompt_file="$(dirname "$BATS_TEST_FILENAME")/../prompts/single-review.md"
+
+  # The fixed prompt must include the same guard present in cascade-action.md.
+  run grep -c 'Do NOT print' "$prompt_file"
+  [ "$status" -eq 0 ]
+  [ "$output" -ge 1 ]
+}
+
+# ---------------------------------------------------------------------------
+# extract_verdict_json: Style 1 (model wrote to dest/OUTPUT_FILE directly)
+# must be preferred over Style 2 (raw contains claude API wrapper).
+# This is the correct post-fix path: model writes via jq to $OUTPUT_FILE.
+# ---------------------------------------------------------------------------
+
+@test "extract_verdict_json: dest has review JSON → decision extracted (Style 1)" {
+  source "$(dirname "$BATS_TEST_FILENAME")/../scripts/engine.sh" >/dev/null 2>&1 || true
+
+  local raw dest
+  raw="$(mktemp)"
+  dest="$(mktemp)"
+
+  # raw = claude API wrapper (what _claude_chain_invoke emits when result="")
+  printf '%s\n' \
+    '{"type":"result","subtype":"success","is_error":false,"result":"","stop_reason":"end_turn"}' \
+    > "$raw"
+
+  # dest = review JSON written by the model via jq > "$OUTPUT_FILE"
+  printf '%s\n' \
+    '{"pr":"https://github.com/org/repo/pull/1","sha":"abc","risk":"LOW","decision":"approve","mode":"triage-approved","summary":"ok","body":"..."}' \
+    > "$dest"
+
+  extract_verdict_json "$raw" "$dest"
+  local decision
+  decision="$(jq -r '.decision // ""' "$dest")"
+  [ "$decision" = "approve" ]
+
+  rm -f "$raw" "$dest"
+}
+
+# ---------------------------------------------------------------------------
+# Regression: engine.sh must NOT tee Copilot stdout into OUTPUT_FILE.
+# The model writes verdict JSON directly via the Bash tool; teeing stdout
+# (which includes assistant text and tool transcripts) would corrupt it.
+# ---------------------------------------------------------------------------
+
+@test "engine.sh: run_agentic copilot branch does not tee stdout to OUTPUT_FILE" {
+  local engine_file
+  engine_file="$(dirname "$BATS_TEST_FILENAME")/../scripts/engine.sh"
+
+  # The run_agentic copilot branch must not contain `tee.*OUTPUT_FILE`.
+  # We extract only the copilot) block of run_agentic and check it is clean.
+  run grep -n 'tee.*OUTPUT_FILE\|OUTPUT_FILE.*tee' "$engine_file"
+  [ "$output" = "" ]
+}
+
+@test "extract_verdict_json: raw=claude-API-wrapper, dest missing → decision empty (bug scenario)" {
+  source "$(dirname "$BATS_TEST_FILENAME")/../scripts/engine.sh" >/dev/null 2>&1 || true
+
+  local raw dest
+  raw="$(mktemp)"
+  dest="$(mktemp)"
+  rm -f "$dest"  # simulate model never writing to OUTPUT_FILE
+
+  # raw = claude API wrapper with empty result (the pre-fix failure mode)
+  printf '%s\n' \
+    '{"type":"result","subtype":"success","is_error":false,"result":"","stop_reason":"end_turn"}' \
+    > "$raw"
+
+  extract_verdict_json "$raw" "$dest"
+  local decision
+  decision="$(jq -r '.decision // ""' "$dest" 2>/dev/null || true)"
+  # The bug: decision is empty because the claude API wrapper has no decision field
+  [ "$decision" = "" ]
+
+  rm -f "$raw" "$dest"
+}