fix(ci): the advisory guard fails OPEN on an API error -- move the || outside (#1209)

.github/workflows/dependabot-auto-merge.yml

@@ -217,7 +217,12 @@ jobs:
           # low first-dep version makes almost every other name match: false confirmation in the
           # PERMISSIVE direction (measured against the live endpoint at @0.7.0: aiohttp 30, pillow 29,
           # urllib3 9, requests 6). `updated-dependencies-json` carries a per-dependency prevVersion.
-          pairs="$(printf '%s' "$DEPS_JSON" | jq -r '.[] | [.dependencyName, .prevVersion] | @tsv' 2>/dev/null || echo "ERR")"
+          # `||` outside the substitution (see the advisory lookup below). It matters here for a
+          # second reason: jq STREAMS, so a mid-array error can leave earlier rows already on stdout,
+          # and the old form would have appended ERR to a TRUNCATED dependency list — iterating a
+          # subset of the PR's dependencies while still reporting success. Assigning on failure
+          # discards the partial output instead of inheriting it.
+          pairs="$(printf '%s' "$DEPS_JSON" | jq -r '.[] | [.dependencyName, .prevVersion] | @tsv' 2>/dev/null)" || pairs="ERR"
           if [ "$pairs" = "ERR" ] || [ -z "$pairs" ]; then
             echo "::warning::no per-dependency metadata — failing closed (manual review)."
             echo "advisory_ok=false" >> "$GITHUB_OUTPUT"
@@ -236,15 +241,30 @@ jobs:
               advisory_ok=false
               break
             fi
+            # The `||` binds the ASSIGNMENT, never the substitution. `x=$(cmd || echo ERR)` APPENDS
+            # the sentinel to whatever cmd already wrote to stdout, and `gh api` copies the JSON
+            # error BODY to stdout on any HTTP error (only the `gh: ... (HTTP nnn)` line goes to
+            # stderr, which `2>/dev/null` eats). The old form left `count` holding
+            # `{"message":"API rate limit exceeded",...}ERR` — neither "ERR" nor empty, so the
+            # sentinel MISSED; `[ "$count" -lt 1 ]` then failed with "integer expression expected"
+            # and returned 2, which an `if` condition is exempt from under `set -e`. The step printed
+            # "advisory confirmed", wrote advisory_ok=true and exited 0 — this guard inverted to
+            # FAIL-OPEN on precisely the rate-limit/API-error class the comment above names first.
             count="$(gh api -X GET /advisories \
               -f ecosystem=pip \
               -f affects="${name}@${prev}" \
-              --jq '[.[] | select(.withdrawn_at == null)] | length' 2>/dev/null || echo "ERR")"
-            if [ "$count" = "ERR" ] || [ -z "$count" ]; then
-              echo "::warning::advisory lookup failed for '$name' — failing closed (manual review)."
-              advisory_ok=false
-              break
-            fi
+              --jq '[.[] | select(.withdrawn_at == null)] | length' 2>/dev/null)" || count="ERR"
+            # A SHAPE check, not equality against one sentinel. The numeric comparison below needs
+            # "is this a number" answered, and only a shape test answers it for values nobody
+            # anticipated — an equality test recognises exactly the failure it was told about, which
+            # is how a JSON body walked through the old guard.
+            case "$count" in
+              ""|*[!0-9]*)
+                echo "::warning::advisory lookup failed for '$name' — failing closed (manual review)."
+                advisory_ok=false
+                break
+                ;;
+            esac
             if [ "$count" -lt 1 ]; then
               echo "::warning::no published advisory covers '${name}@${prev}' — failing closed (manual review)."
               advisory_ok=false
@@ -307,7 +327,8 @@ jobs:
               ;;
           esac
 
