forked from heavy-duty/ceremony
fix(issueflow): the evidence clock is not the claim clock
The nudge rode `last_issue_activity`, which counts `assigned` timeline events because assignment is the claim the 48-hour reclaim protects. `post-merge` has no claim: an assignee there is the invalid composition the flag beside it reports, so counting the assignment let a broken board buy the item another 7 days of silence — this issue's failure direction taken backwards. One computation, two clocks over it: `issue_activity_at` is the body, `last_issue_activity` keeps the reclaim and ruling clocks byte-identical, and `last_issue_comment_activity` is the evidence clock. Both clocks are read before this branch posts anything, the ruling one included — read after, it would date the issue by the evidence nudge's own comment and silence the ruling nudge, which is the self-silencing the branch already guarded against in the other direction. Refs #254
This commit is contained in:
parent
37d138fecf
commit
d4a82707f2
2 changed files with 119 additions and 11 deletions
|
|
@ -491,7 +491,11 @@ offsite_timeline() { # unreadable timelines are deliberately silent
|
||||||
gh api --paginate "repos/$REPO/issues/$1/timeline" 2>/dev/null || return 1
|
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
|
# 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
|
# (#247 D1). Swallowed, the comments read falls back to `created_at`, and a
|
||||||
# `claimed` issue created months ago but commented on seconds earlier is
|
# `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.
|
# 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
|
# gh's stderr is left to flow to this function's own, where the caller's
|
||||||
# guarded_read captures it for the reason line.
|
# 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')" \
|
comments="$(gh api --paginate "repos/$REPO/issues/$n/comments" --jq '.[].created_at')" \
|
||||||
|| return 1
|
|| return 1
|
||||||
# Assignment is the claim itself. Ignoring it would let an old issue be
|
if [ "$mode" = with-assignment ]; then
|
||||||
# reclaimed in the seconds between assignment and its required draft PR.
|
timeline="$(gh api --paginate "repos/$REPO/issues/$n/timeline" \
|
||||||
timeline="$(gh api --paginate "repos/$REPO/issues/$n/timeline" \
|
--jq '.[] | select(.event == "assigned") | .created_at')" || return 1
|
||||||
--jq '.[] | select(.event == "assigned") | .created_at')" || return 1
|
fi
|
||||||
latest="$(printf '%s\n%s\n%s\n' "$created" "$comments" "$timeline" | sort | tail -n1)"
|
latest="$(printf '%s\n%s\n%s\n' "$created" "$comments" "$timeline" | sort | tail -n1)"
|
||||||
date -d "$latest" +%s
|
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() {
|
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 merged_ref_pr="" transition_marker="" transition_handled=false parsed_set="" parse_marker=""
|
||||||
local unchecked="" remove_claimed=claimed
|
local unchecked="" remove_claimed=claimed
|
||||||
local attention_active=true attention_suppression=""
|
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
|
# would let the assigned-flag comment silence the nudge for another 7
|
||||||
# days — the same self-silencing the ruling nudge avoids by reading its
|
# days — the same self-silencing the ruling nudge avoids by reading its
|
||||||
# facts once, at the top of the pass.
|
# 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")"
|
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")"
|
|| 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
|
if [ "$assignees" -gt 0 ] || has_issue_label attention; then
|
||||||
ensure_comment "$n" post-merge-assigned \
|
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.'
|
'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 —
|
# mandatory config — `load_issueflow_config` refuses to run without it —
|
||||||
# so there is nothing to fall back to, and a silent fallback is exactly
|
# so there is nothing to fall back to, and a silent fallback is exactly
|
||||||
# how the wrong addressee comes back.
|
# how the wrong addressee comes back.
|
||||||
if [ "$(ruling_nudge_decision "$NOW" "$age")" = NUDGE ]; then
|
if [ "$(ruling_nudge_decision "$NOW" "$evidence_age")" = NUDGE ]; then
|
||||||
local quiet_days=$(((NOW - age) / 86400))
|
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
|
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
|
Its wake evidence is still owed. \`post-merge\` means the merge landed and
|
||||||
|
|
|
||||||
|
|
@ -637,6 +637,14 @@ check "...only the evidence nudge speaks" 0 "1" \
|
||||||
grep -c -- '^----$' "$TMP/posted-36"
|
grep -c -- '^----$' "$TMP/posted-36"
|
||||||
|
|
||||||
printf '[]\n' >"$(cfix 37)"
|
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
|
issue_probe 37 post-merge 1 >/dev/null
|
||||||
check "assigned post-merge is flagged" 0 "" \
|
check "assigned post-merge is flagged" 0 "" \
|
||||||
grep -qF '<!-- issueflow:post-merge-assigned -->' "$TMP/posted-37"
|
grep -qF '<!-- issueflow:post-merge-assigned -->' "$TMP/posted-37"
|
||||||
|
|
@ -711,6 +719,40 @@ fresh="$(issue_probe 83 post-merge 0)"
|
||||||
check "an hour-old comment does reset it" 1 "" \
|
check "an hour-old comment does reset it" 1 "" \
|
||||||
grep -q 'post-merge evidence nudge' <<<"$fresh"
|
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
|
# 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
|
# one because the other spoke is a starved criterion, which is the failure
|
||||||
# this nudge exists to remove.
|
# 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"
|
"$ROOT/actions/issueflow-reconcile/issueflow-reconcile.sh"
|
||||||
# shellcheck disable=SC2016 # the call site is asserted as a literal
|
# shellcheck disable=SC2016 # the call site is asserted as a literal
|
||||||
check "...it is reused from lib/ruling.sh" 0 "" \
|
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"
|
"$ROOT/actions/issueflow-reconcile/issueflow-reconcile.sh"
|
||||||
|
|
||||||
# `post-merge` is the one queue state whose whole meaning is that the machine
|
# `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"' _ \
|
bash -c 'test "$1" -eq "$(wc -l <"$2")" && test ! -f "$3"' _ \
|
||||||
"$claim_edits_before" "$TMP/issue-edits" "$TMP/posted-50"
|
"$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 suppressed comment: a 504 on the marker read -----------------------
|
||||||
# The marker is on the issue. Read as "no marker", a failed read re-posts the
|
# 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.
|
# comment the marker exists to suppress — every sweep, forever.
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue