diff --git a/.github/scripts/dor_reconcile.sh b/.github/scripts/dor_reconcile.sh index ae2debae7..2cf797d46 100644 --- a/.github/scripts/dor_reconcile.sh +++ b/.github/scripts/dor_reconcile.sh @@ -99,6 +99,17 @@ has_live_run() { # $1 = issue number board_status_of() { printf '%s\n' "$board" | awk -F'\t' -v n="$1" '$1==n {print $3; found=1} END{exit !found}' 2>/dev/null || true; } on_board() { printf '%s\n' "$board" | awk -F'\t' -v n="$1" '$1==n {f=1} END{exit !f}'; } +# Is this board Status one the BUILD side owns? Those phases are tracked on the board alone β€” the +# build drops the issue's `state:*` label (dor_build_flow.sh) and never restores it β€” so a +# label-less issue sitting in one of them is routed, not forgotten, and must still be liveness- +# checked. Names must match dor_set_status.sh's STATUS_NAME map. +build_phase() { + case "$1" in + "Building"|"Awaiting functional acceptance"|"Awaiting merge"|"Exceptions") return 0 ;; + *) return 1 ;; + esac +} + # 2. Walk every OPEN enhancement issue. Capture the list first so a transient failure aborts under # set -e rather than silently reporting "healthy"; state_label is LAST so an empty label (the # common case) is a trailing field that `read` strips cleanly instead of shifting the columns. @@ -140,20 +151,37 @@ while IFS=$'\t' read -r num created_epoch updated_epoch needs_vouch sk_label sta continue fi - if [ -z "$state_label" ]; then - # FLAG: on the board but never routed. If old enough, the agent probably never ran. - age_h=$(( (now - created_epoch) / 3600 )) + # FLAG: on the board but never routed. If old enough, the agent probably never ran. + # + # "No `state:*` label" does NOT mean un-routed. The BUILD side deliberately runs without one: + # dor_build_flow.sh drops `state:awaiting-approval` when it applies `build-done`, and nothing + # re-adds a state label afterwards β€” the phase lives on the board from then on. So every issue + # in the build/feedback phase reaches here label-less, and until #995 this branch `continue`d, + # which made the whole `case "$status"` below unreachable for them. That is exactly the + # population the πŸ’€ liveness arm was written for (#963): every dead build was found by a human + # watching, never by this sweep, because control flow never got that far. Ask the BOARD whether + # something owns the issue, and fall through when it does. + # + # Age is measured from `updated_epoch`, not `created_epoch`: a build that died 18 minutes ago was + # reported as "after 1151h" (#370, opened in June), which reads as ancient backlog noise rather + # than something that just broke. + if [ -z "$state_label" ] && ! build_phase "$status"; then + age_h=$(( (now - updated_epoch) / 3600 )) if [ "$age_h" -ge "$UNROUTED_HOURS" ]; then - add_ex "πŸ•³οΈ #${num} is on the board (Status: ${status:-none}) with no \`state:*\` label after ${age_h}h β€” the agent likely never ran." + add_ex "πŸ•³οΈ #${num} is on the board (Status: ${status:-none}) with no \`state:*\` label and untouched for ${age_h}h β€” the agent likely never ran." fi continue fi # FLAG: Status β‰  label (human moved one, not the other; or a write failed). Human resolves. # (blank $status with a label set is the "Status write failed" case β€” flag it too.) - expected="$(label_to_status "$state_label")" - if [ -n "$expected" ] && [ "$status" != "$expected" ]; then - add_ex "πŸ”€ #${num} drift: label \`${state_label}\` (β†’ ${expected}) but board Status is **${status:-}**." + # Needs a label by definition β€” a build-phase issue reaching here has none, and `label_to_status ""` + # is empty, so the guard below already skips it. Kept explicit so that stays true if either changes. + if [ -n "$state_label" ]; then + expected="$(label_to_status "$state_label")" + if [ -n "$expected" ] && [ "$status" != "$expected" ]; then + add_ex "πŸ”€ #${num} drift: label \`${state_label}\` (β†’ ${expected}) but board Status is **${status:-}**." + fi fi # FLAG: stale waiting on a human. diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index fbd0c6dca..54d288455 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -875,6 +875,7 @@ jobs: bash test/ci-scripts/test-crawler-scope.sh bash test/ci-scripts/test-commit-range.sh bash test/ci-scripts/test-dor-gate-notice.sh + bash test/ci-scripts/test-dor-reconcile-liveness.sh # ── Gate: single required check for branch protection ─────────────────── # Replaces the individual job requirements in branch protection. Update branch diff --git a/changes/reconcile-liveness-shadowed.md b/changes/reconcile-liveness-shadowed.md new file mode 100644 index 000000000..af054d4ac --- /dev/null +++ b/changes/reconcile-liveness-shadowed.md @@ -0,0 +1,2 @@ +- Fixed: an automated build whose machine died mid-run is now detected within the hour and reported on its own issue, telling you to re-dispatch it. The check meant to catch this could never fire β€” every issue in the build phase was instead reported as "the agent likely never ran", which reads as old backlog and prompts no action. One feature's work sat dead for 21 hours until someone noticed by hand. +- The "nobody has picked this up" report now measures how long an issue has actually been untouched, rather than how long ago it was opened β€” so something that broke minutes ago no longer reads as weeks old, and an issue that is being worked on is no longer reported as forgotten. diff --git a/test/ci-scripts/test-dor-reconcile-liveness.sh b/test/ci-scripts/test-dor-reconcile-liveness.sh new file mode 100644 index 000000000..0cc463f40 --- /dev/null +++ b/test/ci-scripts/test-dor-reconcile-liveness.sh @@ -0,0 +1,153 @@ +#!/usr/bin/env bash +# Unit tests for the DoR reconcile sweep's per-issue triage (.github/scripts/dor_reconcile.sh). +# +# The sweep is the only thing that notices a build whose sidekick died mid-flight β€” the flow dies +# with the box, so it never reaches its own error handling. That detector (πŸ’€, #963) was unreachable +# for its entire target population for weeks (#995): the "no `state:*` label" branch `continue`d +# before the liveness check, and the build side deliberately runs without a state label. Nothing +# tested this file, which is why it shipped shadowed and stayed that way. +# +# Approach: put a stub `gh` on PATH that serves fixtures and records writes, then run the REAL +# script end to end and assert on the health-report body it produces. No network, no tokens. +# +# Usage: bash test/ci-scripts/test-dor-reconcile-liveness.sh + +set -euo pipefail + +REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)" +SCRIPT="$REPO_ROOT/.github/scripts/dor_reconcile.sh" + +PASS=0 +FAIL=0 + +assert_contains() { + local desc="$1" needle="$2" haystack="$3" + if printf '%s' "$haystack" | grep -qF -- "$needle"; then + echo " PASS $desc" + PASS=$((PASS + 1)) + else + echo " FAIL $desc" + echo " expected to find: $needle" + echo " in: $(printf '%s' "$haystack" | head -c 400)" + FAIL=$((FAIL + 1)) + fi +} + +assert_lacks() { + local desc="$1" needle="$2" haystack="$3" + if printf '%s' "$haystack" | grep -qF -- "$needle"; then + echo " FAIL $desc" + echo " did NOT expect: $needle" + echo " in: $(printf '%s' "$haystack" | head -c 400)" + FAIL=$((FAIL + 1)) + else + echo " PASS $desc" + PASS=$((PASS + 1)) + fi +} + +# ── The stub ──────────────────────────────────────────────────────────────── +# Dispatches on the argument shape of each call the script makes, serving files from $FIX and +# appending every write to $FIX/writes.log so a test can assert on what reached the issue. +make_stub() { + local dir="$1" + mkdir -p "$dir/bin" + cat > "$dir/bin/gh" <<'STUB' +#!/usr/bin/env bash +args="$*" +log() { printf '%s\n' "$*" >> "$FIX/writes.log"; } +case "$args" in + # Board snapshot: "\t\t" + "api graphql"*) cat "$FIX/board.tsv" ;; + # Live DoR workflow runs β€” display_title lines, one per run. + *"actions/runs?status="*) cat "$FIX/live_runs.txt" ;; + # has_live_run's title lookup β€” `gh issue view --repo … --json title`, so the number is $3. + "issue view"*"--json title"*) sed -n "s/^$3\t//p" "$FIX/titles.tsv" ;; + # The open-issue walk. The script's own --jq is bypassed: we serve the TSV it expects. + *"--state open --label dor-stuck"*) cat "$FIX/marked.txt" 2>/dev/null || true ;; + *"--state open --label"*"--limit 201"*) cat "$FIX/issues.tsv" ;; + *"--state closed --label"*) : ;; # no closed issues claiming a sidekick + *"--state all --limit 100"*) : ;; # no health issue yet -> the script creates one + "pr list"*) : ;; # no open PR (zombie check) + "issue create"*) log "CREATE_BODY: $*" ;; + "issue comment"*) log "COMMENT: $*" ;; + "issue edit"*) log "EDIT: $*" ;; + *) : ;; +esac +exit 0 +STUB + chmod +x "$dir/bin/gh" +} + +# Run the real sweep against one fixture set; echo the health-report body it tried to publish. +run_sweep() { + local fix="$1" + FIX="$fix" PATH="$fix/bin:$PATH" \ + OWNER=Fortigi REPO=IdentityAtlas PROJECT_ID=PVT_test GH_TOKEN=stub LABEL=enhancement \ + bash "$SCRIPT" >/dev/null 2>&1 || true + # The body is multi-line, so take the CREATE_BODY line and everything after it β€” a plain grep + # would return only the report's first line and every assertion below would vacuously fail. + sed -n '/^CREATE_BODY: /,$p' "$fix/writes.log" 2>/dev/null || true +} + +# Build a fixture dir. $2 = board Status, $3 = minutes since the issue was updated, +# $4 = "live" to make a run look alive for it. +scenario() { + local dir="$1" status="$2" upd_min="$3" live="${4:-}" + local now created updated + now="$(date -u +%s)" + created=$(( now - 3600 * 1000 )) # ancient: opened ~42 days ago, like #370 + updated=$(( now - 60 * upd_min )) + rm -rf "$dir"; mkdir -p "$dir" + make_stub "$dir" + : > "$dir/writes.log" + printf '370\tOPEN\t%s\n' "$status" > "$dir/board.tsv" + printf '370\tCollapse managed resources\n' > "$dir/titles.tsv" + # number, created, updated, needs_vouch, sk_label, state_label β€” state_label EMPTY, as the + # build side always leaves it. + printf '370\t%s\t%s\tfalse\tsk:sk3\t\n' "$created" "$updated" > "$dir/issues.tsv" + if [ "$live" = "live" ]; then + printf 'Collapse managed resources\n' > "$dir/live_runs.txt" + else + : > "$dir/live_runs.txt" + fi + printf '%s' "$dir" +} + +TMP="$(mktemp -d)" +trap 'rm -rf "$TMP"' EXIT + +echo "DoR reconcile β€” died-sidekick liveness" +echo + +# ── 1. THE regression (#995): Building + no live run must be flagged πŸ’€ ────── +# Pre-fix this emitted the πŸ•³οΈ un-routed line instead, because the label-less branch `continue`d +# before the liveness check ever ran. +out="$(run_sweep "$(scenario "$TMP/dead" 'Building' 40)")" +assert_contains "a dead Building issue is flagged as a died sidekick" "πŸ’€ #370" "$out" +assert_contains "…and is told to re-dispatch, not that the agent never ran" "re-dispatch it" "$out" +assert_lacks "…and is NOT mis-reported as un-routed" "πŸ•³οΈ #370" "$out" +assert_contains "the issue itself gets marked dor-stuck" "add-label dor-stuck" \ + "$(cat "$TMP/dead/writes.log")" +assert_contains "…and commented on directly" "died mid-flight" \ + "$(cat "$TMP/dead/writes.log")" + +# ── 2. A build that IS alive must stay quiet ──────────────────────────────── +out="$(run_sweep "$(scenario "$TMP/alive" 'Building' 40 live)")" +assert_lacks "a live build is not reported dead" "πŸ’€ #370" "$out" +assert_lacks "a live build is not reported un-routed either" "πŸ•³οΈ #370" "$out" + +# ── 3. Genuinely un-routed still flagged ──────────────────────────────────── +# No state label AND no board Status: nothing owns it, so the original πŸ•³οΈ rule must survive. +out="$(run_sweep "$(scenario "$TMP/unrouted" '' 600)")" +assert_contains "an issue nothing owns is still flagged un-routed" "πŸ•³οΈ #370" "$out" + +# ── 4. Un-routed age is measured from the last update, not from creation ──── +# Same ancient creation date, but touched 60 min ago and UNROUTED_HOURS defaults to 6. Pre-fix this +# reported "after 1000h" purely because the issue was opened in June. +out="$(run_sweep "$(scenario "$TMP/fresh" '' 60)")" +assert_lacks "a recently-updated issue is not flagged on its creation age" "πŸ•³οΈ #370" "$out" + +echo +echo " $PASS passed, $FAIL failed" +[ "$FAIL" -eq 0 ]