fix(issueflow): a failed read never reaches a decision function

`gh api` prints a 5xx response body to stdout AND exits non-zero, and
GitHub's 5xx body is a JSON object. Inside the per-issue subshell that
payload passed `has("pull_request") | not`, emptied `.labels[]`, and
`queue_decision` — correct on the input it was handed — wrote
`needs-triage` onto a healthy epic. The run then logged `reconciled.`
and exited 0 (crew#329, #247).

errexit could not have caught it: a command whose status is tested by
`||` runs with errexit suppressed, and the suppression extends through
the whole subshell body, so the `|| log` handler is what disables the
errexit that would have aborted at the failed read. Removing the handler
revives errexit and loses #91's resilience, and an inline `set -e` does
not re-arm it. Explicit per-read checks are the mechanism.

Every read inside that subshell is now checked — the issue read on its
status AND on its payload shape (an HTTP 200 whose body is `null` exits
0 and empties the label set just the same), both reads in
`last_issue_activity`, and the comments read in
`issue_comment_has_marker`. On failure the issue is left exactly as it
is, the reason rides its own `#$n:` line, and the subshell exits with a
distinguished status the sweep counts, so a deliberate skip is not
reported as a crash and a genuine crash is still named byte-identically.

`read_failure_reason` moves to lib/read.sh beside a new `guarded_read`,
sourced by both reconcilers: labels-reconcile's copy was the only one,
and the issue surface needs the identical rule.

Refs #247

actions/issueflow-reconcile/issueflow-reconcile.sh

@@ -27,10 +27,33 @@ TRIAGE_ACTORS=()
 # The needs-ruling invariants (#52) — one implementation for both surfaces.
 # shellcheck source=lib/ruling.sh
 . "$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/../../lib/ruling.sh"
+# The guarded read and its reason line (#101, #247) — one implementation for
+# both surfaces.
+# shellcheck source=lib/read.sh
+. "$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/../../lib/read.sh"
+
+# The status a per-issue subshell exits with when it walked away from an
+# unreadable fact (#247 D4). Distinguished from every other non-zero status so
+# a deliberate skip is counted rather than reported as a crash — and so the
+# existing crash handler still names a genuine one.
+ISSUEFLOW_SKIP=3
+# Set by reconcile_issue_pass, read once by main for the D6 tail.
+SKIPPED_COUNT=0
+SKIPPED_ISSUES=""
 
 log() { printf 'issueflow: %s\n' "$*"; }
 run() { if [ -n "${DRY_RUN:-}" ]; then log "DRY_RUN: $*"; else "$@"; fi; }
 
+skip_issue() { # $1 = issue, $2 = the whole reason clause — ends this issue's pass
+  # Leaves the issue exactly as it is: nothing is derived from a read that
+  # did not answer. Called from wherever the read lives, so no call site can
+  # forget to check — which is why it exits rather than returns. Every caller
+  # runs inside the per-issue subshell, so the exit ends that issue's pass and
+  # nothing else. The reason rides its own `#$n:`-prefixed line (#247 D5).
+  log "#$1: skipped this pass — $2"
+  exit "$ISSUEFLOW_SKIP"
+}
+
 load_issueflow_config() { # $1 = labels.conf
   local conf="$1" line seen=false
   [ -f "$conf" ] || { echo "issueflow: missing config: $conf" >&2; return 1; }
@@ -287,6 +310,31 @@ offsite_resolved_decision() { # PR states on stdin -> NUDGE | QUIET
   fi
 }
 
+issue_payload_valid() { # $1 = the requested issue; payload on stdin
+  # The second of D3's two required guards, and neither subsumes the other.
+  # The status check catches the 504 whose body is GitHub's JSON error object
+  # — valid JSON that passes every jq guard and empties the label set. THIS
+  # one catches an HTTP 200 whose body is `null`, which exits 0 and empties it
+  # just the same. `.number` is checked against the issue asked for, so a
+  # payload about some other issue can never be reconciled as this one.
+  jq -e --arg n "$1" '
+    type == "object" and (.number | tostring) == $n and (.labels | type) == "array"
+  ' >/dev/null 2>&1
+}
+
+skipped_tail() { # $1 = skip count, $2 = the issue numbers → the D6 line, or nothing
+  # `reconciled.` stays byte-identical when the pass was whole — tests pin that
+  # exact string, and #101 D1 is the precedent for not folding new text into a
+  # matched line. A partial pass says so on a line of its own, after it, so a
+  # consumer reading only the tail of a job log can see it.
+  [ "$1" -gt 0 ] || return 0
+  if [ "$1" -eq 1 ]; then
+    printf '%s issue skipped this pass on an unreadable fact: %s\n' "$1" "$2"
+  else
+    printf '%s issues skipped this pass on unreadable facts: %s\n' "$1" "$2"
+  fi
+}
+
 # API edge. Marker comments make warnings and nudges idempotent across sweeps.
 ensure_comment() { # $1 issue, $2 marker, $3 message
   local n="$1" marker="$2" message="$3"
@@ -295,9 +343,15 @@ ensure_comment() { # $1 issue, $2 marker, $3 message
 $message" >/dev/null
 }
 
-issue_comment_has_marker() { # $1 issue, $2 marker
+issue_comment_has_marker() { # $1 issue, $2 marker → 0 found, 1 genuinely absent
-  gh api --paginate "repos/$REPO/issues/$1/comments" --jq '.[].body' \
+  # A failed read used to answer "no marker", which re-posts the comment the
-    | grep -qF "<!-- issueflow:$2 -->"
+  # marker exists to suppress — absence of evidence read as evidence of
+  # absence (#247 D1). It cannot be a return value: every caller treats
+  # non-zero as "absent", so the skip is taken here, at the read.
+  local bodies
+  guarded_read bodies gh api --paginate "repos/$REPO/issues/$1/comments" --jq '.[].body' \
+    || skip_issue "$1" "could not read its comments: $(read_failure_reason "$READ_FAILURE_STDERR")"
+  grep -qF "<!-- issueflow:$2 -->" <<<"$bodies"
 }
 
 reference_states() {
@@ -324,22 +378,27 @@ offsite_timeline() { # unreadable timelines are deliberately silent
   gh api --paginate "repos/$REPO/issues/$1/timeline" 2>/dev/null || return 1
 }
 
-last_issue_activity() {
+last_issue_activity() { # $1 issue, $2 created_at → epoch; non-zero if a read failed
-  local n="$1" created="$2" latest
+  # Both reads are checked, and a failure reports rather than answering an age
-  latest="$({
+  # (#247 D1). Swallowed, the comments read falls back to `created_at`, and a
-      printf '%s\n' "$created"
+  # `claimed` issue created months ago but commented on seconds earlier is
-      gh api --paginate "repos/$REPO/issues/$n/comments" --jq '.[].created_at'
+  # reclaimed — the live builder unassigned, under a comment asserting 48
-      # Assignment is the claim itself. Ignoring it would let an old issue be
+  # hours of silence. `needs-triage` is cheap to remove; that is not.
-      # reclaimed in the seconds between assignment and its required draft PR.
+  # gh's stderr is left to flow to this function's own, where the caller's
-      gh api --paginate "repos/$REPO/issues/$n/timeline" \
+  # guarded_read captures it for the reason line.
-        --jq '.[] | select(.event == "assigned") | .created_at'
+  local n="$1" created="$2" comments timeline latest
-    } \
+  comments="$(gh api --paginate "repos/$REPO/issues/$n/comments" --jq '.[].created_at')" \
-    | sort | tail -n1)"
+    || return 1
+  # Assignment is the claim itself. Ignoring it would let an old issue be
+  # reclaimed in the seconds between assignment and its required draft PR.
+  timeline="$(gh api --paginate "repos/$REPO/issues/$n/timeline" \
+    --jq '.[] | select(.event == "assigned") | .created_at')" || return 1
+  latest="$(printf '%s\n%s\n%s\n' "$created" "$comments" "$timeline" | sort | tail -n1)"
   date -d "$latest" +%s
 }
 
 reconcile_issue() {
-  local n="$1" decision refs cross_refs states age assignees open_pr=false label owners
+  local n="$1" decision refs cross_refs states age created assignees open_pr=false label owners
   local merged_ref_pr="" transition_marker="" transition_handled=false
   local unchecked="" remove_claimed=claimed
   decision="$(queue_decision <<<"$ISSUE_LABELS")"
@@ -386,7 +445,9 @@ The merge releases the claim; no builder owes a draft. Triage owes completion in
       fi
       log "#$n: merged Refs PR -> post-merge; claim released"
     else
-      age="$(last_issue_activity "$n" "$(jq -r '.created_at' <<<"$ISSUE_JSON")")"
+      created="$(jq -r '.created_at' <<<"$ISSUE_JSON")"
+      guarded_read age last_issue_activity "$n" "$created" \
+        || skip_issue "$n" "could not read its activity history: $(read_failure_reason "$READ_FAILURE_STDERR")"
       if [ "$(claim_clock_exempt <<<"$ISSUE_LABELS")" = EXEMPT ]; then
         # Legitimately quiet work does not run the reclaim clock. Only the
         # clock stops: an unassigned claim is still a repair the decision must
@@ -474,8 +535,11 @@ The merge releases the claim; no builder owes a draft. Triage owes completion in
       run gh issue edit "$n" -R "$REPO" --remove-label stale >/dev/null
       log "#$n: unstale (a ruling is pending)"
     fi
-    [ -n "${age:-}" ] \
+    if [ -z "${age:-}" ]; then
-      || age="$(last_issue_activity "$n" "$(jq -r '.created_at' <<<"$ISSUE_JSON")")"
+      created="$(jq -r '.created_at' <<<"$ISSUE_JSON")"
+      guarded_read age last_issue_activity "$n" "$created" \
+        || skip_issue "$n" "could not read its activity history: $(read_failure_reason "$READ_FAILURE_STDERR")"
+    fi
     reconcile_ruling "$n" "$age" "$NOW"
   fi
 }
@@ -503,6 +567,34 @@ reconcile_opened_issue() {
   log "#$n: needs-triage (opened by $author)"
 }
 
+reconcile_issue_pass() { # $1 = issue — one issue's whole pass, in its own subshell
+  # The subshell is #91's resilience: one unreadable or broken issue must not
+  # take the sweep down. What it is NOT is an errexit boundary — a command
+  # whose status is tested by `||` runs with errexit suppressed, and the
+  # suppression extends through the whole subshell body, so the handler below
+  # is what disables the errexit that would have caught a failed read (#247
+  # D2). Removing it would revive errexit and lose #91. Explicit per-read
+  # checks are the mechanism instead, and each one exits with ISSUEFLOW_SKIP.
+  local n="$1" status=0
+  (
+    guarded_read ISSUE_JSON gh api "repos/$REPO/issues/$n" \
+      || skip_issue "$n" "could not read the issue: $(read_failure_reason "$READ_FAILURE_STDERR")"
+    issue_payload_valid "$n" <<<"$ISSUE_JSON" \
+      || skip_issue "$n" "the issue read answered a payload that is not issue #$n carrying a label array"
+    jq -e 'has("pull_request") | not' <<<"$ISSUE_JSON" >/dev/null || exit 0
+    ISSUE_LABELS="$(jq -r '.labels[].name' <<<"$ISSUE_JSON")"
+    reconcile_issue "$n"
+  ) || status=$?
+  if [ "$status" -eq "$ISSUEFLOW_SKIP" ]; then
+    SKIPPED_COUNT=$((SKIPPED_COUNT + 1))
+    SKIPPED_ISSUES="${SKIPPED_ISSUES:+$SKIPPED_ISSUES }#$n"
+  elif [ "$status" -ne 0 ]; then
+    # Byte-identical, and still owed: a skip is deliberate, a crash is not,
+    # and folding the two together would hide one behind the other (D4).
+    log "#$n: reconcile failed — continuing with the remaining issues"
+  fi
+}
+
 main() {
   local owner name
   REPO="${REPO:?set REPO to owner/name}"
@@ -554,17 +646,21 @@ main() {
         done < <(refs_references <<<"$body")
       done)"
 
-  local n
+  local n tail_line
+  SKIPPED_COUNT=0
+  SKIPPED_ISSUES=""
   for n in $(gh api --paginate "repos/$REPO/issues?state=open&per_page=100" \
       --jq '.[] | select(has("pull_request") | not) | .number'); do
-    (
+    reconcile_issue_pass "$n"
-      ISSUE_JSON="$(gh api "repos/$REPO/issues/$n")"
-      jq -e 'has("pull_request") | not' <<<"$ISSUE_JSON" >/dev/null || exit 0
-      ISSUE_LABELS="$(jq -r '.labels[].name' <<<"$ISSUE_JSON")"
-      reconcile_issue "$n"
-    ) || log "#$n: reconcile failed — continuing with the remaining issues"
   done
   log "reconciled."
+  # The job stays green (D7): an hourly sweep over a hundred-issue board meets
+  # transient 504s as a matter of course, and reddening the whole run for one
+  # skipped issue trains consumers to ignore red — the outcome #95 and #101
+  # both steered away from on the PR surface. This line is what buys back the
+  # auditability that costs.
+  tail_line="$(skipped_tail "$SKIPPED_COUNT" "$SKIPPED_ISSUES")"
+  [ -z "$tail_line" ] || log "$tail_line"
 }
 
 if [ "${BASH_SOURCE[0]}" = "$0" ]; then main "$@"; fi

actions/labels-reconcile/labels-reconcile.sh

@@ -74,6 +74,12 @@ SELF_WORKFLOW="${SELF_WORKFLOW:-${GITHUB_WORKFLOW:-}}"
 # The needs-ruling invariants (#52) — one implementation for both surfaces.
 # shellcheck source=lib/ruling.sh
 . "$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/../../lib/ruling.sh"
+# The guarded read and its reason line (#101) — one implementation for both
+# surfaces. read_failure_reason lived here until the issue surface needed the
+# identical rule (#247); a second copy of it is the failure lib/ruling.sh's
+# own header was written to record.
+# shellcheck source=lib/read.sh
+. "$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/../../lib/read.sh"
 
 log() { printf 'labels: %s\n' "$*"; }
 
@@ -98,25 +104,6 @@ blind_sweep_warning() { # $1 = unreadable PRs, $2 = all open PRs, $3 = sampled r
   fi
 }
 
-read_failure_reason() { # $1 = captured stderr → one bounded line; pure (#101)
-  # Verbatim, collapsed, bounded (D3): gh emits multi-line errors and GraphQL
-  # blobs. Collapsed so the reason is exactly one log line — a raw newline
-  # inside the captured per-PR output block could collide with a matched
-  # string — and truncated because an unbounded paste per PR per sweep is
-  # noise, and annotations are capped anyway.
-  local reason
-  reason="$(printf '%s' "${1-}" | tr '\n' ' ')"
-  if [ -z "$reason" ]; then
-    # Empty stderr is itself a fact (D4): a read that failed silently is a
-    # different observation from a denial, and must not read as one.
-    echo "no error output"
-  elif [ "${#reason}" -gt 300 ]; then
-    printf '%s…\n' "${reason:0:300}"
-  else
-    printf '%s\n' "$reason"
-  fi
-}
-
 missing_core_labels_warning() { # $1 = declared rows, $2 = repo label names
   local rows="$1" repo_labels="$2" row name missing=""
   [ -n "$repo_labels" ] || return 0

changelog.d/247.md

@@ -0,0 +1,16 @@
+### Fixed
+
+- The issue sweep no longer derives label writes from a read that failed. An
+  HTTP 504 whose body is GitHub's JSON error object passed every guard and
+  emptied the label set, so a healthy epic was written `needs-triage` and the
+  pass reported success (#247).
+- A failed comments read no longer reclaims a live claim. Swallowed, it dated
+  the issue by `created_at` and unassigned the builder under a comment
+  asserting 48 hours of silence about an issue commented on seconds earlier
+  (#247).
+- A failed comments read no longer reads as "no marker", which re-posted the
+  comment the marker exists to suppress (#247).
+- Every read inside the per-issue subshell is checked explicitly, on its
+  status and on its payload shape; the issue is left exactly as it is and the
+  sweep continues. A partial pass names its skipped issues after
+  `reconciled.` (#247).

lib/read.sh

@@ -0,0 +1,55 @@
+#!/usr/bin/env bash
+# lib/read.sh — the guarded read: an unreadable fact never invents a verdict.
+#
+# Both reconcilers source this file. The rule is the family's oldest one
+# (#101, #95) and it has now been bought twice: the PR surface learned it
+# when a permissions denial and a network hiccup left byte-identical
+# evidence, and the ISSUE surface learned it when an HTTP 504 whose body is
+# GitHub's JSON error object flowed straight into a decision function —
+# `gh api` prints that body to stdout *and* exits non-zero, so the payload
+# reaching the guards was valid JSON, `.labels[]` came back empty, and the
+# sweep wrote `needs-triage` onto a healthy epic and called the pass a
+# success (crew#329, #247).
+#
+# Two helpers, both used on both surfaces:
+#   - guarded_read  — run a read, keep its stderr, report its status
+#   - read_failure_reason — render that stderr into one bounded log line
+#
+# What the CALLER does with a failed read is the caller's: labels-reconcile
+# leaves the PR alone for the pass, issueflow-reconcile skips the issue. The
+# one thing neither may do is carry a degraded value into a decision.
+
+guarded_read() { # $1 = variable to fill, rest = the read; sets READ_FAILURE_STDERR
+  # The status check and the captured stderr are one operation on purpose: a
+  # read whose failure is noticed but whose reason is thrown away is what
+  # #95 had to infer a cause from a control case for — wrongly, it turned
+  # out (#101 D2). Captured into a file rather than merged into stdout, so
+  # an unlucky error line can never be read back as the read's own payload.
+  local __var="$1" __err __out __rc=0
+  shift
+  __err="$(mktemp)" || return 1
+  __out="$("$@" 2>"$__err")" || __rc=$?
+  READ_FAILURE_STDERR="$(cat "$__err")"
+  rm -f "$__err"
+  printf -v "$__var" '%s' "$__out"
+  return "$__rc"
+}
+
+read_failure_reason() { # $1 = captured stderr → one bounded line; pure (#101)
+  # Verbatim, collapsed, bounded (D3): gh emits multi-line errors and GraphQL
+  # blobs. Collapsed so the reason is exactly one log line — a raw newline
+  # inside the captured per-item output block could collide with a matched
+  # string — and truncated because an unbounded paste per item per sweep is
+  # noise, and annotations are capped anyway.
+  local reason
+  reason="$(printf '%s' "${1-}" | tr '\n' ' ')"
+  if [ -z "$reason" ]; then
+    # Empty stderr is itself a fact (D4): a read that failed silently is a
+    # different observation from a denial, and must not read as one.
+    echo "no error output"
+  elif [ "${#reason}" -gt 300 ]; then
+    printf '%s…\n' "${reason:0:300}"
+  else
+    printf '%s\n' "$reason"
+  fi
+}

test/issueflow-reconcile.test.sh

@@ -289,6 +289,11 @@ check "claimed plus attention is a healthy issue" 0 "KEEP" \
 INOW=2000000000
 iso_at() { date -u -d "@$1" +%Y-%m-%dT%H:%M:%SZ; }
 
+# gh's own rendering of a 5xx whose body carries a `message` key — the line
+# crew#329's job log carried, verbatim (#247), and the payload beside it.
+GH_STUB_STDERR="gh: We couldn't respond to your request in time. (HTTP 504)"
+GH_STUB_ERROR_BODY='{"message":"We could not respond to your request in time.","documentation_url":"https://docs.github.com/rest"}'
+
 issue_stub_gh() {
   if [ "$1" = api ]; then
     shift
@@ -303,7 +308,18 @@ issue_stub_gh() {
     done
     file="$TMP/$(printf '%s' "$endpoint" | tr '/' '_').json"
     printf '%s\n' "$endpoint" >>"$TMP/api-calls"
-    [ ! -f "$file.error" ] || return 1
+    # A `.http-error` sentinel is the real 5xx (#247): `gh api` prints the
+    # response body — GitHub's JSON error object — to STDOUT, says why on
+    # stderr, and exits non-zero. The `.error` sentinel models a failure with
+    # no payload, which is the *safe* path (an empty label set is empty either
+    # way), and is why this class was never caught. Both now speak on stderr,
+    # because the real gh always does and the reason line renders it.
+    if [ -f "$file.http-error" ]; then
+      cat "$file.http-error"
+      printf '%s\n' "$GH_STUB_STDERR" >&2
+      return 1
+    fi
+    [ ! -f "$file.error" ] || { printf '%s\n' "$GH_STUB_STDERR" >&2; return 1; }
     [ -f "$file" ] || { printf '[]\n'; return 0; }
     if [ -n "$jqexpr" ]; then jq -r "$jqexpr" "$file"; else cat "$file"; fi
   elif [ "$1" = issue ] && [ "$2" = comment ]; then