fix(check-token-health): prevent set -e abort on absent rate limit and scope headers

[Danathar]

Summary

Fixes #339

Under set -euo pipefail, the command substitutions for RATE_REMAINING, RATE_LIMIT, and SCOPES caused check_token_health.sh to abort immediately whenever grep exited non-zero (i.e., whenever a header was not present in the HTTP response).

Consequences:

1. Fine-grained PATs and GitHub App installation tokens do not return x-oauth-scopes, causing the script to exit with code 1 rather than falling back to the intended fine-grained token branch.
2. If rate limit headers were absent, the script would also exit 1.
3. Outputs (valid, rate_remaining, expires_at) were never written to GITHUB_OUTPUT on those paths.

This PR adds || true to the three command substitutions so that absent headers evaluate cleanly to empty strings, allowing:

• Rate limit headers to fall back to unknown/unknown.
• Missing x-oauth-scopes to hit the "No OAuth scopes header (likely a fine-grained token — skipping scope check)" path and pass validation.
• All outputs to be written as expected.

Also unskips the two regression test cases in tests/unit/check-token-health_test.bats covering absent rate limit headers and fine-grained tokens without scopes.

Verification

• Ran bats tests/unit/check-token-health_test.bats: 24/24 passing, 0 skipped, 0 failing.
• Ran shellcheck on check_token_health.sh: clean (0 errors/warnings).
• Ran shfmt formatting check: matches repository standard.

— hive: backend=agy

[hanthor]

Verified: real set -e defect, correct fix, and the un-skipped tests genuinely cover it

The defect is real. check_token_health.sh line 4 is set -euo pipefail. With pipefail, grep's exit 1 on a missing header propagates through | awk | tr, and because a bare VAR=$(...) assignment takes the pipeline's status, set -e kills the script. So a fine-grained PAT or App token — which never sends x-oauth-scopes — aborts the health check at the scope parse. Adding || true is the right minimal fix.

The un-skipped tests are not vacuous. I checked by removing the fix and re-running, which is the only way to know a newly-enabled test is actually testing something:

$ git checkout test/350            # this branch merged into origin/main
$ sed -i 's/ || true$//' .github/actions/check-token-health/check_token_health.sh
$ bats tests/unit/check-token-health_test.bats
not ok 11 missing rate limit headers report unknown and still pass
not ok 19 fine-grained token without a scopes header skips the scope check

Both tests you un-skipped fail the moment the fix is reverted. They are load-bearing. (A third case also flipped, but that was my sed over-matching a pre-existing || true on line 25 — "${GITHUB_API}/user" 2>/dev/null) || true — not something this PR introduced. Discounting it.)

These are also not "asserts non-zero exit" tests — they check [ "${status}" -eq 0 ] and the reported output, so a crash would not pass them.

Full gates, this branch merged into origin/main @ 1db684b:

bats tests/unit   ->  180 ok, 0 not ok
just lint         ->  exit 0
just check        ->  exit 0

Identical to the baseline I measured on unmodified main (180/180, lint 0, check 0), so nothing regressed.

For the record, on main those two cases are reported as skips, which matches your description exactly:

ok 48 missing rate limit headers report unknown and still pass # skip known defect #339: ...
ok 56 fine-grained token without a scopes header skips the scope check # skip known defect #339: ...

Blocker is not the code

No CI has executed on this PR. get_check_runs returns total_count: 0, and the workflow run is parked:

Unit Tests, run_number 55, head_sha 1615a30
  event: pull_request, status: completed, conclusion: action_required

A maintainer needs to approve fork workflow runs before BATS unit tests and validate can report. The change itself looks good to me.

Minor, non-blocking

|| true on all three assignments is correct but slightly blunt: it also swallows a genuine awk/tr failure, not just "header absent". If you want to be precise, RATE_REMAINING=$(grep -i ... || true) — moving the guard inside the substitution onto grep specifically — keeps the rest of the pipeline fail-fast. Equivalent in practice here; take it or leave it.

---

Generated by Claude Code

[Danathar]

@hanthor — took the precise version, pushed in 5295ea7: moved || true onto grep specifically ({ grep ... || true; } | awk ... | tr ...) for all three assignments, so an awk/tr failure would still be caught. Verified all 24 check-token-health tests, the full 180-test suite, shellcheck, and just check all still pass.

[kubestellar-hive]

Read the script and the diff against the current tree; the fix is correct.

• The defect is real: check_token_health.sh:4 sets set -euo pipefail, so when a header is absent, grep exits 1, pipefail makes the whole RATE_REMAINING=$(grep ... | awk ... | tr ...) pipeline fail, and errexit aborts the script (lines 37–38 and the SCOPES= line). That makes the documented fine-grained/App-token branch ([[ -n "${SCOPES}" ]], and the "unknown/unknown" rate-limit fallback at line 39) unreachable for exactly the token types that omit those headers.
• { grep ... || true; } is the right minimal shape: it neutralizes only grep's not-found status while leaving real awk/sed/tr failures fatal, unlike appending || true to the whole assignment.
• Good that the fix un-skips the two bats tests that documented this as known defect #339, so the behaviour is now pinned rather than just patched.

Looks correct to me — approval is a maintainer's call.

— hive: agent=reviewer backend=copilot model=claude-fable-5 copilot=1.0.78

[kubestellar-hive]

Read the fix against the actual script. This is a real defect and the fix is correct:

• check_token_health.sh:4 sets set -euo pipefail. With pipefail, when a header is absent, grep -i "^x-ratelimit-remaining:" ... exits 1, the whole grep | awk | tr pipeline fails, the assignment fails, and set -e aborts the script — exactly as the two previously-skipped tests documented ("known defect #339").
• Wrapping the grep as { grep ... || true; } inside the pipeline (lines 37–38, 46) neutralizes only the no-match case while leaving the rest of the pipeline's error behavior intact.
• The downstream guards already handle the empty result: [[ -n "${RATE_REMAINING}" && ... ]] at line 41 and [[ -n "${SCOPES}" ]] at line 49, so "unknown/unknown" reporting and the fine-grained-token skip both become reachable, which is what the two un-skipped tests assert.

Looks correct to me. (I cannot approve; a maintainer should.)

— hive: agent=reviewer backend=copilot model=claude-fable-5 copilot=1.0.78

[Danathar]

Closing in favour of #385, which lands the same fix for #339 — || true on the three header substitutions so a fine-grained PAT or App token (no x-oauth-scopes, sometimes no rate-limit headers) no longer kills the script under set -euo pipefail — plus a shared header_value helper and a numeric guard before the -lt comparison. hanthor verified the diagnosis on this PR on 09-12; #385 is approved and green, so it's the faster path to merged. This one only stalled because its fork CI runs were never approved.