From 13e8f54d605a58918c175e1891915df748680ec2 Mon Sep 17 00:00:00 2001 From: cndgrr <59120057+cndgrr@users.noreply.github.com> Date: Mon, 3 Aug 2026 18:01:58 +0000 Subject: [PATCH 1/5] fix(issueflow): a failed read never reaches a decision function MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 --- .../issueflow-reconcile.sh | 146 +++++++++++++++--- actions/labels-reconcile/labels-reconcile.sh | 25 +-- changelog.d/247.md | 16 ++ lib/read.sh | 55 +++++++ test/issueflow-reconcile.test.sh | 18 ++- 5 files changed, 215 insertions(+), 45 deletions(-) create mode 100644 changelog.d/247.md create mode 100644 lib/read.sh diff --git a/actions/issueflow-reconcile/issueflow-reconcile.sh b/actions/issueflow-reconcile/issueflow-reconcile.sh index a77df04..209ee3f 100644 --- a/actions/issueflow-reconcile/issueflow-reconcile.sh +++ b/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 - gh api --paginate "repos/$REPO/issues/$1/comments" --jq '.[].body' \ - | grep -qF "" +issue_comment_has_marker() { # $1 issue, $2 marker → 0 found, 1 genuinely absent + # A failed read used to answer "no marker", which re-posts the comment the + # 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 "" <<<"$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() { - local n="$1" created="$2" latest - latest="$({ - printf '%s\n' "$created" - gh api --paginate "repos/$REPO/issues/$n/comments" --jq '.[].created_at' - # Assignment is the claim itself. Ignoring it would let an old issue be - # reclaimed in the seconds between assignment and its required draft PR. - gh api --paginate "repos/$REPO/issues/$n/timeline" \ - --jq '.[] | select(.event == "assigned") | .created_at' - } \ - | sort | tail -n1)" +last_issue_activity() { # $1 issue, $2 created_at → epoch; non-zero if a read failed + # Both reads are checked, and a failure reports rather than answering an age + # (#247 D1). Swallowed, the comments read falls back to `created_at`, and a + # `claimed` issue created months ago but commented on seconds earlier is + # reclaimed — the live builder unassigned, under a comment asserting 48 + # hours of silence. `needs-triage` is cheap to remove; that is not. + # gh's stderr is left to flow to this function's own, where the caller's + # guarded_read captures it for the reason line. + local n="$1" created="$2" comments timeline latest + comments="$(gh api --paginate "repos/$REPO/issues/$n/comments" --jq '.[].created_at')" \ + || 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:-}" ] \ - || age="$(last_issue_activity "$n" "$(jq -r '.created_at' <<<"$ISSUE_JSON")")" + if [ -z "${age:-}" ]; then + 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 - ( - 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" + reconcile_issue_pass "$n" 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 diff --git a/actions/labels-reconcile/labels-reconcile.sh b/actions/labels-reconcile/labels-reconcile.sh index 003c92c..97ada8a 100755 --- a/actions/labels-reconcile/labels-reconcile.sh +++ b/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 diff --git a/changelog.d/247.md b/changelog.d/247.md new file mode 100644 index 0000000..0b0c371 --- /dev/null +++ b/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). diff --git a/lib/read.sh b/lib/read.sh new file mode 100644 index 0000000..c391cb2 --- /dev/null +++ b/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 +} diff --git a/test/issueflow-reconcile.test.sh b/test/issueflow-reconcile.test.sh index 837194a..678999f 100644 --- a/test/issueflow-reconcile.test.sh +++ b/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 From 865d5bd1df45e4fd9e09660f098272562e67e7f4 Mon Sep 17 00:00:00 2001 From: cndgrr <59120057+cndgrr@users.noreply.github.com> Date: Mon, 3 Aug 2026 18:06:13 +0000 Subject: [PATCH 2/5] =?UTF-8?q?test(issueflow):=20drive=20the=20real=205xx?= =?UTF-8?q?=20=E2=80=94=20a=20JSON=20error=20body=20on=20stdout?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The PATH-stubbed gh gains a `.http-error` mode: the response body goes to STDOUT, the reason to stderr, the status non-zero. The existing `.error` sentinel produces empty stdout, which is the *safe* path — an empty label set either way — and is why this class was never caught. The three must-fail-before cases, plus the 200-`null` path a status check alone leaves open, the suppressed-marker duplicate, the D6 tail's count and numbers, and the crash handler proven distinct from a skip. Refs #247 --- lib/read.sh | 1 + test/issueflow-reconcile.test.sh | 205 ++++++++++++++++++++++++++++++- 2 files changed, 205 insertions(+), 1 deletion(-) diff --git a/lib/read.sh b/lib/read.sh index c391cb2..1aa26be 100644 --- a/lib/read.sh +++ b/lib/read.sh @@ -29,6 +29,7 @@ guarded_read() { # $1 = variable to fill, rest = the read; sets READ_FAILURE_STD shift __err="$(mktemp)" || return 1 __out="$("$@" 2>"$__err")" || __rc=$? + # shellcheck disable=SC2034 # the out-parameter: every caller reads it beside the status READ_FAILURE_STDERR="$(cat "$__err")" rm -f "$__err" printf -v "$__var" '%s' "$__out" diff --git a/test/issueflow-reconcile.test.sh b/test/issueflow-reconcile.test.sh index 678999f..3ba46c4 100644 --- a/test/issueflow-reconcile.test.sh +++ b/test/issueflow-reconcile.test.sh @@ -293,6 +293,7 @@ iso_at() { date -u -d "@$1" +%Y-%m-%dT%H:%M:%SZ; } # 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"}' +export GH_STUB_STDERR # the PATH-stubbed gh of the executable runs reads it too issue_stub_gh() { if [ "$1" = api ]; then @@ -645,6 +646,119 @@ churned="$(issue_probe 24 $'claimed\nneeds-ruling')" check "8 real-quiet days nudge through a 2-day-old label churn" 0 "" \ grep -q 'ruling nudge' <<<"$churned" +# --------------------------------------------------------------------------- +# An unreadable fact invents no verdict on the issue surface either (#247). +# `gh api` prints a 5xx body to stdout AND exits non-zero, and GitHub's 5xx +# body is a JSON object — so the payload that reached the guards was valid +# JSON, `.labels[]` came back empty, and queue_decision was handed the wrong +# input. The pure guards first, then the two decisions the fall-through +# reached. +# --------------------------------------------------------------------------- +payload_refused() { ! issue_payload_valid "$@"; } # 0 when the payload is refused + +check "a healthy issue payload is accepted" 0 "" \ + issue_payload_valid 40 <<<'{"number":40,"labels":[{"name":"ready"}]}' +check "an issue carrying no labels at all is still a valid payload" 0 "" \ + issue_payload_valid 40 <<<'{"number":40,"labels":[]}' +# The reported shape: gh renders `gh: (HTTP 504)` from a body with a +# `message` key, which proves the body was valid JSON. The status check is what +# catches this one; the shape check refuses it independently. +check "a JSON error object is not an issue payload" 0 "" \ + payload_refused 40 <<<"$GH_STUB_ERROR_BODY" +# The live path a status check alone would leave open (D3): 200, exit 0, and +# `.labels[]` empties exactly as it does on the 504. +check "an HTTP 200 whose body is null is refused" 0 "" payload_refused 40 <<<'null' +check "a payload missing .labels is refused" 0 "" \ + payload_refused 40 <<<'{"number":40}' +check "a payload whose .labels is not an array is refused" 0 "" \ + payload_refused 40 <<<'{"number":40,"labels":"ready"}' +check "a payload about a different issue is refused" 0 "" \ + payload_refused 40 <<<'{"number":41,"labels":[]}' +check "a payload that is not JSON at all is refused" 0 "" \ + payload_refused 40 <<<'not json' +check "an empty payload is refused" 0 "" payload_refused 40 "$(cfix 50)" +printf '%s\n' "$GH_STUB_ERROR_BODY" >"$(cfix 50).http-error" +jq -n --arg at "$(iso_at $((INOW - 10 * 86400)))" \ + '[{"event":"assigned","created_at":$at}]' >"$(tfix 50)" +claim_edits_before="$(wc -l <"$TMP/issue-edits")" +check "a 504 on the comments read skips the issue instead of grading its age" \ + 3 "#50: skipped this pass — could not read its activity history: $GH_STUB_STDERR" \ + issue_probe 50 claimed 1 +check "...so the live claim is not reclaimed" 1 "" \ + grep -q 'stale claim reclaimed -> ready' <<<"$(issue_probe 50 claimed 1)" +# shellcheck disable=SC2016 # positional parameters belong to bash -c +check "...no unassign, no label swap, and no reclaim comment" 0 "" \ + bash -c 'test "$1" -eq "$(wc -l <"$2")" && test ! -f "$3"' _ \ + "$claim_edits_before" "$TMP/issue-edits" "$TMP/posted-50" + +# -- the suppressed comment: a 504 on the marker read ----------------------- +# The marker is on the issue. Read as "no marker", a failed read re-posts the +# comment the marker exists to suppress — every sweep, forever. +jq -n --arg b '' \ + --arg at "$(iso_at $((INOW - 3600)))" \ + '[{"user":{"login":"sweep-bot"},"created_at":$at,"html_url":"https://x/m","body":$b}]' \ + >"$(cfix 51)" +printf '%s\n' "$GH_STUB_ERROR_BODY" >"$(cfix 51).http-error" +check "a 504 on the marker read skips rather than reading it as no marker" \ + 3 "#51: skipped this pass — could not read its comments: $GH_STUB_STDERR" \ + issue_probe 51 blocked 1 false "" "no parseable declaration here" +check "...so no duplicate comment is posted" 1 "" test -f "$TMP/posted-51" + +# -- a deliberate skip is counted; a genuine crash is still named (D4) ------- +printf '%s\n' '{"number":60,"labels":[{"name":"ready"}],"assignees":[]}' \ + >"$TMP/repos_owner_repo_issues_60.json" +printf '%s\n' '{"number":61,"labels":[{"name":"ready"}],"assignees":[]}' \ + >"$TMP/repos_owner_repo_issues_61.json" +printf '%s\n' "$GH_STUB_ERROR_BODY" >"$TMP/repos_owner_repo_issues_61.json.http-error" +pass_probe() { # $1 issue; $2 non-empty makes reconcile_issue crash + ( + REPO=owner/repo + gh() { issue_stub_gh "$@"; } + [ -z "${2:-}" ] || reconcile_issue() { return 9; } + SKIPPED_COUNT=0 + SKIPPED_ISSUES="" + reconcile_issue_pass "$1" + printf 'rc=%s count=%s issues=%s\n' "$?" "$SKIPPED_COUNT" "$SKIPPED_ISSUES" + ) +} +check "a genuine non-read crash still names the failure byte-identically" 0 \ + "issueflow: #60: reconcile failed — continuing with the remaining issues" \ + pass_probe 60 crash +check "...and the pass still returns 0, so the loop reaches the next issue" 0 \ + "rc=0" pass_probe 60 crash +check "...and a crash is not counted as a skip" 0 "count=0" pass_probe 60 crash +check "a skipped issue is counted and named" 0 "count=1 issues=#61" pass_probe 61 +check "...and is not also reported as a crash" 1 "" \ + grep -q 'reconcile failed' <<<"$(pass_probe 61)" +check "...leaving the loop free to continue" 0 "rc=0" pass_probe 61 + # --------------------------------------------------------------------------- # The arrival path, executed the way the action executes it (#91): four # triage-authored mints died silently because the stand-down `return`s in @@ -681,7 +795,15 @@ if [ "$1" = api ]; then *'states: MERGED'*) file="$GH_FIXTURES/graphql-merged.json" ;; esac fi - [ ! -f "$file.error" ] || exit 1 + # `.http-error` is the real 5xx (#247): the response body — GitHub's JSON + # error object — goes to STDOUT, the reason to stderr, and the status is + # non-zero. `.error` is the payload-free failure, which is the safe path. + if [ -f "$file.http-error" ]; then + cat "$file.http-error" + printf '%s\n' "${GH_STUB_STDERR:-}" >&2 + exit 1 + fi + [ ! -f "$file.error" ] || { printf '%s\n' "${GH_STUB_STDERR:-}" >&2; exit 1; } if [ -f "$file" ]; then payload="$(cat "$file")"; else payload='[]'; fi if [ -n "$jqexpr" ]; then jq -r "$jqexpr" <<<"$payload"; else printf '%s\n' "$payload"; fi exit 0 @@ -849,4 +971,85 @@ check "a dead API on the arrival path still fails the run (D2)" 0 "" \ check "...and the sweep does not run over a lying arrival" 1 "" \ grep -qF 'issueflow: reconciled.' <<<"$err_out" +# --------------------------------------------------------------------------- +# The whole sweep over an unreadable board (#247), executed. The sourced +# probes above drive one issue's pass; only this path exercises the loop, the +# counting and the tail — and only this path reproduces crew#329's log, which +# ended `issueflow: reconciled.` with rc=0 over a label it should never have +# written. Its own fixture directory: the arrival fixtures above are stateful +# across their cases. +# --------------------------------------------------------------------------- +SWEEP="$TMP/sweep" +mkdir -p "$SWEEP" +printf '%s\n' \ + '{"data":{"repository":{"pullRequests":{"nodes":[],"pageInfo":{"hasNextPage":false,"endCursor":null}}}}}' \ + >"$SWEEP/graphql-open.json" +cp "$SWEEP/graphql-open.json" "$SWEEP/graphql-merged.json" +# 70: the 504 with a JSON error body on the per-issue read. +printf '%s\n' "$GH_STUB_ERROR_BODY" >"$SWEEP/repos_owner_repo_issues_70.json.http-error" +# 71: healthy, and carrying no queue label — so if the sweep reaches it, it +# writes needs-triage. That write is the evidence the loop continued. +printf '%s\n' \ + '{"number":71,"user":{"login":"triage-one"},"labels":[{"name":"enhancement"}],"assignees":[]}' \ + >"$SWEEP/repos_owner_repo_issues_71.json" +# 72: HTTP 200 whose body is `null` — exit 0, and the label set empties just +# as it does on the 504. The shape check is the only thing that catches it. +printf 'null\n' >"$SWEEP/repos_owner_repo_issues_72.json" + +sweep_board() { printf '%s\n' "$1" >"$SWEEP/repos_owner_repo_issues_state_open_per_page_100.json"; } +sweep_run() { + : >"$SWEEP/edits" + env PATH="$ARRIVAL/stub:$PATH" GH_FIXTURES="$SWEEP" ISSUEFLOW_NOW="$INOW" \ + REPO=owner/repo LABELS_CONF="$ARRIVAL/labels.conf" \ + bash "$ROOT/actions/issueflow-reconcile/issueflow-reconcile.sh" 2>&1 +} + +sweep_board '[{"number":70},{"number":71}]' +sweep_out="$(sweep_run)" +sweep_rc=$? +check "an unreadable issue does not red the sweep (D7)" 0 "" test "$sweep_rc" -eq 0 +check "the 504's JSON error body is skipped, with the reason named" 0 \ + "issueflow: #70: skipped this pass — could not read the issue: $GH_STUB_STDERR" \ + printf '%s\n' "$sweep_out" +check "...and crew#329's label is never written" 1 "" \ + grep -qF '#70: needs-triage (no queue state)' <<<"$sweep_out" +check "...nor any edit at all on the unreadable issue" 1 "" \ + grep -qF 'issue edit 70' "$SWEEP/edits" +check "...while the readable issue beside it is reconciled as before" 0 "" \ + grep -qxF 'issue edit 71 -R owner/repo --add-label needs-triage' "$SWEEP/edits" +check "...and the partial pass names its count and its issue" 0 \ + 'issueflow: 1 issue skipped this pass on an unreadable fact: #70' \ + printf '%s\n' "$sweep_out" +check "...after a byte-identical reconciled. line" 0 "" \ + grep -qxF 'issueflow: reconciled.' <<<"$sweep_out" + +sweep_board '[{"number":72}]' +null_out="$(sweep_run)" +null_rc=$? +check "an HTTP 200 whose body is null exits 0 and writes nothing" 0 "" \ + test "$null_rc" -eq 0 +check "...because the shape check refuses it, on its own line" 0 \ + 'issueflow: #72: skipped this pass — the issue read answered a payload that is not issue #72 carrying a label array' \ + printf '%s\n' "$null_out" +check "...so no label is derived from an empty label set" 1 "" \ + grep -qF 'issue edit 72' "$SWEEP/edits" +check "...and the tail names it too" 0 \ + 'issueflow: 1 issue skipped this pass on an unreadable fact: #72' \ + printf '%s\n' "$null_out" + +sweep_board '[{"number":70},{"number":72}]' +both_out="$(sweep_run)" +check "two skipped issues are both named, in the plural" 0 \ + 'issueflow: 2 issues skipped this pass on unreadable facts: #70 #72' \ + printf '%s\n' "$both_out" + +sweep_board '[{"number":71}]' +whole_out="$(sweep_run)" +whole_rc=$? +check "a whole pass still exits 0" 0 "" test "$whole_rc" -eq 0 +check "...ends on the byte-identical reconciled. line, with no tail after it" 0 \ + "issueflow: reconciled." printf '%s\n' "$(tail -n1 <<<"$whole_out")" +check "...and says nothing about skipping" 1 "" \ + grep -q 'skipped this pass' <<<"$whole_out" + summary From 24f62cc8e96a11c52ca3f36c13de5146e41b57e2 Mon Sep 17 00:00:00 2001 From: cndgrr <59120057+cndgrr@users.noreply.github.com> Date: Mon, 3 Aug 2026 18:08:19 +0000 Subject: [PATCH 3/5] test(issueflow): the failing --jq read yields no timestamps, as gh does MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The .http-error mode applies a requested --jq filter to the error body, so a failing comments read returns nothing rather than a JSON blob — which is what let last_issue_activity fall back to created_at and reclaim a live claim. With it, all three of the issue's must-fail-before cases fail against the pre-change script, the destroyed claim included. Pin offsite_timeline's own deliberate silence directly: the activity read hits the same endpoint, so probe 32 now skips before the offsite verification it used to reach (D8 leaves that read alone). Refs #247 --- test/issueflow-reconcile.test.sh | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/test/issueflow-reconcile.test.sh b/test/issueflow-reconcile.test.sh index 3ba46c4..2ac02b0 100644 --- a/test/issueflow-reconcile.test.sh +++ b/test/issueflow-reconcile.test.sh @@ -316,7 +316,14 @@ issue_stub_gh() { # 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" + # A --jq call gets the filter applied to the error body, as gh does. + # That is what "yields no timestamps" looks like — the shape that let + # last_issue_activity fall back to created_at and reclaim a live claim. + if [ -n "$jqexpr" ]; then + jq -r "$jqexpr" "$file.http-error" 2>/dev/null || true + else + cat "$file.http-error" + fi printf '%s\n' "$GH_STUB_STDERR" >&2 return 1 fi @@ -593,6 +600,16 @@ unreadable="$(issue_probe 32 $'claimed\noffsite')" check "an unreadable timeline stays silent" 1 "" test -f "$TMP/posted-32" check "...and leaves the sweep running without an alarming log" 1 "" \ grep -qiE 'error|failed' <<<"$unreadable" +# Both checks above still hold, and #247 D1 changed what reaches them: +# last_issue_activity reads the same timeline endpoint, so the issue is now +# skipped before the offsite verification runs. The skip is why nothing is +# posted, and its reason line is a deliberate report rather than an alarm +# (D4). D8 leaves offsite_timeline's own silence alone, so it is pinned here +# directly rather than through a probe that can no longer reach it. +offsite_timeline_probe() { ( REPO=owner/repo; gh() { issue_stub_gh "$@"; }; offsite_timeline "$1" ); } +check "an unreadable offsite timeline yields nothing and still fails closed" 1 "" \ + offsite_timeline_probe 32 +check "...while a readable one answers its payload" 0 "[]" offsite_timeline_probe 31 : >"$TMP/api-calls" printf '[]\n' >"$(tfix 33)" @@ -799,7 +816,11 @@ if [ "$1" = api ]; then # error object — goes to STDOUT, the reason to stderr, and the status is # non-zero. `.error` is the payload-free failure, which is the safe path. if [ -f "$file.http-error" ]; then - cat "$file.http-error" + if [ -n "$jqexpr" ]; then + jq -r "$jqexpr" "$file.http-error" 2>/dev/null || true + else + cat "$file.http-error" + fi printf '%s\n' "${GH_STUB_STDERR:-}" >&2 exit 1 fi From f965e8404c3b5312896aeebb990da0d24dfb3e14 Mon Sep 17 00:00:00 2001 From: cndgrr <59120057+cndgrr@users.noreply.github.com> Date: Mon, 3 Aug 2026 18:14:19 +0000 Subject: [PATCH 4/5] test(issueflow): an absent fixture answers nothing to a --jq read, as gh does MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sourced stub returned a literal `[]` to --jq callers when a fixture was missing, where the real API answers an empty list and the filter yields nothing. last_issue_activity then sorted `[]` beside an ISO-8601 timestamp — and `[]` outsorts a timestamp in the C locale but not in a UTF-8 one, so the sweep dated an issue by a stub artifact on the runner and by created_at here. The old code swallowed the resulting `date` failure and graded the claim on a literal 0 anyway; #247's guards turn a failed read into a skip, which is what made the lie visible. Adopt the arrival stub's shape. Suite green under LC_ALL=C, C.UTF-8 and en_US.UTF-8. Refs #247 --- test/issueflow-reconcile.test.sh | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/test/issueflow-reconcile.test.sh b/test/issueflow-reconcile.test.sh index 2ac02b0..07f4d9c 100644 --- a/test/issueflow-reconcile.test.sh +++ b/test/issueflow-reconcile.test.sh @@ -328,8 +328,17 @@ issue_stub_gh() { 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 + # An absent fixture answers an empty list, and a --jq call gets the filter + # applied to it — the arrival stub's shape, and gh's. Returning the raw + # `[]` to a --jq caller made every missing fixture answer a literal `[]` + # where the real API answers nothing, and `[]` outsorts an ISO-8601 + # timestamp in the C locale but not in a UTF-8 one, so last_issue_activity + # dated an issue by a stub artifact on the runner and by its created_at + # here. The old code swallowed the resulting date failure; #247's guards + # turn it into a skip, which is what made the lie visible. + local payload='[]' + [ ! -f "$file" ] || payload="$(cat "$file")" + if [ -n "$jqexpr" ]; then jq -r "$jqexpr" <<<"$payload"; else printf '%s\n' "$payload"; fi elif [ "$1" = issue ] && [ "$2" = comment ]; then local n="$3" body="" file shift 3 From 6217798e1452ac5074f7e5d1018e415ec3bc68a0 Mon Sep 17 00:00:00 2001 From: cndgrr <59120057+cndgrr@users.noreply.github.com> Date: Mon, 3 Aug 2026 18:55:52 +0000 Subject: [PATCH 5/5] fix(issueflow): a per-issue pass commits its whole effect, or none of it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The per-read guards closed the reported class — a failed read never reaches a decision function — and left one layer standing. A pass could mutate and only THEN reach a guarded read, fail it, and report the issue as skipped: `stale` removed, or `needs-triage` minted, under a log line saying the sweep had touched nothing. That is the same false report #247 exists to close, told from the other end, and the panel reproduced it on four separate compositions. Fixed as the ordering invariant rather than per site. Inside reconcile_issue_pass's subshell, run() and log() stage their effects, and commit_staged_effects replays them in order once the pass has completed. skip_issue emits its own line directly and exits, so the buffer dies with the subshell. A skip therefore implies zero `gh issue edit`, zero `gh issue comment`, and no log line about a mutation that never landed — for compositions nobody has written yet, because reconcile_issue has no way to mutate directly. Reads stay where they are: they may happen anywhere, since nothing lands until the end. Stated per site it would hold until the next composition. Two consequences worth naming: reconcile_ruling is covered without touching lib/ruling.sh, because it posts through the sourcing script's run()/log() — the PR surface keeps its own and is unaffected; and a genuine crash mid-pass now also lands nothing, where before it left the earlier mutations applied. D4's handler string, D6's tail and D7's exit 0 are all unchanged, and the healthy path is byte-identical: every staged write commits under the same `>/dev/null` its call site already applied. Refs #247 --- .../issueflow-reconcile.sh | 78 ++++++++- changelog.d/247.md | 4 + lib/read.sh | 8 +- test/issueflow-reconcile.test.sh | 160 ++++++++++++++++++ 4 files changed, 241 insertions(+), 9 deletions(-) diff --git a/actions/issueflow-reconcile/issueflow-reconcile.sh b/actions/issueflow-reconcile/issueflow-reconcile.sh index 209ee3f..4e29f84 100644 --- a/actions/issueflow-reconcile/issueflow-reconcile.sh +++ b/actions/issueflow-reconcile/issueflow-reconcile.sh @@ -41,16 +41,67 @@ ISSUEFLOW_SKIP=3 SKIPPED_COUNT=0 SKIPPED_ISSUES="" -log() { printf 'issueflow: %s\n' "$*"; } -run() { if [ -n "${DRY_RUN:-}" ]; then log "DRY_RUN: $*"; else "$@"; fi; } +# A per-issue pass is ATOMIC: it commits its whole effect or none of it +# (#247 D1). Inside reconcile_issue_pass's subshell, `run` and `log` do not +# act — they append here, and commit_staged_effects replays them in order +# once the pass has completed. Everywhere else (the arrival path, the sweep's +# own lines) they act immediately, as they always did. +# +# This is the ordering invariant itself, not a fix for the two sites that +# happened to violate it: a mutation reached before a later guarded read is +# what let a pass remove `stale`, or mint `needs-triage`, and THEN report the +# issue as skipped — the sweep saying it touched nothing while a write had +# landed, which is the same false-report class #247 exists to close. Stated +# per site it would hold until the next composition; stated here it holds for +# compositions nobody has written yet, because reconcile_issue has no way to +# mutate directly. +# +# Reads are deliberately NOT staged. They may happen anywhere in the pass, +# because nothing lands until the end. +STAGING=false +STAGED_EFFECTS=() + +emit() { printf 'issueflow: %s\n' "$*"; } +apply() { if [ -n "${DRY_RUN:-}" ]; then emit "DRY_RUN: $*"; else "$@"; fi; } + +stage() { # $1 = LOG|WRITE, rest = the effect's argv, kept exact by the count + STAGED_EFFECTS+=("$#" "$@") +} + +log() { if [ "$STAGING" = true ]; then stage LOG "$@"; else emit "$@"; fi; } +run() { if [ "$STAGING" = true ]; then stage WRITE "$@"; else apply "$@"; fi; } + +commit_staged_effects() { + # In staging order, so a completed pass's log and writes read exactly as + # they did when each acted at its own call site. The `>/dev/null` is the one + # every `run` call site already applies: a redirection cannot travel with + # the argv, so it is applied here instead — uniformly, because on this + # surface every staged write has it. + local i=0 argc + STAGING=false + while [ "$i" -lt "${#STAGED_EFFECTS[@]}" ]; do + argc="${STAGED_EFFECTS[i]}" + if [ "${STAGED_EFFECTS[i + 1]}" = LOG ]; then + emit "${STAGED_EFFECTS[@]:i + 2:argc - 1}" + else + apply "${STAGED_EFFECTS[@]:i + 2:argc - 1}" >/dev/null + fi + i=$((i + 1 + argc)) + done + STAGED_EFFECTS=() +} 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" + # did not answer, and nothing this pass staged is ever committed — `exit` + # discards the subshell that holds the buffer. So a skip implies zero + # `gh issue edit`, zero `gh issue comment`, and no log line claiming an + # effect that never landed, wherever in the pass the failed read lives. + # Called from the read itself, so no call site can forget to check — which + # is why it exits rather than returns. The reason rides its own + # `#$n:`-prefixed line (#247 D5), emitted directly: the skip is a fact + # about the pass, not one of the effects the pass staged. + emit "#$1: skipped this pass — $2" exit "$ISSUEFLOW_SKIP" } @@ -575,15 +626,26 @@ reconcile_issue_pass() { # $1 = issue — one issue's whole pass, in its own sub # 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. + # + # What the subshell IS, since #247's first round, is the atomicity + # boundary: the staged effects live in it, so ending it — by a skip, or by + # a crash — discards them, and no partial pass can ever reach the board. local n="$1" status=0 ( + # Everything below stages rather than acts, and commits at the bottom — + # so a skip taken at any read, and a crash at any statement, leaves the + # issue exactly as it was (D1). `|| exit $?` keeps a crash's status the + # subshell's own, as it was when reconcile_issue was the last command + # here: the commit must not overwrite it, and must not run under it. + STAGING=true 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" + reconcile_issue "$n" || exit $? + commit_staged_effects ) || status=$? if [ "$status" -eq "$ISSUEFLOW_SKIP" ]; then SKIPPED_COUNT=$((SKIPPED_COUNT + 1)) diff --git a/changelog.d/247.md b/changelog.d/247.md index 0b0c371..a2243dd 100644 --- a/changelog.d/247.md +++ b/changelog.d/247.md @@ -14,3 +14,7 @@ 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). +- A per-issue pass is now atomic: its writes and its log lines commit only + once the pass completes. A skip could previously land after an earlier + mutation, reporting an issue as untouched when a label had already been + written or removed (#247). diff --git a/lib/read.sh b/lib/read.sh index 1aa26be..b3a0a7a 100644 --- a/lib/read.sh +++ b/lib/read.sh @@ -11,10 +11,16 @@ # sweep wrote `needs-triage` onto a healthy epic and called the pass a # success (crew#329, #247). # -# Two helpers, both used on both surfaces: +# Two helpers: # - guarded_read — run a read, keep its stderr, report its status # - read_failure_reason — render that stderr into one bounded log line # +# `read_failure_reason` is called from both surfaces. `guarded_read` is +# called from the issue surface only, and that is deliberate rather than +# unfinished: labels-reconcile's two capture sites are byte-identical to each +# other and predate this file, and converting them is a cleanup #247 does not +# own. Do not go looking for a labels-side caller — there is none yet. +# # 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. diff --git a/test/issueflow-reconcile.test.sh b/test/issueflow-reconcile.test.sh index 07f4d9c..e36adda 100644 --- a/test/issueflow-reconcile.test.sh +++ b/test/issueflow-reconcile.test.sh @@ -1082,4 +1082,164 @@ check "...ends on the byte-identical reconciled. line, with no tail after it" 0 check "...and says nothing about skipping" 1 "" \ grep -q 'skipped this pass' <<<"$whole_out" +# --------------------------------------------------------------------------- +# The ordering invariant (#247 D1): a skip implies ZERO writes, wherever in +# the pass the failed read lives. Round 1 measured what the per-read guards +# alone left standing — a pass could remove `stale`, or mint `needs-triage`, +# and only then reach a guarded read, fail it, and report the issue as +# skipped. The sweep said it had touched nothing while a write had landed: +# the same false report #247 exists to close, one layer along. +# +# Every composition is driven TWICE against identical fixtures, differing +# only in whether the late read answers. The healthy run is the control — it +# proves the mutation is genuinely on this path, so the failing run's "no +# edit" is a fact about the guard and not about a branch that never fired. +# Executed through the sweep, because staging is a property of the pass. +# --------------------------------------------------------------------------- +ORDER="$TMP/order" +mkdir -p "$ORDER" +cp "$SWEEP/graphql-open.json" "$SWEEP/graphql-merged.json" "$ORDER/" +order_board() { printf '%s\n' "$1" >"$ORDER/repos_owner_repo_issues_state_open_per_page_100.json"; } +order_fixture() { # $1 issue, $2 labels JSON, $3 body + jq -n --argjson n "$1" --argjson labels "$2" --arg body "${3:-}" \ + --arg at "$(iso_at $((INOW - 10 * 86400)))" \ + '{number: $n, created_at: $at, user: {login: "triage-one"}, + labels: $labels, assignees: [], body: $body}' \ + >"$ORDER/repos_owner_repo_issues_$1.json" +} +order_run() { + : >"$ORDER/edits" + env PATH="$ARRIVAL/stub:$PATH" GH_FIXTURES="$ORDER" ISSUEFLOW_NOW="$INOW" \ + REPO=owner/repo LABELS_CONF="$ARRIVAL/labels.conf" \ + bash "$ROOT/actions/issueflow-reconcile/issueflow-reconcile.sh" 2>&1 +} +# The late read fails, or answers. `guarded_read` is what turns either into a +# skip, so which endpoint carries the sentinel is what picks the composition. +order_breaks() { printf '%s\n' "$GH_STUB_ERROR_BODY" >"$ORDER/repos_owner_repo_issues_$1_$2.json.http-error"; } +order_heals() { rm -f "$ORDER/repos_owner_repo_issues_$1_$2.json.http-error"; } +# A skip must leave no trace of the staged effect: not the write, and not the +# log line that would have announced it. Both halves, because a landed write +# under a "skipped" line and a "reconciled" line over no write are the same +# lie told from opposite ends. +order_wrote() { grep -qF "issue $2 $1" "$ORDER/edits"; } + +# -- 1. unstale, then a failed activity read (the round's first composition) - +# `needs-ruling` heals an applied `stale` off before the tail reads the +# issue's activity. The read is two statements later; the write is already +# gone. +order_fixture 80 '[{"name":"ready"},{"name":"needs-ruling"},{"name":"stale"}]' +order_board '[{"number":80}]' +order_heals 80 comments +healthy_unstale="$(order_run)" +check "the control: a healthy pass really does unstale a pending ruling" 0 "" \ + order_wrote 80 edit +check "...and says so" 0 "issueflow: #80: unstale (a ruling is pending)" \ + printf '%s\n' "$healthy_unstale" +order_breaks 80 comments +broken_unstale="$(order_run)" +check "a failed activity read skips the unstale composition" 0 \ + "issueflow: #80: skipped this pass — could not read its activity history: $GH_STUB_STDERR" \ + printf '%s\n' "$broken_unstale" +check "...and the stale label is still on the issue" 1 "" order_wrote 80 edit +check "...and nothing claims it came off" 1 "" \ + grep -qF 'unstale (a ruling is pending)' <<<"$broken_unstale" + +# -- 2. ADD_NEEDS_TRIAGE, then a failed activity read (the second) ----------- +# The mint falls through — unlike FLAG_CONFLICT, which returns — into the +# same tail. crew#329's own label, written and then disowned by the log. +order_fixture 81 '[{"name":"enhancement"},{"name":"needs-ruling"}]' +order_board '[{"number":81}]' +order_heals 81 comments +healthy_mint="$(order_run)" +check "the control: a healthy pass really does mint needs-triage here" 0 "" \ + order_wrote 81 edit +check "...and says so" 0 "issueflow: #81: needs-triage (no queue state)" \ + printf '%s\n' "$healthy_mint" +order_breaks 81 comments +broken_mint="$(order_run)" +check "a failed activity read skips the needs-triage composition" 0 \ + "issueflow: #81: skipped this pass — could not read its activity history: $GH_STUB_STDERR" \ + printf '%s\n' "$broken_mint" +check "...and crew#329's label is not written on the way out" 1 "" \ + order_wrote 81 edit +check "...and nothing claims it was" 1 "" \ + grep -qF '#81: needs-triage (no queue state)' <<<"$broken_mint" + +# -- 3. the blockers->ready flip, then a failed TIMELINE read ---------------- +# The wider class: the failing read is the second one inside +# last_issue_activity, so the comments read answers and the marker check and +# the flip both complete first. A comment AND a label edit are staged. +printf '%s\n' '{"number":82,"state":"closed"}' \ + >"$ORDER/repos_owner_repo_issues_82.json" +order_fixture 83 '[{"name":"blocked"},{"name":"needs-ruling"}]' 'Blocked by #82.' +order_board '[{"number":83}]' +order_heals 83 timeline +healthy_flip="$(order_run)" +check "the control: a healthy pass really does flip cleared blockers to ready" 0 \ + "issueflow: #83: blockers closed -> ready" printf '%s\n' "$healthy_flip" +check "...writing the label edit" 0 "" order_wrote 83 edit +check "...and posting the blockers-cleared comment" 0 "" order_wrote 83 comment +order_breaks 83 timeline +broken_flip="$(order_run)" +check "a failed timeline read skips the blockers->ready composition" 0 \ + "issueflow: #83: skipped this pass — could not read its activity history: $GH_STUB_STDERR" \ + printf '%s\n' "$broken_flip" +check "...leaving the issue blocked" 1 "" order_wrote 83 edit +check "...with no comment posted about it" 1 "" order_wrote 83 comment +check "...and nothing claiming the flip happened" 1 "" \ + grep -qF 'blockers closed -> ready' <<<"$broken_flip" + +# -- 4. a posted nudge, then a failed TIMELINE read ------------------------- +# The comment-only half of the class: an epic nudge is staged, and the +# ruling tail's activity read fails after it. A comment is as much a +# mutation as a label — it is the thing markers exist to make idempotent. +order_fixture 84 '[{"name":"epic"},{"name":"needs-ruling"}]' \ + '## Task list + +- [x] #82' +order_board '[{"number":84}]' +order_heals 84 timeline +healthy_nudge="$(order_run)" +check "the control: a healthy pass really does nudge a completed epic" 0 \ + "issueflow: #84: completed epic nudged" printf '%s\n' "$healthy_nudge" +check "...by posting a comment" 0 "" order_wrote 84 comment +order_breaks 84 timeline +broken_nudge="$(order_run)" +check "a failed timeline read skips the epic-nudge composition" 0 \ + "issueflow: #84: skipped this pass — could not read its activity history: $GH_STUB_STDERR" \ + printf '%s\n' "$broken_nudge" +check "...and the nudge comment is never posted" 1 "" order_wrote 84 comment +check "...and nothing claims it was" 1 "" \ + grep -qF 'completed epic nudged' <<<"$broken_nudge" + +# -- the skip is still just a skip: counted, tailed, and green (D4, D6, D7) -- +check "a mutation-bearing composition that skips is still not a crash" 1 "" \ + grep -qF 'reconcile failed' <<<"$broken_flip" +check "...is still counted in the D6 tail" 0 \ + 'issueflow: 1 issue skipped this pass on an unreadable fact: #83' \ + printf '%s\n' "$broken_flip" +order_board '[{"number":83}]' +order_run >/dev/null +check "...and still leaves the job green (D7)" 0 "" test $? -eq 0 + +# -- the invariant is enforced at the source, not remembered ---------------- +# Staging only holds while every mutation goes through run(). A future call +# site reaching gh directly would reopen this hole silently, so it is pinned +# here rather than left to review — the shape lib/ruling.sh already uses for +# #50 D9. reconcile_opened_issue is deliberately exempt: it runs outside the +# per-issue subshell, under live errexit, and stages nothing (#247 D8). +mutation_calls() { + grep -nE '(^|[^_[:alnum:]])gh issue (edit|comment)' \ + "$ROOT/actions/issueflow-reconcile/issueflow-reconcile.sh" "$ROOT/lib/ruling.sh" \ + | grep -vE '^\S+:[0-9]+: *#' || true +} +# shellcheck disable=SC2016 # positional parameters belong to bash -c +check "every issue mutation on this surface goes through run()" 0 "" \ + bash -c 'while IFS= read -r line; do + [ -n "$line" ] || continue + case "$line" in *"run gh issue "*) ;; *) printf "unstaged mutation: %s\n" "$line"; exit 1 ;; esac + done <<<"$1"' _ "$(mutation_calls)" +check "...and the pin sees the call sites it is guarding" 0 "" \ + test "$(mutation_calls | wc -l)" -ge 8 + summary