diff --git a/.github/workflows/kanban-closure-router.yml b/.github/workflows/kanban-closure-router.yml index 5b75f33..665e112 100644 --- a/.github/workflows/kanban-closure-router.yml +++ b/.github/workflows/kanban-closure-router.yml @@ -274,30 +274,55 @@ jobs: # # Only Done is guarded. A PR-derived Status is a deploy fact and may advance a # card normally. - # THE COLUMN LIST WAS STALE, AND THAT REOPENED THE HOLE IT CLOSES - # (backend#1846). `CURRENT_COL` is read from the BOARD, whose column is - # "Staging (agent review)" -- this list carried only the pre-rename - # "Staging (human review)". So an issue hand-closed while sitting in the - # agent-review column did not match, Done overwrote its deploy state, - # and kanban-archive.yml then hid the card entirely: exactly the - # sequence this guard exists to prevent, through a string that quietly - # stopped being true. + # WHICH COLUMNS ARE DEPLOY STATES, ASKED OF THE BOARD (backend#1846). # - # kanban-reconcile.yml's equivalent guard already lists both names. Two - # copies of one list, one updated at the rename and one not, is how this - # drifted -- so when the rename window closes, drop the legacy name in - # BOTH places. + # This was a hand-maintained list of six names, duplicated in + # kanban-reconcile.yml -- and it had already rotted once: it carried the + # pre-rename "Staging (human review)" and not the board's actual + # "Staging (agent review)", so a card hand-closed in that column lost its + # deploy state and kanban-archive.yml then hid it (.github#237). Fixing + # that instance left the CLASS: the next rename or inserted column + # reopens it, silently, in two files. + # + # The board already answers this. `$PROJ` carries the Status options in + # PIPELINE ORDER, so a deploy state is any column at or after "On dev" + # and at or before "Prod". An inserted column -- which is exactly how + # "Staging (agent review)" arrived -- is classified correctly with no + # edit here, and a renamed intermediate column keeps working because its + # POSITION is what matters, not its name. + # + # Two anchors instead of six names, and both are written by this same + # workflow, so .github#247's checker already asserts they exist. + col_index() { + echo "$PROJ" | jq -r --arg s "$1" \ + '[.data.organization.projectV2.fields.nodes[] + | select(.name=="Status") | .options[].name] | index($s) // -1' + } if [ "$STATUS_NAME" = "Done" ]; then - case "${CURRENT_COL:-}" in - "On dev"|"Staging (agent review)"|"Staging (human review)"|"FR on staging"|"Ready for prod"|"Prod") - echo "::notice::#$NUMBER hand-closed but sits in '$CURRENT_COL', a deploy state - NOT setting Done (D8: follow the PR's stage)" - # AND SAY SO WHERE SOMEONE WILL SEE IT. Refusing is right, but it - # parks the card in a deploy state with no way to self-heal, and a - # run-log notice is invisible by the time anyone looks. The two - # real cases needed OPPOSITE answers -- backend#1493 had shipped - # via cli#452 and belonged in Prod; data-ingestors#488 was - # reverted and belonged in Done -- so no default is correct and - # only the person closing it knows which. + cur_i=$(col_index "${CURRENT_COL:-}") + dev_i=$(col_index "On dev") + prod_i=$(col_index "Prod") + # UNKNOWN MUST NOT FALL OPEN -- the other half of #1846. A column this + # workflow cannot place used to sail past the `case` and let Done erase + # a deploy state. Refusing costs a card sitting where it is; falling + # open erases the fact that it shipped. + if [ "$dev_i" -lt 0 ] || [ "$prod_i" -lt 0 ]; then + echo "::error::the board reports no 'On dev' or 'Prod' column, so a deploy state cannot be recognised - refusing to set Done on #$NUMBER" + exit 1 + fi + if [ "$cur_i" -lt 0 ]; then + echo "::notice::#$NUMBER sits in '${CURRENT_COL:-}', which this board does not report as a column - NOT setting Done rather than guessing" + exit 0 + fi + if [ "$cur_i" -ge "$dev_i" ] && [ "$cur_i" -le "$prod_i" ]; then + echo "::notice::#$NUMBER hand-closed but sits in '$CURRENT_COL', a deploy state - NOT setting Done (D8: follow the PR's stage)" + # AND SAY SO WHERE SOMEONE WILL SEE IT. Refusing is right, but it + # parks the card with no way to self-heal, and a run-log notice is + # invisible by the time anyone looks at the board. The two real + # cases needed OPPOSITE answers -- backend#1493 had shipped via + # cli#452 and belonged in Prod; data-ingestors#488 was reverted and + # belonged in Done -- so no default is correct and only the person + # closing it knows which. CLOSE_NOTE="Closed while the board still shows \`$CURRENT_COL\`, which records a deployment." CLOSE_NOTE="$CLOSE_NOTE The automation will not overwrite a deploy state with \`Done\` (RFC-BACKEND-1405 D8)," CLOSE_NOTE="$CLOSE_NOTE so this card stays where it is until someone says which happened:" @@ -306,8 +331,8 @@ jobs: CLOSE_NOTE="$CLOSE_NOTE Both cases are real and they need opposite answers, which is why this is not decided automatically." gh issue comment "$NUMBER" --repo "$REPO_FULL" --body "$CLOSE_NOTE" >/dev/null 2>&1 \ || echo "::warning::could not comment on #$NUMBER - it is parked in '$CURRENT_COL' with no note on the issue" - exit 0 ;; - esac + exit 0 + fi fi # shellcheck disable=SC2016 # the $names here are GraphQL variables, not shell - keep literal diff --git a/.github/workflows/kanban-deploy-state-selftest.yml b/.github/workflows/kanban-deploy-state-selftest.yml new file mode 100644 index 0000000..e06f545 --- /dev/null +++ b/.github/workflows/kanban-deploy-state-selftest.yml @@ -0,0 +1,44 @@ +name: Kanban deploy-state selftest + +# The classification that decides whether `Done` may overwrite a card's column +# (backend#1846). It is asserted rather than trusted because its predecessor -- a +# hand-maintained list of six column names -- rotted silently and cost a card its +# deploy state (.github#237). +# +# The test reads `col_index()` OUT of kanban-closure-router.yml rather than +# copying it, so it cannot go green against a stale duplicate. Offline, no token. + +on: + pull_request: + paths: + - .github/workflows/kanban-closure-router.yml + - .github/workflows/kanban-deploy-state-selftest.yml + - scripts/tests/kanban-deploy-state-selftest.py + push: + branches: [main, develop, staging] + paths: + - .github/workflows/kanban-closure-router.yml + - .github/workflows/kanban-deploy-state-selftest.yml + - scripts/tests/kanban-deploy-state-selftest.py + +permissions: + contents: read + +concurrency: + group: kanban-deploy-state-selftest-${{ github.ref }} + cancel-in-progress: true + +jobs: + selftest: + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 + - uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5.6.0 + with: + python-version: '3.12' + # PyYAML only: the test parses the workflow to extract the function under + # test. `pip install` without setup-python is what left the bricked-PR + # audit unable to start on Ubuntu 24.04's PEP 668 python (.github#244). + - run: pip install --quiet pyyaml + - run: python scripts/tests/kanban-deploy-state-selftest.py diff --git a/scripts/tests/kanban-deploy-state-selftest.py b/scripts/tests/kanban-deploy-state-selftest.py new file mode 100755 index 0000000..5a17b71 --- /dev/null +++ b/scripts/tests/kanban-deploy-state-selftest.py @@ -0,0 +1,148 @@ +#!/usr/bin/env python3 +"""The deploy-state classification is read OUT of the workflow and exercised. + +WHY THIS EXISTS (backend#1846) + +`kanban-closure-router.yml` must never overwrite a deploy state with `Done` +(RFC-BACKEND-1405 D8). It decided that from a hand-maintained list of six column +names, duplicated in `kanban-reconcile.yml` -- and the list rotted: it carried +the pre-rename "Staging (human review)" while the board's column is +"Staging (agent review)", so a card hand-closed there lost its deploy state and +`kanban-archive.yml` hid it (.github#237). + +The classification now comes from the board's own option ORDER: a deploy state is +any column at or after `On dev` and at or before `Prod`. That is what makes an +inserted column -- which is how "Staging (agent review)" arrived -- correct with +no edit. + +WHY IT IS EXTRACTED RATHER THAN COPIED + +A copy of the logic here would let the workflow drift while this file stays +green, which is the same defect class the classification itself had. So the +`col_index` function is pulled from the YAML and sourced. If someone renames or +reshapes it, this test stops finding it and fails loudly rather than testing a +stale duplicate. + +Exit 0 when every case behaves as specified. +""" +from __future__ import annotations + +import json +import os +import re +import subprocess +import sys + +import yaml + +HERE = os.path.dirname(os.path.abspath(__file__)) +WF = os.path.join(HERE, os.pardir, os.pardir, + ".github", "workflows", "kanban-closure-router.yml") + +RESULTS: "list[tuple[bool, str, str]]" = [] + + +def record(ok: bool, name: str, detail: str) -> None: + RESULTS.append((ok, name, detail)) + print(f"{'PASS' if ok else 'FAIL'} {name}\n {detail}") + + +def extract_col_index() -> str: + """Pull `col_index()` out of the workflow's run: block, verbatim.""" + doc = yaml.safe_load(open(WF)) + runs = [s["run"] for j in doc["jobs"].values() + for s in j.get("steps", []) if "run" in s] + for body in runs: + m = re.search(r"^\s*col_index\(\) \{.*?^\s*\}\s*$", body, + re.S | re.M) + if m: + # Strip the workflow's indentation so it parses standalone. + block = m.group(0) + indent = len(block) - len(block.lstrip()) + return "\n".join(ln[indent:] if ln[:indent].isspace() else ln + for ln in block.splitlines()) + sys.exit("could not find col_index() in the workflow — did it get renamed? " + "This test refuses to fall back to a copy.") + + +COL_INDEX = extract_col_index() + +# The live board's order, as the API returns it. +BOARD = ["Backlog", "North Stars", "Ready", "In progress", "Code review", + "On dev", "Staging (agent review)", "FR on staging", "Ready for prod", + "Prod", "Done", "Cancelled"] + + +def proj(names) -> str: + return json.dumps({"data": {"organization": {"projectV2": {"fields": { + "nodes": [{"name": "Status", + "options": [{"name": n} for n in names]}]}}}}}) + + +def classify(current: str, names=None) -> str: + """Run the EXTRACTED function plus the guard's own comparison.""" + names = BOARD if names is None else names + script = f""" +set -euo pipefail +PROJ='{proj(names)}' +{COL_INDEX} +cur_i=$(col_index "{current}") +dev_i=$(col_index "On dev") +prod_i=$(col_index "Prod") +if [ "$dev_i" -lt 0 ] || [ "$prod_i" -lt 0 ]; then echo ANCHORS_MISSING; exit 0; fi +if [ "$cur_i" -lt 0 ]; then echo UNKNOWN; exit 0; fi +if [ "$cur_i" -ge "$dev_i" ] && [ "$cur_i" -le "$prod_i" ]; then + echo DEPLOY_STATE +else + echo FREE +fi +""" + out = subprocess.run(["bash", "-c", script], capture_output=True, text=True) + if out.returncode != 0: + return f"ERROR: {out.stderr.strip()}" + return out.stdout.strip() + + +# 1. Every deploy state, including the one the old list missed. +for col in ("On dev", "Staging (agent review)", "FR on staging", + "Ready for prod", "Prod"): + record(classify(col) == "DEPLOY_STATE", + f"{col!r} is a deploy state", f"-> {classify(col)}") + +# 2. Everything before On dev is free to be overwritten by Done. +for col in ("Backlog", "Ready", "In progress", "Code review"): + record(classify(col) == "FREE", + f"{col!r} is not a deploy state", f"-> {classify(col)}") + +# 3. TERMINAL columns sit AFTER Prod in the order, so the `<= Prod` bound is +# what stops them being treated as deploy states. Without it a card already +# in Done would refuse to be set to Done -- harmless, but it would also make +# Cancelled unreachable. +for col in ("Done", "Cancelled"): + record(classify(col) == "FREE", + f"{col!r} is terminal, not a deploy state", f"-> {classify(col)}") + +# 4. THE CASE THE OLD LIST GOT WRONG. A column inserted between the anchors is +# classified correctly with no edit here -- which is exactly how +# "Staging (agent review)" arrived and why the list rotted. +inserted = BOARD[:7] + ["Staging (robot review)"] + BOARD[7:] +record(classify("Staging (robot review)", inserted) == "DEPLOY_STATE", + "a NEWLY inserted column between the anchors is a deploy state", + "no edit to the workflow required — this is the rot the list had") + +# 5. UNKNOWN MUST NOT FALL OPEN. +record(classify("Some Column Nobody Declared") == "UNKNOWN", + "a column the board does not report is UNKNOWN, not free", + "the old `case` fell through and let Done erase the deploy state") + +record(classify("") == "UNKNOWN", + "an unreadable current column is UNKNOWN, not free", "empty CURRENT_COL") + +# 6. A board missing an anchor cannot be classified at all. +record(classify("On dev", [n for n in BOARD if n != "Prod"]) == "ANCHORS_MISSING", + "a board with no 'Prod' column refuses rather than guessing", + "the guard exits 1 on this, which is the fail-closed half") + +failed = [r for r in RESULTS if not r[0]] +print(f"\n{len(RESULTS) - len(failed)} passed, {len(failed)} failed") +sys.exit(1 if failed else 0)