-          pairs="$(printf '%s' "$DEPS_JSON" | jq -r '.[] | [.dependencyName, .newVersion] | @tsv' 2>/dev/null || echo "ERR")"
+          # `||` outside the substitution, and for the same streaming-jq reason as guardrail #2's.
+          pairs="$(printf '%s' "$DEPS_JSON" | jq -r '.[] | [.dependencyName, .newVersion] | @tsv' 2>/dev/null)" || pairs="ERR"
           if [ "$pairs" = "ERR" ] || [ -z "$pairs" ]; then
             echo "::warning::no per-dependency metadata — failing closed (manual review)."
             echo "age_ok=false" >> "$GITHUB_OUTPUT"
@@ -348,19 +369,22 @@ jobs:
                 ;;
             esac
 
-            body="$(curl -sS --fail --max-time 20 --retry 2 "https://pypi.org/pypi/${name}/${ver}/json" 2>/dev/null || echo "ERR")"
+            # `--fail` suppresses the 4xx/5xx body, so the old form happened to work for HTTP errors
+            # — but a TRUNCATED transfer (curl 18/56) exits non-zero with partial bytes already on
+            # stdout, and the sentinel would have been appended to a half a JSON document. Outside.
+            body="$(curl -sS --fail --max-time 20 --retry 2 "https://pypi.org/pypi/${name}/${ver}/json" 2>/dev/null)" || body="ERR"
             if [ "$body" = "ERR" ] || [ -z "$body" ]; then
               echo "::warning::PyPI lookup failed for '${name}==${ver}' — failing closed (manual review)."
               age_ok=false
               break
             fi
-            published="$(printf '%s' "$body" | jq -r '[.urls[].upload_time_iso_8601] | sort | .[0] // empty' 2>/dev/null || echo "ERR")"
+            published="$(printf '%s' "$body" | jq -r '[.urls[].upload_time_iso_8601] | sort | .[0] // empty' 2>/dev/null)" || published="ERR"
             if [ "$published" = "ERR" ] || [ -z "$published" ] || [ "$published" = "null" ]; then
               echo "::warning::no upload timestamp for '${name}==${ver}' — failing closed (manual review)."
               age_ok=false
               break
             fi
-            published_epoch="$(date -u -d "$published" +%s 2>/dev/null || echo "ERR")"
+            published_epoch="$(date -u -d "$published" +%s 2>/dev/null)" || published_epoch="ERR"
             case "$published_epoch" in
               ""|*[!0-9]*)
                 echo "::warning::unparseable upload timestamp '$published' for '${name}==${ver}' — failing closed."

docs/BACKLOG.md

@@ -8981,3 +8981,73 @@ which means it cannot catch the next one. Follow the VALUE.
 **Source:** raised by the `asvs-tracking-rework` session on 2026-08-09 after `proxy` -> `proxy_url`
 became the third instance: *"that is not a coincidence to note in a residual; it is an argument that the
 rename boundary itself needs a guard"*. Filed before it was forgotten, per that session's request.
