Repository navigation
fix: filter expired approval runs from health failure candidates - #71164
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Commit pushed:
|
|
✅ Ponytail Reviewer completed successfully! Ponytail ReviewerOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
No actionable blocking issues found in the changed lines. The approval-expiry evidence path fails closed on missing or inaccessible annotations, reuses attempt/update-time cache entries, and keeps filtered failures out of success metrics. Static review only; no tests were run per review instructions.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · 1.94 AIC · ⌖ 0.493 AIC · ⊞ 1.6K
Comment /review to run again
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (384 new lines under 📄 Draft ADR committed: Decision captured: detect approval expiry by fetching and parsing the attempt-specific GitHub run page HTML (because REST payloads omit the annotation), fail-closed, capped at 20 requests per collection and cached by attempt and update time.
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and check that the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. 📋 Michael Nygard ADR Format Reference
All ADRs are stored in
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /diagnosing-bugs — approving with two minor non-blocking suggestions on documentation of implicit invariants.
📋 Key Themes & Highlights
Key Themes
- HTML-scraping fragility: detection depends on an undocumented GitHub DOM attribute with no API contract; the fail-closed design (affirmative evidence required, else
unknown/retained) correctly mitigates the correctness risk, but a short comment would help future maintainers diagnose a sudden rise inunknownevidence. - Representative-replacement subtlety: the candidate-replacement loop in
excludeHealthApprovalExpiryCandidatecan pick an as-yet-unverified run as the new representative since its zero-value status also passes the!= "ignored"check; this self-corrects later in the same collection pass but the invariant isn't obvious from the code alone.
Positive Highlights
- ✅ Excellent test coverage per
/tdd: 9 focused test cases covering annotation parsing (including nested markup, HTML entities, mixed causes, script-tag exclusion, and false positives like page titles/login pages), budget/cache identity (same attempt+update-time reuse, request-limit and API-budget exhaustion), and repository-identity isolation (never trusting a run-provided URL). - ✅ Fails closed by design — missing evidence, inaccessible pages, and ambiguous annotations never establish approval expiry, so genuine failures are never hidden.
- ✅ Clean integration into the existing jobless-run exclusion path without disrupting unrelated
health_sampling.go/health_failures_first.gologic. - ✅ Clear documentation updates in
cli.mdand thehealth_command.gohelp text matching the new behavior.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 55.6 AIC · ⌖ 15.1 AIC · ⊞ 10.2K
Comment /matt to run again
There was a problem hiding this comment.
🟡 Changes recommended
Failure-candidate preservation and collection-stop handling still need correction.
3 open issues
What changed in this PR
This PR updates gh aw health to distinguish expired approval requests from genuine workflow failures without treating excluded runs as successes.
Changes:
- Detect approval-expiry annotations for confirmed jobless failures using cached, budgeted run-page lookups.
- Remove confirmed expiries from failure candidates while retaining evidence and replacing excluded representatives.
- Add regression tests and document the filtering behavior and request limits.
| File | Description |
|---|---|
pkg/cli/health_sampling.go |
Checks jobless attempts for approval expiry during sampling. |
pkg/cli/health_failures_first.go |
Retains the failure inventory and checks jobless candidates. |
pkg/cli/health_command.go |
Advertises approval-expiry filtering and its limits. |
pkg/cli/health_collection.go |
Stores approval evidence and renders text entries. |
pkg/cli/health_approval_expiry.go |
Implements annotation parsing, caching, and candidate exclusion. |
pkg/cli/health_approval_expiry_test.go |
Tests evidence requirements, accounting, caching, and budgets. |
docs/src/content/docs/setup/cli.md |
Documents filtering, evidence fields, and lookup limits. |
Review effort: Max
Preserve bounded failure discovery after expiry exclusions, stop on annotation quota failures, and bound HTML error evidence. Document collection invariants and complete ADR-71164. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
In response to this review notice: accepted the two actionable skills-review suggestions and documented both the unstable DOM selector and replacement-representative invariant. The completion notice itself requires no additional code change. |
|
In response to this gate notice: reviewed and completed the generated ADR and linked it from the PR description. This notification requires no separate code change and is not being treated as proof that the gate passed. |
|
In response to this reviewer result: no code change is needed because the notice contains no specific requested edit. The reported-no-action outcome is not being used as evidence of validation or publication. |
|
In response to this test-quality result: no code change is needed for the notification because it requests no specific edit. Regression checks for this batch were run separately; the reported-no-action result does not substitute for them. |
|
In response to this code-quality result: accepted the non-blocking static review; it identified no actionable change of its own. Its stated limitation remains: that review did not run tests. |
|
In response to the ADR-required finding: completed and linked ADR-71164, documenting the evidence, alternatives, unstable HTML selector, request bounds, and fail-closed behavior. It remains Proposed pending maintainer acceptance; completing it does not claim a passing gate or authorize a workflow dispatch. |
|
🎉 This pull request is included in a new release. Release: |
Summary
GitHub records expired approval requests as failed workflow runs even when no jobs execute. Health already excluded jobless runs from execution metrics, but still surfaced these runs as potential failures.
Detect the explicit approval-expiry annotation on the attempt-specific GitHub run page after confirming that the attempt has zero jobs. Remove confirmed expiries from
failure_candidatesandpotential_issues, while retaining the annotation, attempt URL, and classification in JSON and text evidence. Replace an excluded representative with an older retained candidate so genuine failures remain visible.Annotation lookups share the health request budget, are capped at 20 per collection, and are cached by run attempt and update time. Missing evidence, inaccessible pages, and mixed failure annotations do not establish approval expiry. This uses the run page because REST run/check-suite payloads and approval history omit the expiry annotation.
Confirmed exclusions backfill the bounded failure inventory using already fetched overflow before requesting older pages, so recent expiries cannot hide an older executed failure. Collection-stopping annotation errors propagate immediately, and HTML request-error evidence is bounded before reporting or logging.
ADR: ADR-71164: Detect Approval Expiry via Attempt-Specific Run Page Annotations in Health.
Validation
make build,make fmt, andgo test ./pkg/cli -run '^TestHealth' -count=1, including regressions for both stopping-error paths, bounded HTML error reporting, and same-page/cross-page failure-inventory backfill.golangci-lint run --new-from-rev HEAD --whole-files ./pkg/cli. Custom-linter diagnostics after the final refinements are limited to unchanged files. Schema freshness and impacted unit tests passed in the aggregate gate;make check-workflow-driftpassed separately.approval_expired, zero failure candidates, and the exact annotation evidence. The workflow remainsno_recent_runs, not healthy.No workflows were dispatched or rerun; no credentials or settings were changed.