Skip to content

ci(ai-review): harden Argus per #4816 self-review#3488

Merged
bokelley merged 1 commit into
mainfrom
bokelley/auto-approve-routine-prs
May 22, 2026
Merged

ci(ai-review): harden Argus per #4816 self-review#3488
bokelley merged 1 commit into
mainfrom
bokelley/auto-approve-routine-prs

Conversation

@bokelley

@bokelley bokelley commented Apr 28, 2026

Copy link
Copy Markdown
Contributor

Summary

Argus reviewed its own bootstrap PR (#4816) and flagged 5 real findings + 2 nits in the workflow. The smoke-test PR squash-merged the unhardened Argus to main (an artifact of how #4816 was branched off this PR). This PR now contains only the hardening delta on top of what landed via #4816.

Argus's findings, all addressed

# Argus finding Fix
1 (High) Prompt-injection via paths-ignore + same-PR multi-file edits Prompt loaded from \$BASE_SHA via git show, not the PR working tree. Plus a new workflow-mod gate that bails to --comment if the PR touches .github/ai-review/** or .github/workflows/ai-review.yml alongside other files.
2 Bash(gh api:*) wildcard Narrowed to repos/*/pulls/*, repos/*/contents/*, repos/*/issues/*
3 Verifier matches any bot login Pinned to aao-release-bot via ARGUS_BOT_LOGIN env var, in both verifier and skip-check
4 Unpinned action tags All 4 actions pinned to commit SHAs (actions/checkout, create-github-app-token, github-script, claude-code-action) with version comment
5 Force-push + trivial-skip → stale approval Documented as branch-protection requirement ("Require approval of the most recent reviewable push" on main) in workflow comments
Nit 1 macOS date fallback unreachable Removed
Nit 2 Static heredoc sentinel Randomized via openssl rand -hex 8

Self-attestation

This PR hits the new workflow-mod gate it adds — Argus will post a --comment, not --approve. That's by design. Needs human approval.

Branch protection requirement

Verify "Require approval of the most recent reviewable push" is enabled on main (Settings → Branches → main → Edit rule). Without it, the trivial-skip path can leave a stale bot approval after a force-push that touches only trivial paths.

Refs

🤖 Generated with Claude Code

@bokelley

bokelley commented May 3, 2026

Copy link
Copy Markdown
Contributor Author

Reviewed during PR triage spin. The implementation is sound for what it claims (App-identity approval, branch filter, CI-green gate, idempotency, opt-out label, fast-fail on missing secrets). But two prerequisites need to land before this actually unblocks the routine bottleneck:

1. CODEOWNERS gate, not just review-count gate

Branch ruleset on main (id 8519441) has require_code_owner_review: true. CODEOWNERS lists only @bokelley for:

  • /.agents/
  • /.github/ and /.github/workflows/
  • /static/schemas/source/
  • /.changeset/

Every routine PR touches /.changeset/ (mandatory per CLAUDE.md), and most schema PRs touch /static/schemas/source/. The bot's approving review satisfies "1 approving review", but require_code_owner_review: true will still block merge — only @bokelley can be that owner today.

Three options:

  • (a) Narrow CODEOWNERS — remove /.changeset/ from CODEOWNERS. Changesets are mechanical metadata, not security-sensitive; nothing in a .changeset/*.md file changes behavior. This single move would unblock the vast majority of routine PRs without weakening protection on prompts, workflows, or schemas. Cleanest path.

  • (b) Add the Triage Bot App as a CODEOWNER on a narrow path (e.g. only /.changeset/). Same effective outcome as (a) but more conservative.

  • (c) Disable require_code_owner_review. Don't recommend — loses the security model for .github/workflows/ and /.agents/.

Recommend (a). Schema PRs and workflow PRs would still need your review, which is the right tradeoff.

2. Secret names don't match

Repo has RELEASE_APP_ID / RELEASE_APP_PRIVATE_KEY. PR expects TRIAGE_BOT_APP_ID / TRIAGE_BOT_APP_PRIVATE_KEY. Per the per-purpose-keys principle in our auth memory, this should be a separate App rather than reusing Release App. Need to:

  • Register an "AAO Triage Bot" GitHub App (or confirm one exists)
  • Grant pull_requests:write, contents:read
  • Install on this repo
  • Add its App ID + private key as TRIAGE_BOT_APP_ID / TRIAGE_BOT_APP_PRIVATE_KEY secrets

3. Minor implementation notes

  • pull_request_target + the claude/* / auto/* branch filter is safe — those branches can't come from forks. ✓
  • check_suite.completed resolution via payload.check_suite.pull_requests will silently no-op for cross-repo cases; the pull_request_target trigger covers that. ✓
  • Idempotency check at the bottom (existing approval at same head_sha) is correct. ✓

Suggest landing CODEOWNERS narrowing first, then the App registration, then this workflow.

@bokelley

bokelley commented May 3, 2026

Copy link
Copy Markdown
Contributor Author

Agreed on all three points. The workflow code stands as-is; the two prerequisites are external to this diff.

On CODEOWNERS (item 1): Option (a) — removing /.changeset/ from CODEOWNERS — is the right call. Changesets are mechanical metadata; there's no security value in requiring a code-owner review on them. That said, .github/CODEOWNERS is in the /.github/** path, which is outside the triage routine's edit scope — this change needs to be a @bokelley-authored commit. Once it lands, the vast majority of routine PRs clear the CODEOWNERS gate without touching any protected surface.

On the App identity (item 2): The per-purpose-keys principle is correct; reusing RELEASE_APP_ID would be the wrong call. The workflow already uses TRIAGE_BOT_APP_ID / TRIAGE_BOT_APP_PRIVATE_KEY — those names are correct, the ops work (App registration + secret injection) is the remaining step.

Sequencing confirmed: CODEOWNERS narrowing → App registration + secrets → merge this workflow. PR will stay as a draft until both prerequisites are in place.


Generated by Claude Code

@bokelley

Copy link
Copy Markdown
Contributor Author

Blocked on infra setup — the workflow expects TRIAGE_BOT_APP_ID and TRIAGE_BOT_APP_PRIVATE_KEY secrets, but they're not in the repo. Current bot secrets available: RELEASE_APP_ID/RELEASE_APP_PRIVATE_KEY (release bot — wrong identity for PR review) and TRIAGE_DISPATCH_PAT (different mechanism, not a GitHub App).

To land this, we need to:

  1. Register a new "AAO Triage Bot" GitHub App (or reuse an existing one) with pull_requests:write + contents:read perms
  2. Install on the adcp repo
  3. Add the two secrets

Leaving open as a tracking PR until that infra is in place. Admin-merge has been working fine in the meantime.

— Triage 2026-05-13

@bokelley bokelley force-pushed the bokelley/auto-approve-routine-prs branch from a11ca3a to 73a2e44 Compare May 19, 2026 21:12
@bokelley bokelley changed the title ci(auto-approve): post approval reviews on routine-authored PRs via Triage Bot App ci(ai-review): Argus LLM PR reviewer (port from scope3data/agentic-api) May 19, 2026
@aao-release-bot

Copy link
Copy Markdown
Contributor

⚠️ Argus review could not complete

The automated review encountered an issue (possibly reached max turns, timed out, or failed to post the final gh pr review). A human reviewer should take this PR.

View workflow run

This is an automated message from the Argus AI review workflow.

bokelley added a commit that referenced this pull request May 19, 2026
Two instances of British spelling "behaviour" in
docs/reference/release-notes.mdx. Repo convention is US spelling
(158 "behavior" vs 2 "behaviour" before this fix).

Smoke test for the new Argus AI review workflow (#3488).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bokelley added a commit that referenced this pull request May 19, 2026
* ci(auto-approve): post approval reviews on routine-authored PRs

The Claude Code triage routine opens PRs as @bokelley (the only
GitHub identity available in the Anthropic routine console), which
means the project owner cannot approve their own PRs and branch
protection forces admin-merge for everything the routine produces.

Add a separate auto-approver workflow that uses the AAO Triage Bot
GitHub App to post approving reviews on PRs that are:

  - Routine-authored (head branch starts with `claude/` or `auto/`)
  - Not draft
  - All CI checks green (success / neutral / skipped only)
  - No `do-not-auto-approve` label

Triggers on pull_request_target (every PR event), check_suite.completed
(when CI lands), and workflow_dispatch (manual fire). Idempotent — skips
if the bot already approved the current head_sha.

Required repo secrets:
  - TRIAGE_BOT_APP_ID
  - TRIAGE_BOT_APP_PRIVATE_KEY

The App needs `pull_requests:write` + `contents:read` and must be
installed on this repo. If the secrets are missing, the workflow
fails fast at the token-generation step (no silent skip).

Opt-out: add the `do-not-auto-approve` label to any PR that needs
human eyes (e.g., feature work the routine couldn't fully verify).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* ci(ai-review): replace mechanical auto-approve with Argus LLM reviewer

The original workflow only rubber-stamped CI-green PRs on claude/* and
auto/* branches. It solved the routine-PR admin-merge bottleneck but
ignored the bigger surface: bokelley/* PRs that ship spec changes still
need real review.

Replace with Argus, an LLM reviewer adapted from scope3data/agentic-api.
Reads the diff, delegates to AdCP subagents in parallel, posts a
substantive approve/comment/request-changes review on every non-dependabot
PR. Uses the same RELEASE_APP_* secrets the changeset release bot already
has - no separate App registration needed.

AdCP-specific MUST-FIX gates:
- Spec drift on static/schemas/source/**
- Breaking wire change without a major changeset
- Missing changeset on wire-touching PR
- oneOf discriminator regression against the audit walker baseline

Requires ANTHROPIC_API_KEY to be added to repo or org secrets before the
workflow can complete - claude-code-action mints API calls under that key.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* docs(release-notes): behaviour → behavior to match repo convention

Two instances of British spelling "behaviour" in
docs/reference/release-notes.mdx. Repo convention is US spelling
(158 "behavior" vs 2 "behaviour" before this fix).

Smoke test for the new Argus AI review workflow (#3488).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Argus reviewed PR #4816 and flagged five real issues in its own
workflow. Addressing them:

- Prompt loaded from base SHA, not the PR head working tree. Closes
  the same-repo prompt-injection vector even when the PR doesn't
  directly modify the prompt file.

- Workflow-mod gate: PRs that touch .github/ai-review/** or
  .github/workflows/ai-review.yml alongside other files are not
  auto-reviewed. Bot posts a --comment requesting human review.

- Verifier and skip-check filters pin to the Argus bot login
  (aao-release-bot) rather than matching any bot user. Stops
  changesets-release[bot] and other bots from false-positiving.

- Bash(gh api:*) allowlist narrowed to read-only patterns:
  repos/*/pulls/*, repos/*/contents/*, repos/*/issues/*.

- Third-party actions pinned to commit SHAs to close the moving-tag
  vector. Includes actions/checkout, actions/create-github-app-token,
  actions/github-script, anthropics/claude-code-action.

- Heredoc sentinel randomized (openssl rand -hex 8) so prompt content
  can't break framing.

- macOS date fallback removed (workflow is ubuntu-latest only).

Branch-protection requirement documented in workflow comments:
"Require approval of the most recent reviewable push" MUST be enabled
for the trivial-skip path to be fail-closed against force-pushes that
touch only trivial paths.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@bokelley bokelley force-pushed the bokelley/auto-approve-routine-prs branch from 4553b58 to c4183f3 Compare May 19, 2026 21:49
@bokelley bokelley changed the title ci(ai-review): Argus LLM PR reviewer (port from scope3data/agentic-api) ci(ai-review): harden Argus per #4816 self-review May 19, 2026

@aao-release-bot aao-release-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Argus is not auto-reviewing this PR because it modifies the review workflow itself (.github/workflows/ai-review.yml). Running with the PR head's modified prompt/workflow is a prompt-injection vector. A human reviewer should review and merge this PR; Argus will resume on subsequent PRs once these changes land on main.

@bokelley

Copy link
Copy Markdown
Contributor Author

@EmmaLouise2018 — could you take a pass when you have a window? This is the hardening delta on top of what landed via #4816 (the unhardened Argus is live on main right now). Five real findings + 2 nits that Argus surfaced reviewing its own bootstrap PR. Self-attestation note: the workflow-mod gate this PR adds correctly blocks me from auto-approving — Argus posted --comment not --approve here, which is exactly the behavior we want. PR body has the full table of what each commit addresses. Branch protection note in the description too — worth confirming "Require approval of the most recent reviewable push" is on for main.

@bokelley bokelley merged commit 00cd1d9 into main May 22, 2026
14 checks passed
@bokelley bokelley deleted the bokelley/auto-approve-routine-prs branch May 22, 2026 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant