feat(scheduled-failures): report failing scheduled workflow runs to Matrix - #44
Open
CybotTM wants to merge 9 commits into
Open
feat(scheduled-failures): report failing scheduled workflow runs to Matrix#44CybotTM wants to merge 9 commits into
CybotTM wants to merge 9 commits into
Conversation
check-stars.py's rate-limit policy — a 60-second floor for secondary limits, Retry-After and x-ratelimit-reset handling, a 600-second per-run sleep budget and a give-up message that names the limit — was paid for in two broken scheduled runs. A second script needs the same GitHub client, and a second implementation of that policy would simply re-earn those failures, so the client moves to scripts/github_api.py and check-stars.py imports it. The move is docstring-only: is_rate_limited, parse_retry_after, rate_limit_reset_text, rate_limit_wait, spend_rate_limit_budget, describe_rate_limit, github_request and RateLimitError are byte-identical to their previous definitions. Two additions come with it. post_to_matrix now passes a timeout, which the inline webhook POST did not: a wedged webhook used to hold the runner open until the Actions timeout. reset_rate_limit_budget exists so tests cannot inherit each other's budget through the module global. The rate-limit tests move to tests/test_github_api.py, where the code they exercise now lives, and gain two cases: a dict body must cost exactly one request (the Actions endpoints return an envelope, and paginating it would walk thousands of runs), and post_to_matrix must set a timeout. tests/test_check_stars.py gains a regression test for the scrape path in get_dependents. It calls parse_retry_after directly, because it scrapes github.com HTML rather than the API, which made it the one caller the extraction left behind; the break is only reachable through a transient 5xx, so nothing else provoked it. Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
netresearch.github.io's nightly Build & Deploy was red for 13 days and 14 consecutive runs before anyone noticed. GitHub notifies whoever last edited the workflow file, which for a shared reusable workflow is frequently nobody who watches that repo, so the signal reached no one — and scheduled runs are the org's early-warning system for dependency drift, since they install fresh where PR runs sit on warm caches. A daily job walks every non-archived repo in the org and reports transitions rather than states: green -> red once, one reminder a week while it stays red, and a short note on recovery. Each failure message carries the repo, the workflow, the consecutive-failure count, the date the streak started and a link to the run, so triage does not need GitHub. Repos with no scheduled runs are skipped, not reported. Design points worth knowing: State lives in a workflow artifact, matching star-notifications.yml. A committed file would put a commit on main every run; the Actions cache is evicted after 7 days without a read, which is shorter than the reminder interval this has to measure. An artifact can still expire, so a missing baseline produces one summary line naming everything already failing rather than re-announcing each as a new break. Daily at 07:00 UTC, because the runs being watched are overwhelmingly nightly: polling faster buys no detection speed against a 13-day incident and just multiplies the ~200 requests a pass costs. 07:00 UTC puts the report at the start of the CET working day. One request per repo covers the whole history until a high-frequency workflow crowds the page. In netresearch/maint, 98 of the 100 most recent scheduled runs are Star Notifications and only 2 are the nightly Impact Dashboard, so a nightly sibling can fall off the page entirely. When the page is truncated the workflow list is fetched and each missing workflow is queried on its own; when the count of failures runs past the window it is reported as a lower bound rather than as a total. Runs concluding cancelled, skipped, neutral, stale or action_required are ignored in both directions. They are verdicts on GitHub's plumbing, not on the software: counting them red would generate noise, counting them green would silently reset a real failure streak. Needs a new SCHEDULED_FAILURES_PAT secret with Metadata: read and Actions: read on the org; the job's own GITHUB_TOKEN is scoped to this repository and cannot read another repository's workflow runs. Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files
|
The quality gate came back at security rating E on new code. All three findings are real; none is silenced. S2083 (blocker, path injection): STATE_FILE was built from an environment variable and then written to, so anything able to set that variable chose where the process writes. The override existed only for test convenience the tests do not need — they rebind the module attribute directly — so the path is now fixed, matching check-stars.py. S8541 and S8544 (dependency install): `pip install requests` resolved whatever the index offered at run time and would happily build an sdist, executing its setup.py on the runner. Installs now come from a hash-pinned requirements file with --require-hashes and --only-binary :all:, which fixes the entire resolved tree rather than the top-level name. Renovate's config:recommended already manages pip requirements files, so this stays current without manual bumps. Verified by running the exact install command in a clean Python 3.14 venv: wheels exist for every pinned dependency and the hashes check out. The install line needs a block scalar — actionlint caught that `:all:` parses as a YAML mapping otherwise, which would have broken the workflow outright. The two older workflows install the same way and are not touched here: Sonar only gates new code, bs4/pyyaml wheel availability under --only-binary on 3.14 is unverified, and unrelated workflow changes do not belong in this PR. Worth its own change. Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Fixing the path did not clear Sonar's S2083: reading the flow rather than
the rule title shows the source is the state file's CONTENTS, not its
name. json.load returns whatever the downloaded artifact held, that
object becomes `state`, and `state` goes straight back into the write.
The analyzer is pointing at a real weakness even if path traversal is
not it. The rest of the module reads that object assuming a shape —
previous.get("failing"), a timedelta against last_notified, an int
failure count — so one malformed entry was an AttributeError waiting to
kill a run several hundred API calls in, at the point where the state
save happens and the run would otherwise have been useful.
load_state now rebuilds the map from the four fields the transition
rules actually read, coercing each and re-emitting timestamps from
parsed datetimes; a file holding a list, or an entry that is not an
object, degrades to an empty baseline with a warning. save_state takes
the new workflow map rather than the loaded dict, so nothing from the
artifact is carried back into the write untouched. Reads and writes go
through open(..., encoding="utf-8") because a JSON file should not be
decoded with whatever the runner's locale happens to be.
state_entry normalises failing_since on the way in, which the round-trip
test caught: GitHub sends "...Z", the loader re-emits "...+00:00", and
without normalising the file churned representation on every reload.
Verified against the live organisation through a local webhook stub: a
baseline run posts one summary, an immediate second run posts nothing
and reports 186 unchanged with a byte-identical workflow map, and after
backdating the notification stamps by eight days a third run posts
exactly the 10 weekly reminders — including the "at least 5" lower bound
for ofelia, whose failure streak runs past the fetch window.
Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
…ment Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
The baseline this PR produced contained its own counter-example. netresearch/ofelia's "Cleanup Container Images" was reported as failing for 141 days, but it is not a workflow failing for 141 days — it is a workflow deleted in April whose last run happens to be red and always will be. Under the previous logic that entry stayed red forever and earned a weekly reminder in perpetuity: noise nobody can action, in the channel this exists to keep credible. Someone mutes it, and the real signals go with it. Measuring all ten baseline entries found two shapes, not one. The id is absent from GET /actions/workflows, meaning the file was deleted (ofelia); or the state is not `active` — disabled_manually, disabled_inactivity, disabled_fork (claude-code-marketplace-P / Pages). The other eight are live and stay. So the rule is GitHub's own answer, with deliberately no age threshold. A threshold gets it wrong both ways: a monthly cron whose last run is 45 days old is alive and must still be reported, while a workflow deleted yesterday is already dead. It is also precisely what separates retired from merely dormant. The exclusion is never silent — that would be the same disappearing act in a quieter register. It is logged every cycle, named in the baseline message, and a workflow that was being actively reported when it retires gets one closing message instead of just ceasing to appear. Re-adding or re-enabling brings it back on the next poll. Cost: the workflow list is now fetched for every repo that has scheduled runs, two requests instead of one across ~79 repos. The list doubles as the index for the crowded-page gapfill, so it is fetched once and used twice, and a repo with no scheduled runs still costs exactly one. Live dry run: 10 reported entries become 8, with both exclusions named. Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
…sted While verifying the retired-workflow change, a dry run reported netresearch/t3x-rte_ckeditor_image's weekly CI as "failing 1x since 2026-07-13". It is not failing: that run is four back, and the newest is a success from 2026-08-10. The same query fifteen minutes later, and twelve times consecutively after that, came back correctly ordered, so the trigger is not established and this commit does not claim one. What is established is that nothing made the right answer inevitable. summarise_workflow documented "newest run first" as a contract and then relied entirely on the endpoint to honour it — no sort anywhere. Every answer it produces is an ordering claim: which run is latest, whether the workflow is red at all, how long the streak is, when it started. One mis-ordered response turns all four wrong at once, and the failure is silent: a false red, then a false recovery on the following run once the order corrects. Sorting the runs by timestamp before reading them costs one sort per workflow and removes the assumption entirely. The test drives the same runs in three orders — newest first, oldest first, failure first — and requires the same verdict from each; without the sort it fails on the live case's exact shape. Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
CodeQL flagged py/incomplete-url-substring-sanitization (high) on `"netresearch.github.io" in message`: a substring check against a literal that parses as a hostname is the shape of a URL-sanitisation bypass, and the rule cannot tell that this one is asserting a chat message names a repository. The literal was duplicating REPO["name"] anyway, so the assertions now read it from the fixture. That clears the pattern at the source rather than dismissing the alert, and the tests no longer restate a value they already have. Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
CybotTM
added a commit
that referenced
this pull request
Aug 10, 2026
`tests/` exists and passes, but none of the four workflows referenced pytest, unittest or `tests/` — so the suite had never been executed by CI. Two changes to production-behaviour scripts were merged with tests attached that nothing ran. This adds `.github/workflows/tests.yml`, running `pytest tests/` on every pull request and on pushes to `main`, with `contents: read` and both actions pinned to the same SHAs the sibling workflows already use. Dependencies come from a new `requirements/tests.txt`, hash-pinned and wheels-only. `beautifulsoup4` is not optional there: `tests/test_check_stars.py` imports `scripts/check-stars.py` at module scope, so in a venv without it pytest fails during collection rather than in a single test — verified, `ModuleNotFoundError: No module named 'bs4'` at `scripts/check-stars.py:11`, `Interrupted: 1 error during collection`. ## Verification Run locally in a clean venv, with the same two commands the workflow issues: ``` $ pip install --only-binary :all: --require-hashes -r requirements/tests.txt Successfully installed beautifulsoup4-4.15.0 certifi-2026.7.22 charset-normalizer-3.4.9 idna-3.18 iniconfig-2.3.0 packaging-26.3 pluggy-1.6.0 pygments-2.20.0 pytest-9.1.1 requests-2.34.2 soupsieve-2.9.2 typing-extensions-4.16.0 urllib3-2.7.0 $ pytest tests/ -v platform linux -- Python 3.14.6, pytest-9.1.1, pluggy-1.6.0 collected 10 items tests/test_check_stars.py::test_empty_repo_returns_no_dependents_without_scraping PASSED tests/test_check_stars.py::test_get_org_repos_maps_archived_flag PASSED tests/test_check_stars.py::test_archived_repo_skips_token_gated_fetches PASSED tests/test_check_stars.py::test_per_repo_autherror_is_skipped_not_fatal PASSED tests/test_check_stars.py::test_rate_limit_wait_prefers_retry_after PASSED tests/test_check_stars.py::test_rate_limit_wait_waits_for_the_primary_reset PASSED tests/test_check_stars.py::test_rate_limit_wait_floors_secondary_backoff_at_a_minute PASSED tests/test_check_stars.py::test_secondary_rate_limit_waits_minutes_and_stays_within_budget PASSED tests/test_check_stars.py::test_rate_limit_give_up_message_names_the_cause PASSED tests/test_check_stars.py::test_permission_403_is_still_an_autherror PASSED ============================== 10 passed in 0.12s ============================== ``` `actionlint .github/workflows/tests.yml` exits 0. ## Notes No `ruff format` was run and no ruff config was added — at ruff's 88-column default it would reformat every existing file, and this PR adds no Python. `persist-credentials: false` is not set on the checkout, because no sibling workflow in this repo sets it; adding it here alone would be the first instance of a convention rather than an application of one. Worth doing across all five workflows in a separate change if wanted. This does not depend on #44 and shares no file with it. #44 adds `requirements/scheduled-failures.txt`; this adds `requirements/tests.txt`. In whichever order they land, the workflow here picks up the test files #44 brings with it.
Three corrections to the retirement rule, from a closer read of the org. Ofelia is a RENAME, not a deletion. A netresearch/.github template sync on 2026-04-19/20 replaced "Cleanup Container Images" with "Container Retention" at .github/workflows/container-retention.yml, which is alive and active today; only the old identity's run history is frozen red. The rule was already right — a moved file mints a new id and the old one leaves the list — but "the workflow no longer exists" was not, so the reason now reads "renamed or removed". Template syncs rename workflows across the fleet, so this is the shape to expect, not deletion. Keying on id rather than name is load-bearing, and worth stating because the obvious rule is to match names. An id follows the FILE: a sync that edits `name:` in place keeps the id, and matching on name would retire a workflow that is still running on schedule. There is now a test for that case; matching on name fails it. The workflow list must be complete or not believed. An absent id means "retired", so a list short by one page retires live workflows and silences their real failures — worse than the noise the exclusion removes. netresearch/.github already has 63 workflows against a 100-item page. The list is now read to the end and refuses to answer unless the pages add up to total_count; an empty list for a repo that demonstrably has scheduled runs is likewise treated as unreadable rather than as "everything here is retired". Verified against the live org: baseline unchanged at 8 reported and 2 excluded, with skill-repo-skill / A/B Evals Schedule confirmed `active` by direct query rather than assumed. Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
netresearch.github.io's nightlyBuild & Deploywas red for 13 days and 14 consecutive runs before anyone noticed. GitHub's own notification for a failing scheduled run goes to whoever last edited the workflow file, which for a shared reusable workflow is frequently nobody who watches that repo — so the signal reached no one. Scheduled runs are the organisation's early-warning system for dependency drift, because they install fresh where PR runs sit on warm caches; losing that signal is what let two more repos sit red for days after a PHPUnit release.Run against the live organisation, this branch finds that incident exactly —
netresearch.github.io / Build & Deploy (14x, since 2026-07-28 (13 days))— plus seven more failing scheduled workflows nobody was tracking — includingskill-repo-skill / A/B Evals Schedule, red since 2026-07-27 under a workflow confirmedactive.What it does
A daily job walks every non-archived repo in the org and reports transitions, not states, so a workflow that is still red does not produce a message on every poll: 🔴 on green → red, 🔁 one reminder per week while it stays red, 🟢 on recovery, 🗄 when a still-failing workflow is retired, and 📋 a single baseline summary on a first run or an expired state artifact. Every failure message carries the repository, the workflow name, the consecutive-failure count, the date the streak started and a link to the run, so triage does not require opening GitHub. Repositories with no scheduled runs are skipped and never reported.
Runs concluding
cancelled,skipped,neutral,staleoraction_requiredare ignored in both directions — they are verdicts on GitHub's plumbing rather than on the software, so counting them as red would generate noise and counting them as green would silently reset a real failure streak.Reuse rather than a second mechanism
check-stars.py's rate-limit policy from #43 — a 60-second floor for secondary limits,Retry-Afterandx-ratelimit-resethandling, a 600-second per-run sleep budget and a give-up message that names the limit — moves toscripts/github_api.pyand is now shared by both scripts rather than reimplemented weaker. The move is docstring-only:is_rate_limited,parse_retry_after,rate_limit_reset_text,rate_limit_wait,spend_rate_limit_budget,describe_rate_limit,github_requestandRateLimitErrorare byte-identical to their previous definitions, verified by AST-level comparison againstorigin/main. Matrix posting followscheck-stars.py's conventions and the repo iteration followscollect_impact.py's;MATRIX_WEBHOOK_URLis the existing secret.The extraction surfaced one real defect, caught by
ruff check:get_dependentsscrapes github.com HTML rather than the API and therefore callsparse_retry_afterdirectly, which made it the one caller the move left behind. It is fixed and now covered by a regression test that fails withNameErrorwhen the import is removed again.Design decisions
State: a workflow artifact. Matches
star-notifications.yml. A committed state file was rejected because it would put a commit onmainon every run; the Actions cache was rejected because entries are evicted after 7 days without a read, which is shorter than the 7-day reminder interval this has to measure. An artifact can still expire — handled by the baseline summary, which names everything already failing in one line instead of re-announcing each as a fresh break. The file's contents are validated on load rather than trusted, so a malformed artifact degrades to a baseline instead of raising anAttributeErrorseveral hundred API calls into a run.Retired workflows are excluded, by GitHub's answer and never by age. The first baseline this PR produced contained its own counter-example:
ofelia / Cleanup Container Imagesreported as failing for months. It is not a failing workflow — anetresearch/.githubtemplate sync on 2026-04-19/20 renamed it toContainer Retentionat.github/workflows/container-retention.yml, which is alive and active; only the old identity's run history is frozen red. It would have earned a weekly reminder in perpetuity about something nobody can action. The rule needs both halves, and each catches a live case: the id has no entry inGET /actions/workflows(renamed or removed —ofelia), or that entry's state is notactive(disabled_manually—claude-code-marketplace-P / Pages; alsodisabled_inactivity, GitHub's 60-day auto-pause, anddisabled_fork). A disabled workflow still appears in the list, so the first half alone would miss it entirely. Keyed on workflow id, not name: an id follows the file, so a sync that editsname:in place keeps the id and the entry is correctly kept, whereas name-matching would retire a workflow still running on schedule — there is a test for exactly that. There is deliberately no age threshold: it is wrong in both directions, since a monthly cron whose last run is 45 days old is alive and must still be reported while a workflow retired yesterday is already dead, and GitHub's state is precisely what separates retired from merely dormant. Because an absent id means "retired", a workflow list wrong in the short direction would silence real failures, so anything less than a confidently complete list keeps every entry: a failed request, pages that do not add up tototal_count(netresearch/.githubhas 63 workflows against a 100-item page, so the list is read to the end), or an empty list for a repo that demonstrably has scheduled runs. The exclusion is never silent — logged every cycle, named in the baseline message, and a workflow that was being actively reported when it retires gets one closing 🗄 message rather than just ceasing to appear. Re-adding or re-enabling brings it back on the next poll. Trade-off accepted: a repo GitHub auto-paused for inactivity stops being reported, correct in the narrow sense but it does mean a dormant repo's broken nightly goes quiet.The verdict is read from the newest run, not the first one listed. While verifying the above, a dry run reported
t3x-rte_ckeditor_image's weekly CI as "failing 1x since 2026-07-13" when that run is four back and the newest is a success. Twelve consecutive queries afterwards were correctly ordered, so the trigger is not established and the commit does not claim one — butsummarise_workflowdocumented "newest run first" as a contract and then relied entirely on the endpoint to honour it. Every answer it gives is an ordering claim (which run is latest, whether it is red at all, how long the streak is, when it started), so one mis-ordered response turns all four wrong at once, silently: a false red, then a false recovery once the order corrects. It now sorts by timestamp before reading. The test drives the same runs in three orders and requires the same verdict from each.Schedule: daily at 07:00 UTC. The runs being watched are overwhelmingly nightly, so polling faster buys no detection speed and just multiplies the ~280 requests a pass costs; the star notifier's 15-minute cadence would be tens of thousands a day for a signal that changes once. Against a 13-day incident, a worst case of 24 hours is not the constraint worth optimising, and 07:00 UTC puts the report at the start of the CET working day rather than at 03:00.
Two requests per repo that has schedules, one for a repo that does not. The runs page plus the workflow list — the list is the only way to answer "can this still run", and it doubles as the index for the crowded-page gapfill, so it is fetched once and used twice. The gapfill matters because one runs page stops covering a repo as soon as a high-frequency workflow crowds it: in
netresearch/maint, 98 of the 100 most recent scheduled runs are Star Notifications and only 2 are the nightly Impact Dashboard, so a nightly sibling can vanish from the page entirely. When the page is truncated every workflow missing from it is queried on its own, and when a failure streak runs past the window it is reported as "at least N" rather than stated as a total.Verification
Verified at
de1f3cd927913b088dc10a26fb950e1c420205da. No message was ever sent to the real Matrix room.python -m pytest tests/), all green, none making a network call. Both commits are independently green on their own tree. Mutation probes confirm the tests fail in position: stampinglast_notifiedon every cycle, dropping the transition check, disabling the crowded-page gapfill, treatingcancelledas a failure, removing theparse_retry_afterimport, removing the retirement check, treating an unreadable or truncated workflow list as "everything retired", matching on workflow name instead of id, and removing the sort each break exactly the test claiming to guard them.--dry-runover 192 non-archived repos, read-only. The reported set matches an independently instrumented walk of the same org exactly: 8 failing, 2 excluded and named.186 unchanged, workflow map byte-identical to run 1's. Run 3, after backdating the notification stamps by eight days: exactly the 10 weekly reminders due. Requirements 2, 3 and 4 proven end to end rather than only in unit tests.The exact
pip installline was run in a clean Python 3.14 venv: wheels exist for every pinned dependency and the hashes verify.actionlintpasses on all workflows — it caught that--only-binary :all:parses as a YAML mapping unless therun:is a block scalar, which would have broken the workflow outright.ruff checkreports 4 findings, all pre-existing and identical toorigin/main(I001inbackfill_archived_history.py,DTZ003incheck-stars.py,F541incollect_impact.py,B023intest_check_stars.py) — this branch adds none.Before this can work
SCHEDULED_FAILURES_PATdoes not exist yet and must be created: a fine-grained PAT on thenetresearchorg, all repositories,Metadata: read+Actions: read. The job's ownGITHUB_TOKENis scoped to this repository and cannot read another repository's workflow runs, and reusingSTAR_NOTIFICATIONS_PATorIMPACT_DASHBOARD_PATwould couple three schedules to one token's expiry. Until the secret exists the workflow fails with an::error::naming it. Setup steps are in the README.Notes for the reviewer
The brief asked for
ruff formatas well asruff check. The repo has no ruff configuration, no pyproject and no lint job, and existing files run to 225 characters, soruff formatat its 88-column default would reformat all eight files — and even at the repo's own width it flips quote styles across existing code. I ranruff checkand left formatting alone rather than bury this change under a repo-wide reformat.Two things this PR deliberately does not do, both worth their own change. Nothing in CI runs
tests/today, which is the same class of problem this PR is about — a signal nobody receives. Andstar-notifications.ymlandimpact-dashboard.ymlstill install dependencies unpinned and sdist-allowed; Sonar only gates new code so they are not flagged, but the hardening applies equally. I left them out because wheel availability forbeautifulsoup4andpyyamlunder--only-binaryon 3.14 is unverified and a broken nightly dashboard does not belong in this diff.