anthropics / anthropics/claude-code-action

agent-approval-check: 1,828-line merge-gating script has no tests and no CI coverage

Open
#1,663 0 comments 0 reactions 0 assignees View on GitHub
dev-experience enhancement p3
Dominant language
TypeScript
Stars
8.9k
Forks
2.1k
PR merge metrics
PR metrics pending

Description

## Summary

`agent-approval-check/agent_approval_check.py` is 1,828 lines across 54 functions and has **no test file anywhere in the repository** and **no CI coverage** — no pytest, ruff, or mypy job references it in any workflow.

This is the component whose own module docstring describes it as a fail-closed merge gate:

> Enforces that PRs containing agent-authored commits (e.g. from Claude Code) receive N human approvals before the `agent-approval-check` commit status turns green. Mark that status as a required check on protected branches to gate merges.

and whose README states it is "the same gate Anthropic runs internally on every agent-authored PR."

## Evidence

```console
$ find . -name "test_*.py" -o -name "*_test.py" -o -name "conftest.py" | grep -v node_modules
(no output)

$ grep -rn "python\|pytest\|ruff\|mypy\|agent-approval" .github/workflows/*.yml
(no output)

$ wc -l agent-approval-check/agent_approval_check.py
1828
```

Every other test-bearing module in this repo is TypeScript and covered by `bun test` via `.github/workflows/ci.yml`. That workflow runs `bun install` and `bun test` only, so the Python sub-action is invisible to CI: a syntax error, a bad import, or a regression in approval-counting logic would ship undetected.

## Why this matters

The security model is explicitly fail-closed and depends on the correctness of pure decision logic:

> It is fail-closed: any unhandled exception exits non-zero and the required status stays non-success.

Fail-closed protects against crashes, but not against logic that returns a confidently wrong answer. The functions that decide whether a merge gate goes green have no regression protection at all. A change that causes `count_approvers` to over-count — for example by mishandling `author_association`, or by counting an agent identity — would turn the gate green on an unreviewed agent-authored PR, and nothing in CI would catch it.

## Scope: this module is unusually testable

Roughly 25 of the 54 functions are pure and require no network, no fixtures beyond plain dicts, and no GitHub credentials. Directly unit-testable today:

| Function | Line | Decision it makes |
|---|---|---|
| `normalize_graphql_login` | 478 | Author identity normalisation |
| `get_committer_email` | 493 | Agent-commit detection input |
| `is_agent_commit` | 497 | Whether a commit counts as agent-authored |
| `is_review_exempt_pr` | 506 | Whether the gate applies at all |
| `is_protected_base` | 518 | Whether this base branch is gated |
| `select_pr_candidate` | 548 | Which PR a shared head SHA belongs to |
| `is_exempt_branch` | 610 | Glob-based head-branch exemption |
| `is_agent_user` / `is_excluded_approver` | 620 / 626 | Who may never approve |
| `is_pr_created_by_agent` | 637 | Agent-authored PR detection |
| `parse_approve_command` | 646 | Parsing `/approve ` |
| `sha_matches` | 661 | Prefix match of approved SHA against head |
| `iter_approve_commands` | 665 | Filtering valid `/approve` comments |
| `get_latest_review_per_user` | 693 | Latest decision review per reviewer |
| `has_agent_approval` | 861 | Agent approval detection |
| `count_approvers` | 935 | **The core approval count** |
| `find_stale_approvals` | 996 | Post-push staleness |
| `generate_notification_comment` | 1062 | Comment rendering |
| `format_status_description` | 1545 | Status text |

The docstrings already encode precise expected behaviour that maps one-to-one onto test cases. Two examples that are effectively pre-written specs:

`parse_approve_command` (line 646):
> Only the first line of the comment is considered. GitHub email replies append the quoted notification below the user's text, so a reply of just `/approve ` arrives as `"/approve \r\n\r\nOn ... wrote:"`. The first line must still be exactly the command — leading text or extra tokens on that line are rejected.

`get_latest_review_per_user` (line 693):
> Only considers "decision" reviews (APPROVED, CHANGES_REQUESTED) that change approval status. COMMENTED reviews are ignored because a reviewer who approves then adds a comment is still approving, matching GitHub's native behavior.

Neither behaviour is currently protected by a test.

## Suggested approach

Two separate, independently reviewable changes:

1. **`agent-approval-check/test_agent_approval_check.py`** — pytest suite covering the pure functions above, with emphasis on the security-relevant paths: agent identities never counting as approvers, `/approve` SHA mismatch rejection, `CHANGES_REQUESTED` overriding an earlier `APPROVED` from the same user, and `author_association` filtering against `WRITE_ACCESS_ASSOCIATIONS`.
2. **CI job** — a `test-agent-approval-check` workflow (or a job appended to `ci-all.yml`) running `actions/setup-python@v5` with Python 3.12 and the same pinned dependency set already declared in `agent-approval-check/action.yml` (`httpx==0.28.1`, `pyyaml==6.0.3`, `tenacity==9.1.4`), plus `pytest`.

Open questions for maintainers, since these are conventions rather than code:

- Is pytest acceptable, or is stdlib `unittest` preferred to avoid adding a dev dependency?
- Should the CI job be a separate reusable workflow called from `ci-all.yml`, matching the existing `test-*.yml` pattern?
- The script declares its dependencies in a PEP 723 inline block. Should the test job use `uv run` to honour that, rather than duplicating the pinned versions in a `pip install` step?

I'm happy to open the PR once there's agreement on those three points.

## Environment

- Repository at `d721746d683d812e669ce117cebe55a85fbd9c3e` (`main`)

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.