fix(liveness): judge holds by the newest run, not the held share - #3188
Conversation
Two defects in #3184, both found by running the sweep against this repo right after nine workflows were approved. FALSE POSITIVES. A long outage fills the sample window, so a workflow that has just been approved and executed still scores ~19/20 held and keeps being reported. health-68 succeeded at 05:21:42 and the sweep still called it HELD. A detector that cannot see a recovery is as useless as the silence it replaced. FALSE NEGATIVES, and these are worse. The 90%-share rule silently hid every LOW-FREQUENCY workflow, because a weekly or scheduled workflow's last 20 runs still contain successes from before it was blocked. Nine more held workflows were invisible: agents-keepalive-dispatch-handler (9d), agents-weekly-metrics (19d), health-40-repo-selfcheck (19d), health-41-repo-health (5d), maint-50-tool-version-check (19d), maint-69-sync-labels (22d), maint-72-fix-pr-body-conflicts (7d), maint-81-langsmith-fleet-conformance (19d), maint-coverage-guard (10d). The true count for this repo is 14, not 11 - the gate was under-reporting the thing it exists to report, which is this repo's founding defect wearing a different hat. Held is now decided by the NEWEST run: that is what "blocked right now" means, it sees recoveries immediately, and it catches a single fresh hold the day it appears instead of waiting for a share to build. --threshold survives as an optional noise filter, now defaulting to 0.0, and --min-runs drops to 1 so a rarely-run workflow is judged rather than skipped. Also stops rendering a missing onset as the literal string "Noned"; a held entry with an unknown age now reads "age unknown". That garbling is what exposed the first defect, since a held entry was printing "0 consecutive held runs" - self-evidently contradictory. Tests: 26 pass. test_sweep_does_not_report_a_workflow_that_has_recovered is the regression guard; deleting the newest-run check makes it fail with held_count == 1, and reverting restores 26/26. Verified live: the fixed sweep drops all six recovered workflows and adds the nine hidden ones. ruff, black and actionlint clean. The host health-40-sweep.yml is itself healthy, so the detector is still outside its own blast radius. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 35 minutes Limit details: You’ve used the included review currently available. Your 70 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05f7581682
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| threshold: float = 0.0, | ||
| min_runs: int = 1, |
There was a problem hiding this comment.
Changing these defaults makes a single newest action_required run sufficient to fail the sweep, but docs/INTEGRATION_GUIDE.md:754-756, docs/ops/DURABLE_TRACKING_ISSUES.md:78, and this module's top-level documentation still say the detector requires recent runs to be dominated by holds. Operators may therefore interpret a 1/20 hold failure using the obsolete 90%-share contract; update the contract documentation to describe newest-run detection and the now-optional share filter.
AGENTS.md reference: AGENTS.md:L70-L70
Useful? React with 👍 / 👎.
Automated Status SummaryHead SHA: 9f9913c
Coverage Overview
Coverage Trend
Top Coverage Hotspots (lowest coverage)
Low Coverage Files (<50.0%)
Updated automatically; will refresh on subsequent CI/Docker completions. Keepalive checklistScopeNo scope information available Tasks
Acceptance criteria
|
Follow-up to #3184, found by running the sweep against this repo immediately after nine workflows were approved. Two defects, and the second is worse than the first.
1. False positives — it could not see a recovery
A long outage fills the sample window. When a workflow is finally approved and executes, the held share is still ~19/20, so the sweep kept reporting it.
health-68succeeded at05:21:42and the sweep still called itHELD.A detector that cannot see a recovery is as useless as the silence it replaced — it just becomes noise you learn to ignore.
2. False negatives — it hid every low-frequency workflow
The 90%-share rule silently suppressed workflows that run rarely: a weekly or scheduled workflow's last 20 runs still contain successes from before it was blocked, so the share never reached the threshold. Nine held workflows were invisible:
maint-69-sync-labelsagents-weekly-metricshealth-40-repo-selfcheckmaint-50-tool-version-checkmaint-81-langsmith-fleet-conformancemaint-coverage-guardagents-keepalive-dispatch-handlermaint-72-fix-pr-body-conflictshealth-41-repo-healthThe true count for this repo is 14, not 11. The gate was under-reporting the very thing it exists to report — this repo's founding defect wearing a different hat, and shipped by me in #3184.
The fix
Held is decided by the newest run. That is what "blocked right now" means: it sees recoveries immediately, and it catches a single fresh hold the day it appears instead of waiting for a share to accumulate — which is the whole point of detecting this in a day instead of three weeks.
--thresholdsurvives as an optional noise filter, default now0.0.--min-runsdrops to1, so a rarely-run workflow is judged rather than skipped."Noned"; it readsage unknown.That garbled output is what exposed defect 1 — a held entry was printing
0 consecutive held runs, which is self-evidently contradictory. Worth noting as a small lesson: the cosmetic bug was the only visible symptom of the logic bug.Verification
test_sweep_does_not_report_a_workflow_that_has_recoveredis the regression guard — deleting the newest-run check makes it fail withheld_count == 1; reverting restores 26/26.test_sweep_still_reports_a_single_fresh_holdpins the early-detection case a share threshold would have suppressed.ruff,black,actionlintclean.health-40-sweep.ymlis itself healthy (3× success), so the detector remains outside its own blast radius.Consequence beyond this repo
The same undercount applied to the fleet scan, so the "34 consumer-repo holds" figure in the audit is a floor, not a count. Worth re-running with
--reposafter this merges.🤖 Generated with Claude Code