feat: implement issue #898 — pr-review self-review deadlock: ci-pending skip + "already reviewed" marker blocks approval once CI goes green

.github/workflows/actions-fleet-monitor.yml

@@ -74,6 +74,90 @@ jobs:
           GH_TOKEN: ${{ secrets.GH_PAT_WORKFLOWS }}
         run: bash scripts/fleet_monitor.sh
 
+      - name: Summarize token usage by workflow
+        env:
+          GH_TOKEN: ${{ secrets.GH_PAT_WORKFLOWS }}
+          REPO: petry-projects/.github-private
+          LOOKBACK_DAYS: ${{ env.LOOKBACK_DAYS }}
+        run: |
+          set -euo pipefail
+
+          workdir="$(mktemp -d)"
+          trap 'rm -rf "$workdir"' EXIT
+          jsonl_dir="$workdir/jsonl"
+          artifacts_file="$workdir/token-artifacts.base64"
+          mkdir -p "$jsonl_dir"
+
+          cutoff_epoch="$(date -u -d "-${LOOKBACK_DAYS} days" +%s)"
+          artifact_count=0
+
+          gh api "repos/$REPO/actions/artifacts" --paginate \
+            --jq '.artifacts[] | select(.name | startswith("token-usage-")) | @base64' \
+            > "$artifacts_file"
+
+          while IFS= read -r encoded; do
+            [ -n "$encoded" ] || continue
+            item="$(printf '%s' "$encoded" | base64 -d)"
+            id="$(printf '%s' "$item" | jq -r '.id')"
+            created_at="$(printf '%s' "$item" | jq -r '.created_at')"
+            expired="$(printf '%s' "$item" | jq -r '.expired')"
+            created_epoch="$(date -u -d "$created_at" +%s)"
+
+            if [ "$expired" = "true" ] || [ "$created_epoch" -lt "$cutoff_epoch" ]; then
+              continue
+            fi
+
+            zip_file="$workdir/artifact-$id.zip"
+            unzip_dir="$workdir/unzip-$id"
+            mkdir -p "$unzip_dir"
+            gh api "repos/$REPO/actions/artifacts/$id/zip" > "$zip_file"
+            unzip -q -o "$zip_file" -d "$unzip_dir"
+
+            found_any=false
+            while IFS= read -r f; do
+              found_any=true
+              cp "$f" "$jsonl_dir/${id}-$(basename "$f")"
+            done < <(find "$unzip_dir" -type f -name '*.jsonl' -print)
+
+            if [ "$found_any" = true ]; then
+              artifact_count=$((artifact_count + 1))
+            fi
+          done < "$artifacts_file"
+
+          if ! find "$jsonl_dir" -type f -name '*.jsonl' | grep -q .; then
+            {
+              echo "## Token usage (${LOOKBACK_DAYS} day lookback)"
+              echo
+              echo "No token-usage artifacts found in the lookback window."
+            } >> "$GITHUB_STEP_SUMMARY"
+            exit 0
+          fi
+
+          aggregate="$workdir/token-usage-summary.json"
+          jq -s '
+            sort_by(.workflow) |
+            group_by(.workflow) |
+            map({
+              workflow: (.[0].workflow // "unknown"),
+              calls: length,
+              input_tokens: (map(.input_tokens // 0) | add),
+              cache_read_tokens: (map(.cache_read_tokens // 0) | add),
+              output_tokens: (map(.output_tokens // 0) | add),
+              et: (map(.et // 0) | add)
+            }) |
+            sort_by(.et) | reverse
+          ' "$jsonl_dir"/*.jsonl > "$aggregate"
+
+          {
+            echo "## Token usage (${LOOKBACK_DAYS} day lookback)"
+            echo
+            echo "Processed ${artifact_count} token-usage artifact(s)."
+            echo
+            echo "| Workflow | Calls | Input Tokens | Cache Read Tokens | Output Tokens | Total ET |"
+            echo "|---|---:|---:|---:|---:|---:|"
+            jq -r '.[] | "| \(.workflow) | \(.calls) | \(.input_tokens) | \(.cache_read_tokens) | \(.output_tokens) | \(.et | tostring) |"' "$aggregate"
+          } >> "$GITHUB_STEP_SUMMARY"
+
       - name: Create issue with findings
         if: env.HAS_FAILURES == 'true' && hashFiles('fleet_monitor_report.md') != ''
         uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3  # v9.0.0

.github/workflows/lint.yml

@@ -85,7 +85,9 @@ jobs:
             tests/test_downstream_impact_assemble.bats \
             tests/test_downstream_impact_triage_inline.bats \
             tests/test_downstream_impact_regression.bats \
-            tests/test_engine_unavailable_notice.bats
+            tests/test_engine_unavailable_notice.bats \
+            tests/test_batch_skip_reporting.bats \
+            tests/test_review_one_pr_ci_pending.bats
 
   validate-agent-profiles:
     runs-on: ubuntu-latest

scripts/review-batch.sh

@@ -65,6 +65,30 @@ Every configured review engine (Claude → Copilot → Gemini) reported a usage/
   return 0
 }
 
+# ── Accurate exit-100 skip reporting (issue #898) ───────────────────────────
+# review-one-pr.sh returns 100 for every no-op/skip and prints a JSON verdict
+# carrying the *reason* (ci-pending, already-reviewed-at-head, …). To report the
+# real reason instead of a blanket "already reviewed" line — which mislabelled a
+# CI-pending deferral as a completed review and made the #892 stuck-green case
+# look like an idempotency deadlock — capture the reviewer's stdout per PR and
+# parse the trailing reason out of it.
+run_review_capture() {
+  local url="$1" rc=0
+  set +e
+  bash scripts/review-one-pr.sh "$url" | tee "$REVIEW_OUT"
+  rc=${PIPESTATUS[0]}
+  set -e
+  return "$rc"
+}
+
+# Extract the last "reason":"…" value from a captured review log (the verdict
+# JSON is the final line review-one-pr.sh emits on a skip). Empty if none.
+skip_reason_from() {
+  grep -oE '"reason"[[:space:]]*:[[:space:]]*"[^"]*"' "$1" 2>/dev/null \
+    | tail -n1 \
+    | sed -E 's/.*"reason"[[:space:]]*:[[:space:]]*"([^"]*)".*/\1/'
+}
+
 # Pre-flight: validate engine availability before touching any PR.
 # Sets CLAUDE_AVAILABLE, GEMINI_AVAILABLE, COPILOT_AVAILABLE and emits
 # ::warning:: annotations + job-summary table for any unavailable engine.
@@ -168,6 +192,7 @@ fi
 
 actual=0
 skipped_noops=0
+deferred=0
 failed=0
 engine_fallbacks=0
 fallback_engines=""
@@ -177,6 +202,10 @@ abort_pr=""
 abort_reason=""
 total_candidates=$(grep -c . "$PRS_FILE" || true)
 
+# Per-PR capture buffer for run_review_capture / skip_reason_from (issue #898).
+REVIEW_OUT=$(mktemp) || { echo "::error::Failed to create temporary file for review capture" >&2; exit 1; }
+trap 'rm -f "$REVIEW_OUT"' EXIT
+
 while IFS= read -r pr_url; do
   [ -z "$pr_url" ] && continue
   processed=$((processed + 1))
@@ -192,7 +221,7 @@ while IFS= read -r pr_url; do
 
   echo "::group::Reviewing $pr_url"
   rc=0
-  bash scripts/review-one-pr.sh "$pr_url" || rc=$?
+  run_review_capture "$pr_url" || rc=$?
 
   # Treat known engine-unavailable setup/runtime errors as fallback-eligible
   if [ "$rc" -eq 55 ] || [ "$rc" -eq 127 ]; then
@@ -210,7 +239,7 @@ while IFS= read -r pr_url; do
       engine_fallbacks=$((engine_fallbacks + 1))
       fallback_engines="${fallback_engines:+$fallback_engines, }copilot"
       rc=0
-      bash scripts/review-one-pr.sh "$pr_url" || rc=$?
+      run_review_capture "$pr_url" || rc=$?
 
       # Handle Copilot engine-unavailable setup/runtime errors post-fallback
       if [ "$rc" -eq 55 ] || [ "$rc" -eq 127 ]; then
@@ -243,7 +272,7 @@ while IFS= read -r pr_url; do
     engine_fallbacks=$((engine_fallbacks + 1))
     fallback_engines="${fallback_engines:+$fallback_engines, }gemini"
     rc=0
-    bash scripts/review-one-pr.sh "$pr_url" || rc=$?
+    run_review_capture "$pr_url" || rc=$?
   fi
 
   case "$rc" in
@@ -253,7 +282,34 @@ while IFS= read -r pr_url; do
       ;;
     100)
       skipped_noops=$((skipped_noops + 1))
-      echo "::notice::No-op (already reviewed)"
+      # Report the real skip reason instead of a blanket "already reviewed".
+      # Reasons that mean "a review is still owed once checks settle" bump the
+      # deferred counter so the run summary flags them (issue #898).
+      reason=$(skip_reason_from "$REVIEW_OUT")
+      case "$reason" in
+        already-reviewed-at-head)
+          echo "::notice::No-op — already reviewed at current head" ;;
+        ci-pending|ci-pending-after-poll)
+          deferred=$((deferred + 1))
+          echo "::notice::Deferred — CI still pending; a review is owed once checks go green" ;;
+        ci-failing)
+          deferred=$((deferred + 1))
+          echo "::notice::Deferred — CI failing; will re-review after fixes land" ;;
+        waiting-for-advisory-bots)
+          deferred=$((deferred + 1))
+          echo "::notice::Deferred — awaiting advisory bot reviews" ;;
+        advisory-gate-api-error)
+          deferred=$((deferred + 1))
+          echo "::notice::Deferred — advisory-bot gate API error; will re-evaluate next run" ;;
+        changes-requested)
+          echo "::notice::Skipped — human changes requested; awaiting author" ;;
+        human-escalated|max-cycles-reached)
+          echo "::notice::Skipped — escalated to a human reviewer" ;;
+        gh-rate-limited)
+          echo "::notice::Skipped — gh API rate limited; will retry next run" ;;
+        *)
+          echo "::notice::No-op${reason:+ — $reason}" ;;
+      esac
       ;;
     2)
       failed=$((failed + 1))
