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