diff --git a/.github/scripts/dor_reconcile.sh b/.github/scripts/dor_reconcile.sh index 2cf797d46..6c6405187 100644 --- a/.github/scripts/dor_reconcile.sh +++ b/.github/scripts/dor_reconcile.sh @@ -111,22 +111,29 @@ build_phase() { } # 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. -issues_tsv="$(gh issue list --repo "$OWNER/$REPO" --state open --label "$LABEL" --limit 201 \ +# set -e rather than silently reporting "healthy". +# +# Records are joined on US (\x1f), NOT tabs. Tab is an *IFS whitespace* character, so under +# `IFS=$'\t'` bash collapses a run of tabs into ONE delimiter and an empty field in the middle of +# the record silently shifts every field after it left. `sk_label` is empty on almost every issue +# (only a live build holds a sidekick), so the overwhelmingly common record — +# `…state:decompose` — parsed as sk_label='state:decompose', state_label='' and made +# every correctly-routed issue look un-routed. US is not IFS whitespace, so empty fields survive. +# Neither an issue number nor a GitHub label name can contain a control character. +issues_rows="$(gh issue list --repo "$OWNER/$REPO" --state open --label "$LABEL" --limit 201 \ --json number,labels,createdAt,updatedAt \ --jq '.[] | [(.number|tostring), (.createdAt | fromdateiso8601 | tostring), (.updatedAt | fromdateiso8601 | tostring), ((([.labels[].name] | index("needs-vouch")) != null) | tostring), ([.labels[].name | select(startswith("sk:"))][0] // ""), - ([.labels[].name | select(startswith("state:"))][0] // "")] | @tsv')" -if [ "$(printf '%s' "$issues_tsv" | grep -c .)" -ge 201 ]; then + ([.labels[].name | select(startswith("state:"))][0] // "")] | join("\u001f")')" +if [ "$(printf '%s' "$issues_rows" | grep -c .)" -ge 201 ]; then add_ex "⚠️ Over 200 open ${LABEL} issues — reconcile inspected only the first 200; add pagination." - issues_tsv="$(printf '%s\n' "$issues_tsv" | head -n 200)" + issues_rows="$(printf '%s\n' "$issues_rows" | head -n 200)" fi approval_backlog=0 -while IFS=$'\t' read -r num created_epoch updated_epoch needs_vouch sk_label state_label; do +while IFS=$'\x1f' read -r num created_epoch updated_epoch needs_vouch sk_label state_label; do [ -n "$num" ] || continue status="$(board_status_of "$num")" @@ -213,11 +220,14 @@ while IFS=$'\t' read -r num created_epoch updated_epoch needs_vouch sk_label sta [ -z "$open_pr" ] && add_ex "🧟 #${num} still claims \`${sk_label}\` with no open PR — that box is probably holding a stale env. Release it, or drop the label if it already was." fi fi -done < <(printf '%s\n' "$issues_tsv") +done < <(printf '%s\n' "$issues_rows") [ "$approval_backlog" -gt 0 ] && add_ex "🚦 ${approval_backlog} issue(s) waiting in **Awaiting approval** — the Product board's value gate." # 3. Closed issues still parked in a non-terminal board Status. +# "Out of pipeline" is NOT terminal here, deliberately: an issue that left the pipeline and has since +# been closed still has to be walked over to Done, and this line is the only reminder that it is +# sitting there. Done is the single resting state on the board. while IFS=$'\t' read -r num istate status; do [ "$istate" = "CLOSED" ] || continue case "$status" in ""|"Done") : ;; *) add_ex "🔚 #${num} is CLOSED but still on the board as **${status}** — move it to Done or off the board." ;; esac diff --git a/changes/reconcile-field-shift.md b/changes/reconcile-field-shift.md new file mode 100644 index 000000000..c90c225f0 --- /dev/null +++ b/changes/reconcile-field-shift.md @@ -0,0 +1,3 @@ +- Fixed the DoR pipeline health report claiming that correctly-routed issues were "on the board with no `state:*` label — the agent likely never ran". Any issue that had a routing label but no reserved sidekick was misread as un-routed, which was every routed issue on both boards: all 31 items the two health issues were reporting were false alarms. +- Restored the health report's drift detection (routing label disagrees with the board's Status column), which had never been able to run for the same reason. +- The waiting-on-a-human signals — the approval-gate backlog count and the long-stale warnings — now reach the health report; they were being skipped alongside the drift check. diff --git a/test/ci-scripts/test-dor-reconcile-liveness.sh b/test/ci-scripts/test-dor-reconcile-liveness.sh index 0cc463f40..53e6d3c19 100644 --- a/test/ci-scripts/test-dor-reconcile-liveness.sh +++ b/test/ci-scripts/test-dor-reconcile-liveness.sh @@ -7,6 +7,14 @@ # 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. # +# The complementary failure was the record encoding itself. `@tsv` + `IFS=$'\t'` collapses a run of +# tabs (tab is IFS *whitespace*), so an empty `sk_label` in the middle of the record shifted every +# field after it: `state_label` came out empty on every issue that had a state label but no sidekick +# — i.e. nearly all of them. Every correctly-routed issue was reported "un-routed", and the drift +# check, which needs the label, never ran at all. The first version of this harness could not see it +# because the fixture hand-encoded the record; the stub now serves JSON and applies the script's OWN +# --jq program to it, so the jq → `read` contract is under test rather than duplicated here. +# # 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. # @@ -17,6 +25,11 @@ set -euo pipefail REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)" SCRIPT="$REPO_ROOT/.github/scripts/dor_reconcile.sh" +command -v jq >/dev/null 2>&1 || { + echo "jq is required: the stub applies the script's real --jq program to the JSON fixtures." >&2 + exit 1 +} + PASS=0 FAIL=0 @@ -63,9 +76,17 @@ case "$args" in *"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. + # The open-issue walk. Serve the JSON real `gh` would return and run the script's OWN --jq program + # over it, so the record encoding is exercised instead of being hand-copied into the fixture. *"--state open --label dor-stuck"*) cat "$FIX/marked.txt" 2>/dev/null || true ;; - *"--state open --label"*"--limit 201"*) cat "$FIX/issues.tsv" ;; + *"--state open --label"*"--limit 201"*) + jq_prog=""; prev="" + for a in "$@"; do + if [ "$prev" = "--jq" ]; then jq_prog="$a"; break; fi + prev="$a" + done + jq -r "$jq_prog" "$FIX/issues.json" + ;; *"--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) @@ -90,10 +111,11 @@ run_sweep() { 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. +# Build a fixture dir holding ONE open issue (#370). +# $2 board Status $3 minutes since the issue was last updated $4 "live" to give it a live run +# $5 its `sk:*` label ('' for none) $6 its `state:*` label ('' for none, as the build side leaves it) scenario() { - local dir="$1" status="$2" upd_min="$3" live="${4:-}" + local dir="$1" status="$2" upd_min="$3" live="${4:-}" sk="${5-sk:sk3}" state="${6-}" local now created updated now="$(date -u +%s)" created=$(( now - 3600 * 1000 )) # ancient: opened ~42 days ago, like #370 @@ -103,9 +125,11 @@ scenario() { : > "$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" + jq -n --argjson c "$created" --argjson u "$updated" --arg sk "$sk" --arg st "$state" \ + '[{ number: 370, createdAt: ($c|todate), updatedAt: ($u|todate), + labels: ([{name:"enhancement"}] + + (if $sk == "" then [] else [{name:$sk}] end) + + (if $st == "" then [] else [{name:$st}] end)) }]' > "$dir/issues.json" if [ "$live" = "live" ]; then printf 'Collapse managed resources\n' > "$dir/live_runs.txt" else @@ -114,6 +138,18 @@ scenario() { printf '%s' "$dir" } +# A board that still lists CLOSED issues and no open ones, so only the closed-issue check speaks. +# Two rows, so every assertion is two-sided: #370 carries the status under test, #371 is the control +# that must always be flagged (which also proves a report was published at all). +closed_scenario() { + local dir="$1" status="$2" + rm -rf "$dir"; mkdir -p "$dir"; make_stub "$dir" + : > "$dir/writes.log"; : > "$dir/live_runs.txt"; : > "$dir/titles.tsv" + printf '370\tCLOSED\t%s\n371\tCLOSED\tBuilding\n' "$status" > "$dir/board.tsv" + printf '[]\n' > "$dir/issues.json" + printf '%s' "$dir" +} + TMP="$(mktemp -d)" trap 'rm -rf "$TMP"' EXIT @@ -148,6 +184,44 @@ assert_contains "an issue nothing owns is still flagged un-routed" "🕳️ #370 out="$(run_sweep "$(scenario "$TMP/fresh" '' 60)")" assert_lacks "a recently-updated issue is not flagged on its creation age" "🕳️ #370" "$out" +echo +echo "DoR reconcile — the record must not shift when a field is empty" +echo + +# ── 5. An issue WITH a state label and NO sidekick — the common shape ─────── +# The record is `…state:decompose`: `sk_label` empty in the MIDDLE. Under @tsv/IFS-tab the +# two delimiters collapsed, `state_label` came out empty, and the sweep reported the issue as +# un-routed while the drift check silently never ran. Status here disagrees with the label, so a +# working parse MUST produce 🔀 — which also makes the 🕳️ assertion non-vacuous. +out="$(run_sweep "$(scenario "$TMP/drift" 'Awaiting design' 600 '' '' 'state:decompose')")" +assert_lacks "a labelled issue is not mis-reported as un-routed" "🕳️ #370" "$out" +assert_contains "the drift check can see the state label again" "🔀 #370" "$out" +assert_contains "…and names both sides of the disagreement" \ + 'label `state:decompose` (→ Decompose) but board Status is **Awaiting design**' "$out" + +# ── 6. …and when the label and the board agree, the sweep says nothing ────── +run_sweep "$(scenario "$TMP/routed" 'Decompose' 600 '' '' 'state:decompose')" >/dev/null +assert_lacks "a correctly-routed issue publishes no health report at all" "CREATE_BODY" \ + "$(cat "$TMP/routed/writes.log")" + +echo +echo "DoR reconcile — closed issues on the board" +echo + +# ── 7. Done is the ONLY resting state — "Out of pipeline" must still nag ──── +# Leaving the pipeline is not the same as being filed away: a closed issue parked in "Out of +# pipeline" still has to be walked over to Done, and this report line is the only reminder that it +# is sitting there. Pinned because it is tempting to read that column as terminal and silence it. +out="$(run_sweep "$(closed_scenario "$TMP/closed-oop" 'Out of pipeline')")" +assert_contains "a closed issue parked mid-pipeline is flagged" "🔚 #371" "$out" +assert_contains "…and a closed 'Out of pipeline' issue is flagged too" "🔚 #370" "$out" + +# ── 8. …but a closed issue that reached Done is left alone ────────────────── +# #371 still flags, so the report exists and the assertion below is not vacuous. +out="$(run_sweep "$(closed_scenario "$TMP/closed-done" 'Done')")" +assert_contains "the control row still flags" "🔚 #371" "$out" +assert_lacks "a closed Done issue is not flagged" "🔚 #370" "$out" + echo echo " $PASS passed, $FAIL failed" [ "$FAIL" -eq 0 ]