feat: implement issue #2196 — persona-runner discards a finished advisory when the posting-identity read fails — an API error body is read as a wrong login (first router-served PR dispatch, #2188)

.github/workflows/persona-runner-reusable.yml

@@ -108,7 +108,7 @@
         # restoring the ADR-0005 position; reverses #1650, fixes #1734). Chosen per
         # persona from runtime.identity.credential, never a repo-wide default.
         required: false
       # GH_PAT_WORKFLOWS removed (#1650): the broadly-scoped workflows PAT is no
       # longer a posting fallback here — least privilege, and posting under it
       # attributed advisories to a human owner account.
 
@@ -294,15 +294,19 @@
           # write access — otherwise a writable token for the WRONG account passes
           # the capability probe and triggers the paid engine, only to be rejected
           # by the post step's identity check afterward (the engine work is already
-          # spent). A definitely-wrong login fails closed here; an unreadable login
-          # (transient/rate-limit) is left to the post step's fail-closed check,
-          # matching this preflight's fail-open-on-indeterminate contract.
-          if [ -n "${GH_TOKEN}" ]; then
-            actual_login="$(gh api user -q .login 2>/dev/null || true)"
-            if [ -n "$actual_login" ] && [ "$actual_login" != "$POSTING_ACCOUNT" ]; then
-              echo "::error::authz-preflight: posting token authenticates as '${actual_login}', not the manifest identity '${POSTING_ACCOUNT}' (credential '${POSTING_CREDENTIAL}') — refusing to run the engine under the wrong identity"
-              exit 1
-            fi
+          # spent). A readable, definitely-wrong login fails closed here; an
+          # UNREADABLE read (a failed call or an error body — never a login,
+          # #2196) is indeterminate and left to the post step's retrying,
+          # fail-closed check, matching this preflight's fail-open contract.
+          # shellcheck source=scripts/lib/persona-runner.sh
+          source scripts/lib/persona-runner.sh
+          login_rc=0
+          actual_login="$(pr_read_posting_login)" || login_rc=$?
+          if [ "$login_rc" -eq 0 ] && [ "$actual_login" != "$POSTING_ACCOUNT" ]; then
+            echo "::error::authz-preflight: posting token authenticates as '${actual_login}', not the manifest identity '${POSTING_ACCOUNT}' (credential '${POSTING_CREDENTIAL}') — refusing to run the engine under the wrong identity"
+            exit 1
+          elif [ "$login_rc" -eq 1 ]; then
+            echo "::warning::authz-preflight: posting identity unreadable (${actual_login}) — indeterminate, proceeding; the post step verifies it"
           fi
 
           bash scripts/persona-authz-preflight.sh
@@ -435,65 +439,27 @@
           fi
           export GH_TOKEN
 
-          if [ -z "${GH_TOKEN:-}" ]; then
-            echo "::error::no PAT available to post as '${POSTING_ACCOUNT}' (credential '${POSTING_CREDENTIAL}') — is that secret set on this repo/org?"
-            exit 1
-          fi
-
-          # Defense in depth: the selected token MUST authenticate as the
-          # manifest-declared account. If the chosen secret is unset, we
-          # could silently post under the wrong identity — the exact #1650
-          # regression. Fail closed (a future mention retries) rather than
-          # post under the wrong identity.
-          actual_login="$(gh api user -q .login 2>/dev/null || true)"
-          if [ "$actual_login" != "$POSTING_ACCOUNT" ]; then
-            msg="posting token authenticates as '${actual_login:-<unknown>}', "
-            msg="${msg}not the manifest identity '${POSTING_ACCOUNT}' "
-            msg="${msg}(credential '${POSTING_CREDENTIAL}') — refusing to post"
-            echo "::error::${msg} under the wrong identity"
-            exit 1
-          fi
-
-          raw="$(cat "$RUNNER_TEMP/agent-out.txt")"
-          body="$(pr_extract_advisory "$raw")"
-          if [ -z "${body//[[:space:]]/}" ]; then
-            echo "::warning::agent produced no advisory between the sentinels for ${SOURCE_REPO}#${ITEM_NUMBER} — nothing to post"
-            exit 0
-          fi
-
-          # Mechanically guarantee the recursion marker — the agent could not post
-          # unmarked (no write token), and now cannot post unmarked via us either.
-          # LOAD-BEARING under ADR-0008: personas now post as the owner account
-          # don-petry, which the router's actor axis cannot exclude, so THIS marker
-          # is the SOLE mechanical recursion guard for persona output. Enforcing it
-          # here in the post step — the only writer — is what keeps it workflow-
-          # enforced, not prompt-enforced (the #860 runaway failure mode). The
-          # forgot-the-marker path is covered by tests/persona_runner.bats
-          # ("pr_ensure_marker PREPENDS the marker when the agent forgot it"); keep
-          # that coverage green — do not weaken this call.
-          body="$(pr_ensure_marker "$PERSONA" "$body")"
-
-          if [ -z "${ITEM_NUMBER}" ]; then
-            echo "::notice::discussion advisory not yet supported by the runner post-path — skipping post for ${SOURCE_REPO}"
-            exit 0
-          fi
-
-          # Post via the orchestrator so a failing gh api cannot take the advisory
-          # with it: on failure it preserves the redacted, marker-complete body to
-          # $body_file (uploaded by the next step) and mirrors a truncated copy to
-          # the job summary, then returns non-zero so the run STILL fails (#1775).
-          # Issue-comments endpoint serves PRs too.
-          pr_post_advisory_or_preserve \
+          # Extract the advisory, guarantee the recursion marker, and write the
+          # redacted body to $body_file FIRST — so the upload step below has it
+          # on ANY failure here (#2196) — then verify the token authenticates as
+          # the manifest account (defense in depth against the #1650
+          # wrong-identity regression: a readable mismatch or a missing PAT fails
+          # closed at once; an unreadable read is retried, then fails closed),
+          # then post. A failing post preserves the body and still fails the run
+          # (#1775). The marker enforcement is load-bearing (ADR-0008) — see
+          # pr_run_post in scripts/lib/persona-runner.sh.
+          pr_run_post \
             "$PERSONA" "$SOURCE_REPO" "$ITEM_NUMBER" \
             "$POSTING_ACCOUNT" "$POSTING_CREDENTIAL" \
