diff --git a/LABELS.md b/LABELS.md index 0f6acd4..406455c 100644 --- a/LABELS.md +++ b/LABELS.md @@ -68,7 +68,16 @@ assignee, and comments with the remaining criteria verbatim. The comment says that the claim is released and that triage owes a follow-up naming the owner and wake condition for completion. Triage writes that full transition comment in the same tick when it or the operator makes the move by hand. The sweep -never reclaims `post-merge`: weeks of quiet can be the state working. +never reclaims `post-merge`: weeks of quiet can be the state working. It does +make the quiet visible — after 7 days with no comment on the issue, the sweep +posts one nudge naming the triage actor, saying the wake evidence is owed and +linking the item. Only a comment resets that clock: label churn does not, and +neither does an assignment, which is the claim clock's fact and on this queue +state is the invalid composition flagged below. Which criterion starved is +prose the machine never judges; the link is the payload. Like the ruling nudge +it carries no idempotency marker on purpose — the comment is itself activity, +so the rule self-rate-limits to one nudge per 7 quiet days — and it writes no +label. `post-merge` never composes with `blocked`; the transition comment carries the wait. It never composes with `attention`, because releasing the claim clears diff --git a/actions/issueflow-reconcile/issueflow-reconcile.sh b/actions/issueflow-reconcile/issueflow-reconcile.sh index 7c3bda2..ab331bd 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="" @@ -606,12 +633,66 @@ The merge releases the claim; no builder owes a draft. Triage owes completion in fi elif has_issue_label post-merge; then assignees="$(jq '.assignees | length' <<<"$ISSUE_JSON")" + # The evidence nudge's clock is read BEFORE any comment this branch + # posts. `ensure_comment` below is itself activity, so reading after it + # 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 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.' log "#$n: assigned or attention-bearing post-merge issue flagged" fi attention_suppression=post-merge-assigned + # ---- the post-merge evidence nudge (#254), the ruling nudge's twin ---- + # A `post-merge` item waits on named evidence with a named owner, and + # nothing nudged when the wait went quiet: crew#181's real-host criterion + # starved four separate times across two releases, crew#240/#264 sat + # until an operator happened to run the right read. Same 7-day rule, same + # constant, same no-marker property — `ruling_nudge_decision` is the one + # spelling of all three (lib/ruling.sh), and a second `7 * 24 * 3600` + # here is the drift that file exists to prevent. + # + # The addressee is the triage actor, not `HUMAN_REVIEWER`: `post-merge` + # is triage's completion queue by contract (TRIAGE.md), so a starving + # wake condition is triage's to answer, and routing it to the operator + # asks the wrong party for a move it does not owe. `triage-actors=` is + # 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" "$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 comment for ${quiet_days} days: https://github.com/$REPO/issues/$n + +Its wake evidence is still owed. \`post-merge\` means the merge landed and +triage owns completion — judge the remaining criteria against the evidence +and close the issue, or say what is still outstanding and who owes it. The +sweep names no criterion: which one starved is prose, and the machine never +judges prose (the link is the payload). + +*This nudge is comment-only and carries no idempotency marker on purpose: the comment itself is activity, so posting it resets the 7-day window and the rule self-rate-limits to one nudge per 7 quiet days. Do not add a marker.*" >/dev/null + log "#$n: post-merge evidence nudge (${quiet_days}d quiet — triage owes the wake evidence)" + fi elif has_issue_label blocked; then refs="$(blocked_references <<<"$(jq -r '.body // ""' <<<"$ISSUE_JSON")")" cross_refs="$(blocked_cross_references <<<"$(jq -r '.body // ""' <<<"$ISSUE_JSON")")" diff --git a/changelog.d/254.md b/changelog.d/254.md new file mode 100644 index 0000000..92ca2d5 --- /dev/null +++ b/changelog.d/254.md @@ -0,0 +1,16 @@ +### Added + +- A `post-merge` item with no comment for 7 days now draws one nudge from the + issue sweep: the wake evidence is owed. A starving criterion used to be + found only when someone happened to run the right read (#254). +- Label churn does not reset that clock, and neither does an assignment: on + `post-merge` an assignee is an invalid composition, not activity, and it + must not buy the item another 7 days of silence (#254). +- The nudge names the triage actor from `triage-actors=`, not the human + reviewer: `post-merge` is triage's completion queue, so the starved wake + condition is triage's to answer (#254). +- It links the item and parses nothing from the body — which criterion + starved is prose, and the machine never judges prose (#254). +- Like the ruling nudge it carries no idempotency marker on purpose: the + comment is itself activity, so the rule self-rate-limits to one nudge per 7 + quiet days. Comment-only — no path here writes a label (#254). diff --git a/test/issueflow-reconcile.test.sh b/test/issueflow-reconcile.test.sh index ec9e83b..a1b9486 100644 --- a/test/issueflow-reconcile.test.sh +++ b/test/issueflow-reconcile.test.sh @@ -431,7 +431,11 @@ issue_probe() { # $1 issue, $2 labels, $3 assignees, $4 false|closing|refs, $5 m local assignees="${3:-1}" open_pr="${4:-false}" merged_ref_prs="${5:-}" local body="${6:-}" assignee_json='[]' open_pr_records="" spec pr merged_at [ "$assignees" -eq 0 ] || assignee_json='[{"login":"owner-bot"}]' - REPO=owner/repo NOW="$INOW" + # `PROBE_NOW` moves the sweep's clock without moving the fixtures — the + # only way to prove a rule that self-rate-limits on its own comment's + # timestamp (#254): sweep, then sweep again a day later and watch the + # nudge stay silent because the comment it posted is now the activity. + REPO=owner/repo NOW="${PROBE_NOW:-$INOW}" ISSUE_LABELS="$2" ISSUE_JSON="$(jq -n --arg at "$(iso_at $((INOW - 10 * 86400)))" \ --argjson assignees "$assignee_json" --arg body "$body" \ @@ -517,6 +521,13 @@ check "...drawing the #252 parse echo and nothing else" 0 "1" \ grep -c -- '^----$' "$TMP/posted-66" attention_episode 67 "$(iso_at $((INOW - 60)))" +# Recent activity keeps the evidence nudge (#254) off this probe: it is a +# precedence case, and "exactly one comment" is the assertion doing the work. +# The nudge's own coexistence with the post-merge diagnostic is pinned in its +# section below, on a probe that is quiet on purpose. +jq -n --arg at "$(iso_at $((INOW - 60)))" \ + '[{"user":{"login":"triage-one"},"created_at":$at,"html_url":"https://x/c67","body":"still waiting on the tag"}]' \ + >"$(cfix 67)" post_merge_attention="$(issue_probe 67 $'post-merge\nattention' 0)" check "post-merge precedence leaves exactly its existing comment" 0 "1" \ grep -c -- '^----$' "$TMP/posted-67" @@ -617,14 +628,196 @@ printf '[]\n' >"$(cfix 36)" post_merge_quiet="$(issue_probe 36 post-merge 0)" check "quiet unassigned post-merge work is not reclaimed" 1 "" \ grep -q 'reclaimed' <<<"$post_merge_quiet" -check "...and causes no comment or edit" 1 "" test -f "$TMP/posted-36" +# The quiet itself is now visible (#254) — this probe is 10 days old with no +# activity, so it draws the evidence nudge and nothing else. It used to +# assert no comment at all; that assertion described the starvation this +# issue exists to end, and the edit half of it is what still matters. +check "...and causes no edit" 1 "" grep -qF -- 'issue edit 36' "$TMP/issue-edits" +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" check "...and the hand-assignment is not repaired" 1 "" \ grep -qF -- 'issue edit 37' "$TMP/issue-edits" +# The flag and the nudge answer different questions — a board bug and a +# starved wake condition — so neither suppresses the other (#254). +check "...and the evidence nudge rides beside it, neither suppressed" 0 "2" \ + grep -c -- '^----$' "$TMP/posted-37" + +# -- the post-merge evidence nudge (#254), the ruling nudge's twin ---------- +# The ruling nudge solved "a wait goes quiet and nobody is told" for +# `needs-ruling`; `post-merge` had no equivalent, and crew#181's real-host +# criterion starved four times across two releases for want of one. Same +# 7-day constant (`ruling_nudge_decision`, reused not mirrored), same +# deliberate absence of an idempotency marker, and — unlike the ruling +# nudge — addressed to the triage actor, because `post-merge` is triage's +# completion queue and the operator owes nothing here (#254 D1). +nudge_edits_before="$(wc -l <"$TMP/issue-edits")" + +quiet_comment() { # $1 issue, $2 seconds of quiet — one ordinary comment, then silence + jq -n --arg at "$(iso_at $((INOW - $2)))" \ + '[{"user":{"login":"triage-one"},"created_at":$at,"html_url":"https://x/c","body":"evidence pending"}]' \ + >"$(cfix "$1")" + printf '[]\n' >"$(tfix "$1")" +} + +quiet_comment 80 $((8 * 86400)) +nudged="$(issue_probe 80 post-merge 0)" +check "8 quiet days on a post-merge item draws the evidence nudge" 0 "" \ + grep -q 'post-merge evidence nudge' <<<"$nudged" +# shellcheck disable=SC2016 # expansions belong to the isolated bash -c process +check "...addressed to the triage actor, never the human reviewer" 0 "" \ + bash -c 'grep -qF "@triage-one" "$1" && ! grep -qF "@danmt" "$1"' _ "$TMP/posted-80" +check "...with the issue link as the payload" 0 "" \ + grep -qF 'https://github.com/owner/repo/issues/80' "$TMP/posted-80" +check "...carrying the do-not-add-a-marker warning in the comment" 0 "" \ + grep -qF 'Do not add a marker.' "$TMP/posted-80" +# Asserted directly, not merely omitted: a marker would turn "once per 7 +# quiet days" into "once per issue, forever" — the exact "fix" lib/ruling.sh's +# header records as the thing that breaks this rule. +check "...and no idempotency marker on the path" 1 "" \ + grep -qF '\nrung"}, + {"user":{"login":"sweep-bot"},"created_at":$r24,"html_url":"https://x/r24","body":"\nrung"}]' \ + >"$(cfix 84)" +both="$(issue_probe 84 $'post-merge\nneeds-ruling' 0)" +check "a quiet post-merge item under a pending ruling nudges both waits" 0 "" \ + grep -q 'post-merge evidence nudge' <<<"$both" +check "...and the ruling nudge is not suppressed by it" 0 "" \ + grep -q 'ruling nudge' <<<"$both" +# shellcheck disable=SC2016 # expansions belong to the isolated bash -c process +check "...each addressing its own party" 0 "" \ + bash -c 'grep -qF "@triage-one" "$1" && grep -qF "@danmt" "$1"' _ "$TMP/posted-84" + +# Every other queue state: the nudge is `post-merge`'s alone. Each is equally +# quiet, and `claimed` carries an open PR so its own reclaim clock — the one +# other 10-day rule on this path — stays out of the way. +non_post_merge=(85:ready:0:false 86:claimed:1:true 87:blocked:0:false 88:epic:0:false 89:needs-triage:0:false) +for spec in "${non_post_merge[@]}"; do + IFS=: read -r n state probe_assignees probe_pr <<<"$spec" + printf '[]\n' >"$(cfix "$n")" + printf '[]\n' >"$(tfix "$n")" + check "a 10-day-quiet $state issue draws no evidence nudge" 1 "" \ + grep -q 'post-merge evidence nudge' \ + <<<"$(issue_probe "$n" "$state" "$probe_assignees" "$probe_pr")" +done + +# The machine never judges prose: which criterion starved is not a fact the +# sweep reads, so a body it could not parse if it tried still nudges. +quiet_comment 90 $((8 * 86400)) +unparseable_body="$(issue_probe 90 post-merge 0 false "" '¯\_(ツ)_/¯ wake: ask danmt sometime')" +check "an unparseable body still nudges — the link is the payload" 0 "" \ + grep -q 'post-merge evidence nudge' <<<"$unparseable_body" +check "...and the nudge quotes none of it" 1 "" \ + grep -qF 'ask danmt sometime' "$TMP/posted-90" + +# One spelling of the 7-day rule. `lib/ruling.sh` exists because this family +# already paid for two copies of a constant; a second one here is the drift, +# and it is cheap to pin at the grep level. +# Code lines only: the branch's comment names the constant it must not +# respell, which is the sentence a future reader needs and not a second copy. +check "the 7-day rule is not respelled in the sweep" 1 "" \ + grep -nE '^[^#]*(7 \* 24 \* 3600|604800)' \ + "$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" "$evidence_age"' \ + "$ROOT/actions/issueflow-reconcile/issueflow-reconcile.sh" + +# `post-merge` is the one queue state whose whole meaning is that the machine +# owes nothing (LABELS.md: the sweep never reclaims it). This issue makes the +# quiet visible; it must never make it actionable. +# shellcheck disable=SC2016 # positional parameter belongs to the isolated shell +check "the evidence-nudge probes perform no issue edits" 0 "$nudge_edits_before" \ + bash -c 'wc -l <"$1"' _ "$TMP/issue-edits" # -- non-triggers stay byte-for-byte outside the transition ------------------ recent_timeline() { @@ -1038,6 +1231,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.