ci(known-failures): close four fail-open / false-positive paths (CIP-3488)

[freshtonic]

Closes CIP-3488.

tasks/test/known-failures.sh — the CI gate #389 introduced — had four independent defects that let it pass a broken state. All four were raised in review on #389, none addressed before merge, all still live on main. This gate exists to stop known-failure registrations from silently rotting, but in its current form it can report success while doing nothing at all — the exact failure mode it was built to prevent.

Fixes

• T14 — fails open (blocking). The registry parser was fully anchored and hardcoded u64: any deviation (u32, indentation, a trailing comment, 1_000) parsed zero rows, printed "nothing to verify", and exit 0. Now: the strict pub const ISSUE_<NAME>: u64 = <n>; parse tolerates indentation / trailing line comments / _ digit separators, and the declaration count is taken independently with a loose matcher — a declared-vs-parsed mismatch is a hard error (exit 2), so format drift can never silently skip a marker.
• T3 — transient failure read as "issue missing" (blocking). Replaced gh issue view … 2>/dev/null || echo "MISSING" (which conflated 5xx / rate-limit / auth / DNS with a real 404) with gh api repos/<repo>/issues/<n>: a genuine (HTTP 404) classifies as NOT_FOUND, everything else as a distinct transient ERROR with a "re-run the gate" hint. A network blip no longer red-lights ci-required under the wrong message.
• T15 — PR numbers pass as issues. Issues and PRs share a number space and gh issue view resolves both. The REST issues object carries a pull_request key iff the number is a PR, so PR numbers are now rejected instead of validating as open issues.
• T16 — not portable. Replaced mapfile (bash 4+) with a bash 3.2-safe while read loop — mise.toml invokes this as bash tasks/test/known-failures.sh and stock macOS /bin/bash is 3.2.

(T2, the set -euo pipefail dead-code branch, was already fixed on main via a || true guard — not included.)

Verification

Tested end-to-end against the live registry and crafted fixtures:

Scenario	Result
real registry (issue #387)	OPEN, gate OK
u32 drift (declared 1, parsed 0)	exit 2, hard error
PR #399 as an ISSUE_*	rejected as PULL REQUEST
missing #99999999	NOT_FOUND — "does not exist"
3_87 + indentation + // comment	parsed 387, OPEN (tolerant)

Also confirmed clean under stock /bin/bash 3.2.57 (T16). CI-only change, no changeset (per CLAUDE.md exclusions).

Summary by CodeRabbit

• Bug Fixes
  • Improved validation of known-failure markers to detect formatting inconsistencies.
  • Added clearer diagnostics for open, closed, missing, and invalid issue references.
  • Prevented pull requests from being incorrectly treated as issues.
  • Improved handling of verification and connectivity errors.
  • Retained checks for unused known-failure markers.

[coderabbitai]

📝 Walkthrough

Walkthrough

The known-failure verification script now uses strict Bash 3.2-compatible marker parsing and REST-based classification of issue targets, with distinct handling for pull requests, missing issues, and verification errors.

Changes

Known failure verification

Layer / File(s)	Summary
Strict marker parsing tasks/test/known-failures.sh	Documents Bash 3.2 constraints, validates ISSUE_* declarations with a strict numeric parser, and fails when declaration counts differ.
Issue classification and diagnostics tasks/test/known-failures.sh	Adds REST issue classification, preserves dead-weight checks, and reports distinct failures for pull requests, missing issues, and classifier errors.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant KnownFailuresScript
  participant GitHubRESTAPI
  participant TestReferences
  KnownFailuresScript->>KnownFailuresScript: Parse known_failure markers
  KnownFailuresScript->>GitHubRESTAPI: Query issue number
  GitHubRESTAPI-->>KnownFailuresScript: Return classification
  KnownFailuresScript->>TestReferences: Check marker references
  TestReferences-->>KnownFailuresScript: Return reference status
  KnownFailuresScript-->>KnownFailuresScript: Emit diagnostics and exit status

Loading

Suggested reviewers: tobyhede

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the CI hardening work in known-failures.sh and matches the main failure-path fixes in this PR.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch james/cip-3488-harden-the-known-failuressh-ci-gate-multiple-fail-open-false

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

tasks/test/known-failures.sh (1)

83-114: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Declare function variables as local to prevent accidental scope leakage.

Although classify_number is currently safely executed inside a subshell (result=$(classify_number "$number")), preventing its variable assignments from polluting the global scope, it doesn't explicitly declare its variables as local.

If this script is ever refactored to call the function directly in the current shell, it will silently overwrite the main loop's $number variable and leak $err_file, $out, $state, and $err globally. Consider scoping these variables locally as a defensive measure.

Note: When making this change, be careful to keep local out separate from the if out=$(...); then assignment. Combining local out=$(...) directly in an if condition masks the exit code of the command substitution in bash.

💡 Proposed defensive refactor

 classify_number() {
-  number="$1"
-  err_file=$(mktemp)
+  local number="$1"
+  local err_file out state is_pr err
+
+  err_file=$(mktemp)
   # On HTTP 200 the bundled `--jq` prints "<state>\t<is_pr>"; on any HTTP/network

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tasks/test/known-failures.sh` around lines 83 - 114, Update classify_number
to declare number, err_file, out, state, is_pr, and err as local variables near
the start of the function. Keep local out separate from the if out=$(gh api
...); then condition so gh’s exit status remains the condition being evaluated.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@tasks/test/known-failures.sh`:
- Around line 83-114: Update classify_number to declare number, err_file, out,
state, is_pr, and err as local variables near the start of the function. Keep
local out separate from the if out=$(gh api ...); then condition so gh’s exit
status remains the condition being evaluated.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: eb77241b-e59a-4246-89d9-c5a1ad694e0a

📥 Commits

Reviewing files that changed from the base of the PR and between 4119c42 and 59aafb7.

📒 Files selected for processing (1)

• tasks/test/known-failures.sh