From bcde474c1570e2988f74fbe24ede706a8a63dbde Mon Sep 17 00:00:00 2001 From: Alexandre <44178713+alexbelgium@users.noreply.github.com> Date: Mon, 7 Sep 2026 13:23:28 +0200 Subject: [PATCH] skill(hassio-addon-workflow): stop pr_review.sh watch reporting a false pass (#3052) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * skill(hassio-addon-workflow): stop pr_review.sh watch reporting a false pass `gh pr checks` emits TAB-separated columns, but `watch` parsed it with awk's default field splitting. Every check name containing a space was truncated to its first word and the state column was never read: Codacy Static Code Analysisfail -> Codacy=Static Addon linting (wger)pass -> Addon=linting Test addon build (wger)pending -> Test=addon All three blocking gates have multi-word names, so the `case` matched neither *fail* nor *pending* and fell through to "settled - all passing". That is a false pass from the one command whose job is to report CI truthfully: #3044 was called green with Codacy red, and #3042 was called green while the HA add-on linter was failing. A build that had not started would also have read as a pass. - parse with `awk -F'\t'` - judge the state column alone, never the joined name=state text, so a check named e.g. `flaky-fail-detector` cannot read as a failure - allowlist the good states (pass/skipping/pending) and treat anything unrecognised as a failure, so a new state cannot reach the passing branch - name the checks that failed instead of only saying FAILURES - keep waiting when only advisory checks have reported Codacy is red on essentially every add-on PR here (#3019, #3044 and #3050 all merged with it failing; master has no branch protection), so it is excluded from the verdict but printed every poll and called out explicitly on settle. Agreed with the maintainer. It is a denylist of known noise rather than an allowlist of gates, so a job added to CI later counts as blocking by default. Verified against real PRs: #3042 (blocking linter failure) now exits 1 and names the check where it previously exited 0; #3018/#3019/#3044/#3050 report correctly; pending, advisory-only, unknown-state and empty-output cases checked against a stubbed gh. shellcheck clean. Co-Authored-By: Claude Opus 5 * skill: record PR number in the traps entry Co-Authored-By: Claude Opus 5 * skill: address review — skipped-checks wording, document TSV contract CodeRabbit (correct): the header claimed exit 0 means "every blocking check passed", but a skipped gate also yields 0. A PR touching no add-on skips all three gates, so that wording overstated what a 0 means. Reworded; the runtime warning about skipped jobs was already there. Copilot recommended switching to `gh pr checks --json`. Not applied: that flag does not exist before gh 2.36 and 2.23 ships in this add-on, where it fails with `unknown flag: --json`. Its premises are also wrong for the path the script takes — piped output carries no header and uses real tabs; the aligned ANSI table is the TTY renderer, which $(... | awk) never gets. Documented the non-TTY contract and the gh-version constraint in the comment and traps.md so this is not "corrected" back into a break later. The underlying worry — a format change reintroducing a false pass — is already answered by the allowlist design, now verified explicitly: a header row lands in the failure branch (exit 1) and a space-aligned table parses to zero rows, so watch keeps waiting. Neither can return 0. Co-Authored-By: Claude Opus 5 --------- Co-authored-by: BirdNET-Go Addon Builder Co-authored-by: Claude Opus 5 --- .../hassio-addon-workflow/references/traps.md | 31 ++++++++- .../scripts/pr_review.sh | 69 ++++++++++++++++--- 2 files changed, 88 insertions(+), 12 deletions(-) diff --git a/.claude/skills/hassio-addon-workflow/references/traps.md b/.claude/skills/hassio-addon-workflow/references/traps.md index d7063b4481..a6fefa7d7d 100644 --- a/.claude/skills/hassio-addon-workflow/references/traps.md +++ b/.claude/skills/hassio-addon-workflow/references/traps.md @@ -254,13 +254,38 @@ without your involvement — another reason a shared checkout goes stale mid-tas **Reviewers**: CodeRabbit (deepest — often runs scripts to prove a claim; reviews ~9 minutes after the PR opens, or on `@coderabbitai review`), chatgpt-codex-connector, Copilot, Codacy. -**Codacy `action_required` is this repo's normal state.** Other open PRs show the same. It -exposes no annotations via the API, so its findings are only visible in the maintainer's Codacy -account. Note it and move on rather than guessing. +**Codacy is red on essentially every add-on PR and gates nothing.** `gh pr checks` reports it as +`fail` (older runs showed `action_required`); #3019, #3044 and #3050 all merged with it failing, +and `master` carries no branch protection, so no check is required in the GitHub sense. It exposes +no annotations via the API, so its findings are only visible in the maintainer's Codacy account. +Note it and move on rather than guessing. `pr_review.sh watch` therefore prints it every poll but +keeps it out of the verdict — the one check on that list, which is a denylist of known noise, not +an allowlist of gates, so a job added to CI later counts as blocking until someone exempts it. **Resolving a review thread requires GraphQL** (`resolveReviewThread`); the REST API cannot do it. `scripts/pr_review.sh` wraps fetch / reply / resolve. +**`gh pr checks` output is TAB-separated, and every blocking gate here has spaces in its name.** +Parsing it with awk's default field splitting truncates each check to its first word and reads the +wrong column as the state: `Codacy Static Code Analysisfail` becomes `Codacy=Static`, and +`Test addon build (wger)pending` becomes `Test=addon`. A `case` over that string then matches +neither `*fail*` nor `*pending*` and falls through to the "all passing" branch — the failure mode +that makes a CI-reporting command lie. `pr_review.sh watch` called #3044 green while Codacy was +red, and on #3042 printed "settled — all passing" while the HA add-on linter was failing; it would +also have called a build that had not started a pass. Use `awk -F'\t'`, judge the state column +alone (never the joined `name=state` text, or a check named `flaky-fail-detector` reads as a +failure), and treat an unrecognised state as a failure instead of letting it reach the passing +branch. Fixed in #3052. + +The TSV is gh's *non-TTY* renderer, which is what `$(gh pr checks ... | awk)` always gets; attached +to a terminal the same command prints a coloured, aligned table with a summary line, so never +sanity-check the format by eye in a shell and assume the script sees that. `gh pr checks --json` +would be sturdier, and Copilot recommends it (#3052), but it does not exist before gh 2.36 and the +add-on ships 2.23 — it fails with `unknown flag: --json`. The parse is therefore built to fail +safe instead: states are allowlisted, so a header row would land in the failure branch and a +space-aligned table would parse to zero rows and keep `watch` waiting. Either way it cannot +return a false pass. + **CHANGELOG heading dates are ISO, whatever the bots' defaults say.** Match the format already in the add-on's file. Repo-wide that is `## (YYYY-MM-DD)`: 7705 dated headings against 363 in `DD-MM-YYYY`, and the newest entry is ISO in 125 of 135 add-ons. Copilot flags an ISO file that diff --git a/.claude/skills/hassio-addon-workflow/scripts/pr_review.sh b/.claude/skills/hassio-addon-workflow/scripts/pr_review.sh index 56fa01e982..6f4570822c 100755 --- a/.claude/skills/hassio-addon-workflow/scripts/pr_review.sh +++ b/.claude/skills/hassio-addon-workflow/scripts/pr_review.sh @@ -8,6 +8,10 @@ # pr_review.sh resolve --all = every unresolved, asks first # pr_review.sh watch [minutes] poll checks (run this backgrounded) # +# watch exits 0 when every blocking check passed *or was skipped* — a PR touching no add-on +# skips all three gates, and it says so — 1 on failure, 2 if it ran out of minutes. Codacy is +# advisory here: printed every poll, excluded from the verdict. +# # Reviewers seen here: coderabbitai (deepest; reviews ~9 min after open, or on # "@coderabbitai review"), chatgpt-codex-connector, Copilot, Codacy. # @@ -21,7 +25,7 @@ REPO="${HASSIO_REPO:-}" [ -z "$REPO" ] && { echo "cannot determine repo; set HASSIO_REPO=owner/name" >&2; exit 1; } echo "repo: $REPO" >&2 CMD="${1:-}"; PR="${2:-}" -[ -z "$CMD" ] || [ -z "$PR" ] && { sed -n '2,16p' "$0" | sed 's/^# \?//'; exit 1; } +[ -z "$CMD" ] || [ -z "$PR" ] && { sed -n '2,20p' "$0" | sed 's/^# \?//'; exit 1; } case "$CMD" in list) @@ -90,25 +94,72 @@ resolve) ;; watch) MINS="${3:-180}" # the addon build alone has taken ~3h; 20 was far too short + # Checks that are red on essentially every add-on PR here and gate nothing: master carries no + # branch protection, and #3019, #3044 and #3050 all merged with Codacy failing. They are kept + # out of the verdict but always printed, so the reader still sees them and can judge. This is + # deliberately a denylist of known noise, not an allowlist of blocking checks — a job added to + # CI later counts as blocking until someone puts it here on purpose. + ADVISORY_CHECKS="Codacy Static Code Analysis" # one per line if more are ever added wfail=2 # not 0: running out of minutes with checks still pending is not a pass + c=""; bstates=""; adv="" for i in $(seq 1 "$MINS"); do - c=$(gh pr checks "$PR" 2> /dev/null | awk '{print $1"="$2}' | tr '\n' ' ') - if [ -z "$c" ]; then + # gh pr checks emits TAB-separated columns with no header when its output is not a TTY, + # which inside this $(... | awk) it never is. (Attached to a terminal it prints a wholly + # different ANSI table; --json would be sturdier still but does not exist before gh 2.36, + # and 2.23 ships here.) Every blocking gate has spaces in its name — "Addon linting + # (wger)", "Test addon build (wger)" — so awk's default separator split them on + # whitespace: "Codacy Static Code Analysisfail" became "Codacy=Static" and the state + # column was never read at all. watch printed "all passing" on a red #3044 and on #3042 + # with the linter failing, and could not see a pending build either. + # If that format ever does change, the allowlist below fails safe rather than passing: a + # header row lands in the failure branch, and a space-aligned table parses to no rows, + # which keeps watch waiting instead of returning 0. + rows=$(gh pr checks "$PR" 2> /dev/null | awk -F'\t' -v ADV="$ADVISORY_CHECKS" ' + BEGIN { n = split(ADV, a, "\n"); for (j = 1; j <= n; j++) adv[a[j]] = 1 } + NF >= 2 { print (($1 in adv) ? "A" : "B") "\t" $1 "=" $2 "\t" $2 }') + if [ -z "$rows" ]; then # Normal in the first minutes after `gh pr create`, and also whenever gh errors. # Calling that "settled" would report success for checks that never ran. echo "[$i] no checks reported yet (gh returned nothing) — still waiting" sleep 60; continue fi + c=$(printf '%s\n' "$rows" | cut -f2 | tr '\n' ' ') echo "[$i] $c" - case "$c" in - *pending*) sleep 60 ;; - *fail* | *error* | *cancel*) echo "settled — with FAILURES (see above)"; wfail=1; break ;; - *) echo "settled — all passing"; wfail=0; break ;; - esac + # Judge the state column only, never the joined name=state line: a check whose NAME + # contains "fail" must not read as a failure. + bstates=$(printf '%s\n' "$rows" | awk -F'\t' '$1 == "B" { print $3 }' | tr '\n' ' ') + adv=$(printf '%s\n' "$rows" | awk -F'\t' '$1 == "A" { print $2 }' | tr '\n' ' ') + if [ -z "$bstates" ]; then + echo " only advisory checks have reported — no blocking check has run yet" + sleep 60; continue + fi + # Allowlist the good states rather than denylisting the bad ones: an unrecognised state + # must land in the failure branch, because falling through to "passing" is this command's + # worst outcome. + nbad=0; npend=0 + for s in $bstates; do + case "$s" in + pass | skipping) ;; + pending) npend=$((npend + 1)) ;; + *) nbad=$((nbad + 1)) ;; + esac + done + if [ "$nbad" -gt 0 ]; then + echo "settled — blocking checks FAILED:" + printf '%s\n' "$rows" | + awk -F'\t' '$1 == "B" && $3 != "pass" && $3 != "skipping" && $3 != "pending" { print " " $2 }' + wfail=1; break + elif [ "$npend" -gt 0 ]; then + sleep 60; continue + else + echo "settled — blocking checks passing"; wfail=0; break + fi done [ "$wfail" -eq 2 ] && echo "gave up after ${MINS}m, checks still unsettled — NOT a pass" + # Printed on pass and on failure alike: it is excluded from the verdict, not hidden. + [ -n "$adv" ] && echo " advisory (non-blocking, not counted in the verdict): $adv" # A PR touching no */config.* skips the CHANGELOG, linter and build jobs outright (#3018). - case "${c:-}" in *skipping*) echo " ...of which some were SKIPPED — a skipped job tested nothing" ;; esac + case " $bstates " in *" skipping "*) echo " ...of which some were SKIPPED — a skipped job tested nothing" ;; esac echo "note: long queues here are usually account runner contention, not your diff." exit "$wfail" ;;