@@ -278,6 +334,7 @@ done < "$PRS_FILE"
 remaining=$((total_candidates - processed))
 summary="Summary: $actual reviews posted, $skipped_noops no-ops skipped, $failed failures"
 [ "$engine_fallbacks" -gt 0 ] && summary="$summary, $engine_fallbacks engine fallback(s) to $fallback_engines"
+[ "$deferred" -gt 0 ] && summary="$summary ($deferred deferred pending CI/checks — owed a review once green)"
 summary="$summary (processed $processed/$total_candidates candidates)"
 
 if [ "$session_aborted" -eq 1 ]; then

tests/test_batch_skip_reporting.bats

@@ -0,0 +1,127 @@
+#!/usr/bin/env bats
+# Regression tests for accurate exit-100 skip reporting in scripts/review-batch.sh
+# (issue #898).
+#
+# The reviewer (review-one-pr.sh) returns exit 100 for *every* no-op/skip: an
+# already-reviewed head, a CI-pending deferral, a CI-failing deferral, a
+# human-changes-requested skip, an escalation skip, etc. The batch driver used
+# to collapse all of these into a single "No-op (already reviewed)" line. That
+# message is actively misleading for a CI-pending deferral — it claims a review
+# already happened at the current head when in fact the PR is *owed* a review
+# once CI goes green. That mislabelling is what made the #892 "deadlock" look
+# like a stuck idempotency marker.
+#
+# These tests assert the batch reports the *reason* behind an exit-100 skip:
+#   - a ci-pending skip must NOT claim "already reviewed" and must surface as a
+#     deferral ("Deferred" / "CI still pending"),
+#   - an already-reviewed-at-head skip must still say "already reviewed",
+#   - a batch with at least one CI deferral must note "deferred" in the summary
+#     so an operator scanning the run knows a review is still owed.
+#
+# Run with: bats tests/test_batch_skip_reporting.bats
+
+setup() {
+  export TEST_DIR="$BATS_TMPDIR/batch-skip-report-test"
+  mkdir -p "$TEST_DIR/scripts"
+  mkdir -p "$TEST_DIR/bin"
+  cd "$TEST_DIR"
+
+  export PRS_FILE="prs.txt"
+  echo "https://github.com/fake/pull/1" > "$PRS_FILE"
+  export CANDIDATE_LIMIT=1
+  export MAX_PRS=1
+  export REVIEW_ENGINE="claude"
+  export PATH="$TEST_DIR/bin:$PATH"
+
+  cp "$BATS_TEST_DIRNAME/../scripts/review-batch.sh" "scripts/"
+
+  cat > "scripts/validate-engines.sh" <<'EOF'
+validate_engines() {
+  export CLAUDE_AVAILABLE="true"
+  export GEMINI_AVAILABLE="true"
+  export COPILOT_AVAILABLE="true"
+}
+EOF
+
+  cat > "scripts/engine.sh" <<'EOF'
+export COPILOT_API_MODEL="openai/o4-mini"
+EOF
+
+  cat > "$TEST_DIR/bin/gh" <<'EOF'
+#!/bin/bash
+exit 0
+EOF
+  chmod +x "$TEST_DIR/bin/gh"
+}
+
+teardown() {
+  rm -rf "$TEST_DIR"
+}
+
+# review-one-pr.sh stub that emits a CI-pending skip exactly like the real
+# reviewer's non-force ci-pending gate: a human-readable line plus a JSON
+# verdict carrying reason=ci-pending, then exit 100.
+_stub_ci_pending() {
+  cat > "scripts/review-one-pr.sh" <<'EOF'
+#!/bin/bash
+echo "    skip: CI checks still in progress for $1"
+echo '{"pr":"'"$1"'","decision":"skip","reason":"ci-pending"}'
+exit 100
+EOF
+  chmod +x "scripts/review-one-pr.sh"
+}
+
+_stub_already_reviewed() {
+  cat > "scripts/review-one-pr.sh" <<'EOF'
+#!/bin/bash
+echo "    noop: already reviewed at deadbeef"
+echo '{"pr":"'"$1"'","decision":"skip","reason":"already-reviewed-at-head"}'
+exit 100
+EOF
+  chmod +x "scripts/review-one-pr.sh"
+}
+
+@test "batch: ci-pending skip is not reported as 'already reviewed'" {
+  _stub_ci_pending
+
+  run bash scripts/review-batch.sh
+  echo "$output" >&2
+
+  [ "$status" -eq 0 ]
+  # The defect: a CI-pending deferral was mislabelled as an already-reviewed
+  # no-op. The batch must NOT claim the PR was already reviewed.
+  [[ "$output" != *"already reviewed"* ]]
+}
+
+@test "batch: ci-pending skip is reported as a deferral" {
+  _stub_ci_pending
+
+  run bash scripts/review-batch.sh
+  echo "$output" >&2
+
+  [ "$status" -eq 0 ]
+  [[ "$output" == *"Deferred"* ]]
+  [[ "$output" == *"CI"* ]]
+}
+
+@test "batch: ci-pending deferral is surfaced in the session summary" {
+  _stub_ci_pending
+
+  run bash scripts/review-batch.sh
+  echo "$output" >&2
+
+  [ "$status" -eq 0 ]
+  [[ "$output" == *"deferred"* ]]
+}
+
+@test "batch: already-reviewed-at-head skip still reports 'already reviewed'" {
+  _stub_already_reviewed
+
+  run bash scripts/review-batch.sh
+  echo "$output" >&2
+
+  [ "$status" -eq 0 ]
+  [[ "$output" == *"already reviewed"* ]]
+  # An already-reviewed no-op is genuinely done — it is NOT a CI deferral.
+  [[ "$output" != *"deferred"* ]]
+}