From 1ac6d3ec9861afc4d2ba368014d6554473990581 Mon Sep 17 00:00:00 2001 From: MarketDataApp <112879596+MarketDataApp@users.noreply.github.com> Date: Wed, 23 Sep 2026 07:15:07 -0400 Subject: [PATCH] feat: the audit runs weekly and says so in Slack when the fleet drifts Two changes to `.github/workflows/audit-callers.yml`, and the second is the reason the first is safe. THE SCHEDULE IS NOW WEEKLY, 07:00 UTC on Monday rather than 07:00 UTC daily. What this audit catches is a rename, a deleted caller or a workflow switched off in the Actions UI -- a human edit, not a background process -- and every merge to main runs the whole audit as well. Daily bought little over weekly and spent a run on it. THE AUDIT NOW POSTS TO SLACK WHEN IT FAILS. Email was the only alert, so a weekly schedule would have meant up to seven days between a repository losing its reviews and anyone hearing about it. That silence is precisely what this workflow exists to break, so the workflow must not fall into it. It uses the same path support-agent, daily-standup, proxy and codex-keeper already use -- POST to notify.marketdata.app with the organisation's SLACK_NOTIFY_TOKEN -- so the alarm brings no new credential. An alarm with its own secret is an alarm with its own way to fail. The token goes in an `Authorization: Bearer` header rather than the `?token=` query string the older callers in the fleet use, because a query string lands in the run log, in any redirect, and in the receiving proxy's access log. Three properties the step depends on, each asserted by a test: * `if: failure()`, and `|| true` on the curl. The audit's own verdict stays the job's conclusion. A notifier that can fail the job would report a Slack outage as fleet drift, and the next real failure would then read as another outage. * It runs AFTER both audit steps. The second carries `if: always()` so one broken repository cannot hide the rest; an alarm between them would fire while half the fleet is still unchecked and report one repository as the whole answer. * The token is not in the URL. The gap documented in the README is unchanged and is not made worse: GitHub still disables a scheduled workflow after 60 days of repository inactivity, and a workflow already disabled that way does not process a later push. 737 tests pass. Co-authored-by: MarketDataApp --- .github/workflows/audit-callers.yml | 32 ++++++++++++++++++-- README.md | 7 +++-- tests/test_callers.py | 46 +++++++++++++++++++++++++++++ 3 files changed, 81 insertions(+), 4 deletions(-) diff --git a/.github/workflows/audit-callers.yml b/.github/workflows/audit-callers.yml index 3d0b8e7..da42488 100644 --- a/.github/workflows/audit-callers.yml +++ b/.github/workflows/audit-callers.yml @@ -20,8 +20,11 @@ name: Audit callers # A warning printed into a green run would reproduce the bug it is checking. on: schedule: - # 07:00 UTC daily, before the working day in every timezone the team uses. - - cron: "0 7 * * *" + # 07:00 UTC on Monday, before the working week starts in every timezone + # the team uses. Weekly, because the drift this catches is a rename or a + # deleted caller -- a human edit, not a background process -- and every + # merge here runs the audit as well. + - cron: "0 7 * * 1" # AND ON EVERY MERGE, because GitHub disables a SCHEDULED workflow after 60 # days of repository inactivity -- which would switch this audit off in # exactly the silence it exists to detect. @@ -153,3 +156,28 @@ jobs: # check failed for all six SDK repositories and the audit still # reported green. REVIEWBOT_BOT_TOKEN: ${{ steps.org.outputs.token }} + + # SAY SO, LOUDLY. Email is the only other alert here, and the schedule + # is weekly -- a missed email is up to seven days of a repository not + # being reviewed while nothing says so. That is the silence this whole + # workflow exists to break, so the workflow must not fall into it. + # + # The same path support-agent, daily-standup and codex-keeper already + # use, with the organisation secret every private repository in + # MarketData-App already has. No new credential for the alarm: an alarm + # with its own secret is an alarm with its own way to fail. + # + # `if: failure()` rather than `always()`, and `|| true` at the end: the + # audit's own verdict stays the job's conclusion. A notifier that can + # fail the job would report a Slack outage as fleet drift. + - name: Say so, loudly + if: failure() + env: + SLACK_NOTIFY_TOKEN: ${{ secrets.SLACK_NOTIFY_TOKEN }} + run: | + curl -s -d "Audit callers FAILED. At least one target repository will no longer start a review, and it fails in silence -- every other check there stays green. The run log names the repository and the reason." \ + -H "Authorization: Bearer ${SLACK_NOTIFY_TOKEN}" \ + -H "X-workflow: ${GITHUB_WORKFLOW}" \ + -H "X-repo: ${GITHUB_REPOSITORY}" \ + -H "X-run_id: ${GITHUB_RUN_ID}" \ + "https://notify.marketdata.app/slack/notify?type=ci_failure&channel=alerts" || true diff --git a/README.md b/README.md index 101c3b2..fb66da0 100644 --- a/README.md +++ b/README.md @@ -143,8 +143,11 @@ GITHUB_TOKEN=$(gh auth token) uv run reviewbot audit-callers MarketDataApp/sdk-p It exits non-zero when any repository will not start a review, and says why: a missing or commented-out `workflow_run`, a name matching no workflow, a workflow that does not run on pull requests, or a repository it could not -read. `.github/workflows/audit-callers.yml` runs it over the whole fleet on a -daily schedule and on every push to `main`. +read. `.github/workflows/audit-callers.yml` runs it over the whole fleet every +Monday at 07:00 UTC and on every push to `main`, and a failure posts to the +`alerts` Slack channel through the organisation's `SLACK_NOTIFY_TOKEN`. The +post cannot decide the run's verdict: a Slack outage must not turn a green +fleet red, and a red run must not read as another outage. **One monitoring gap, accepted deliberately.** GitHub disables a scheduled workflow after 60 days with no activity in its repository, and a workflow diff --git a/tests/test_callers.py b/tests/test_callers.py index 07e5d28..17a26ec 100644 --- a/tests/test_callers.py +++ b/tests/test_callers.py @@ -341,6 +341,52 @@ def test_the_second_audit_runs_even_when_the_first_fails(audit_workflow): # --- fixes from the bot's review of PR #21 --------------------------------- +def _alarm_step(spec): + steps = spec["jobs"]["audit"]["steps"] + found = [s for s in steps if "notify.marketdata.app" in s.get("run", "")] + assert found, "the audit has no Slack alarm" + return found[0] + + +def test_the_audit_tells_slack_when_it_fails(audit_workflow): + # Email is the only other alert, and the schedule is weekly -- so a missed + # email is up to seven days of a repository not being reviewed while + # nothing says so. An audit against silent failure cannot itself fail + # quietly. + alarm = _alarm_step(audit_workflow) + assert alarm.get("if") == "failure()" + assert alarm["env"]["SLACK_NOTIFY_TOKEN"] == "${{ secrets.SLACK_NOTIFY_TOKEN }}" + + +def test_the_alarm_runs_after_both_audits(audit_workflow): + # The second audit carries `if: always()`, so it runs after the first + # fails. An alarm placed between them would fire while half the fleet is + # still unchecked, and report one repository as the whole answer. + steps = audit_workflow["jobs"]["audit"]["steps"] + audits = [ + i + for i, s in enumerate(steps) + if s.get("run", "").strip().startswith("uv run reviewbot audit") + ] + assert steps.index(_alarm_step(audit_workflow)) > max(audits) + + +def test_the_alarm_cannot_decide_the_jobs_verdict(audit_workflow): + # `|| true`, because the audit's own verdict is the answer. Without it a + # Slack outage turns a green fleet red, and the next red audit gets read + # as another outage. + assert _alarm_step(audit_workflow)["run"].rstrip().endswith("|| true") + + +def test_the_alarm_keeps_the_token_out_of_the_url(audit_workflow): + # Older callers in the fleet pass `?token=...`. A query string lands in + # the run log, in any redirect and in the receiving proxy's access log; + # the header does not. New code uses the header. + run = _alarm_step(audit_workflow)["run"] + assert "Authorization: Bearer ${SLACK_NOTIFY_TOKEN}" in run + assert "token=" not in run + + def test_the_audit_never_runs_on_a_pull_request(audit_workflow): # c12ba7a7, and it was a real hole rather than a theoretical one. The job # mints installation tokens covering EVERY repository of each owner and