-            "$body" "$body_file" "$GITHUB_STEP_SUMMARY"
+            "$RUNNER_TEMP/agent-out.txt" "$body_file" "$GITHUB_STEP_SUMMARY"
 
       - name: Preserve the advisory as a run artifact (post failed)
         # Failure-only, byte-for-byte re-postable copy of record. Named
         # deterministically from the item so the right one is findable when
-        # several runs have failed. The step above wrote $body_file only on a
-        # post failure; on any other failure (or if it never got that far)
-        # if-no-files-found:ignore keeps this a clean no-op (#1775 AC #1/#2).
+        # several runs have failed. The step above writes $body_file before its
+        # identity check and post (#2196), so any failure after extraction
+        # uploads it; if it never got that far, if-no-files-found:ignore keeps
+        # this a clean no-op (#1775 AC #1/#2).
         if: failure() && steps.post.outputs.artifact_name != ''
         uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1
         with:

scripts/lib/persona-runner.sh

@@ -227,3 +227,140 @@ pr_post_advisory_or_preserve() {
   printf '::error::%s\n' "$diag" >&2
   return "$rc"
 }
+
+# ----------------------------------------------------------------------------
+# Posting-identity read (#2196)
+# ----------------------------------------------------------------------------
+# On a failed `gh api user` call gh prints the API error BODY on stdout, so an
+# inline `$(gh api user -q .login || true)` held '{' — and the post step reported
+# the token as authenticating as '{' and discarded a paid-for advisory over a
+# transient error. Reading the login is therefore tri-state, and both the
+# authorization preflight and the post step use this one reader.
+
+# pr_valid_login <value> — 0 if value is a syntactically valid GitHub login (one
+# line; alphanumerics and hyphens, not leading with a hyphen; at most 39 chars;
+# an optional [bot] suffix). A JSON error body, an HTML page, or multi-line
+# output is never a login.
+pr_valid_login() {
+  [[ "$1" =~ ^[A-Za-z0-9][A-Za-z0-9-]{0,38}(\[bot\])?$ ]]
+}
+
+# pr_read_posting_login — read the login GH_TOKEN authenticates as.
+#   rc 0: stdout is a valid login.
+#   rc 1: UNREADABLE — the call failed or its output is not a login; stdout is a
+#         one-line, redacted reason (it says why, it is never treated as a login).
+#   rc 2: nothing configured — GH_TOKEN is empty; gh is not called.
+pr_read_posting_login() {
+  if [ -z "${GH_TOKEN:-}" ]; then
+    return 2
+  fi
+  local out err errfile rc=0
+  errfile="$(mktemp)" || {
+    printf 'could not create a temporary file to read the posting identity'
+    return 1
+  }
+  out="$(gh api user -q .login 2>"$errfile")" || rc=$?
+  err="$(cat "$errfile")"
+  rm -f "$errfile"
+  if [ "$rc" -eq 0 ] && pr_valid_login "$out"; then
+    printf '%s' "$out"
+    return 0
+  fi
+  local why
+  if [ "$rc" -ne 0 ]; then
+    why="gh api user failed (rc=${rc}): ${err:-$out}"
+  else
+    why="gh api user returned a value that is not a GitHub login: ${out}"
+  fi
+  # Redact BEFORE truncating: a cut through a token leaves a prefix too short
+  # for redact_secrets to match.
+  printf '%s' "$why" | tr '\r\n' '  ' | redact_secrets | cut -c1-300
+  return 1
+}
+
+# pr_verify_posting_identity <account> <credential> [attempts]
+# Fail closed unless GH_TOKEN reads as <account>. A readable login that differs
+# fails AT ONCE (a definite answer — the #1650 wrong-identity regression). An
+# unreadable read is retried with linear backoff (PR_LOGIN_RETRY_DELAY seconds,
+# default 5, times the attempt number) up to <attempts> (default 3) reads, then
+# fails with a message that says the identity could not be read and why. No
+# token fails at once. Posting under an unverified identity is never an outcome.
+pr_verify_posting_identity() {
+  local account="$1" credential="$2" attempts="${3:-3}"
+  local delay="${PR_LOGIN_RETRY_DELAY:-5}" attempt=1 login rc
+  while :; do
+    rc=0
+    login="$(pr_read_posting_login)" || rc=$?
+    case "$rc" in
+      0)
+        if [ "$login" = "$account" ]; then
+          return 0
+        fi
+        printf "::error::posting token authenticates as '%s', not the manifest identity '%s' (credential '%s') — refusing to post under the wrong identity\n" \
+          "$login" "$account" "$credential" >&2
+        return 1
+        ;;
+      2)
+        printf "::error::no PAT available to post as '%s' (credential '%s') — is that secret set on this repo/org?\n" \
+          "$account" "$credential" >&2
+        return 1
+        ;;
+    esac
+    if [ "$attempt" -ge "$attempts" ]; then
+      printf "::error::could not read the posting identity for '%s' (credential '%s') after %s attempts: %s — refusing to post under an unverified identity\n" \
+        "$account" "$credential" "$attempts" "$login" >&2
+      return 1
+    fi
+    printf "::warning::posting identity unreadable (attempt %s/%s): %s — retrying\n" \
+      "$attempt" "$attempts" "$login" >&2
+    sleep "$(( delay * attempt ))"
+    attempt=$(( attempt + 1 ))
+  done
+}
+
+# pr_run_post <persona> <source_repo> <item_number> <account> <credential>
+#     <agent_out_file> <body_file> <summary_file>
+# The post step, in order: extract the advisory from the agent output (nothing
+# between the sentinels → nothing to post, rc 0); guarantee the recursion
+# marker; write the REDACTED body to <body_file> BEFORE any identity check, so
+# the failure-only upload step preserves it on ANY post-step failure (#2196 —
+# #1775 only covered a failing `gh api` post); verify the posting identity;
+# then post via pr_post_advisory_or_preserve. Caller exports GH_TOKEN.
+pr_run_post() {
+  local persona="$1" source_repo="$2" item_number="$3" account="$4" credential="$5"
+  local agent_out="$6" body_file="$7" summary_file="$8" body
+  body="$(pr_extract_advisory "$(cat "$agent_out")")"
+  if [ -z "${body//[[:space:]]/}" ]; then
+    echo "::warning::agent produced no advisory between the sentinels for ${source_repo}#${item_number} — nothing to post"
+    return 0
+  fi
+
+  # Mechanically guarantee the recursion marker — the agent could not post
+  # unmarked (no write token), and now cannot post unmarked via us either.
+  # LOAD-BEARING under ADR-0008: personas now post as the owner account
+  # don-petry, which the router's actor axis cannot exclude, so THIS marker
+  # is the SOLE mechanical recursion guard for persona output. Enforcing it
+  # here in the post step — the only writer — is what keeps it workflow-
+  # enforced, not prompt-enforced (the #860 runaway failure mode). The
+  # forgot-the-marker path is covered by tests/persona_runner.bats
+  # ("pr_ensure_marker PREPENDS the marker when the agent forgot it"); keep
+  # that coverage green — do not weaken this call.
+  body="$(pr_ensure_marker "$persona" "$body")"
+  printf '%s' "$body" | redact_secrets > "$body_file"
+
+  pr_verify_posting_identity "$account" "$credential" || return 1
+
+  if [ -z "${item_number}" ]; then
+    echo "::notice::discussion advisory not yet supported by the runner post-path — skipping post for ${source_repo}"
+    return 0
+  fi
+
+  # Post via the orchestrator so a failing gh api cannot take the advisory
+  # with it: on failure it re-preserves the redacted, marker-complete body to
+  # <body_file> and mirrors a truncated copy to the job summary, then returns
+  # non-zero so the run STILL fails (#1775). Issue-comments endpoint serves PRs.
+  pr_post_advisory_or_preserve \
+    "$persona" "$source_repo" "$item_number" \
+    "$account" "$credential" \
+    "$body" "$body_file" "$summary_file"
+}