diff --git a/actions/issueflow-reconcile/issueflow-reconcile.sh b/actions/issueflow-reconcile/issueflow-reconcile.sh index 1c15f8b..40292b8 100644 --- a/actions/issueflow-reconcile/issueflow-reconcile.sh +++ b/actions/issueflow-reconcile/issueflow-reconcile.sh @@ -491,7 +491,11 @@ offsite_timeline() { # unreadable timelines are deliberately silent gh api --paginate "repos/$REPO/issues/$1/timeline" 2>/dev/null || return 1 } -last_issue_activity() { # $1 issue, $2 created_at → epoch; non-zero if a read failed +issue_activity_at() { # $1 issue, $2 created_at, $3 with-assignment|comments-only + # One body, two clocks over it — a second activity computation is the drift + # the reuse exists to prevent, and the two callers below are the whole + # difference between them. + # # 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 @@ -499,19 +503,42 @@ last_issue_activity() { # $1 issue, $2 created_at → epoch; non-zero if a read # 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 + local n="$1" created="$2" mode="$3" 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 + if [ "$mode" = with-assignment ]; then + timeline="$(gh api --paginate "repos/$REPO/issues/$n/timeline" \ + --jq '.[] | select(.event == "assigned") | .created_at')" || return 1 + fi latest="$(printf '%s\n%s\n%s\n' "$created" "$comments" "$timeline" | sort | tail -n1)" date -d "$latest" +%s } +last_issue_activity() { # $1 issue, $2 created_at → epoch; non-zero if a read failed + # The claim clock, and the ruling clock with it. Assignment is the claim + # itself. Ignoring it would let an old issue be reclaimed in the seconds + # between assignment and its required draft PR. + issue_activity_at "$1" "$2" with-assignment +} + +last_issue_comment_activity() { # $1 issue, $2 created_at → epoch; non-zero on a failed read + # The evidence nudge's clock (#254). Same computation, one input fewer, and + # the input it drops is the one that would starve the criterion: on + # `post-merge` there is no claim for an assignment to protect, and an + # assignee there is the invalid composition the `post-merge-assigned` flag + # reports. Counting it would let a broken board buy the item another 7 days + # of silence — the failure direction of #254 taken backwards. + # + # A comment the sweep itself wrote is still activity here, deliberately: + # the nudge carries no marker, so its own comment is what rate-limits it, + # and no machine comment can be exempted without exempting that one too. + # Reading authorship back into the clock would mean a body read this issue + # forbids. + issue_activity_at "$1" "$2" comments-only +} + reconcile_issue() { - local n="$1" decision refs cross_refs states age created assignees open_pr=false label owners + local n="$1" decision refs cross_refs states age evidence_age created assignees open_pr=false label owners local merged_ref_pr="" transition_marker="" transition_handled=false parsed_set="" parse_marker="" local unchecked="" remove_claimed=claimed local attention_active=true attention_suppression="" @@ -611,9 +638,26 @@ The merge releases the claim; no builder owes a draft. Triage owes completion in # would let the assigned-flag comment silence the nudge for another 7 # days — the same self-silencing the ruling nudge avoids by reading its # facts once, at the top of the pass. + # + # Its own variable, not `age`: the ruling block below reuses `age` when + # it is already set, and the evidence clock is deliberately narrower than + # the ruling clock. Leaking it there would silently change what a ruling + # nudge means depending on which queue label the issue sits under. created="$(jq -r '.created_at' <<<"$ISSUE_JSON")" - guarded_read age last_issue_activity "$n" "$created" \ + guarded_read evidence_age last_issue_comment_activity "$n" "$created" \ || skip_issue "$n" "could not read its activity history: $(read_failure_reason "$READ_FAILURE_STDERR")" + # The ruling clock is read HERE, not in the ruling block, for the same + # reason the evidence clock is: that block reads only when `age` is + # unset, and by the time it runs this branch may have posted the + # evidence nudge — so its read would date the issue by this sweep's own + # comment and silence the ruling nudge. Both waits are answered from + # facts that predate anything this pass writes. The cost is one extra + # comments read on `post-merge` + `needs-ruling`, and only there: an + # ordinary `post-merge` issue reads once. + if has_issue_label needs-ruling; then + guarded_read age last_issue_activity "$n" "$created" \ + || skip_issue "$n" "could not read its activity history: $(read_failure_reason "$READ_FAILURE_STDERR")" + fi if [ "$assignees" -gt 0 ] || has_issue_label attention; then ensure_comment "$n" post-merge-assigned \ 'This `post-merge` issue has an assignee or `attention`. The sweep will not undo hand-set intent; triage must clear the invalid composition or move the issue back into buildable queue state.' @@ -636,8 +680,8 @@ The merge releases the claim; no builder owes a draft. Triage owes completion in # mandatory config — `load_issueflow_config` refuses to run without it — # so there is nothing to fall back to, and a silent fallback is exactly # how the wrong addressee comes back. - if [ "$(ruling_nudge_decision "$NOW" "$age")" = NUDGE ]; then - local quiet_days=$(((NOW - age) / 86400)) + if [ "$(ruling_nudge_decision "$NOW" "$evidence_age")" = NUDGE ]; then + local quiet_days=$(((NOW - evidence_age) / 86400)) run gh issue comment "$n" -R "$REPO" --body "@${TRIAGE_ACTORS[0]} — this \`post-merge\` item has had no activity for ${quiet_days} days: https://github.com/$REPO/issues/$n Its wake evidence is still owed. \`post-merge\` means the merge landed and diff --git a/test/issueflow-reconcile.test.sh b/test/issueflow-reconcile.test.sh index eec84b1..7c9fde9 100644 --- a/test/issueflow-reconcile.test.sh +++ b/test/issueflow-reconcile.test.sh @@ -637,6 +637,14 @@ check "...only the evidence nudge speaks" 0 "1" \ grep -c -- '^----$' "$TMP/posted-36" printf '[]\n' >"$(cfix 37)" +# The live shape of an assigned `post-merge` issue: the assignee in the issue +# payload AND the `assigned` event that put it there in the timeline. With an +# empty timeline this fixture could not see the defect it exists to guard — +# the evidence clock counting that hour-old assignment as activity and +# silencing the nudge for another 7 days, on the one board state where an +# assignee is itself the bug being reported. +jq -n --arg at "$(iso_at $((INOW - 3600)))" \ + '[{"event":"assigned","created_at":$at}]' >"$(tfix 37)" issue_probe 37 post-merge 1 >/dev/null check "assigned post-merge is flagged" 0 "" \ grep -qF '' "$TMP/posted-37" @@ -711,6 +719,40 @@ fresh="$(issue_probe 83 post-merge 0)" check "an hour-old comment does reset it" 1 "" \ grep -q 'post-merge evidence nudge' <<<"$fresh" +# Neither is an assignment. That event is the *claim* clock's activity fact — +# 48 hours of silence must not include the seconds between a claim and its +# required draft PR — and `post-merge` has no claim for it to protect: an +# assignee here is the invalid composition the flag above reports. Counting +# it would let a broken board buy the item another 7 days of quiet, which is +# this issue's failure direction taken backwards. No current assignee on this +# probe, so nothing stands between the clock rule and the nudge. +quiet_comment 93 $((8 * 86400)) +jq -n --arg at "$(iso_at $((INOW - 3600)))" \ + '[{"event":"assigned","created_at":$at},{"event":"unassigned","created_at":$at}]' >"$(tfix 93)" +assigned_clock="$(issue_probe 93 post-merge 0)" +check "an hour-old assignment does not reset the evidence clock either" 0 "" \ + grep -q 'post-merge evidence nudge' <<<"$assigned_clock" + +# The two clocks over the one computation, asserted directly rather than +# through a probe: same fixture, one input's difference, and the reclaim +# clock is pinned unmoved by the split. +jq -n --arg at "$(iso_at $((INOW - 5 * 86400)))" \ + '[{"user":{"login":"triage-one"},"created_at":$at,"html_url":"https://x/c95","body":"evidence pending"}]' \ + >"$(cfix 95)" +jq -n --arg at "$(iso_at $((INOW - 3600)))" \ + '[{"event":"assigned","created_at":$at}]' >"$(tfix 95)" +two_clocks="$( (REPO=owner/repo; gh() { issue_stub_gh "$@"; } + printf '%s %s\n' \ + "$(last_issue_activity 95 "$(iso_at $((INOW - 10 * 86400)))")" \ + "$(last_issue_comment_activity 95 "$(iso_at $((INOW - 10 * 86400)))")") )" +check "the claim clock counts the assignment, the evidence clock the comment" 0 \ + "$((INOW - 3600)) $((INOW - 5 * 86400))" printf '%s\n' "$two_clocks" +# One body, two callers: a second activity computation is the drift the +# reuse exists to prevent, so the timeline read has exactly one spelling. +check "the timeline read is not respelled for the evidence clock" 0 "1" \ + grep -c 'issues/\$n/timeline' \ + "$ROOT/actions/issueflow-reconcile/issueflow-reconcile.sh" + # Both waits are quiet, both are owed, and to different parties: suppressing # one because the other spoke is a starved criterion, which is the failure # this nudge exists to remove. @@ -766,7 +808,7 @@ check "the 7-day rule is not respelled in the sweep" 1 "" \ "$ROOT/actions/issueflow-reconcile/issueflow-reconcile.sh" # shellcheck disable=SC2016 # the call site is asserted as a literal check "...it is reused from lib/ruling.sh" 0 "" \ - grep -qF 'ruling_nudge_decision "$NOW" "$age"' \ + grep -qF 'ruling_nudge_decision "$NOW" "$evidence_age"' \ "$ROOT/actions/issueflow-reconcile/issueflow-reconcile.sh" # `post-merge` is the one queue state whose whole meaning is that the machine @@ -1188,6 +1230,28 @@ 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 quiet diagnostic: a 504 on a post-merge activity read (#254) -------- +# The guarded read is unconditional at the top of the branch, so a +# `post-merge` issue can be skipped where before this change it never could — +# and the assigned flag, which needed no read at all, goes quiet with it. +# That is #247 D1's direction (a whole pass or none of it, never a verdict +# derived from a read that did not answer) and the trade `claimed`, `blocked` +# and `needs-ruling` already make. It is still a new way for that diagnostic +# to fall silent, so it is pinned here rather than left to inspection. +jq -n --arg at "$(iso_at $((INOW - 10 * 86400)))" \ + '[{"user":{"login":"triage-one"},"created_at":$at,"html_url":"https://x/c94","body":"evidence pending"}]' \ + >"$(cfix 94)" +printf '%s\n' "$GH_STUB_ERROR_BODY" >"$(cfix 94).http-error" +post_merge_skip_edits="$(wc -l <"$TMP/issue-edits")" +check "a 504 on a post-merge activity read skips the issue" \ + 3 "#94: skipped this pass — could not read its activity history: $GH_STUB_STDERR" \ + issue_probe 94 post-merge 1 +check "...so neither the nudge nor the assigned flag speaks" 1 "" test -f "$TMP/posted-94" +# shellcheck disable=SC2016 # positional parameters belong to bash -c +check "...and the skipped pass edits nothing" 0 "" \ + bash -c 'test "$1" -eq "$(wc -l <"$2")"' _ \ + "$post_merge_skip_edits" "$TMP/issue-edits" + # -- 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.