+## 1209. the dependency advisory guard inverts to FAIL-OPEN when the advisory API errors
+
+> 🔢 **Filed 2026-08-09 - FIXED IN THE SAME CHANGE, entry published WITH the fix.** Value **9/10** · Difficulty **2/10**. Guardrail #2 of `dependabot-auto-merge.yml` reads `count="$(gh api ... || echo "ERR")"`. The `||` runs INSIDE the command substitution, so it APPENDS to stdout rather than replacing it - and `gh api` copies the JSON error BODY to stdout on any HTTP error. The sentinel `[ "$count" = "ERR" ]` therefore misses, and the guard emits `advisory_ok=true` for a lookup that never succeeded.
+
+**Cluster:** CI / supply chain. **Priority:** P1. **Verdict:** build (done).
+**Severity:** unlike the redaction items above, this is not conditional on a first deployment - the
+workflow runs in CI today. What bounds it is narrower and worth stating exactly: the engine's merge
+condition also requires `age_ok`, and the age step returns false for every ecosystem that can be
+`eligible`, a disjointness the file documents about itself. So the falsely-true `advisory_ok` cannot
+ALONE merge anything as shipped. It flips a security decision the workflow publishes, and the file
+labels the surviving blocker "a FORWARD guard ... load-bearing the day a Python allow row is
+populated" - one line's edit away from making this directly merge-affecting.
+
+**The mechanism, reproduced end to end against the shipped step body:**
+
+```
+gh api on any HTTP error:  JSON body -> STDOUT, "gh: ... (HTTP nnn)" -> stderr (eaten by 2>/dev/null)
+  count = '{"message":"API rate limit exceeded","status":"403"}ERR'
+  [ "$count" = "ERR" ] || [ -z "$count" ]   -> MISSES (neither)
+  [ "$count" -lt 1 ]                        -> "integer expression expected", returns 2
+                                            -> an `if` CONDITION is exempt from `set -e`
+  -> "::notice::published advisory confirmed", advisory_ok=true, step exits 0
+```
+
+Measured by running the real `ghsa` body from `origin/main` and from the fix, under `bash -e`, with a
+`gh` stub reproducing the stream split:
+
+```
+                    gh ERRORS          gh returns 1
+pre-fix (main)      advisory_ok=true   advisory_ok=true     <- FAIL OPEN
+fixed               advisory_ok=false  advisory_ok=true     <- fails closed, happy path intact
+```
+
+**A stub that merely exits non-zero would have proved nothing** - it would pass against the defective
+code too. The defect is that the BODY reached the variable, so the stub has to write the body.
+
+**The comment directly above the defect asserted the opposite:** "Fail closed on any error", and the
+header, "a rate-limit/API error or no-matching-advisory routes to manual review, never auto-merge."
+A compensating control resting on a false premise, which is the shape SDS-3.7 names.
+
+**The existing test could not see it.** `test_ghsa_step_queries_the_advisory_api_and_emits_a_guard`
+asserted the STRING `"advisory_ok=false" in body` - satisfied by a step that merely CONTAINS the words,
+and the fail-open lived underneath a passing version of exactly that check. The file already had the
+right instrument: `_run_step_body` executes shipped `run:` bodies under `bash -e` and returns the
+parsed `$GITHUB_OUTPUT`. Guardrail #2 was the one guard not using it.
+`test_the_advisory_guard_fails_closed_when_the_api_errors` now executes the body across three rows,
+including a discriminating PASS so the suite cannot be satisfied by a step that denies unconditionally.
+
+**The domain, because fixing one instance is how this class survives:** a sweep of 63 workflow and
+script files across both repositories found 24 instances of the idiom - 16 provably harmless (`git
+rev-parse --verify --quiet` writes nothing on failure), and the rest fixed here. Moving the `||` outside
+the substitution also fixes the streaming cases for free: jq emits rows before a mid-array error, and
+the old form would have appended the sentinel to a TRUNCATED dependency list while still reporting
+success. Assigning on failure discards partial output instead of inheriting it.
+
+The `count` guard additionally moved from an equality test against one sentinel to a SHAPE test
+(`case "$count" in ""|*[!0-9]*)`). An equality test recognises exactly the failure it was told about,
+which is how a JSON body walked through it; the numeric comparison's real question is "is this a
+number", and only a shape test answers that for values nobody anticipated.
+
+**Sibling, same idiom, in the private scorecard repo:** its `asvs-verifier-drift.yml` mirror-decision
+step fails the opposite way - `remote_tip` holds the 404 body instead of the empty string, so it
+refuses to decide on EVERY run where the mirror branch does not exist, which is the steady state. That
+one fails closed and is therefore a dead control rather than a disclosure; it is why the daily drift
+job has never completed its decision step.
+
+**Source:** found 2026-08-09 while sweeping for siblings of the drift-workflow defect, after a peer
+correctly refuted my first diagnosis of that job's failure (I said the control "detected drift and
+could not act"; the scheduled run predated the drift by 88 minutes and its parity step passed - the
+control has never yet detected this class at all).

tests/test_dependabot_automerge_guardrails.py

