diff --git a/.github/workflows/auto-merge-docs.yml b/.github/workflows/auto-merge-docs.yml index 37098bbb6..890cb076f 100644 --- a/.github/workflows/auto-merge-docs.yml +++ b/.github/workflows/auto-merge-docs.yml @@ -101,12 +101,91 @@ jobs: const docsOnly = files.data.length > 0 && files.data.every(f => isDocsFile(f.filename)); core.setOutput('docs-only', docsOnly); + // Named, so the refusal comment can say *which* file broke the rule instead of only that + // something did. Capped: a PR that breaks the rule in forty files does not need forty lines. + core.setOutput('offenders', files.data.filter(f => !isDocsFile(f.filename)) + .map(f => f.filename).slice(0, 20).join('\n')); core.info(`Docs-only: ${docsOnly} (${files.data.length} files)`); + # A judgement nobody can see did not happen. Until 2026-09-21 this workflow had **no** comment + # step at all (the lesson channel has three), so a refusal existed only as one `core.info` line + # inside a green run: #1801 was given the maintainer's opt-in label four times on 2026-09-19, the + # gate ran and refused it (`Docs-only: false (3 files)`), and from the author's side nothing ever + # happened — the PR sat there labelled and untouched for three days. + - name: Report the refusal on the pull request + if: steps.check.outputs.docs-only != 'true' + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9 + env: + OFFENDERS: ${{ steps.check.outputs.offenders }} + with: + script: | + const marker = ''; + const offenders = (process.env.OFFENDERS || '').split('\n').filter(Boolean); + const listed = offenders.length + ? offenders.map(f => `- \`${f}\``).join('\n') + : '- (the file list came back empty)'; + const body = [ + marker, + '### ⏭️ Auto-merge skipped: this PR is not docs-only', + '', + 'The `auto-merge-eligible` label is on, so this gate looked at the PR — and stopped here,', + 'because the docs-only rule was not satisfied. A PR qualifies only when **every** file is', + 'under `docs/` (or is one of `CONTRIBUTING.md`, `README.md`, `JOIN.md`, `CHANGELOG.md`).', + '`lessons/**`, `docs/.well-known/**` and `docs/index.html` are never auto-merged: lesson', + 'content is executed by agents, and those two paths are filed as machine input and the', + 'site frame.', + '', + 'Files that broke the rule:', + '', + listed, + '', + '**What to do.** Either split the non-docs files into a separate PR, or — if the PR is', + 'docs-only after all, because the list above looks wrong — remove and re-apply the', + '`auto-merge-eligible` label. That re-label is not busywork: this gate only re-evaluates on', + '`labeled`, `unlabeled`, `edited`, `synchronize` and `ready_for_review`, so a corrected PR', + 'that is pushed *after* the label was applied does get looked at again, while one that was', + 'already pushed does not.', + '', + `_Posted by [the docs auto-merge gate](${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/blob/main/.github/workflows/auto-merge-docs.yml); this comment is updated in place, not stacked._`, + ].join('\n'); + + const issue_number = context.payload.pull_request.number; + const { data: comments } = await github.rest.issues.listComments({ + owner: context.repo.owner, repo: context.repo.repo, issue_number, per_page: 100, + }); + const previous = comments.find(c => (c.body || '').includes(marker)); + if (previous) { + await github.rest.issues.updateComment({ + owner: context.repo.owner, repo: context.repo.repo, comment_id: previous.id, body, + }); + core.info(`Updated the refusal comment on #${issue_number}`); + } else { + await github.rest.issues.createComment({ + owner: context.repo.owner, repo: context.repo.repo, issue_number, body, + }); + core.info(`Posted the refusal comment on #${issue_number}`); + } + - name: Enable auto-merge if: steps.check.outputs.docs-only == 'true' env: - GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + # `SHELDON_PAT`, not `GITHUB_TOKEN`, and this is not a preference. + # + # A merge performed with `GITHUB_TOKEN` produces a push that **starts no workflows**, so the + # whole post-merge chain silently does not run for PRs merged here: measured on 2026-09-21, + # the merge commit of #1969 (this gate, GITHUB_TOKEN) has **0** push-triggered runs, while + # #1965 (merged with the PAT) and #1962 have 5 each — `Release Please`, `Leaderboard Watch`, + # `Build Live Feed`, `Update Badge Counts`, `Deploy Documentation` and the rest all keyed on + # `push`. The repository has paid for this lesson twice already (`auto-sync-prs.yml`, and + # `lessons/contrib/ci-github-token-push-does-not-trigger-workflows.md`); this was the third + # place. + # + # It does **not** need the `workflow` scope: GitHub refuses a PAT-without-`workflow` only for + # PRs that modify `.github/workflows/`, and by the rule above this gate merges only docs + # (see the docs-only check: nothing outside `docs/` plus four root files ever gets here). + # Do not widen this to `workflow` "just in case" — that would let this channel merge a change + # to CI itself. + GH_TOKEN: ${{ secrets.SHELDON_PAT }} run: | PR_NUM="${{ github.event.pull_request.number }}" diff --git a/tests/test_auto_merge_docs.py b/tests/test_auto_merge_docs.py index d92af3fac..f3cde2bc5 100644 --- a/tests/test_auto_merge_docs.py +++ b/tests/test_auto_merge_docs.py @@ -174,3 +174,125 @@ def test_the_rule_returns_what_the_gate_needs_for_each_path(tmp_path): "the docs-only rule misclassifies paths:\n" + (result.stderr or result.stdout) ) assert "as expected" in result.stdout + +# ── the two gaps that share this file: what the merge does downstream, and what a refusal says ─── +# +# Both were measured on 2026-09-21, and they fail in opposite directions: the merge was invisible to +# the rest of the repository, and the refusal was invisible to the person who opted in. +# +# Each check is a function over the workflow *text*, so the mutation cases below feed a mutated copy +# through the same code the repository is judged by — asserting "the mutation took" and then asserting +# something trivially true is not a guard, it is decoration. + + +def _steps_of(text: str) -> list[dict]: + return yaml.safe_load(text)["jobs"]["auto-merge"]["steps"] + + +def _step_named(text: str, name: str) -> dict: + for step in _steps_of(text): + if step.get("name") == name: + return step + raise AssertionError(f"no step named {name!r} — the steps are {[s.get('name') for s in _steps_of(text)]}") + + +def _executable(step: dict) -> str: + """A step's code, comments stripped. + + The comments in this workflow name the constructs the rules forbid (that is what they are for), so + a check that reads the raw text is satisfied by the explanation of the bug it is looking for. + """ + text = (step.get("run") or "") + "\n" + (step.get("with", {}).get("script") or "") + return "\n".join(line for line in text.splitlines() if not line.strip().startswith("#")) + + +def merge_token_problems(text: str) -> list[str]: + """`GITHUB_TOKEN` on the merge, or a token widened to the `workflow` scope.""" + problems = [] + step = _step_named(text, "Enable auto-merge") + token = (step.get("env") or {}).get("GH_TOKEN", "") + if "SHELDON_PAT" not in token: + problems.append(f"the merge does not use the PAT (GH_TOKEN={token!r})") + if "GITHUB_TOKEN" in token: + problems.append( + "a `GITHUB_TOKEN` merge produces a push that starts no workflows, so the post-merge chain " + "(Release Please, Leaderboard Watch, the count/badge mirrors, docs deploy) never runs") + if "workflow" in json.dumps(step.get("env") or {}): + problems.append( + "the PAT must not need the `workflow` scope: this gate merges docs only, and that scope " + "would let an auto-merged PR change CI itself") + return problems + + +def refusal_report_problems(text: str) -> list[str]: + """The refusal path must post (and update) a comment that names the reason and the way back in.""" + problems = [] + reporting = [s for s in _steps_of(text) + if (s.get("if") or "").strip() == "steps.check.outputs.docs-only != 'true'"] + if not reporting: + return ["no step runs when docs-only is false: the gate can refuse a PR carrying the " + "maintainer's opt-in label and leave no trace anywhere a human looks"] + code = _executable(reporting[0]) + if "createComment" not in code or "updateComment" not in code: + problems.append( + "the comment must be upserted (create + update): the gate wakes on labeled/unlabeled/" + "edited/synchronize, so one PR can be evaluated many times") + if "OFFENDERS" not in code and "offenders" not in code: + problems.append("the refusal must name the files that broke the rule") + if "label" not in code: + problems.append( + "the refusal must say how to be re-evaluated (re-apply `auto-merge-eligible`): " + "eligibility can change without a push, and the reader cannot guess which events wake " + "this gate") + if "pr merge" in code: + problems.append("the refusal path must not be able to merge") + # A refusal that cannot see the check step's output cannot name the files. + if "steps.check.outputs.offenders" not in json.dumps(reporting[0]): + problems.append("the reporting step does not read the offender list from the check step") + return problems + + +@pytest.mark.parametrize("check", [merge_token_problems, refusal_report_problems]) +def test_the_two_gaps_stay_closed(check): + text = WORKFLOW.read_text(encoding="utf-8") + assert check(text) == [], "; ".join(check(text)) + + +def _mutate(tmp_path, mutate) -> str: + text = WORKFLOW.read_text(encoding="utf-8") + mutated = mutate(text) + assert mutated != text, "the mutation did not take — a mutation that does not mutate asserts nothing" + return mutated + + +def test_reverting_the_merge_token_is_caught(tmp_path): + mutated = _mutate(tmp_path, lambda t: t.replace("GH_TOKEN: ${{ secrets.SHELDON_PAT }}", + "GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}")) + assert merge_token_problems(mutated), "putting `GITHUB_TOKEN` back must be caught" + + +def test_granting_the_workflow_scope_is_caught(tmp_path): + """The tempting "just in case" edit: it would let this channel merge a change to CI.""" + mutated = _mutate(tmp_path, lambda t: t.replace( + "GH_TOKEN: ${{ secrets.SHELDON_PAT }}", + "GH_TOKEN: ${{ secrets.WORKFLOW_PAT }}\n WIDENED: workflow")) + problems = merge_token_problems(mutated) + assert any("workflow" in p for p in problems), problems + + +def test_dropping_the_refusal_report_is_caught(tmp_path): + def drop(text: str) -> str: + lines = text.splitlines(keepends=True) + start = next(i for i, l in enumerate(lines) if "name: Report the refusal on the pull request" in l) + end = next(i for i, l in enumerate(lines) if "name: Enable auto-merge" in l) + return "".join(lines[:start - 1] + lines[end:]) + + assert refusal_report_problems(_mutate(tmp_path, drop)), ( + "removing the reporting step must leave the rule with nothing to point at") + + +def test_a_stacked_comment_is_caught(tmp_path): + """Regression shape: a refusal that only creates comments stacks one per event on a busy PR.""" + mutated = _mutate(tmp_path, lambda t: t.replace("updateComment", "createComment")) + problems = refusal_report_problems(mutated) + assert any("upsert" in p for p in problems), problems