fix(stats): follow artifact redirect so detection verdicts are actually fetched (#893)

* fix(stats): follow artifact redirect so verdicts are actually fetched

The verdict fetcher passed `--no-location-trusted` to curl on the
artifact-zip download, believing it would only disable credential
forwarding on redirects. It also silently disables `--location`, so
curl stopped at the 302 the GitHub API returns and every download was
recorded as `download_failed`. Yesterday's report on github/gh-aw
(555 external-detector runs) reflected this: 377 `download_failed`,
0 runs with a parsed verdict, threat rate reported as 0%.

Drop the flag. Plain `--location` already strips the Authorization
header on the cross-host hop to the signed blob URL, which is exactly
what the blob endpoint needs.

Extend the offline stub to answer artifact zips with a 302 → separate
blob path so the collector has to follow the redirect (verified: the
test now reproduces the production `download_failed` counts when the
flag is restored).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Co-authored-by: David Slater <12449447+davidslater@users.noreply.github.com>

* docs(stats): make verdict-availability table self-describing

The verdict-availability table dumped raw internal keys (`present`,
`not_fetched`, `download_failed`, ...) and left readers to guess what
each one meant. In particular:

* `not_fetched` was easy to misread as a fetch failure; it actually
  means the detection job was skipped so no artifact was ever
  expected. Reviewer of #893 flagged this.
* Successful downloads land under `present`, not a fictional
  `download_succeeded`. That was invisible in yesterday's #892 report
  only because the redirect bug reclassified every success as
  `download_failed`; the fix in #893 puts them back where they belong.

Render an explicit Meaning column with a fixed row order, so both the
healthy state (`present`) and the "skipped, nothing to fetch" state
(`not_fetched`) explain themselves in-place. Unknown states (which the
aggregator is defensive about but should never emit) still round-trip
verbatim.

Assertions in the offline test now verify the Meaning column is
rendered and that `not_fetched` is described as the skipped-job state.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Co-authored-by: David Slater <12449447+davidslater@users.noreply.github.com>

* docs(stats): split verdict-availability `not_fetched` into `skipped` vs `not_fetched`

`not_fetched` was overloaded: it counted both detection jobs that were
skipped (never eligible for a verdict artifact) and eligible jobs the
collector never reached (budget exhausted, in-progress at collection
time, etc.). In a healthy run the former is expected and the latter is
a truncation signal, so they should not share a bucket.

Split them in the aggregator:

* `skipped` — `conclusion == "skipped"` or `status != "completed"`. The
  detection job was never eligible, so no verdict was ever expected.
* `not_fetched` — anything else without a verdict record. The
  collector meant to fetch this run and didn't get to it.

Records and notable_runs use the same split, so downstream views stay
consistent, and the summary now describes each row explicitly. Tests
assert the split (`not_fetched` must be absent when collection is
complete) and that both descriptions are rendered.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Co-authored-by: David Slater <12449447+davidslater@users.noreply.github.com>

* chore(stats): give the collector ~2h so a full day fits in the budget

Yesterday's rerun of the fixed collector against 2026-08-17 hit the
`DEADLINE_MINUTES=18` wall-clock ceiling after 147/412 verdict targets
(the collector step ran exactly 18 min). Verdict fetching is currently
serial, so a full day of ~400 real downloads plus rate-limit pauses
needs on the order of an hour, not eighteen minutes.

Extend the collector's wall-clock window to ~2h and give the wrapping
workflow / job timeouts enough headroom to actually reach it:

* workflow timeout: 30 -> 150 (2h collector + ~30 min for the agent job)
* collector job timeout: 25 -> 130 (2h + a small setup/upload margin)
* DEADLINE_MINUTES: 18 -> 120
* MAX_REQUESTS: 3000 -> 8000 (a full day at ~85 req/min plus rate-limit
  pauses can reach ~6-7k; leave headroom so the API budget doesn't
  bind before the wall-clock deadline does)

This is a temporary buffer — verdict fetching should be parallelized
so the collector finishes in minutes, not hours. Filed as follow-up.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Co-authored-by: David Slater <12449447+davidslater@users.noreply.github.com>

---------

Co-authored-by: GitHub Ace <githubnext@users.noreply.github.com>
Co-authored-by: David Slater <12449447+davidslater@users.noreply.github.com>

Author: davidslater

.gitattributes

@@ -1,2 +1,2 @@
-.github/workflows/*.lock.yml linguist-generated=true
+.github/workflows/*.lock.yml linguist-generated=true merge=ours
 .github/workflows/*-container.md linguist-generated=true merge=ours

.github/aw/actions-lock.json

@@ -5,15 +5,10 @@
       "version": "v9.0.0",
       "sha": "3a2844b7e9c422d3c10d287c895573f7108da1b3"
     },
-    "github/gh-aw-actions/setup@v0.86.2": {
+    "github/gh-aw-actions/setup@v0.81.6": {
       "repo": "github/gh-aw-actions/setup",
-      "version": "v0.86.2",
-      "sha": "6aab9e5b5c91c615506061f09bedd81a23babe3c"
-    },
-    "github/gh-aw-actions/setup@v0.87.0": {
-      "repo": "github/gh-aw-actions/setup",
-      "version": "v0.87.0",
-      "sha": "b77e0d501fd2d2243d1f72722617e80d513f674e"
+      "version": "v0.81.6",
+      "sha": "ba6380cc6e5be5d21677bebe04d52fb48e3abec7"
     }
   }
 }

.github/workflows/detection-stats-daily.lock.yml

@@ -46,13 +46,17 @@ safe-outputs:
     title-prefix: "[detection-stats] "
     labels: [automation, detection-stats]
     max: 1
-timeout-minutes: 30
+timeout-minutes: 150
 jobs:
   # The collector runs in its own job so the token that reads the target
   # repository never enters the agent job's environment.
   collect_detection_stats:
     runs-on: ubuntu-latest
-    timeout-minutes: 25
+    # Verdict fetching is currently serial (~7 targets/min after a rate-limit
+    # pause), so a full day of ~400 external-detector runs needs on the order
+    # of an hour. Give it ~2h of wall clock; the agent job that renders the
+    # report only needs a few minutes after that.
+    timeout-minutes: 130
     permissions:
       contents: read
     steps:
@@ -67,9 +71,12 @@ jobs:
           TARGET_REPO: ${{ inputs.target_repo || 'github/gh-aw' }}
           TARGET_DATE: ${{ inputs.date }}
           FETCH_RESULTS: ${{ inputs.fetch_results || 'true' }}
-          MAX_REQUESTS: ${{ inputs.max_requests || '3000' }}
+          # A full day at ~85 req/min plus rate-limit pauses can reach ~6-7k
+          # requests; leave headroom above that so the API budget doesn't
+          # bind before the wall-clock deadline does.
+          MAX_REQUESTS: ${{ inputs.max_requests || '8000' }}
           OUTPUT_DIR: /tmp/gh-aw/detection-stats
-          DEADLINE_MINUTES: "18"
+          DEADLINE_MINUTES: "120"
         run: bash scripts/collect-detection-stats.sh
       - name: Upload detection statistics
         if: always()

.github/workflows/detection-stats-daily.md

@@ -170,14 +170,14 @@ throttle_if_low() {
   return 0
 }
 
-# api_request <url> <body-output-path> [--binary]
+# api_request <url> <body-output-path>
 #
 # Performs one GET against the GitHub API with retry, primary/secondary
 # rate-limit backoff and budget accounting. Returns 0 on 2xx, 1 otherwise (the
 # caller decides whether a miss is fatal). Never aborts the run on a single
 # failed read.
 api_request() {
-  local url="$1" out="$2" binary="${3:-}"
+  local url="$1" out="$2"
   local header_file="${WORK_DIR}/headers.$$"
   local attempt=0 max_attempts=5 status backoff
 
@@ -198,13 +198,13 @@ api_request() {
       --header "X-GitHub-Api-Version: 2022-11-28"
       --header "User-Agent: gh-aw-threat-detection-stats"
     )
-    # Artifact downloads answer with a 302 to a signed blob URL; --location
-    # follows it, but the redirect target rejects the Authorization header.
-    if [ "$binary" = "--binary" ]; then
-      curl_args+=(--header "Accept: application/vnd.github+json" --no-location-trusted)
-    else
-      curl_args+=(--header "Accept: application/vnd.github+json")
-    fi
+    # Artifact downloads answer with a 302 to a signed blob URL on a different
+    # host; --location follows it and curl strips the Authorization header on
+    # the cross-host hop, which is exactly what the blob URL needs (it carries
+    # its own signature). Do NOT add --no-location-trusted here: despite the
+    # name, that flag also disables --location itself, so every artifact fetch
+    # would stop at the 302 and be recorded as download_failed.
+    curl_args+=(--header "Accept: application/vnd.github+json")
 
     status="$(curl "${curl_args[@]}" "$url" 2>/dev/null || echo 000)"
     sleep "$REQUEST_PAUSE_SECONDS"
@@ -523,7 +523,7 @@ if [ "$FETCH_RESULTS" = "true" ]; then
 
     artifact_id="$(jq -r '.id' <<<"$artifact")"
     zip="${WORK_DIR}/detection.zip"
-    if ! api_request "${API_URL}/repos/${TARGET_REPO}/actions/artifacts/${artifact_id}/zip" "$zip" --binary; then
+    if ! api_request "${API_URL}/repos/${TARGET_REPO}/actions/artifacts/${artifact_id}/zip" "$zip"; then
       jq -nc --argjson id "$run_id" '{run_id: $id, result: "download_failed"}' >>"$VERDICTS_FILE"
       continue
     fi
@@ -688,7 +688,16 @@ jq -n \
         and (.detection.conclusion == "failure" or .detection.conclusion == "timed_out"
              or .detection.conclusion == "action_required"))) | length),
       verdict_availability: (if $fetch_results != "true" then {not_fetched: $n}
-        else ($ext | map(.verdict.result // "not_fetched")
+        else ($ext | map(
+          if .verdict.result then .verdict.result
+          # Detection jobs that were skipped or were still in progress at
+          # collection time were never eligible for verdict fetching — call
+          # that out explicitly instead of lumping them into not_fetched,
+          # which is reserved for eligible targets the collector never
+          # reached (budget exhausted, etc.).
+          elif .detection.conclusion == "skipped"
+               or .detection.status != "completed" then "skipped"
+          else "not_fetched" end)
               | group_by(.) | map({key: .[0], value: length}) | from_entries) end),
       soft_failures: {
         description: "green detection job that published no verdict artifact",
@@ -731,7 +740,10 @@ jq -n \
             job_url: .detection.job_url,
             conclusion: (.detection.conclusion // .detection.status),
             failed_steps: (.detection.failed_steps // []),
-            verdict: (if .verdict == null then "not_fetched" else .verdict.result end),
+            verdict: (if .verdict != null then .verdict.result
+                      elif .detection.conclusion == "skipped"
+                           or .detection.status != "completed" then "skipped"
+                      else "not_fetched" end),
             threats: (if .verdict.result == "present" then
                 ([if .verdict.prompt_injection then "prompt_injection" else empty end,
                   if .verdict.secret_leak then "secret_leak" else empty end,
@@ -757,7 +769,10 @@ jq -n \
           job_status: .detection.status,
           job_conclusion: .detection.conclusion,
           failed_steps: (.detection.failed_steps // []),
-          verdict: (if .verdict == null then "not_fetched" else .verdict.result end),
+          verdict: (if .verdict != null then .verdict.result
+                    elif .detection.conclusion == "skipped"
+                         or .detection.status != "completed" then "skipped"
+                    else "not_fetched" end),
           prompt_injection: (.verdict.prompt_injection // null),
           secret_leak: (.verdict.secret_leak // null),
           malicious_patch: (.verdict.malicious_patch // null),
@@ -832,10 +847,30 @@ w("")
 
 w("## Verdict availability")
 w("")
-w("| State | Count |")
-w("|---|---|")
-for key, count in sorted(s["verdict_availability"].items(), key=lambda kv: -kv[1]):
-    w(f"| `{key}` | {count} |")
+w("| State | Meaning | Count |")
+w("|---|---|---|")
+# Fixed ordering + human-readable descriptions so readers don't have to guess
+# what `not_fetched` or `present` mean. Zero-count rows are omitted; unknown
+# states (should never happen, but the aggregator is defensive) are appended
+# verbatim at the end.
+VERDICT_STATE_DESCRIPTIONS = [
+    ("present", "detection artifact downloaded and parsed"),
+    ("absent", "detection job ran but published no artifact (soft failure)"),
+    ("expired", "detection artifact existed but had already expired"),
+    ("skipped", "detection job was skipped or was still running (nothing to fetch)"),
+    ("not_fetched", "detection job ran but the collector didn't reach it (budget exhausted)"),
+    ("download_failed", "artifact zip download failed (HTTP error)"),
+    ("lookup_failed", "artifact listing failed (HTTP error)"),
+    ("unreadable", "artifact zip could not be unpacked"),
+    ("malformed", "detection_result.json was missing required fields"),
+]
+availability = dict(s["verdict_availability"])
+for key, description in VERDICT_STATE_DESCRIPTIONS:
+    count = availability.pop(key, 0)
+    if count:
+        w(f"| `{key}` | {description} | {count} |")
+for key, count in sorted(availability.items(), key=lambda kv: -kv[1]):
+    w(f"| `{key}` | (unknown state) | {count} |")
 w("")
 w(f"Green detection jobs that published no verdict: **{s['soft_failures']['count']}** "
   "(detection steps are `continue-on-error`, so a missing verdict artifact is the "

scripts/collect-detection-stats.sh

@@ -250,6 +250,24 @@ class Handler(BaseHTTPRequestHandler):
 
         m = re.fullmatch(r"/repos/[^/]+/[^/]+/actions/artifacts/(\d+)/zip", path)
         if m:
+            # The real API answers with a 302 redirect to a signed blob URL on
+            # a different host. Mirror that here so the collector's curl config
+            # has to actually follow the redirect (and drop the Authorization
+            # header) to succeed — the way production has to.
+            self.send_response(302)
+            self.send_header(
+                "Location", f"/blob/artifacts/{m.group(1)}/download"
+            )
+            self.end_headers()
+            return
+
+        m = re.fullmatch(r"/blob/artifacts/(\d+)/download", path)
+        if m:
+            # The signed blob URL is unauthenticated in reality; curl strips
+            # the Authorization header on the cross-host hop. Our stub is
+            # single-host, so we don't reject on Authorization here — the
+            # regression we're guarding against is curl not following the
+            # 302 at all (--no-location-trusted silently disabled --location).
             verdict = detection_artifact(int(m.group(1)) - 700000)
             buf = io.BytesIO()
             with zipfile.ZipFile(buf, "w") as zf:
@@ -412,6 +430,13 @@ av = s["verdict_availability"]
 assert av.get("present") == expect_present, (av, expect_present)
 assert av.get("expired") == expect_expired, (av, expect_expired)
 assert av.get("absent") == expect_absent, (av, expect_absent)
+# Skipped detection jobs must be reported under `skipped`, distinct from
+# `not_fetched` (which is reserved for eligible targets the collector
+# didn't reach — e.g. budget exhausted). In the happy-path fixture we
+# reach every target so `not_fetched` must be absent entirely.
+expect_skipped = sum(1 for i in ext if outcome(i) in ("skipped", "in_progress"))
+assert av.get("skipped") == expect_skipped, (av, expect_skipped)
+assert "not_fetched" not in av, av
 
 d = s["detection_results"]
 assert d["with_verdict"] == expect_present, d
@@ -460,6 +485,15 @@ for section in "## Totals" "## Detection job outcomes" "## Verdict availability"
   "## Detection results" "## Reasons reported by gh-aw" "## By workflow" "## Notable runs"; do
   grep -qF "$section" "${out}/summary.md" || fail "summary.md is missing section: ${section}"
 done
+# The verdict-availability table must explain each state — the raw keys
+# (`present`, `not_fetched`) are opaque to a reader. `not_fetched` in particular
+# needs to be tied to skipped detection jobs, not to a fetch failure.
+grep -qF "| State | Meaning | Count |" "${out}/summary.md" ||
+  fail "summary.md verdict table is missing its Meaning column"
+grep -qF "detection job was skipped" "${out}/summary.md" ||
+  fail "summary.md must explain that skipped means the detection job was skipped"
+grep -qF "detection artifact downloaded and parsed" "${out}/summary.md" ||
+  fail "summary.md must describe successful (present) downloads"
 
 # --- budget exhaustion degrades gracefully rather than failing ---------------
 out2="${work}/out-budget"