@@ -456,6 +456,79 @@ def test_release_age_holds_the_version_track_and_undatable_ecosystems(
     )
 
 
+def _gh_stub(tmp_path: Path, count: str | None) -> Path:
+    """A ``gh`` on PATH that ignores its arguments.
+
+    ``count=None`` reproduces an HTTP error the way the real client does, and that split is the
+    whole point: **``gh api`` writes the JSON error BODY to stdout** and only its ``gh: ... (HTTP
+    nnn)`` line to stderr. A stub that merely exits non-zero would pass against the defective code
+    and prove nothing — the defect was that the body reached the variable.
+    """
+    stub_dir = tmp_path / "ghstub"
+    stub_dir.mkdir()
+    stub = stub_dir / "gh"
+    if count is None:
+        stub.write_text(
+            "#!/usr/bin/env bash\n"
+            'echo \'{"message":"API rate limit exceeded","status":"403"}\'\n'
+            "echo 'gh: API rate limit exceeded (HTTP 403)' >&2\n"
+            "exit 1\n",
+            encoding="utf-8",
+        )
+    else:
+        stub.write_text(f"#!/usr/bin/env bash\necho '{count}'\n", encoding="utf-8")
+    stub.chmod(0o755)
+    return stub_dir
+
+
+@pytest.mark.skipif(
+    shutil.which("bash") is None or shutil.which("jq") is None,
+    reason="needs bash + jq, same runner matrix as the release-age rows below",
+)
+@pytest.mark.parametrize(
+    ("label", "count", "expected"),
+    [
+        # The discriminating PASS — without it the suite would be satisfied by a step that denies
+        # unconditionally, which is the failure mode a fail-closed guard degrades into.
+        ("one published advisory", "1", "true"),
+        ("no advisory covers the version", "0", "false"),
+        # THE REGRESSION ROW. This is the one that was red before the fix: `gh` exits non-zero with
+        # a JSON error body on STDOUT, `$(cmd || echo ERR)` APPENDED the sentinel to that body, and
+        # the resulting value was neither "ERR" nor empty. The equality sentinel missed, the numeric
+        # comparison failed with "integer expression expected" and returned 2, an `if` condition is
+        # exempt from `set -e`, and the step printed "advisory confirmed" and emitted
+        # advisory_ok=true while exiting 0. The guard inverted to FAIL-OPEN on exactly the
+        # rate-limit/API-error class its own header promises routes to manual review.
+        ("advisory API errors (rate limit)", None, "false"),
+    ],
+)
+def test_the_advisory_guard_fails_closed_when_the_api_errors(
+    label: str, count: str | None, expected: str, tmp_path: Path
+) -> None:
+    """Guardrail #2 EXECUTED, not string-matched.
+
+    The sibling assertion ``"advisory_ok=false" in body`` cannot distinguish a guard that fails
+    closed from one that merely contains the words — the fail-open above lived underneath a passing
+    version of exactly that check. Asserting on the emitted OUTPUT is what makes the difference
+    visible.
+    """
+    rc, out = _run_step_body(
+        "ghsa",
+        {
+            "DEP_GROUP": "pip-security",
+            "DEPS_JSON": '[{"dependencyName":"requests","prevVersion":"2.19.0"}]',
+            "GH_TOKEN": "stub",
+        },
+        tmp_path,
+        path_prepend=_gh_stub(tmp_path, count),
+    )
+    assert rc == 0, f"the ghsa body aborted under `bash -e` (rc={rc}) — CI would fail the step"
+    assert out.get("security_track") == "true", "the fixture should select the security track"
+    assert out.get("advisory_ok") == expected, (
+        f"{label} -> advisory_ok={out.get('advisory_ok')!r}, expected {expected!r}"
+    )
+
+
 def _curl_stub(tmp_path: Path, payload: str | None) -> Path:
     """A ``curl`` on PATH that ignores its arguments. ``payload=None`` makes it fail like a network
     or HTTP error would (``--fail`` exits non-zero), which must route to manual review."""