From bfe18d5d68763669e8d5549413aea95fc3c0ced6 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Fri, 23 Oct 2026 14:05:33 -0400 Subject: [PATCH 1/9] Fail pull requests that add agent job folders or hand-off notes Subfleet build jobs keep their state in .job/, and #704 and #699 committed .job/HANDOFF.md and .job/PR_BODY.md on 23 October 2026. The new Agent notes workflow runs .github/check-agent-notes.sh on every pull request, with no path filter, and fails when the merged tree holds a .job/, .job-git/ or .diag/ folder at any depth, or HANDOFF.md, handoffs.jsonl, PR_BODY.md, PR-BODY.md, REPORT.md or PROGRESS.md at the root, in any capitalisation. It mirrors policyengine-uk's root-note guard. .gitignore now ignores the three folders. Fixes #712 Co-Authored-By: Claude Opus 5.5 --- .github/check-agent-notes.sh | 40 ++++ .github/workflows/pr_agent_notes.yaml | 37 +++ .gitignore | 5 + changelog.d/712.added.md | 1 + docs/engineering/skills/github-prs.md | 14 ++ tests/test_check_agent_notes.py | 309 ++++++++++++++++++++++++++ 6 files changed, 406 insertions(+) create mode 100755 .github/check-agent-notes.sh create mode 100644 .github/workflows/pr_agent_notes.yaml create mode 100644 changelog.d/712.added.md create mode 100644 tests/test_check_agent_notes.py diff --git a/.github/check-agent-notes.sh b/.github/check-agent-notes.sh new file mode 100755 index 000000000..dc69c6c56 --- /dev/null +++ b/.github/check-agent-notes.sh @@ -0,0 +1,40 @@ +#!/usr/bin/env bash +# Fail when the checked-out tree holds agent or job notes. +# +# Subfleet build jobs keep their state in a .job/ folder at the root of +# their worktree: HANDOFF.md, PR_BODY.md, env, a git bundle and sometimes a +# whole repo.git. #704 and #699 committed .job/HANDOFF.md and .job/PR_BODY.md +# on 23 October 2026. Agents also write hand-off notes and reports at the +# root of a worktree: policyengine-uk#2376 put REPORT.md and PROGRESS.md on +# its main on 12 October 2026. Their content belongs on the pull request. +# +# It reads the index, which a fresh checkout fills from the commit it checks +# out: on a pull request, the merge of its head into its base. Main has none +# of these paths, so a pull request to main fails exactly when it adds one. +# The patterns are git pathspecs. "**/" matches any depth, the root +# included, and icase ignores capitalisation. The folders are rejected at any +# depth, the note names only at the root, since a note of the same name +# deeper in the tree can be real documentation. +set -euo pipefail + +notes=$(git -c core.quotePath=false ls-files -- \ + ':(glob,icase)**/.job' ':(glob,icase)**/.job/**' \ + ':(glob,icase)**/.job-git' ':(glob,icase)**/.job-git/**' \ + ':(glob,icase)**/.diag' ':(glob,icase)**/.diag/**' \ + ':(top,icase)HANDOFF.md' ':(top,icase)handoffs.jsonl' \ + ':(top,icase)PR_BODY.md' ':(top,icase)PR-BODY.md' \ + ':(top,icase)REPORT.md' ':(top,icase)PROGRESS.md') + +if [ -n "$notes" ]; then + # One path per line: git quotes a name that holds a newline. The prefix + # keeps a path that starts with "::" from being read as a workflow command. + count=0 + while IFS= read -r path; do + printf 'Agent or job note: %s\n' "$path" + count=$((count + 1)) + done <<<"$notes" + first=${notes%%$'\n'*} + echo "::error::${count} agent or job note path(s) in the tree, starting with ${first} (all listed in the log) - post their content on the pull request and remove them." + exit 1 +fi +echo "No agent or job notes in the tree." diff --git a/.github/workflows/pr_agent_notes.yaml b/.github/workflows/pr_agent_notes.yaml new file mode 100644 index 000000000..ebcf4da79 --- /dev/null +++ b/.github/workflows/pr_agent_notes.yaml @@ -0,0 +1,37 @@ +# Workflow that fails a pull request that adds agent or job notes. +# +# Subfleet build jobs keep their state in .job/ at the root of their +# worktree, and #704 and #699 committed .job/HANDOFF.md and .job/PR_BODY.md +# on 23 October 2026. .github/check-agent-notes.sh lists the paths it +# rejects. pr_code_changes.yaml has a path filter, so a pull request that +# adds only such files runs none of its jobs. This workflow has no path +# filter and runs on every pull request, whatever it changes and whatever +# its base: a stacked pull request carries its notes to main when its base +# merges. + +name: Agent notes +on: + pull_request: + workflow_dispatch: + +# A new push to a pull request supersedes the run on its previous head, so +# cancel that run. Manual dispatches are never cancelled. +concurrency: + group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.run_id }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} + +permissions: + contents: read + +jobs: + AgentNotes: + name: No agent or job notes in the tree + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - uses: actions/checkout@v6 + - name: No agent or job notes in the tree + # An explicit bash shell adds pipefail, so a git failure fails the + # step instead of passing it. The script sets it too. + shell: bash + run: .github/check-agent-notes.sh diff --git a/.gitignore b/.gitignore index 3cf0cd51e..4ead7cad8 100644 --- a/.gitignore +++ b/.gitignore @@ -8,3 +8,8 @@ _build/ **/.DS_Store build/ .hypothesis/ +# Agent job state and diagnostics: .github/check-agent-notes.sh fails a pull +# request that adds them. +.job/ +.job-git/ +.diag/ diff --git a/changelog.d/712.added.md b/changelog.d/712.added.md new file mode 100644 index 000000000..d247a0cdc --- /dev/null +++ b/changelog.d/712.added.md @@ -0,0 +1 @@ +CI now fails a pull request whose tree holds agent or job notes: a `.job/`, `.job-git/` or `.diag/` folder at any depth, or `HANDOFF.md`, `handoffs.jsonl`, `PR_BODY.md`, `PR-BODY.md`, `REPORT.md` or `PROGRESS.md` at the repository root. The new Agent notes workflow runs on every pull request, since the code workflow's path filter skips a pull request that adds only such files. `.gitignore` now ignores the three folders. diff --git a/docs/engineering/skills/github-prs.md b/docs/engineering/skills/github-prs.md index b360a8bee..9f24e14c8 100644 --- a/docs/engineering/skills/github-prs.md +++ b/docs/engineering/skills/github-prs.md @@ -43,3 +43,17 @@ same-repository draft PR. Do not add `[codex]`, `[claude]`, `[copilot]`, or other agent labels to PR titles. Use a plain descriptive title. + +## No Agent Notes In The Tree + +Keep job state and working notes out of commits. The Agent notes workflow +(`.github/workflows/pr_agent_notes.yaml`) runs `.github/check-agent-notes.sh` +on every PR and fails when the merged tree contains: + +- a `.job/`, `.job-git/` or `.diag/` folder at any depth. Subfleet build jobs + keep their state, hand-off and PR body in `.job/`. +- `HANDOFF.md`, `handoffs.jsonl`, `PR_BODY.md`, `PR-BODY.md`, `REPORT.md` or + `PROGRESS.md` at the repository root, in any capitalisation. + +Put their content in the PR description or a PR comment instead. `.gitignore` +ignores the three folders, so `git add -A` leaves them out. diff --git a/tests/test_check_agent_notes.py b/tests/test_check_agent_notes.py new file mode 100644 index 000000000..0874c22e1 --- /dev/null +++ b/tests/test_check_agent_notes.py @@ -0,0 +1,309 @@ +"""The agent-notes guard (.github/check-agent-notes.sh) and its workflow. + +Invariants: + +1. The guard fails exactly when the tree holds a note: a path with a + ``.job``, ``.job-git`` or ``.diag`` component at any depth, or a root-level + entry named ``HANDOFF.md``, ``handoffs.jsonl``, ``PR_BODY.md``, + ``PR-BODY.md``, ``REPORT.md`` or ``PROGRESS.md``, matched without regard to + ASCII case. It reports every such path and no other. +2. A git failure fails the guard instead of passing it. +3. No line it prints for a path can be read as a workflow command. +4. The workflow runs it on every pull request: no path or branch filter. + +Invariant 1 is checked on examples and, against a Python reference, on +generated trees. The trees are built in the index alone, which is what the +guard reads, so they need no files on disk. +""" + +from __future__ import annotations + +import os +import shutil +import subprocess +from pathlib import Path + +import pytest +import yaml +from hypothesis import given, settings +from hypothesis import strategies as st + +REPO_ROOT = Path(__file__).resolve().parents[1] +SCRIPT = REPO_ROOT / ".github" / "check-agent-notes.sh" +WORKFLOW = REPO_ROOT / ".github" / "workflows" / "pr_agent_notes.yaml" + +PREFIX = "Agent or job note: " +NOTE_DIRS = (".job", ".job-git", ".diag") +ROOT_NOTES = ( + "HANDOFF.md", + "handoffs.jsonl", + "PR_BODY.md", + "PR-BODY.md", + "REPORT.md", + "PROGRESS.md", +) + +pytestmark = pytest.mark.skipif(shutil.which("git") is None, reason="needs git") + +ISOLATED_GIT = { + **os.environ, + "GIT_CONFIG_GLOBAL": os.devnull, + "GIT_CONFIG_NOSYSTEM": "1", +} + + +def _git(*args, cwd, input=None): + return subprocess.run( + ["git", *args], + cwd=cwd, + env=ISOLATED_GIT, + input=input, + check=True, + capture_output=True, + ) + + +@pytest.fixture(scope="module") +def repo(tmp_path_factory): + root = tmp_path_factory.mktemp("notes") + _git("init", "--quiet", cwd=root) + return root + + +def _tree(repo: Path, paths) -> Path: + """Make the index hold exactly these paths, each a one-line file.""" + (repo / ".git" / "index").unlink(missing_ok=True) + blob = _git("hash-object", "-w", "--stdin", cwd=repo, input=b"x\n") + blob_id = blob.stdout.decode().strip() + entries = b"".join(f"100644 {blob_id}\t{path}".encode() + b"\0" for path in paths) + _git("update-index", "-z", "--add", "--index-info", cwd=repo, input=entries) + return repo + + +def _check(cwd: Path, env=None): + return subprocess.run( + ["bash", str(SCRIPT)], + cwd=cwd, + env=env or ISOLATED_GIT, + capture_output=True, + text=True, + encoding="utf-8", + ) + + +def _reported(result) -> set[str]: + return { + line[len(PREFIX) :] + for line in result.stdout.splitlines() + if line.startswith(PREFIX) + } + + +def is_note(path: str) -> bool: + """Invariant 1's reference: git's icase folds ASCII letters only.""" + parts = [part.encode().lower() for part in path.split("/")] + note_dirs = {name.encode().lower() for name in NOTE_DIRS} + root_notes = {name.encode().lower() for name in ROOT_NOTES} + return any(part in note_dirs for part in parts) or parts[0] in root_notes + + +def test_a_job_hand_off_fails(repo): + """What #704 committed.""" + result = _check(_tree(repo, [".job/HANDOFF.md", "src/policyengine/x.py"])) + assert result.returncode == 1 + assert _reported(result) == {".job/HANDOFF.md"} + assert ( + "::error::1 agent or job note path(s) in the tree, starting with " + ".job/HANDOFF.md" in result.stdout + ) + + +def test_a_normal_change_passes(repo): + result = _check( + _tree( + repo, + [ + "src/policyengine/core/simulation.py", + "tests/test_simulation.py", + "changelog.d/712.added.md", + "README.md", + "CHANGELOG.md", + "docs/engineering/skills/github-prs.md", + ], + ) + ) + assert result.returncode == 0, result.stdout + assert _reported(result) == set() + assert "No agent or job notes in the tree." in result.stdout + + +@pytest.mark.parametrize( + "path", + [ + ".job/PR_BODY.md", + ".job/env", + ".job/repo.git/HEAD", + ".JOB/HANDOFF.md", + "src/.job/HANDOFF.md", + ".job", + ".job-git/HEAD", + "tests/fixtures/.job-git", + ".diag/run.log", + "docs/.Diag/notes.md", + "HANDOFF.md", + "handoff.md", + "handoffs.jsonl", + "PR_BODY.md", + "pr_body.md", + "pr-body.md", + "PR-BODY.md", + "REPORT.md", + "Report.md", + "PROGRESS.md", + "progress.MD", + ], +) +def test_each_note_fails(repo, path): + result = _check(_tree(repo, [path, "src/policyengine/x.py"])) + assert result.returncode == 1, result.stdout + assert _reported(result) == {path} + + +@pytest.mark.parametrize( + "path", + [ + "docs/HANDOFF.md", + "docs/report.md", + "tests/fixtures/PROGRESS.md", + ".jobs/x", + "job/x", + "x.job/y", + "my.job", + ".job.md", + ".job-gitx/y", + ".diagnostics/y", + "HANDOFF.txt", + "REPORTS.md", + "PR BODY.md", + ], +) +def test_near_misses_pass(repo, path): + result = _check(_tree(repo, [path])) + assert result.returncode == 0, result.stdout + + +def test_every_path_of_a_tracked_job_folder_is_reported(repo): + paths = [".job/repo.git/HEAD", ".job/repo.git/config", ".job/HANDOFF.md"] + result = _check(_tree(repo, [*paths, "README.md"])) + assert result.returncode == 1 + assert _reported(result) == set(paths) + assert "::error::3 agent or job note path(s)" in result.stdout + + +def test_no_path_line_is_a_workflow_command(repo): + """Invariant 3: a folder named like a command is printed after a prefix.""" + result = _check(_tree(repo, ["::warning::x/.job/HANDOFF.md", "a\nb/.job/y"])) + assert result.returncode == 1 + commands = [line for line in result.stdout.splitlines() if line.startswith("::")] + assert len(commands) == 1 and commands[0].startswith("::error::2 ") + # git quotes a name that holds a newline, so each path is one line. + assert _reported(result) == {"::warning::x/.job/HANDOFF.md", '"a\\nb/.job/y"'} + + +def test_a_git_failure_fails(tmp_path): + """Invariant 2: outside a repository git fails, and so does the guard.""" + env = {**ISOLATED_GIT, "GIT_CEILING_DIRECTORIES": str(tmp_path.parent)} + result = _check(tmp_path, env=env) + assert result.returncode != 0 + assert "No agent or job notes in the tree." not in result.stdout + + +@pytest.mark.skipif(not (REPO_ROOT / ".git").exists(), reason="needs a git checkout") +def test_this_repository_has_no_notes(): + result = subprocess.run( + ["bash", str(SCRIPT)], cwd=REPO_ROOT, capture_output=True, text=True + ) + assert result.returncode == 0, result.stdout + + +@pytest.mark.skipif(not (REPO_ROOT / ".git").exists(), reason="needs a git checkout") +@pytest.mark.parametrize( + "path", [".job/HANDOFF.md", ".job-git/HEAD", ".diag/run.log", "src/.job/env"] +) +def test_gitignore_keeps_the_folders_out(path): + result = subprocess.run( + ["git", "check-ignore", "--quiet", "--no-index", path], cwd=REPO_ROOT + ) + assert result.returncode == 0, path + + +def test_the_workflow_runs_the_guard_on_every_pull_request(): + """Invariant 4.""" + workflow = yaml.safe_load(WORKFLOW.read_text(encoding="utf-8")) + # PyYAML reads the bare key "on" as True. + triggers = workflow.get("on", workflow.get(True)) + assert "pull_request" in triggers + assert not (triggers["pull_request"] or {}), "no filters on pull_request" + assert workflow["permissions"] == {"contents": "read"} + (job,) = workflow["jobs"].values() + step = next(step for step in job["steps"] if "run" in step) + assert step["run"] == ".github/check-agent-notes.sh" + assert step["shell"] == "bash" + assert "if" not in step and "continue-on-error" not in step + assert "continue-on-error" not in job + assert os.access(SCRIPT, os.X_OK) + + +# Invariant 1 on generated trees. Segments mix the note names, in any ASCII +# case, with near misses and arbitrary names, including glob characters, +# colons and letters outside ASCII. + +NEAR_MISSES = (".jobs", "job", ".job.md", "x.job", ".job-gi", ".dia", "HANDOFF") + + +def _any_case(name: str): + return st.lists(st.booleans(), min_size=len(name), max_size=len(name)).map( + lambda upper: "".join( + c.upper() if u else c.lower() for c, u in zip(name, upper) + ) + ) + + +SEGMENT = st.one_of( + st.sampled_from(NOTE_DIRS + ROOT_NOTES + NEAR_MISSES).flatmap(_any_case), + st.text( + alphabet="abcxyzHJ019._- :[]*?éüßſİ", + min_size=1, + max_size=8, + ).filter( + lambda segment: ( + segment not in (".", "..") + and segment.rstrip(". ").lower() not in (".git", "git~1", "") + ) + ), +) +PATH = st.lists(SEGMENT, min_size=1, max_size=4).map("/".join) + + +def _without_conflicts(paths): + """A path cannot be both a file and a folder of another path.""" + unique = sorted(set(paths)) + return [ + path + for path in unique + if not any(other.startswith(path + "/") for other in unique) + ] + + +TREE = st.lists(PATH, max_size=8).map(_without_conflicts) + + +@settings(max_examples=200, deadline=None) +@given(paths=TREE) +def test_the_guard_matches_the_reference_on_generated_trees(repo, paths): + expected = {path for path in paths if is_note(path)} + result = _check(_tree(repo, paths)) + assert result.returncode == (1 if expected else 0), (paths, result.stdout) + assert _reported(result) == expected + if expected: + assert f"::error::{len(expected)} agent or job note path(s)" in result.stdout From 682dccdf872ac0e784914bb3f3e68144b4ad6614 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Fri, 23 Oct 2026 14:06:49 -0400 Subject: [PATCH 2/9] Demonstration: commit a sample .job/HANDOFF.md, which the Agent notes check must reject Reverted in the next commit; the PR is squash-merged, so neither reaches main. Co-Authored-By: Claude Opus 5.5 --- .job/HANDOFF.md | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .job/HANDOFF.md diff --git a/.job/HANDOFF.md b/.job/HANDOFF.md new file mode 100644 index 000000000..dab2609e8 --- /dev/null +++ b/.job/HANDOFF.md @@ -0,0 +1,3 @@ +# Sample hand-off + +A sample note that shows the Agent notes check failing. The next commit reverts it. From f67ec7f3a091531d750005afa6bdf40888ea38e6 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Fri, 23 Oct 2026 14:07:26 -0400 Subject: [PATCH 3/9] Revert the demonstration .job/HANDOFF.md The Agent notes check rejected it (run 40000640876). Co-Authored-By: Claude Opus 5.5 --- .job/HANDOFF.md | 3 --- 1 file changed, 3 deletions(-) delete mode 100644 .job/HANDOFF.md diff --git a/.job/HANDOFF.md b/.job/HANDOFF.md deleted file mode 100644 index dab2609e8..000000000 --- a/.job/HANDOFF.md +++ /dev/null @@ -1,3 +0,0 @@ -# Sample hand-off - -A sample note that shows the Agent notes check failing. The next commit reverts it. From 0fe7a2dad9b48a4cb705f134068134bc5602d1dc Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Fri, 23 Oct 2026 15:09:08 -0400 Subject: [PATCH 4/9] Run the agent-notes guard on pull_request_target against the head commit The review of #713 (GPT-6.1 Sol) found three gaps in the pull_request version. A "[skip ci]" head commit skipped it. Retargeting a stacked pull request to main did not re-run it. And a path holding the legacy "##[" command syntax could stop the runner's commands and swallow the error annotation. The workflow now runs main's guard on pull_request_target, which skip instructions do not affect and which also runs on conflicted pull requests. It fetches the head commit as data and checks that commit's tree through a temporary index, so the base, and so a retarget, cannot change the result. A pull request cannot turn the guard off by editing it. The script prints the paths between stop-commands and a random resume token, and percent-encodes the annotation data. Co-Authored-By: Claude Opus 5.5 --- .github/check-agent-notes.sh | 31 +++- .github/workflows/pr_agent_notes.yaml | 41 ++++- changelog.d/712.added.md | 2 +- docs/engineering/skills/github-prs.md | 8 +- tests/test_check_agent_notes.py | 230 +++++++++++++++++++++----- 5 files changed, 253 insertions(+), 59 deletions(-) diff --git a/.github/check-agent-notes.sh b/.github/check-agent-notes.sh index dc69c6c56..11553303c 100755 --- a/.github/check-agent-notes.sh +++ b/.github/check-agent-notes.sh @@ -1,5 +1,12 @@ #!/usr/bin/env bash -# Fail when the checked-out tree holds agent or job notes. +# Fail when a tree holds agent or job notes. +# +# Usage: .github/check-agent-notes.sh [COMMIT] +# +# With no argument it lists the index, which a fresh checkout fills from the +# commit it checks out. With a commit, it lists that commit's tree through a +# temporary index, without checking it out: CI fetches a pull request's head +# as data and never runs anything from it. # # Subfleet build jobs keep their state in a .job/ folder at the root of # their worktree: HANDOFF.md, PR_BODY.md, env, a git bundle and sometimes a @@ -8,15 +15,19 @@ # root of a worktree: policyengine-uk#2376 put REPORT.md and PROGRESS.md on # its main on 12 October 2026. Their content belongs on the pull request. # -# It reads the index, which a fresh checkout fills from the commit it checks -# out: on a pull request, the merge of its head into its base. Main has none -# of these paths, so a pull request to main fails exactly when it adds one. # The patterns are git pathspecs. "**/" matches any depth, the root # included, and icase ignores capitalisation. The folders are rejected at any # depth, the note names only at the root, since a note of the same name # deeper in the tree can be real documentation. set -euo pipefail +if [ $# -gt 0 ]; then + scratch=$(mktemp -d) + trap 'rm -rf "$scratch"' EXIT + export GIT_INDEX_FILE="$scratch/index" + git read-tree "$1^{tree}" +fi + notes=$(git -c core.quotePath=false ls-files -- \ ':(glob,icase)**/.job' ':(glob,icase)**/.job/**' \ ':(glob,icase)**/.job-git' ':(glob,icase)**/.job-git/**' \ @@ -26,14 +37,22 @@ notes=$(git -c core.quotePath=false ls-files -- \ ':(top,icase)REPORT.md' ':(top,icase)PROGRESS.md') if [ -n "$notes" ]; then - # One path per line: git quotes a name that holds a newline. The prefix - # keeps a path that starts with "::" from being read as a workflow command. + # The paths come from the pull request, so the runner must not read them + # as workflow commands: "::" at the start of a line, or the older "##[" + # anywhere in it. stop-commands with a random token turns both off until + # the token comes back. Git quotes a name that holds a newline, so each + # path is one line. + token=$(od -An -N16 -tx1 /dev/urandom | tr -d ' \n') + echo "::stop-commands::${token}" count=0 while IFS= read -r path; do printf 'Agent or job note: %s\n' "$path" count=$((count + 1)) done <<<"$notes" + echo "::${token}::" + # Annotation data decodes "%", so encode it. first=${notes%%$'\n'*} + first=${first//%/%25} echo "::error::${count} agent or job note path(s) in the tree, starting with ${first} (all listed in the log) - post their content on the pull request and remove them." exit 1 fi diff --git a/.github/workflows/pr_agent_notes.yaml b/.github/workflows/pr_agent_notes.yaml index ebcf4da79..19438eede 100644 --- a/.github/workflows/pr_agent_notes.yaml +++ b/.github/workflows/pr_agent_notes.yaml @@ -1,24 +1,38 @@ -# Workflow that fails a pull request that adds agent or job notes. +# Workflow that fails a pull request whose head holds agent or job notes. # # Subfleet build jobs keep their state in .job/ at the root of their # worktree, and #704 and #699 committed .job/HANDOFF.md and .job/PR_BODY.md # on 23 October 2026. .github/check-agent-notes.sh lists the paths it -# rejects. pr_code_changes.yaml has a path filter, so a pull request that -# adds only such files runs none of its jobs. This workflow has no path -# filter and runs on every pull request, whatever it changes and whatever -# its base: a stacked pull request carries its notes to main when its base -# merges. +# rejects. +# +# It runs on pull_request_target, not pull_request, so that: +# - a "[skip ci]" in the head commit cannot skip it (skip instructions apply +# to push and pull_request only); +# - it runs on a pull request with a merge conflict, which pull_request skips; +# - it runs main's guard, so a pull request cannot turn it off by editing +# this file or the script. A change to either takes effect when it merges; +# tests/test_check_agent_notes.py tests it before then. +# It checks the head commit's whole tree, not the merge with the base, so +# retargeting a stacked pull request cannot change the result. Main has +# none of these paths, so a pull request based on main fails exactly when +# it adds one. pull_request_target has no path filter here and runs for +# every base branch. +# +# pull_request_target runs with this repository's token, so the job must +# never run code from the pull request: it checks out main, fetches the head +# commit as data, and reads its tree into a temporary index. The token can +# only read, and no secret is used. name: Agent notes on: - pull_request: + pull_request_target: workflow_dispatch: # A new push to a pull request supersedes the run on its previous head, so # cancel that run. Manual dispatches are never cancelled. concurrency: group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.run_id }} - cancel-in-progress: ${{ github.event_name == 'pull_request' }} + cancel-in-progress: ${{ github.event_name == 'pull_request_target' }} permissions: contents: read @@ -29,9 +43,18 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 5 steps: + # Main (the commit that triggered a manual run), never the pull + # request's code. - uses: actions/checkout@v6 + - name: Fetch the pull request's head commit as data + if: github.event_name == 'pull_request_target' + env: + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + run: git fetch --no-tags --depth=1 origin "$HEAD_SHA" - name: No agent or job notes in the tree # An explicit bash shell adds pipefail, so a git failure fails the # step instead of passing it. The script sets it too. shell: bash - run: .github/check-agent-notes.sh + env: + COMMIT: ${{ github.event.pull_request.head.sha || github.sha }} + run: .github/check-agent-notes.sh "$COMMIT" diff --git a/changelog.d/712.added.md b/changelog.d/712.added.md index d247a0cdc..88dc73558 100644 --- a/changelog.d/712.added.md +++ b/changelog.d/712.added.md @@ -1 +1 @@ -CI now fails a pull request whose tree holds agent or job notes: a `.job/`, `.job-git/` or `.diag/` folder at any depth, or `HANDOFF.md`, `handoffs.jsonl`, `PR_BODY.md`, `PR-BODY.md`, `REPORT.md` or `PROGRESS.md` at the repository root. The new Agent notes workflow runs on every pull request, since the code workflow's path filter skips a pull request that adds only such files. `.gitignore` now ignores the three folders. +CI now fails a pull request whose head commit holds agent or job notes: a `.job/`, `.job-git/` or `.diag/` folder at any depth, or `HANDOFF.md`, `handoffs.jsonl`, `PR_BODY.md`, `PR-BODY.md`, `REPORT.md` or `PROGRESS.md` at the repository root. The new Agent notes workflow runs main's copy of `.github/check-agent-notes.sh` on every pull request, through `pull_request_target`, so neither the code workflow's path filter nor `[skip ci]` skips it, and it reads the head commit as data without running code from it. `.gitignore` now ignores the three folders. diff --git a/docs/engineering/skills/github-prs.md b/docs/engineering/skills/github-prs.md index 9f24e14c8..658ccc4de 100644 --- a/docs/engineering/skills/github-prs.md +++ b/docs/engineering/skills/github-prs.md @@ -48,7 +48,7 @@ titles. Use a plain descriptive title. Keep job state and working notes out of commits. The Agent notes workflow (`.github/workflows/pr_agent_notes.yaml`) runs `.github/check-agent-notes.sh` -on every PR and fails when the merged tree contains: +on every PR and fails when the PR's head commit contains: - a `.job/`, `.job-git/` or `.diag/` folder at any depth. Subfleet build jobs keep their state, hand-off and PR body in `.job/`. @@ -57,3 +57,9 @@ on every PR and fails when the merged tree contains: Put their content in the PR description or a PR comment instead. `.gitignore` ignores the three folders, so `git add -A` leaves them out. + +The workflow runs on `pull_request_target`, so `[skip ci]` does not skip it +and it runs main's copy of the check: a change to the workflow or the script +takes effect only once it merges. Test such a change with +`tests/test_check_agent_notes.py`, or run the script on a commit: +`.github/check-agent-notes.sh `. diff --git a/tests/test_check_agent_notes.py b/tests/test_check_agent_notes.py index 0874c22e1..ce7be539a 100644 --- a/tests/test_check_agent_notes.py +++ b/tests/test_check_agent_notes.py @@ -7,13 +7,18 @@ entry named ``HANDOFF.md``, ``handoffs.jsonl``, ``PR_BODY.md``, ``PR-BODY.md``, ``REPORT.md`` or ``PROGRESS.md``, matched without regard to ASCII case. It reports every such path and no other. -2. A git failure fails the guard instead of passing it. -3. No line it prints for a path can be read as a workflow command. -4. The workflow runs it on every pull request: no path or branch filter. +2. Given a commit, it judges that commit's tree, whatever the index holds, + and leaves the index as it was. +3. A git failure fails the guard instead of passing it. +4. The runner reads no path it prints as a workflow command: the only + commands are its own stop-commands, the matching resume and one error. +5. The workflow runs main's guard on every pull request's head, on an event + that "[skip ci]" cannot skip, without running code from the pull request. Invariant 1 is checked on examples and, against a Python reference, on -generated trees. The trees are built in the index alone, which is what the -guard reads, so they need no files on disk. +generated trees, where invariant 2 is checked against the index mode. The +trees are built in the index alone, which is what the guard reads, so they +need no files on disk. """ from __future__ import annotations @@ -49,6 +54,10 @@ **os.environ, "GIT_CONFIG_GLOBAL": os.devnull, "GIT_CONFIG_NOSYSTEM": "1", + "GIT_AUTHOR_NAME": "test", + "GIT_AUTHOR_EMAIL": "test@example.org", + "GIT_COMMITTER_NAME": "test", + "GIT_COMMITTER_EMAIL": "test@example.org", } @@ -80,9 +89,16 @@ def _tree(repo: Path, paths) -> Path: return repo -def _check(cwd: Path, env=None): +def _commit(repo: Path, paths) -> str: + """A commit whose tree holds exactly these paths.""" + tree = _git("write-tree", cwd=_tree(repo, paths)).stdout.decode().strip() + commit = _git("commit-tree", tree, "-m", "tree", cwd=repo) + return commit.stdout.decode().strip() + + +def _check(cwd: Path, *args, env=None): return subprocess.run( - ["bash", str(SCRIPT)], + ["bash", str(SCRIPT), *args], cwd=cwd, env=env or ISOLATED_GIT, capture_output=True, @@ -107,14 +123,85 @@ def is_note(path: str) -> bool: return any(part in note_dirs for part in parts) or parts[0] in root_notes +# How the GitHub Actions runner reads a step's output, from actions/runner's +# src/Runner.Common/ActionCommand.cs (TryParseV2, TryParse, UnescapeData) and +# src/Runner.Worker/ActionCommandManager.cs (TryProcessCommand). A line is a +# command when, after leading whitespace, it starts with "::name ...::data", +# or when "##[name ...]data" appears anywhere in it, and the name is +# registered. While commands are stopped, only the stop token resumes them. + +RUNNER_COMMANDS = { + "add-mask", + "add-matcher", + "add-path", + "debug", + "echo", + "endgroup", + "error", + "group", + "notice", + "remove-matcher", + "save-state", + "set-env", + "set-output", + "stop-commands", + "warning", +} + + +def _parse_command(line: str, registered: set[str]): + message = line.lstrip() + if message.startswith("::"): + end = message.find("::", 2) + if end >= 0: + name = message[2:end].split(" ", 1)[0] + if name in registered: + return name, message[end + 2 :] + start = line.find("##[") + if start >= 0: + end = line.find("]", start) + if end >= 0: + name = line[start + 3 : end].split(" ", 1)[0] + if name in registered: + return name, line[end + 1 :] + return None + + +def _unescape_data(data: str) -> str: + return data.replace("%0D", "\r").replace("%0A", "\n").replace("%25", "%") + + +def _runner_commands(stdout: str) -> list[tuple[str, str]]: + registered = set(RUNNER_COMMANDS) + processed, token = [], None + for line in stdout.splitlines(): + command = _parse_command(line, registered) + if command is None: + continue + name, data = command + if token is not None: + if name.lower() == token.lower(): + processed.append((name, data)) + registered.discard(token) + token = None + continue + if name == "stop-commands": + token = data + registered.add(token) + processed.append((name, _unescape_data(data))) + return processed + + def test_a_job_hand_off_fails(repo): """What #704 committed.""" result = _check(_tree(repo, [".job/HANDOFF.md", "src/policyengine/x.py"])) assert result.returncode == 1 assert _reported(result) == {".job/HANDOFF.md"} - assert ( - "::error::1 agent or job note path(s) in the tree, starting with " - ".job/HANDOFF.md" in result.stdout + assert _runner_commands(result.stdout)[-1] == ( + "error", + "1 agent or job note path(s) in the tree, starting with .job/HANDOFF.md " + "(all listed in the log) - post their content on the pull request and " + "remove them.", ) @@ -134,6 +221,7 @@ def test_a_normal_change_passes(repo): ) assert result.returncode == 0, result.stdout assert _reported(result) == set() + assert _runner_commands(result.stdout) == [] assert "No agent or job notes in the tree." in result.stdout @@ -200,28 +288,72 @@ def test_every_path_of_a_tracked_job_folder_is_reported(repo): assert "::error::3 agent or job note path(s)" in result.stdout -def test_no_path_line_is_a_workflow_command(repo): - """Invariant 3: a folder named like a command is printed after a prefix.""" - result = _check(_tree(repo, ["::warning::x/.job/HANDOFF.md", "a\nb/.job/y"])) +def test_a_commit_is_judged_by_its_tree_not_the_index(repo): + """Invariant 2.""" + with_note = _commit(repo, [".job/HANDOFF.md", "src/policyengine/x.py"]) + clean = _commit(repo, ["src/policyengine/x.py"]) + index = _tree(repo, ["PROGRESS.md"]) / ".git" / "index" + before = index.read_bytes() + + result = _check(repo, with_note) + assert result.returncode == 1, result.stdout + assert _reported(result) == {".job/HANDOFF.md"} + + result = _check(repo, clean) + assert result.returncode == 0, result.stdout + + assert index.read_bytes() == before + assert _reported(_check(repo)) == {"PROGRESS.md"} + + +def test_the_runner_reads_no_path_as_a_command(repo): + """Invariant 4, with names built from both command syntaxes. + + Before stop-commands, a "##[stop-commands]" folder name stopped the + runner's commands and swallowed the error annotation. + """ + paths = [ + ".job/##[stop-commands]agent-note-stop", + ".job/##[add-mask]x", + "::warning::x/.job/HANDOFF.md", + " ::error::forged/.job/y", + "a\nb/.job/y", + ] + result = _check(_tree(repo, paths)) assert result.returncode == 1 - commands = [line for line in result.stdout.splitlines() if line.startswith("::")] - assert len(commands) == 1 and commands[0].startswith("::error::2 ") - # git quotes a name that holds a newline, so each path is one line. - assert _reported(result) == {"::warning::x/.job/HANDOFF.md", '"a\\nb/.job/y"'} + commands = _runner_commands(result.stdout) + assert [name for name, _ in commands][0] == "stop-commands" + token = commands[0][1] + assert len(token) == 32 and all(c in "0123456789abcdef" for c in token) + assert [name for name, _ in commands] == ["stop-commands", token, "error"] + assert commands[2][1].startswith("5 agent or job note path(s) in the tree") + # Git quotes a name that holds a newline, so each path is one line. + assert _reported(result) == {*paths[:4], '"a\\nb/.job/y"'} + + +def test_the_annotation_shows_a_percent_sign_as_written(repo): + result = _check(_tree(repo, [".job/%0Aforged%25"])) + (*_, (name, data)) = _runner_commands(result.stdout) + assert name == "error" + assert "starting with .job/%0Aforged%25 (all listed" in data -def test_a_git_failure_fails(tmp_path): - """Invariant 2: outside a repository git fails, and so does the guard.""" +def test_a_git_failure_fails(tmp_path, repo): + """Invariant 3: outside a repository, or given no commit, git fails.""" env = {**ISOLATED_GIT, "GIT_CEILING_DIRECTORIES": str(tmp_path.parent)} - result = _check(tmp_path, env=env) - assert result.returncode != 0 - assert "No agent or job notes in the tree." not in result.stdout + for result in ( + _check(tmp_path, env=env), + _check(repo, "0123456789abcdef0123456789abcdef01234567"), + ): + assert result.returncode != 0 + assert "No agent or job notes in the tree." not in result.stdout @pytest.mark.skipif(not (REPO_ROOT / ".git").exists(), reason="needs a git checkout") -def test_this_repository_has_no_notes(): +@pytest.mark.parametrize("args", [(), ("HEAD",)]) +def test_this_repository_has_no_notes(args): result = subprocess.run( - ["bash", str(SCRIPT)], cwd=REPO_ROOT, capture_output=True, text=True + ["bash", str(SCRIPT), *args], cwd=REPO_ROOT, capture_output=True, text=True ) assert result.returncode == 0, result.stdout @@ -237,26 +369,37 @@ def test_gitignore_keeps_the_folders_out(path): assert result.returncode == 0, path -def test_the_workflow_runs_the_guard_on_every_pull_request(): - """Invariant 4.""" - workflow = yaml.safe_load(WORKFLOW.read_text(encoding="utf-8")) +def test_the_workflow_runs_mains_guard_on_every_pull_request_head(): + """Invariant 5.""" + text = WORKFLOW.read_text(encoding="utf-8") + workflow = yaml.safe_load(text) # PyYAML reads the bare key "on" as True. triggers = workflow.get("on", workflow.get(True)) - assert "pull_request" in triggers - assert not (triggers["pull_request"] or {}), "no filters on pull_request" + # "[skip ci]" skips push and pull_request runs only. + assert set(triggers) == {"pull_request_target", "workflow_dispatch"} + assert triggers["pull_request_target"] is None, "no filters or types" assert workflow["permissions"] == {"contents": "read"} + assert "secrets." not in text (job,) = workflow["jobs"].values() - step = next(step for step in job["steps"] if "run" in step) - assert step["run"] == ".github/check-agent-notes.sh" - assert step["shell"] == "bash" - assert "if" not in step and "continue-on-error" not in step assert "continue-on-error" not in job + checkout, fetch, guard = job["steps"] + # Main's code, never the pull request's: no ref, so the default branch. + assert checkout == {"uses": "actions/checkout@v6"} + assert fetch["if"] == "github.event_name == 'pull_request_target'" + assert fetch["env"] == {"HEAD_SHA": "${{ github.event.pull_request.head.sha }}"} + assert fetch["run"] == 'git fetch --no-tags --depth=1 origin "$HEAD_SHA"' + assert guard["run"] == '.github/check-agent-notes.sh "$COMMIT"' + assert guard["env"] == { + "COMMIT": "${{ github.event.pull_request.head.sha || github.sha }}" + } + assert guard["shell"] == "bash" + assert "if" not in guard and "continue-on-error" not in guard assert os.access(SCRIPT, os.X_OK) -# Invariant 1 on generated trees. Segments mix the note names, in any ASCII -# case, with near misses and arbitrary names, including glob characters, -# colons and letters outside ASCII. +# Invariants 1 and 2 on generated trees. Segments mix the note names, in any +# ASCII case, with near misses and arbitrary names, including glob +# characters, colons and letters outside ASCII. NEAR_MISSES = (".jobs", "job", ".job.md", "x.job", ".job-gi", ".dia", "HANDOFF") @@ -298,12 +441,15 @@ def _without_conflicts(paths): TREE = st.lists(PATH, max_size=8).map(_without_conflicts) -@settings(max_examples=200, deadline=None) +@settings(max_examples=150, deadline=None) @given(paths=TREE) def test_the_guard_matches_the_reference_on_generated_trees(repo, paths): expected = {path for path in paths if is_note(path)} - result = _check(_tree(repo, paths)) - assert result.returncode == (1 if expected else 0), (paths, result.stdout) - assert _reported(result) == expected - if expected: - assert f"::error::{len(expected)} agent or job note path(s)" in result.stdout + commit = _commit(repo, paths) + for result in (_check(_tree(repo, paths)), _check(_tree(repo, []), commit)): + assert result.returncode == (1 if expected else 0), (paths, result.stdout) + assert _reported(result) == expected + if expected: + (*_, (name, data)) = _runner_commands(result.stdout) + assert name == "error" + assert data.startswith(f"{len(expected)} agent or job note path(s)") From 693d0120170bbdf4283122ed040ee6476995e12f Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Fri, 23 Oct 2026 15:50:20 -0400 Subject: [PATCH 5/9] Check pull request heads on pull_request, and main on push and daily Round 2 of the review of #713 (GPT-6.1 Sol) found that GitHub blocks pull_request_target in public repositories without an Actions event policy from 2 December 2026, and this repository has none. Its pull request workflows also never use pull_request_target (docs/release-bundles.md). The guard goes back to pull_request but keeps checking the head commit's tree, fetched by SHA, so a retarget still cannot change the result. A skip instruction in a head commit's message, or a direct push, can still get past a pull request run, so the workflow also checks main on every push and on a daily schedule, which skip instructions do not affect. The stop-commands hardening stays. (The previous head's message quoted a skip instruction while describing this gap, and GitHub skipped every pull request workflow on it.) Co-Authored-By: Claude Opus 5.5 --- .github/check-agent-notes.sh | 2 +- .github/workflows/pr_agent_notes.yaml | 45 +++++++++++++-------------- changelog.d/712.added.md | 2 +- docs/engineering/skills/github-prs.md | 7 ++--- tests/test_check_agent_notes.py | 19 ++++++----- 5 files changed, 38 insertions(+), 37 deletions(-) diff --git a/.github/check-agent-notes.sh b/.github/check-agent-notes.sh index 11553303c..8889b70db 100755 --- a/.github/check-agent-notes.sh +++ b/.github/check-agent-notes.sh @@ -6,7 +6,7 @@ # With no argument it lists the index, which a fresh checkout fills from the # commit it checks out. With a commit, it lists that commit's tree through a # temporary index, without checking it out: CI fetches a pull request's head -# as data and never runs anything from it. +# commit and checks it, whatever the pull request's base. # # Subfleet build jobs keep their state in a .job/ folder at the root of # their worktree: HANDOFF.md, PR_BODY.md, env, a git bundle and sometimes a diff --git a/.github/workflows/pr_agent_notes.yaml b/.github/workflows/pr_agent_notes.yaml index 19438eede..071080faf 100644 --- a/.github/workflows/pr_agent_notes.yaml +++ b/.github/workflows/pr_agent_notes.yaml @@ -5,34 +5,35 @@ # on 23 October 2026. .github/check-agent-notes.sh lists the paths it # rejects. # -# It runs on pull_request_target, not pull_request, so that: -# - a "[skip ci]" in the head commit cannot skip it (skip instructions apply -# to push and pull_request only); -# - it runs on a pull request with a merge conflict, which pull_request skips; -# - it runs main's guard, so a pull request cannot turn it off by editing -# this file or the script. A change to either takes effect when it merges; -# tests/test_check_agent_notes.py tests it before then. -# It checks the head commit's whole tree, not the merge with the base, so -# retargeting a stacked pull request cannot change the result. Main has -# none of these paths, so a pull request based on main fails exactly when -# it adds one. pull_request_target has no path filter here and runs for -# every base branch. +# pr_code_changes.yaml has a path filter, so a pull request that adds only +# such files runs none of its jobs. This workflow has no path or branch +# filter. It checks the pull request's head commit, fetched as data, not the +# merge with the base, so retargeting a stacked pull request cannot change +# the result. Main has none of these paths, so a pull request based on main +# fails exactly when it adds one. # -# pull_request_target runs with this repository's token, so the job must -# never run code from the pull request: it checks out main, fetches the head -# commit as data, and reads its tree into a temporary index. The token can -# only read, and no secret is used. +# A "[skip ci]" in a pull request's head commit skips pull_request runs, and +# a direct push can reach main without one. So it also checks main on every +# push and every day: a schedule is never skipped. It uses pull_request, not +# pull_request_target, like the other pull request workflows here +# (docs/release-bundles.md): GitHub blocks pull_request_target in public +# repositories without an event policy from 2 December 2026. name: Agent notes on: - pull_request_target: + pull_request: + push: + branches: + - main + schedule: + - cron: "17 6 * * *" workflow_dispatch: # A new push to a pull request supersedes the run on its previous head, so -# cancel that run. Manual dispatches are never cancelled. +# cancel that run. Runs on main are never cancelled. concurrency: group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.run_id }} - cancel-in-progress: ${{ github.event_name == 'pull_request_target' }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} permissions: contents: read @@ -43,11 +44,9 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 5 steps: - # Main (the commit that triggered a manual run), never the pull - # request's code. - uses: actions/checkout@v6 - - name: Fetch the pull request's head commit as data - if: github.event_name == 'pull_request_target' + - name: Fetch the pull request's head commit + if: github.event_name == 'pull_request' env: HEAD_SHA: ${{ github.event.pull_request.head.sha }} run: git fetch --no-tags --depth=1 origin "$HEAD_SHA" diff --git a/changelog.d/712.added.md b/changelog.d/712.added.md index 88dc73558..7c50d5d0d 100644 --- a/changelog.d/712.added.md +++ b/changelog.d/712.added.md @@ -1 +1 @@ -CI now fails a pull request whose head commit holds agent or job notes: a `.job/`, `.job-git/` or `.diag/` folder at any depth, or `HANDOFF.md`, `handoffs.jsonl`, `PR_BODY.md`, `PR-BODY.md`, `REPORT.md` or `PROGRESS.md` at the repository root. The new Agent notes workflow runs main's copy of `.github/check-agent-notes.sh` on every pull request, through `pull_request_target`, so neither the code workflow's path filter nor `[skip ci]` skips it, and it reads the head commit as data without running code from it. `.gitignore` now ignores the three folders. +CI now fails a pull request whose head commit holds agent or job notes: a `.job/`, `.job-git/` or `.diag/` folder at any depth, or `HANDOFF.md`, `handoffs.jsonl`, `PR_BODY.md`, `PR-BODY.md`, `REPORT.md` or `PROGRESS.md` at the repository root. The new Agent notes workflow runs `.github/check-agent-notes.sh` on every pull request, which the code workflow's path filter would skip, and on main on every push and once a day, which `[skip ci]` cannot skip. `.gitignore` now ignores the three folders. diff --git a/docs/engineering/skills/github-prs.md b/docs/engineering/skills/github-prs.md index 658ccc4de..e76166f12 100644 --- a/docs/engineering/skills/github-prs.md +++ b/docs/engineering/skills/github-prs.md @@ -58,8 +58,7 @@ on every PR and fails when the PR's head commit contains: Put their content in the PR description or a PR comment instead. `.gitignore` ignores the three folders, so `git add -A` leaves them out. -The workflow runs on `pull_request_target`, so `[skip ci]` does not skip it -and it runs main's copy of the check: a change to the workflow or the script -takes effect only once it merges. Test such a change with -`tests/test_check_agent_notes.py`, or run the script on a commit: +The workflow checks the PR's head commit, so the PR's base does not change +the result. A `[skip ci]` in the head commit skips it, so it also checks main +on every push and once a day. To check a commit yourself, run `.github/check-agent-notes.sh `. diff --git a/tests/test_check_agent_notes.py b/tests/test_check_agent_notes.py index ce7be539a..55055679c 100644 --- a/tests/test_check_agent_notes.py +++ b/tests/test_check_agent_notes.py @@ -12,8 +12,9 @@ 3. A git failure fails the guard instead of passing it. 4. The runner reads no path it prints as a workflow command: the only commands are its own stop-commands, the matching resume and one error. -5. The workflow runs main's guard on every pull request's head, on an event - that "[skip ci]" cannot skip, without running code from the pull request. +5. The workflow checks every pull request's head, with no path or branch + filter, and checks main on every push and on a schedule, which "[skip ci]" + cannot skip. It holds a read-only token and no pull_request_target. Invariant 1 is checked on examples and, against a Python reference, on generated trees, where invariant 2 is checked against the index mode. The @@ -369,23 +370,25 @@ def test_gitignore_keeps_the_folders_out(path): assert result.returncode == 0, path -def test_the_workflow_runs_mains_guard_on_every_pull_request_head(): +def test_the_workflow_checks_every_pull_request_head_and_main(): """Invariant 5.""" text = WORKFLOW.read_text(encoding="utf-8") workflow = yaml.safe_load(text) # PyYAML reads the bare key "on" as True. triggers = workflow.get("on", workflow.get(True)) - # "[skip ci]" skips push and pull_request runs only. - assert set(triggers) == {"pull_request_target", "workflow_dispatch"} - assert triggers["pull_request_target"] is None, "no filters or types" + # No pull_request_target: pull request workflows here never use it + # (docs/release-bundles.md). + assert set(triggers) == {"pull_request", "push", "schedule", "workflow_dispatch"} + assert triggers["pull_request"] is None, "no filters or types" + assert triggers["push"] == {"branches": ["main"]} + assert len(triggers["schedule"]) == 1 assert workflow["permissions"] == {"contents": "read"} assert "secrets." not in text (job,) = workflow["jobs"].values() assert "continue-on-error" not in job checkout, fetch, guard = job["steps"] - # Main's code, never the pull request's: no ref, so the default branch. assert checkout == {"uses": "actions/checkout@v6"} - assert fetch["if"] == "github.event_name == 'pull_request_target'" + assert fetch["if"] == "github.event_name == 'pull_request'" assert fetch["env"] == {"HEAD_SHA": "${{ github.event.pull_request.head.sha }}"} assert fetch["run"] == 'git fetch --no-tags --depth=1 origin "$HEAD_SHA"' assert guard["run"] == '.github/check-agent-notes.sh "$COMMIT"' From 4008eb6d1d28083f5082bb75c76537e80886fbc0 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Fri, 23 Oct 2026 15:56:58 -0400 Subject: [PATCH 6/9] Demonstration: commit a sample .job/HANDOFF.md, which the Agent notes check must reject Reverted in the next commit; the PR is squash-merged, so neither reaches main. Co-Authored-By: Claude Opus 5.5 --- .job/HANDOFF.md | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .job/HANDOFF.md diff --git a/.job/HANDOFF.md b/.job/HANDOFF.md new file mode 100644 index 000000000..dab2609e8 --- /dev/null +++ b/.job/HANDOFF.md @@ -0,0 +1,3 @@ +# Sample hand-off + +A sample note that shows the Agent notes check failing. The next commit reverts it. From af2c210249ca0476c03219a07583d218e7744e87 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Fri, 23 Oct 2026 15:57:28 -0400 Subject: [PATCH 7/9] Revert the demonstration .job/HANDOFF.md The Agent notes check rejected it (run 40015409561). Co-Authored-By: Claude Opus 5.5 --- .job/HANDOFF.md | 3 --- 1 file changed, 3 deletions(-) delete mode 100644 .job/HANDOFF.md diff --git a/.job/HANDOFF.md b/.job/HANDOFF.md deleted file mode 100644 index dab2609e8..000000000 --- a/.job/HANDOFF.md +++ /dev/null @@ -1,3 +0,0 @@ -# Sample hand-off - -A sample note that shows the Agent notes check failing. The next commit reverts it. From 8656ee4844759203192cea951afb97b210aa0cc5 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Fri, 23 Oct 2026 16:21:21 -0400 Subject: [PATCH 8/9] Run the default branch's guard on pull requests, and re-check on retarget Round 3 of the review of #713 (GPT-6.1 Sol) found two gaps. A pull request could replace the script that judges it. The workflow ran the checked-out copy, so a head that added .job/HANDOFF.md and a pass-always script passed. On a pull request the job now fetches the default branch with the head, and runs the default branch's copy of the script. Only while the default branch has none, on the pull request that adds it, does the checked-out copy run. Without the default branch the step fails rather than fall back. A stacked pull request whose old base had no guard got its first run only on its next push. Retargeting emits "edited", which the workflow now handles. Every edit runs the full check on the head, so a title edit cannot replace a failure with a skipped run. The docs and changelog now say that a skip instruction can skip push runs as well as pull request runs; only the daily schedule cannot be skipped. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/pr_agent_notes.yaml | 47 +++++++--- changelog.d/712.added.md | 2 +- docs/engineering/skills/github-prs.md | 9 +- tests/test_check_agent_notes.py | 120 ++++++++++++++++++++++++-- 4 files changed, 153 insertions(+), 25 deletions(-) diff --git a/.github/workflows/pr_agent_notes.yaml b/.github/workflows/pr_agent_notes.yaml index 071080faf..f3db3cd82 100644 --- a/.github/workflows/pr_agent_notes.yaml +++ b/.github/workflows/pr_agent_notes.yaml @@ -8,20 +8,24 @@ # pr_code_changes.yaml has a path filter, so a pull request that adds only # such files runs none of its jobs. This workflow has no path or branch # filter. It checks the pull request's head commit, fetched as data, not the -# merge with the base, so retargeting a stacked pull request cannot change -# the result. Main has none of these paths, so a pull request based on main -# fails exactly when it adds one. +# merge with the base, so the base cannot change the result. Main has none +# of these paths, so a pull request based on main fails exactly when it adds +# one. "edited" covers a retarget: a stacked pull request whose old base had +# no guard first gets one when it is retargeted to main. Every edit runs the +# full check, so a title edit cannot replace a failure with a skipped run. # -# A "[skip ci]" in a pull request's head commit skips pull_request runs, and -# a direct push can reach main without one. So it also checks main on every -# push and every day: a schedule is never skipped. It uses pull_request, not -# pull_request_target, like the other pull request workflows here -# (docs/release-bundles.md): GitHub blocks pull_request_target in public -# repositories without an event policy from 2 December 2026. +# A skip instruction in a pushed commit's message skips pull_request and +# push runs, and so can a merge that carries one to main. The daily schedule +# is never skipped, so a note that reaches main that way is found within a +# day. It uses pull_request, not pull_request_target, like the other pull +# request workflows here (docs/release-bundles.md): GitHub blocks +# pull_request_target in public repositories without an event policy from +# 2 December 2026. name: Agent notes on: pull_request: + types: [opened, synchronize, reopened, edited] push: branches: - main @@ -45,15 +49,34 @@ jobs: timeout-minutes: 5 steps: - uses: actions/checkout@v6 - - name: Fetch the pull request's head commit + - name: Fetch the pull request's head and the default branch if: github.event_name == 'pull_request' env: HEAD_SHA: ${{ github.event.pull_request.head.sha }} - run: git fetch --no-tags --depth=1 origin "$HEAD_SHA" + DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} + run: >- + git fetch --no-tags --depth=1 origin "$HEAD_SHA" + "+refs/heads/$DEFAULT_BRANCH:refs/remotes/origin/$DEFAULT_BRANCH" - name: No agent or job notes in the tree # An explicit bash shell adds pipefail, so a git failure fails the # step instead of passing it. The script sets it too. shell: bash env: + EVENT: ${{ github.event_name }} COMMIT: ${{ github.event.pull_request.head.sha || github.sha }} - run: .github/check-agent-notes.sh "$COMMIT" + DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} + # On a pull request, run the default branch's guard, so a pull + # request cannot pass by editing the script. Only while the default + # branch has none, on the pull request that adds it, does the + # checked-out copy run. Other events check out the commit they check. + run: | + guard=.github/check-agent-notes.sh + if [ "$EVENT" = pull_request ]; then + main="refs/remotes/origin/$DEFAULT_BRANCH" + git rev-parse --verify --quiet "$main^{commit}" >/dev/null + if git cat-file -e "$main:$guard" 2>/dev/null; then + git show "$main:$guard" >"$RUNNER_TEMP/check-agent-notes.sh" + guard="$RUNNER_TEMP/check-agent-notes.sh" + fi + fi + bash "$guard" "$COMMIT" diff --git a/changelog.d/712.added.md b/changelog.d/712.added.md index 7c50d5d0d..fc2b27d17 100644 --- a/changelog.d/712.added.md +++ b/changelog.d/712.added.md @@ -1 +1 @@ -CI now fails a pull request whose head commit holds agent or job notes: a `.job/`, `.job-git/` or `.diag/` folder at any depth, or `HANDOFF.md`, `handoffs.jsonl`, `PR_BODY.md`, `PR-BODY.md`, `REPORT.md` or `PROGRESS.md` at the repository root. The new Agent notes workflow runs `.github/check-agent-notes.sh` on every pull request, which the code workflow's path filter would skip, and on main on every push and once a day, which `[skip ci]` cannot skip. `.gitignore` now ignores the three folders. +CI now fails a pull request whose head commit holds agent or job notes: a `.job/`, `.job-git/` or `.diag/` folder at any depth, or `HANDOFF.md`, `handoffs.jsonl`, `PR_BODY.md`, `PR-BODY.md`, `REPORT.md` or `PROGRESS.md` at the repository root. The new Agent notes workflow runs the default branch's `.github/check-agent-notes.sh` on every pull request, which the code workflow's path filter would skip, including when one is retargeted. It also checks main on every push and once a day, since a skip instruction in a commit message can skip the pull request and push runs but not the daily one. `.gitignore` now ignores the three folders. diff --git a/docs/engineering/skills/github-prs.md b/docs/engineering/skills/github-prs.md index e76166f12..a293bd74d 100644 --- a/docs/engineering/skills/github-prs.md +++ b/docs/engineering/skills/github-prs.md @@ -58,7 +58,8 @@ on every PR and fails when the PR's head commit contains: Put their content in the PR description or a PR comment instead. `.gitignore` ignores the three folders, so `git add -A` leaves them out. -The workflow checks the PR's head commit, so the PR's base does not change -the result. A `[skip ci]` in the head commit skips it, so it also checks main -on every push and once a day. To check a commit yourself, run -`.github/check-agent-notes.sh `. +The workflow checks the PR's head commit with the default branch's copy of the +script, so neither the PR's base nor an edit to the script changes the +result. A skip instruction in a commit message skips the PR and push runs, so +it also checks main once a day, which nothing skips. To check a commit +yourself, run `.github/check-agent-notes.sh `. diff --git a/tests/test_check_agent_notes.py b/tests/test_check_agent_notes.py index 55055679c..35a62e8c8 100644 --- a/tests/test_check_agent_notes.py +++ b/tests/test_check_agent_notes.py @@ -13,8 +13,12 @@ 4. The runner reads no path it prints as a workflow command: the only commands are its own stop-commands, the matching resume and one error. 5. The workflow checks every pull request's head, with no path or branch - filter, and checks main on every push and on a schedule, which "[skip ci]" - cannot skip. It holds a read-only token and no pull_request_target. + filter, on every push, retarget or other edit, and checks main on every + push and on a schedule, which no commit message can skip. It holds a + read-only token and no pull_request_target. +6. On a pull request it runs the default branch's guard, so editing the + script cannot pass the pull request; only while the default branch has no + guard does the checked-out copy run. Invariant 1 is checked on examples and, against a Python reference, on generated trees, where invariant 2 is checked against the index mode. The @@ -370,16 +374,23 @@ def test_gitignore_keeps_the_folders_out(path): assert result.returncode == 0, path +def _workflow(): + return yaml.safe_load(WORKFLOW.read_text(encoding="utf-8")) + + def test_the_workflow_checks_every_pull_request_head_and_main(): """Invariant 5.""" text = WORKFLOW.read_text(encoding="utf-8") - workflow = yaml.safe_load(text) + workflow = _workflow() # PyYAML reads the bare key "on" as True. triggers = workflow.get("on", workflow.get(True)) # No pull_request_target: pull request workflows here never use it # (docs/release-bundles.md). assert set(triggers) == {"pull_request", "push", "schedule", "workflow_dispatch"} - assert triggers["pull_request"] is None, "no filters or types" + # No path or branch filter; "edited" catches a retarget. + assert triggers["pull_request"] == { + "types": ["opened", "synchronize", "reopened", "edited"] + } assert triggers["push"] == {"branches": ["main"]} assert len(triggers["schedule"]) == 1 assert workflow["permissions"] == {"contents": "read"} @@ -389,17 +400,110 @@ def test_the_workflow_checks_every_pull_request_head_and_main(): checkout, fetch, guard = job["steps"] assert checkout == {"uses": "actions/checkout@v6"} assert fetch["if"] == "github.event_name == 'pull_request'" - assert fetch["env"] == {"HEAD_SHA": "${{ github.event.pull_request.head.sha }}"} - assert fetch["run"] == 'git fetch --no-tags --depth=1 origin "$HEAD_SHA"' - assert guard["run"] == '.github/check-agent-notes.sh "$COMMIT"' + assert fetch["env"] == { + "HEAD_SHA": "${{ github.event.pull_request.head.sha }}", + "DEFAULT_BRANCH": "${{ github.event.repository.default_branch }}", + } + assert fetch["run"] == ( + 'git fetch --no-tags --depth=1 origin "$HEAD_SHA" ' + '"+refs/heads/$DEFAULT_BRANCH:refs/remotes/origin/$DEFAULT_BRANCH"' + ) assert guard["env"] == { - "COMMIT": "${{ github.event.pull_request.head.sha || github.sha }}" + "EVENT": "${{ github.event_name }}", + "COMMIT": "${{ github.event.pull_request.head.sha || github.sha }}", + "DEFAULT_BRANCH": "${{ github.event.repository.default_branch }}", } assert guard["shell"] == "bash" assert "if" not in guard and "continue-on-error" not in guard assert os.access(SCRIPT, os.X_OK) +# Invariant 6: the workflow's guard step, run as GitHub runs a bash step, in +# a repository laid out as the job leaves it. The default branch is at +# refs/remotes/origin/main, and the pull request's head is checked out (its +# merge with a base it descends from has the same tree). + +PASS_ALWAYS = "#!/usr/bin/env bash\necho 'No agent or job notes in the tree.'\n" + + +def _step_repo(tmp_path, main_guard, head_guard): + repo = tmp_path / "repo" + (repo / ".github").mkdir(parents=True) + _git("init", "--quiet", cwd=repo) + (repo / "README.md").write_text("x\n") + if main_guard is not None: + (repo / ".github" / "check-agent-notes.sh").write_text(main_guard) + _git("add", "-A", cwd=repo) + _git("commit", "--quiet", "-m", "main", cwd=repo) + _git("update-ref", "refs/remotes/origin/main", "HEAD", cwd=repo) + (repo / ".github" / "check-agent-notes.sh").write_text(head_guard) + (repo / ".job").mkdir() + (repo / ".job" / "HANDOFF.md").write_text("note\n") + _git("add", "-A", cwd=repo) + _git("commit", "--quiet", "-m", "head", cwd=repo) + return repo, _git("rev-parse", "HEAD", cwd=repo).stdout.decode().strip() + + +def _run_step(repo, event, commit): + (*_, guard) = next(iter(_workflow()["jobs"].values()))["steps"] + step = repo.parent / "step.sh" + step.write_text(guard["run"]) + runner_temp = repo.parent / "runner-temp" + runner_temp.mkdir(exist_ok=True) + env = { + **ISOLATED_GIT, + "EVENT": event, + "COMMIT": commit, + "DEFAULT_BRANCH": "main", + "RUNNER_TEMP": str(runner_temp), + } + return subprocess.run( + ["bash", "--noprofile", "--norc", "-eo", "pipefail", str(step)], + cwd=repo, + env=env, + capture_output=True, + text=True, + encoding="utf-8", + ) + + +def test_a_pull_request_cannot_pass_by_editing_the_guard(tmp_path): + """Round 3 of the review of #713 reproduced this pass on the PR's copy.""" + guard = SCRIPT.read_text(encoding="utf-8") + repo, head = _step_repo(tmp_path, main_guard=guard, head_guard=PASS_ALWAYS) + result = _run_step(repo, "pull_request", head) + assert result.returncode == 1, result.stdout + assert _reported(result) == {".job/HANDOFF.md"} + + +def test_until_main_has_a_guard_the_checked_out_copy_runs(tmp_path): + guard = SCRIPT.read_text(encoding="utf-8") + repo, head = _step_repo(tmp_path, main_guard=None, head_guard=guard) + result = _run_step(repo, "pull_request", head) + assert result.returncode == 1, result.stdout + assert _reported(result) == {".job/HANDOFF.md"} + + +def test_a_pull_request_run_without_the_default_branch_fails(tmp_path): + """Fails closed, rather than falling back to the pull request's copy.""" + guard = SCRIPT.read_text(encoding="utf-8") + repo, head = _step_repo(tmp_path, main_guard=guard, head_guard=PASS_ALWAYS) + _git("update-ref", "-d", "refs/remotes/origin/main", cwd=repo) + result = _run_step(repo, "pull_request", head) + assert result.returncode != 0 + assert "No agent or job notes in the tree." not in result.stdout + + +@pytest.mark.parametrize("event", ["push", "schedule", "workflow_dispatch"]) +def test_other_events_run_the_checked_out_guard_on_their_commit(tmp_path, event): + guard = SCRIPT.read_text(encoding="utf-8") + repo, head = _step_repo(tmp_path, main_guard=None, head_guard=guard) + result = _run_step(repo, event, head) + assert result.returncode == 1, result.stdout + assert _reported(result) == {".job/HANDOFF.md"} + assert _run_step(repo, event, f"{head}~1").returncode == 0 + + # Invariants 1 and 2 on generated trees. Segments mix the note names, in any # ASCII case, with near misses and arbitrary names, including glob # characters, colons and letters outside ASCII. From 3f30759e5ad655518deb1d30b47b273bdeca53a6 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Fri, 23 Oct 2026 17:18:38 -0400 Subject: [PATCH 9/9] Say the daily run finds a note by its next run, not within a day Round 4 of the review of #713 (GPT-6.1 Sol) approved, with one nit: GitHub can delay or drop a scheduled run, so the daily check finds a note that reached main by the next scheduled run, if the note is still there. The concurrency comment now says that an edit, as well as a push, supersedes a run in progress. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/pr_agent_notes.yaml | 19 +++++++++++-------- docs/engineering/skills/github-prs.md | 4 ++-- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/.github/workflows/pr_agent_notes.yaml b/.github/workflows/pr_agent_notes.yaml index f3db3cd82..2f479a627 100644 --- a/.github/workflows/pr_agent_notes.yaml +++ b/.github/workflows/pr_agent_notes.yaml @@ -15,12 +15,14 @@ # full check, so a title edit cannot replace a failure with a skipped run. # # A skip instruction in a pushed commit's message skips pull_request and -# push runs, and so can a merge that carries one to main. The daily schedule -# is never skipped, so a note that reaches main that way is found within a -# day. It uses pull_request, not pull_request_target, like the other pull -# request workflows here (docs/release-bundles.md): GitHub blocks -# pull_request_target in public repositories without an event policy from -# 2 December 2026. +# push runs, and so can a merge that carries one to main. No commit message +# can skip the daily schedule, so a note that reaches main that way is found +# by the next scheduled run, if it is still there; GitHub can delay or drop +# a scheduled run. +# +# It uses pull_request, not pull_request_target, like the other pull request +# workflows here (docs/release-bundles.md): GitHub blocks pull_request_target +# in public repositories without an event policy from 2 December 2026. name: Agent notes on: @@ -33,8 +35,9 @@ on: - cron: "17 6 * * *" workflow_dispatch: -# A new push to a pull request supersedes the run on its previous head, so -# cancel that run. Runs on main are never cancelled. +# A newer run for the same pull request, after a push or an edit, supersedes +# the one in progress, so cancel that one: the newer run checks the current +# head in full. Runs on main are never cancelled. concurrency: group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.run_id }} cancel-in-progress: ${{ github.event_name == 'pull_request' }} diff --git a/docs/engineering/skills/github-prs.md b/docs/engineering/skills/github-prs.md index a293bd74d..4e22dd50f 100644 --- a/docs/engineering/skills/github-prs.md +++ b/docs/engineering/skills/github-prs.md @@ -61,5 +61,5 @@ ignores the three folders, so `git add -A` leaves them out. The workflow checks the PR's head commit with the default branch's copy of the script, so neither the PR's base nor an edit to the script changes the result. A skip instruction in a commit message skips the PR and push runs, so -it also checks main once a day, which nothing skips. To check a commit -yourself, run `.github/check-agent-notes.sh `. +it also checks main once a day, which no commit message can skip. To check a +commit yourself, run `.github/check-agent-notes.sh `.