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" ;;