diff --git a/actions/issueflow-reconcile/issueflow-reconcile.sh b/actions/issueflow-reconcile/issueflow-reconcile.sh index 5a09755..7c3bda2 100644 --- a/actions/issueflow-reconcile/issueflow-reconcile.sh +++ b/actions/issueflow-reconcile/issueflow-reconcile.sh @@ -317,6 +317,65 @@ blocked_cross_references() { # body on stdin -> qualified refs, one per line blocked_reference_records | awk -F '\t' '$1 == "CROSS" { print $2 }' | sort -u } +blocked_parse_set() { # $1 local refs, $2 cross refs -> "{#7, #12}" | "{}" + # The parse, rendered once. The comment, the marker and the log line all + # read this one string, so the three can never disagree about what the + # machine read. Both classes are shown because both are parsed: the locals + # in the numeric order blocked_references answers, then the qualified + # references blocked_cross_references answers — a cross-repo clause is as + # capable of being readable-but-wrong as a local one. + local rendered + rendered="$( + { [ -z "$1" ] || awk '{ print "#" $0 }' <<<"$1" + [ -z "${2:-}" ] || printf '%s\n' "$2" + } | awk '{ printf "%s%s", (NR > 1 ? ", " : ""), $0 } END { printf "\n" }' + )" + printf '{%s}\n' "$rendered" +} + +blocked_parse_marker() { # $1 rendered set -> the echo's idempotency marker + # Scoped to the SET's value, not to the issue and not to the sweep: the + # marker names WHAT was echoed, and blocked_parse_echo_needed decides whether + # it is still what the thread is saying. + # + # The identity is the DIGEST, not the slug beside it. Slugging is many-to-one + # — `{acme/widgets#9}` and `{acme-widgets#9}` are both parses this reconciler + # accepts, and both slug to `acme-widgets-9` — so a slug-keyed marker lets a + # changed set find the old marker and say nothing, silence in precisely the + # case the echo exists to speak about. Distinguishing `/` would close that + # pair and leave the class: `-`, `_` and `.` are legal in a qualifier token + # and all collapse the same way. The slug stays in front so a human reading + # the raw comment can still see which set it belongs to; it decides nothing. + local slug digest + slug="$(printf '%s' "$1" | tr -c '[:alnum:]' '-' | sed 's/--*/-/g; s/^-//; s/-$//')" + digest="$(printf '%s' "$1" | sha256sum | cut -c1-12)" + printf 'blockers-parsed-%s-%s\n' "${slug:-none}" "$digest" +} + +blocked_parse_echo_needed() { # $1 issue, $2 this parse's marker → 0 echo, 1 quiet + # Idempotency for the parse echo is against the LAST parse echo on the + # thread, not against any historical one. ensure_comment's any-occurrence + # grep is right for a flag like `blocked-unparseable`, whose question is + # "have I ever said this"; it is wrong for a value that changes, whose + # question is "is this still what I am saying". The difference is A -> B -> A: + # under an any-occurrence search the return to A finds A's own first echo and + # stays silent, leaving the thread's most recent echo asserting B while the + # sweep gates on A. A stale parse presented as the current one is the exact + # failure #252 exists to kill, and the third edit changed the parsed set, so + # the criterion says it speaks. + # + # Comparing markers rather than re-rendering the last set keeps the digest as + # the only identity: two sets are the same here iff blocked_parse_marker says + # so, the same rule the marker itself is built on. + local bodies last + 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")" + # The read fails closed above (#247 D1): an unreadable history skips the + # issue rather than answering "nothing echoed yet" and re-posting. + last="$(grep -o '' <<<"$bodies" | tail -n 1)" + [ "$last" != "" ] +} + blocked_decision() { # $1 local refs, $2 OPEN/CLOSED states, $3 cross-repo refs local refs="$1" states="$2" cross_refs="${3:-}" if [ -n "$cross_refs" ]; then echo FLAG_CROSS_REPO @@ -453,7 +512,7 @@ last_issue_activity() { # $1 issue, $2 created_at → epoch; non-zero if a read reconcile_issue() { 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 merged_ref_pr="" transition_marker="" transition_handled=false parsed_set="" parse_marker="" local unchecked="" remove_claimed=claimed local attention_active=true attention_suppression="" decision="$(queue_decision <<<"$ISSUE_LABELS")" @@ -556,6 +615,40 @@ The merge releases the claim; no builder owes a draft. Triage owes completion in elif has_issue_label blocked; then refs="$(blocked_references <<<"$(jq -r '.body // ""' <<<"$ISSUE_JSON")")" cross_refs="$(blocked_cross_references <<<"$(jq -r '.body // ""' <<<"$ISSUE_JSON")")" + # The parse is echoed before any verdict is derived from it (#252). The + # clause parse is exact and unforgiving, and its output was invisible: + # crew#308 silently parsed a negated "no longer blocked by #221" as a + # blocker, crew#71 spent five days as an unresolvable queue conflict, and + # crew#284's declaration had to be re-derived by hand-running the parser. + # Every one of those was found by a human running the parser, hours or + # days late. `blocked-unparseable` already catches the UNREADABLE + # declaration; this catches the readable-but-wrong one, which no flag can + # detect because the machine cannot judge what a human meant — only state + # what it read, and let the human see the divergence in one sweep. + # + # The illustrative `#9` in the body below is code-spanned for the same + # reason `blocked-unparseable` code-spans its `Blocked by #N`: an + # unbackticked `#N` in a comment this sweep posts on a cron linkifies, and + # writes a "mentioned in" event onto an unrelated issue once per echo. + parsed_set="$(blocked_parse_set "$refs" "$cross_refs")" + parse_marker="$(blocked_parse_marker "$parsed_set")" + if blocked_parse_echo_needed "$n" "$parse_marker"; then + run gh issue comment "$n" -R "$REPO" --body " +This issue's \`Blocked by\` declarations parse to: $parsed_set + +That is the exact set this sweep gates on — what the machine read, never a +judgment about whether it is what you meant. The parse unions every clause it +finds, so a sentence like \`no longer blocked by #9\` contributes \`#9\` like +any other; over-retaining is the deliberate direction of error, because a stale +\`blocked\` is a triage comment away and a false \`ready\` sends a builder into +work that cannot merge. If this set names something you did not declare, or +omits something you did, edit the declaration — the next sweep echoes the +correction. + +*Comment only: nothing on this path writes a label. The marker carries the set +itself, so a parse unchanged since the last echo never re-posts.*" >/dev/null + fi + log "#$n: blocked declarations parse to $parsed_set" states="$(reference_states <<<"$refs")" decision="$(blocked_decision "$refs" "$states" "$cross_refs")" case "$decision" in diff --git a/changelog.d/252.md b/changelog.d/252.md new file mode 100644 index 0000000..3b47994 --- /dev/null +++ b/changelog.d/252.md @@ -0,0 +1,9 @@ +### Added + +- The issue sweep now echoes an issue's parsed `Blocked by` set as a comment + whenever that set changes, so a readable-but-wrong declaration is visible in + one sweep instead of days later, when a human happens to run the parser by + hand (#252). +- The echo's marker carries the parsed set itself: an unchanged parse never + re-posts on a 15-minute cron, and a changed one always speaks. Comment-only + — no path here writes a label (#252). diff --git a/test/issueflow-reconcile.test.sh b/test/issueflow-reconcile.test.sh index 3893fe7..ec9e83b 100644 --- a/test/issueflow-reconcile.test.sh +++ b/test/issueflow-reconcile.test.sh @@ -222,6 +222,75 @@ check "cross-repo-only blocker is flagged distinctly" 0 "FLAG_CROSS_REPO" \ blocked_decision "" "" "rig#112" check "cross-repo blocker prevents false promotion when locals close" 0 "FLAG_CROSS_REPO" \ blocked_decision "9" "CLOSED" "rig#9" + +# The parse echo (#252): the machine states what it read, so a +# readable-but-wrong declaration is visible in one sweep instead of five days. +check "the rendered set names the locals in parse order" 0 "{#7, #12}" \ + blocked_parse_set "$(printf '7\n12\n')" "" +check "a single blocker still renders as a set" 0 "{#12}" blocked_parse_set "12" "" +check "an empty parse renders as the empty set" 0 "{}" blocked_parse_set "" "" +check "cross-repo references are echoed beside the locals" 0 "{#12, rig#9}" \ + blocked_parse_set "12" "rig#9" +check "a cross-repo-only parse is echoed too" 0 "{heavy-duty/box#9}" \ + blocked_parse_set "" "heavy-duty/box#9" +# crew#308: a *negated* marker phrase unions as the thing it denies, and the +# silent result was a set nobody saw until a human ran the parser. Echoed, the +# union is visible in the thread that contains the declaration. +echo_308="$(blocked_parse_set \ + "$(blocked_references <<<'Blocked by #162, #265. It is no longer blocked by #221.')" \ + "$(blocked_cross_references <<<'Blocked by #162, #265. It is no longer blocked by #221.')")" +check "the #308 shape echoes the negation-unioned blocker verbatim" 0 "" test \ + "$echo_308" = "{#162, #221, #265}" +# The marker is scoped to the SET's value — the whole idempotency contract. +# Mutation proof, both directions: same set must reuse its marker (or a +# 15-minute cron repeats itself forever), different set must not (or a +# misparse is echoed under a marker the thread already carries, and stays +# invisible — exactly the failure this change exists to close). +check "an unchanged set reuses its marker" 0 "" test \ + "$(blocked_parse_marker '{#7, #12}')" = "$(blocked_parse_marker '{#7, #12}')" +check "a changed set takes a different marker" 1 "" test \ + "$(blocked_parse_marker '{#7, #12}')" = "$(blocked_parse_marker '{#7, #12, #19}')" +check "the empty set has a marker of its own" 0 "blockers-parsed-none-44136fa355b3" \ + blocked_parse_marker "{}" +check "the marker survives a cross-repo reference's punctuation" 0 \ + "blockers-parsed-12-heavy-duty-box-9-c8e36fe3b793" \ + blocked_parse_marker "{#12, heavy-duty/box#9}" +# The readable half of the marker is many-to-one and must not be the identity. +# Every pair below is two reference tokens `issue_references` classifies, that +# slug identically; a slug-keyed marker made the second one find the first +# one's marker and post nothing — silence in the one case the echo exists to +# speak about. Distinguishing `/` alone would close the first pair and leave +# the rest, so the digest of the whole rendered set is what decides. +# +# Classifies, not parses: three of the four are reachable as a declared set — +# `{acme.widgets#9}` is not, because the clause parser stops at the `.` and +# `blocked_reference_records` never hands that token through, though +# `issue_references` does answer CROSS for it. It stays in the family because +# the marker's contract is over the tokens the classifier admits, not over the +# subset today's clause parser happens to reach; the class proof does not rest +# on it either way, since `{acme-widgets#9}` vs `{acme_widgets#9}` is reachable +# on both sides and reds the cheap fix on its own. +check "the slug alone cannot separate a qualified ref from a hyphenated one" 0 "" \ + test "$(printf '%s' '{acme/widgets#9}' | tr -c '[:alnum:]' '-' | sed 's/--*/-/g; s/^-//; s/-$//')" \ + = "$(printf '%s' '{acme-widgets#9}' | tr -c '[:alnum:]' '-' | sed 's/--*/-/g; s/^-//; s/-$//')" +# Pairwise, and deliberately so. Anchoring every pair on the `/` spelling +# would pass under a fix that only taught the slug about `/` — and that fix +# leaves `{acme-widgets#9}`, `{acme_widgets#9}` and `{acme.widgets#9}` sharing +# one marker. The contract is that no two distinct parses collide, so the test +# is every pair, not every pair through one representative. +marker_family=( + '{acme/widgets#9}' '{acme-widgets#9}' '{acme_widgets#9}' '{acme.widgets#9}' +) +for left in "${marker_family[@]}"; do + for right in "${marker_family[@]}"; do + [ "$left" != "$right" ] || continue + check "...but the marker does: $left vs $right" 1 "" test \ + "$(blocked_parse_marker "$left")" = "$(blocked_parse_marker "$right")" + done +done +check "the qualifier's punctuation reaches the marker's identity" 1 "" test \ + "$(blocked_parse_marker '{#12, heavy-duty/box#9}')" \ + = "$(blocked_parse_marker '{#12, heavy-duty-box#9}')" # shellcheck disable=SC2016 # expansions belong to the generated fake gh printf '%s\n' \ '#!/usr/bin/env bash' \ @@ -437,7 +506,15 @@ issue_probe 65 $'claimed\nattention' 1 true >/dev/null check "assigned attention under claimed is healthy" 1 "" test -f "$TMP/posted-65" attention_episode 66 "$(iso_at $((INOW - 60)))" issue_probe 66 $'blocked\nattention' 1 false "" 'Blocked by #999' >/dev/null -check "assigned attention under blocked is healthy" 1 "" test -f "$TMP/posted-66" +# A `blocked` issue now always carries one comment — the parse echo (#252) — +# so "healthy" can no longer be spelled "no comment file at all". It is spelled +# the way this section's other cases already spell it: no attention diagnostic. +# Both halves are pinned, so the case still fails if an attention comment +# appears beside the echo, or if a second comment of any kind does. +check "assigned attention under blocked is healthy" 1 "" \ + grep -qF '" "$TMP/posted-35" +# AC-1's other input, and it is not the sweep above. A re-sweep of a +# BYTE-IDENTICAL body is quiet under both spellings of the decision — the one +# that keys on the parse and the one that keys on the body — so it cannot tell +# them apart. Only an edit that changes the prose and preserves the parse can: +# the refs are reordered and sentences are added on either side, and the set is +# still {#90, #91}. What this pins is that the marker is a function of the +# PARSE and not of the prose around it, which is the property the echo's whole +# idempotency rests on and which nothing else in the suite states. +preserved_edits_before="$(wc -l <"$TMP/issue-edits")" +issue_probe 35 blocked 1 false "" \ + "Some new prose here. Blocked by #91, #90. And more text." >/dev/null +check "a body edit that preserves the parse draws nothing" 0 "1" \ + grep -cF "" "$TMP/posted-35" +check "...and adds no echo under any other marker either" 0 "1" \ + grep -cF '" "$TMP/posted-35" +check "...naming the new set" 0 "" \ + grep -qF 'parse to: {#90, #91, #92}' "$TMP/posted-35" +check "...and saying so in the sweep log" 0 \ + "issueflow: #35: blocked declarations parse to {#90, #91, #92}" \ + printf '%s\n' "$changed_echo" +check "...and leaving the first echo alone" 0 "1" \ + grep -cF "" "$TMP/posted-35" +# shellcheck disable=SC2016 # positional parameters belong to bash -c +check "no label write comes from the echo path" 0 "" \ + bash -c 'test "$1" -eq "$(wc -l <"$2")"' _ "$echo_edits_before" "$TMP/issue-edits" + +# crew#308, replayed through the sweep: the declaration denies #221 and the +# parse unions it anyway. Nobody saw that set for as long as it stayed inside +# the machine; the echo puts it in the thread that contains the declaration. +printf '{"state":"open"}\n' >"$TMP/repos_owner_repo_issues_162.json" +printf '{"state":"open"}\n' >"$TMP/repos_owner_repo_issues_221.json" +printf '{"state":"open"}\n' >"$TMP/repos_owner_repo_issues_265.json" +printf '[]\n' >"$(cfix 36)" +issue_probe 36 blocked 1 false "" \ + 'Blocked by #162, #265. It is no longer blocked by #221.' >/dev/null +check "the #308 misparse is echoed verbatim, denial and all" 0 "" \ + grep -qF 'parse to: {#162, #221, #265}' "$TMP/posted-36" + +# The empty parse says so, and the flag that catches the UNREADABLE +# declaration is untouched beside it: one comment states what was read, the +# other states that nothing was. +printf '[]\n' >"$(cfix 37)" +issue_probe 37 blocked 1 false "" 'No declaration anywhere in this body.' >/dev/null +check "an empty parse is echoed as the empty set" 0 "" \ + grep -qF 'parse to: {}' "$TMP/posted-37" +check "...and blocked-unparseable still fires beside it" 0 "" \ + grep -qF '' "$TMP/posted-37" + +# The collision the round found, replayed through the sweep: two declarations +# whose parsed sets differ but whose slugs do not. Keyed on the slug, the +# second edit found the first echo's marker and posted nothing — the machine +# silently gating on `acme-widgets#9` while the thread said `acme/widgets#9`, +# which is the readable-but-wrong shape this whole change exists to surface. +# Asserted end-to-end, so it is the second echo landing that is observed. +printf '[]\n' >"$(cfix 38)" +issue_probe 38 blocked 1 false "" 'Blocked by acme/widgets#9.' >/dev/null +check "a qualified cross-repo declaration is echoed" 0 "1" \ + grep -cF 'parse to: {acme/widgets#9}' "$TMP/posted-38" +collision_edits_before="$(wc -l <"$TMP/issue-edits")" +issue_probe 38 blocked 1 false "" 'Blocked by acme-widgets#9.' >/dev/null +check "a slug-colliding edit still draws its own echo" 0 "1" \ + grep -cF 'parse to: {acme-widgets#9}' "$TMP/posted-38" +check "...under a marker of its own" 0 "1" \ + grep -cF "" "$TMP/posted-38" +check "...leaving the colliding first echo alone" 0 "1" \ + grep -cF "" "$TMP/posted-38" +# The cross-repo flag is marker-constant across both parses, so it stays at one +# while the echo moves: what spoke on the second sweep was the changed set. +check "...and not re-flagging cross-repo, which did not change" 0 "1" \ + grep -cF '' "$TMP/posted-38" +# shellcheck disable=SC2016 # positional parameters belong to bash -c +check "no label write comes from the colliding-edit path either" 0 "" \ + bash -c 'test "$1" -eq "$(wc -l <"$2")"' _ "$collision_edits_before" "$TMP/issue-edits" + +# A -> B -> A. The marker names the SET, so the return to A is a marker this +# thread has carried before; the question the echo has to answer is not "have I +# ever said this" but "is this still what I am saying". Searching the whole +# history answers the first, and the return went silent while the thread's +# newest echo asserted B and the sweep gated on A — a stale parse presented as +# the current one, which is the readable-but-wrong shape #252 exists to kill. +# All four sweeps are driven, because the bug is only visible as a sequence. +printf '[]\n' >"$(cfix 52)" +issue_probe 52 blocked 1 false "" 'Blocked by #90, #91.' >/dev/null +issue_probe 52 blocked 1 false "" 'Blocked by #90, #91, #92.' >/dev/null +return_edits_before="$(wc -l <"$TMP/issue-edits")" +issue_probe 52 blocked 1 false "" 'Blocked by #90, #91.' >/dev/null +check "a set edited back to a previously echoed one speaks again" 0 "3" \ + grep -cF '" "$TMP/posted-52" +# The assertion the silence used to fail: it is the NEWEST echo that has to +# name what the sweep gates on, not merely some echo somewhere in the thread. +# shellcheck disable=SC2016 # positional parameters belong to bash -c +check "...leaving the newest echo naming the set the sweep now gates on" 0 \ + "parse to: {#90, #91}" \ + bash -c 'grep -o "parse to: {[^}]*}" "$1" | tail -n 1' _ "$TMP/posted-52" +issue_probe 52 blocked 1 false "" 'Blocked by #90, #91.' >/dev/null +check "an unchanged sweep after the return still draws nothing" 0 "3" \ + grep -cF '