avm/hassio-addons
1
0
Fork 0
mirror of https://github.com/alexbelgium/hassio-addons.git synced 2026-09-09 19:29:10 +02:00
Code Issues Packages Projects Releases Wiki Activity

skill(hassio-addon-workflow): stop pr_review.sh watch reporting a false pass (#3052)

Browse Source 
* 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 Analysis<TAB>fail   ->  Codacy=Static
    Addon linting (wger)<TAB>pass          ->  Addon=linting
    Test addon build (wger)<TAB>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 <noreply@anthropic.com>

* skill: record PR number in the traps entry

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

---------

Co-authored-by: BirdNET-Go Addon Builder <addon-builder@users.noreply.github.com>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Alexandre
2026-09-07 13:23:28 +02:00
committed by GitHub
parent 15b6736818
commit bcde474c15
2 changed files with 88 additions and 12 deletions
Download Patch File Download Diff File Expand all files Collapse all files

31
.claude/skills/hassio-addon-workflow/references/traps.md
View File

@@ -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 Analysis<TAB>fail` becomes `Codacy=Static`, and
`Test addon build (wger)<TAB>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 `## <version> (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

69
.claude/skills/hassio-addon-workflow/scripts/pr_review.sh
View File

@@ -8,6 +8,10 @@
# pr_review.sh resolve <PR> <THREAD_ID...|--all> --all = every unresolved, asks first
# pr_review.sh watch <PR> [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 Analysis<TAB>fail" 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"
;;