fix: Prevent extract-result.sh crashes and ensure webhook fires on failure (#72)

sylvansys · claude · web-flow · commit 9d14d05003d7 · 2026-01-30T11:35:18.000-08:00
Three bugs caused review checks to pass in comments but fail in GitHub checks: Bug A: extract-result.sh crashed under set -e -o pipefail because grep/perl return non-zero on no match, and Strategy 4's Python regex was fundamentally broken. Rewrote all strategies with || true guards, proper jq -e '.checks' validation, and a brace-depth-counting Python extractor. Strategy 1 now correctly picks the last valid json block (skipping examples). Bug B: Webhook steps used bare `if: matrix.agent == 'X'` conditions, which GitHub Actions implicitly wraps as `if: success() && ...`. When the review action failed, the webhook was skipped and comments showed stale data. Changed to `if: always() && ...` with default output guards. Bug C: Added REQUIRED comment on pull-requests: read permission to flag it as critical for provisioning (external repos had actions: read instead). Closes #71 Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
diff --git a/.github/actions/shared/extract-result.sh b/.github/actions/shared/extract-result.sh
@@ -49,25 +49,101 @@ extract_review_result() {
     # Get ALL text from assistant messages, joined together
     ALL_TEXT=$(jq -r '[.[] | select(.type=="assistant") | .message.content[]? | select(.type=="text") | .text] | join("\n")' "$EXEC" 2>/dev/null || echo "")
 
-    # Strategy 1: Extract from ```json block
-    JSON=$(echo "$ALL_TEXT" | sed -n '/```json/,/```/{/```/d;p;}' | head -100)
+    JSON=""
 
-    # Strategy 2: If no json block, try to find raw JSON object with "checks" key
-    if ! echo "$JSON" | jq . >/dev/null 2>&1; then
-      JSON=$(echo "$ALL_TEXT" | grep -oP '\{"checks":\s*\[.*\]\s*(,"message":[^}]*)?\}' | head -1)
+    # Strategy 1: Extract from ```json blocks — take the LAST valid one with .checks
+    # (Claude may output example JSON before the actual result)
+    if [ -z "$JSON" ]; then
+      local BLOCK=""
+      local IN_BLOCK=false
+      while IFS= read -r line; do
+        if [[ "$line" == '```json'* ]]; then
+          IN_BLOCK=true
+          BLOCK=""
+          continue
+        fi
+        if [[ "$line" == '```'* ]] && [ "$IN_BLOCK" = true ]; then
+          IN_BLOCK=false
+          if echo "$BLOCK" | jq -e '.checks' >/dev/null 2>&1; then
+            JSON="$BLOCK"
+            # Don't break — keep going to find the LAST valid block
+          fi
+          continue
+        fi
+        if [ "$IN_BLOCK" = true ]; then
+          BLOCK="${BLOCK}${line}"$'\n'
+        fi
+      done <<< "$ALL_TEXT"
+      if [ -n "$JSON" ]; then
+        echo "  [extract] Strategy 1 matched (json code block)" >&2
+      fi
     fi
 
-    # Strategy 3: Extract any JSON object starting with {"checks"
-    if ! echo "$JSON" | jq . >/dev/null 2>&1; then
-      JSON=$(echo "$ALL_TEXT" | perl -ne 'print if /\{"checks":\[/.../"message":/' | tr '\n' ' ')
+    # Strategy 2: Try to find raw JSON object with "checks" key on a single line
+    if [ -z "$JSON" ] || ! echo "$JSON" | jq -e '.checks' >/dev/null 2>&1; then
+      JSON=""
+      local CANDIDATE
+      CANDIDATE=$(echo "$ALL_TEXT" | grep -oP '\{"checks":\s*\[.*\]\s*(,"message":[^}]*)?\}' 2>/dev/null | head -1 || true)
+      if [ -n "$CANDIDATE" ] && echo "$CANDIDATE" | jq -e '.checks' >/dev/null 2>&1; then
+        JSON="$CANDIDATE"
+        echo "  [extract] Strategy 2 matched (grep single-line)" >&2
+      fi
     fi
 
-    # Strategy 4: Use Python for robust multiline JSON extraction
-    if ! echo "$JSON" | jq . >/dev/null 2>&1; then
-      JSON=$(echo "$ALL_TEXT" | python3 -c "import re,sys; t=sys.stdin.read(); m=re.search(r'\{[^{}]*\"checks\"[^{}]*\[.*?\][^{}]*\}',t,re.DOTALL); print(m.group() if m else '')" 2>/dev/null)
+    # Strategy 3: Extract JSON starting with {"checks" using perl range operator
+    if [ -z "$JSON" ] || ! echo "$JSON" | jq -e '.checks' >/dev/null 2>&1; then
+      JSON=""
+      local CANDIDATE
+      CANDIDATE=$(echo "$ALL_TEXT" | perl -ne 'print if /\{"checks":\s*\[/.../"message"\s*:/' 2>/dev/null | tr '\n' ' ' || true)
+      if [ -n "$CANDIDATE" ] && echo "$CANDIDATE" | jq -e '.checks' >/dev/null 2>&1; then
+        JSON="$CANDIDATE"
+        echo "  [extract] Strategy 3 matched (perl range)" >&2
+      fi
     fi
 
-    if echo "$JSON" | jq . >/dev/null 2>&1; then
+    # Strategy 4: Use Python brace-depth counter for robust multiline extraction
+    if [ -z "$JSON" ] || ! echo "$JSON" | jq -e '.checks' >/dev/null 2>&1; then
+      JSON=""
+      local CANDIDATE
+      CANDIDATE=$(echo "$ALL_TEXT" | python3 -c "
+import sys, json
+
+text = sys.stdin.read()
+# Find all top-level JSON objects containing 'checks'
+results = []
+i = 0
+while i < len(text):
+    if text[i] == '{':
+        depth = 0
+        start = i
+        while i < len(text):
+            if text[i] == '{':
+                depth += 1
+            elif text[i] == '}':
+                depth -= 1
+                if depth == 0:
+                    candidate = text[start:i+1]
+                    try:
+                        obj = json.loads(candidate)
+                        if 'checks' in obj:
+                            results.append(candidate)
+                    except (json.JSONDecodeError, ValueError):
+                        pass
+                    break
+            i += 1
+    i += 1
+
+# Print the last valid match (actual result comes after examples)
+if results:
+    print(results[-1])
+" 2>/dev/null || true)
+      if [ -n "$CANDIDATE" ] && echo "$CANDIDATE" | jq -e '.checks' >/dev/null 2>&1; then
+        JSON="$CANDIDATE"
+        echo "  [extract] Strategy 4 matched (python brace-depth)" >&2
+      fi
+    fi
+
+    if [ -n "$JSON" ] && echo "$JSON" | jq -e '.checks' >/dev/null 2>&1; then
       # Count checks by status
       CHECKS_PASSED=$(echo "$JSON" | jq '[.checks[]? | select(.status == "passed")] | length')
       CHECKS_FAILED=$(echo "$JSON" | jq '[.checks[]? | select(.status == "failed")] | length')
@@ -82,6 +158,8 @@ extract_review_result() {
       fi
 
       RESULT=$(echo "$JSON" | jq -c '.')
+    else
+      echo "  [extract] No strategy matched — using default failed result" >&2
     fi
   fi
 }
diff --git a/.github/templates/constellos-review.yml b/.github/templates/constellos-review.yml
@@ -12,7 +12,7 @@ on:
 permissions:
   contents: read
   issues: read
-  pull-requests: read
+  pull-requests: read  # REQUIRED: agents need this to fetch changed files via gh pr view
 
 jobs:
   # Read config and determine which agents to run
@@ -55,9 +55,13 @@ jobs:
           github_token: ${{ secrets.GITHUB_TOKEN }}
 
       - name: Send requirements results to webhook
-        if: matrix.agent == 'requirements'
+        if: always() && matrix.agent == 'requirements'
         env:
-          RESULT_JSON: ${{ steps.requirements.outputs.result }}
+          RESULT_JSON: ${{ steps.requirements.outputs.result || '{"checks":[],"message":"Review failed to complete"}' }}
+          PASSED: ${{ steps.requirements.outputs.passed || 'false' }}
+          CHECKS_PASSED: ${{ steps.requirements.outputs.checks_passed || '0' }}
+          CHECKS_FAILED: ${{ steps.requirements.outputs.checks_failed || '0' }}
+          CHECKS_SKIPPED: ${{ steps.requirements.outputs.checks_skipped || '0' }}
         run: |
           # Build JSON payload safely using jq
           jq -n \
@@ -67,11 +71,11 @@ jobs:
             --arg sha "${{ github.event.pull_request.head.sha }}" \
             --argjson run_id "${{ github.run_id }}" \
             --arg review_name "Requirements" \
-            --argjson passed "${{ steps.requirements.outputs.passed }}" \
+            --argjson passed "$PASSED" \
             --argjson result_json "$RESULT_JSON" \
-            --argjson checks_passed "${{ steps.requirements.outputs.checks_passed || 0 }}" \
-            --argjson checks_failed "${{ steps.requirements.outputs.checks_failed || 0 }}" \
-            --argjson checks_skipped "${{ steps.requirements.outputs.checks_skipped || 0 }}" \
+            --argjson checks_passed "$CHECKS_PASSED" \
+            --argjson checks_failed "$CHECKS_FAILED" \
+            --argjson checks_skipped "$CHECKS_SKIPPED" \
             --arg prompt_file ".constellos/agents/requirements.md" \
             '{
               owner: $owner,
@@ -103,9 +107,13 @@ jobs:
           github_token: ${{ secrets.GITHUB_TOKEN }}
 
       - name: Send code-quality results to webhook
-        if: matrix.agent == 'code-quality'
+        if: always() && matrix.agent == 'code-quality'
         env:
-          RESULT_JSON: ${{ steps.code-quality.outputs.result }}
+          RESULT_JSON: ${{ steps.code-quality.outputs.result || '{"checks":[],"message":"Review failed to complete"}' }}
+          PASSED: ${{ steps.code-quality.outputs.passed || 'false' }}
+          CHECKS_PASSED: ${{ steps.code-quality.outputs.checks_passed || '0' }}
+          CHECKS_FAILED: ${{ steps.code-quality.outputs.checks_failed || '0' }}
+          CHECKS_SKIPPED: ${{ steps.code-quality.outputs.checks_skipped || '0' }}
         run: |
           # Build JSON payload safely using jq
           jq -n \
@@ -115,11 +123,11 @@ jobs:
             --arg sha "${{ github.event.pull_request.head.sha }}" \
             --argjson run_id "${{ github.run_id }}" \
             --arg review_name "Code Quality" \
-            --argjson passed "${{ steps.code-quality.outputs.passed }}" \
+            --argjson passed "$PASSED" \
             --argjson result_json "$RESULT_JSON" \
-            --argjson checks_passed "${{ steps.code-quality.outputs.checks_passed || 0 }}" \
-            --argjson checks_failed "${{ steps.code-quality.outputs.checks_failed || 0 }}" \
-            --argjson checks_skipped "${{ steps.code-quality.outputs.checks_skipped || 0 }}" \
+            --argjson checks_passed "$CHECKS_PASSED" \
+            --argjson checks_failed "$CHECKS_FAILED" \
+            --argjson checks_skipped "$CHECKS_SKIPPED" \
             --arg prompt_file ".constellos/agents/code-quality.md" \
             '{
               owner: $owner,
@@ -151,9 +159,13 @@ jobs:
           github_token: ${{ secrets.GITHUB_TOKEN }}
 
       - name: Send context results to webhook
-        if: matrix.agent == 'context'
+        if: always() && matrix.agent == 'context'
         env:
-          RESULT_JSON: ${{ steps.context.outputs.result }}
+          RESULT_JSON: ${{ steps.context.outputs.result || '{"checks":[],"message":"Review failed to complete"}' }}
+          PASSED: ${{ steps.context.outputs.passed || 'false' }}
+          CHECKS_PASSED: ${{ steps.context.outputs.checks_passed || '0' }}
+          CHECKS_FAILED: ${{ steps.context.outputs.checks_failed || '0' }}
+          CHECKS_SKIPPED: ${{ steps.context.outputs.checks_skipped || '0' }}
         run: |
           # Build JSON payload safely using jq
           jq -n \
@@ -163,11 +175,11 @@ jobs:
             --arg sha "${{ github.event.pull_request.head.sha }}" \
             --argjson run_id "${{ github.run_id }}" \
             --arg review_name "Context" \
-            --argjson passed "${{ steps.context.outputs.passed }}" \
+            --argjson passed "$PASSED" \
             --argjson result_json "$RESULT_JSON" \
-            --argjson checks_passed "${{ steps.context.outputs.checks_passed || 0 }}" \
-            --argjson checks_failed "${{ steps.context.outputs.checks_failed || 0 }}" \
-            --argjson checks_skipped "${{ steps.context.outputs.checks_skipped || 0 }}" \
+            --argjson checks_passed "$CHECKS_PASSED" \
+            --argjson checks_failed "$CHECKS_FAILED" \
+            --argjson checks_skipped "$CHECKS_SKIPPED" \
             --arg prompt_file ".constellos/agents/context.md" \
             '{
               owner: $owner,
