ci(aao-secretariat): fork the PR reviewer into a TypeScript GitHub Action#2354
Conversation
…/arbiter/review) into .github/aao-secretariat Node/TS GitHub Action set forked from Scope3's Argus review action and adapted for AdCP: validated findings-json contract, arbiter decides via a constrained submit_decision enum, auth via the AAO Secretariat App, pull_request_target head-read handling, and the WG constitution fetched from adcp@main at review time. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… tree from published package Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…; retire bash reviewer Supersedes the bash Argus reviewer (incl. #2338's inline WG-constitution/App-identity edits, now carried by the TS reviewer composite + orchestrator). Keeps pull_request_target base-SHA checkout + head fetch + workflow-mod gate. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…list With CLAUDE_CODE_SUBPROCESS_ENV_SCRUB=0 the installation token is in the Bash env; the `gh api repos/*/issues/*` grant allowed a POST, so fork-PR prompt-injection could exfiltrate the token via an issue comment. The reviewer posts inline comments through the MCP tool, not Bash gh, so the grant is unnecessary. Removed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![aao-secretariat[bot]](https://avatars.githubusercontent.com/u/221690821?s=60&v=4){kind=small}
There was a problem hiding this comment.
Argus is not auto-reviewing this PR because it modifies the review workflow itself (.github/ai-review/expert-adcp-reviewer.md, .github/workflows/ai-review.yml). A human reviewer should review and merge this PR; Argus will resume on subsequent PRs once these changes land on main.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
…only) Root-level files (AAO-SECRETARIAT.md, .prettierignore) map to the @adcp/sdk root package (workspaces includes "."), so `changeset status` requires an entry. This PR ships no library change, so an empty (no-release) changeset satisfies the gate. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ull_request_target head fetch CodeQL flagged actions/untrusted-checkout (critical) on the `git fetch <head>` step: fetching PR-head code in a privileged pull_request_target workflow. setup now derives changed files (pulls.listFiles), the delta (compareCommits), and the diff patches (diff media type) from the GitHub API — matching the pattern in adcontextprotocol/adcp and this repo's prior reviewer — so the head is never fetched, checked out, or executed. Removed the fetch step. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Argus is not auto-reviewing this PR because it modifies the review workflow itself (.github/ai-review/expert-adcp-reviewer.md, .github/workflows/ai-review.yml). A human reviewer should review and merge this PR; Argus will resume on subsequent PRs once these changes land on main.
github-code-quality flagged the initial `let mergeable = true` as always overwritten. The catch re-assigned the same default; removing it makes the initial value the transient-failure default, resolving the finding. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Argus is not auto-reviewing this PR because it modifies the review workflow itself (.github/ai-review/expert-adcp-reviewer.md, .github/workflows/ai-review.yml). A human reviewer should review and merge this PR; Argus will resume on subsequent PRs once these changes land on main.
Relocates the forked review action tree from .github/aao-secretariat/ to a root-level, dot-prefixed .secretariat/ai-review/ (Option C: the Secretariat's ai-review desk). The dot prefix marks it as repo infrastructure (like .github) and keeps it out of glob-based tooling; root placement matches the actions-repo layout and simplifies the rebuild hook. Rewires uses:/paths-ignore/mod-gate/ codeql/prettierignore refs. Identity (AAO-SECRETARIAT bot, labels, AAO-SECRETARIAT.md, input names) is unchanged — the desk vs argus naming call is still pending. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Argus is not auto-reviewing this PR because it modifies the review workflow itself (.github/ai-review/expert-adcp-reviewer.md, .github/workflows/ai-review.yml). A human reviewer should review and merge this PR; Argus will resume on subsequent PRs once these changes land on main.
…at/ai-review The tree-move commit missed these three reference files (staging error), leaving the workflow uses:/paths-ignore/mod-gate pointing at the removed .github/aao-secretariat path. Point them at .secretariat/ai-review. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Argus is not auto-reviewing this PR because it modifies the review workflow itself (.github/ai-review/expert-adcp-reviewer.md, .github/workflows/ai-review.yml). A human reviewer should review and merge this PR; Argus will resume on subsequent PRs once these changes land on main.
… wired via install-hooks.js) Keeps the setup/arbiter committed dist/ in sync with source: when a staged change touches a node action's src/build-config, rebuild that action and re-stage its dist/. Logic lives in .secretariat/ai-review/scripts/precommit.sh so it travels with the tree (Phase-2 extraction); install-hooks.js installs a thin delegating pre-commit that calls it. No husky, no core.hooksPath change — existing commit-msg/pre-push hooks are untouched. Fast no-op for commits that don't touch the tree. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Argus is not auto-reviewing this PR because it modifies the review workflow itself (.github/ai-review/expert-adcp-reviewer.md, .github/workflows/ai-review.yml). A human reviewer should review and merge this PR; Argus will resume on subsequent PRs once these changes land on main.
…pable gh grant Review deep-dive fixes: - #1/#2: computePrSurfaceFiles now returns {files, reliable}. reliable=false on a pulls.listFiles error OR the 3000-file cap; setup fails closed (throws) rather than clearing the high-risk/gated gates on an incomplete surface — a transient API blip or huge PR can no longer let a security-critical PR auto-approve. - #3: removed the write-capable `gh api repos/*/contents/*` grant from the reviewer allowlist (gh api can't be method-restricted, and with ENV_SCRUB=0 the token is in the Bash env → prompt-injection exfil path). The agent now reads changes via the diff files + gh pr diff. Also removed # comment lines from inside claude_args (that block is passed verbatim to the CLI as args). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Argus is not auto-reviewing this PR because it modifies the review workflow itself (.github/ai-review/expert-adcp-reviewer.md, .github/workflows/ai-review.yml). A human reviewer should review and merge this PR; Argus will resume on subsequent PRs once these changes land on main.
…guard - #5: writeDiffFile degrades to a placeholder (not a hard fail) on an oversized diff / HTTP 406, so large PRs still get a human-flagged review. - #6: coverage.md largest-file rule reconciled to the base-SHA checkout — read changes from the diff files, cite head line numbers from the diff's + side. - #8: install-hooks.js no longer clobbers a foreign pre-commit hook (warns + prints how to chain the delegator instead). - #9: WG-constitution fetch gets --max-filesize + a fail-open warning; the prompt/findings GITHUB_OUTPUT heredocs use randomized sentinels (was fixed, a predictable-sentinel injection risk). - #10: reviewer prompt no longer claims the delta diff is intersected/trivial- filtered (the API compare diff is not); wording now matches reality. - CI dist guard: .secretariat/ai-review/scripts/check-dist.sh + a workflow that rebuilds each node action and fails if committed dist/ is stale — the real backstop for the bypassable pre-commit hook. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Argus is not auto-reviewing this PR because it modifies the review workflow itself (.github/ai-review/expert-adcp-reviewer.md, .github/workflows/ai-review.yml). A human reviewer should review and merge this PR; Argus will resume on subsequent PRs once these changes land on main.
…k workflow CodeQL flagged the new workflow for not restricting GITHUB_TOKEN. The job only checks out, builds, and diffs — add a top-level `permissions: contents: read`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Argus is not auto-reviewing this PR because it modifies the review workflow itself (.github/ai-review/expert-adcp-reviewer.md, .github/workflows/ai-review.yml). A human reviewer should review and merge this PR; Argus will resume on subsequent PRs once these changes land on main.
The #8 edit introduced a formatting drift that failed the format:check lane. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Argus is not auto-reviewing this PR because it modifies the review workflow itself (.github/ai-review/expert-adcp-reviewer.md, .github/workflows/ai-review.yml). A human reviewer should review and merge this PR; Argus will resume on subsequent PRs once these changes land on main.
3 participants
Why
The repo's PR reviewer was a single bash "arbiter" that parsed a free-text verdict word with shell globs — a fragile design with confirmed fail-open/fail-closed bugs (a decorated
approvecontaining "no blockers" flipped to request-changes; "changes requested" fell through to a non-blocking comment). This replaces it with a faithful fork of Scope3's Argus review action, where the verdict is a validated enum decided in tested TypeScript, eliminating the whole free-text-parsing bug class.This is Phase 1: prove the fork end-to-end in this repo, wired as local actions. Phase 2 (later) lifts the tree into a central AdCP-org repo consumed by every AdCP repo.
What Changed
.github/aao-secretariat/(revieworchestrator →setup→reviewer→arbiter).setup/arbiterarenode24TS actions (ncc-bundled,dist/committed,vitestsuites ported — 59 + 61 tests).revieweris a composite wrappingclaude-code-action@v1that emits schema-validatedfindings-json;arbiterdecidesapprove/request-changes/comment/escalatevia a constrainedsubmit_decisiontool and posts once.AAO-SECRETARIAT.md). "Argus" survives only as accurate provenance in the tree README.SECRETARIAT_APP_ID/SECRETARIAT_APP_PRIVATE_KEY,aao-secretariat[bot]); the reviewer fetches the WG constitution fromadcp@mainat review time (fail-open) and applies it; persona reframed to the Secretariat's review desk citingDR-NNNNdecision records.pull_request_targetwith base-SHA checkout + head fetch (never executes PR-head code), the review-workflow-modification gate, and theclaude-opus-4-8pin.ai-review.ymlreviewer andexpert-adcp-reviewer.md.Security
pull_request_targetand reviews fork PRs. Invariant upheld: base-SHA checkout, head is fetch-only (never checked out/executed), allgithub.event.*reach shells viaenv:(no inline interpolation), the WG constitution is trusted-ref/fail-open/data-only.Bash(gh api repos/*/issues/*)grant from the reviewer allowlist — withENV_SCRUB=0the token is in the Bash env, and that grant was a prompt-injection exfil path. Inline comments post via the MCP tool, so no functional loss.gh api .../contents/*read grant would otherwise permit a PUT. Not a regression vsmain(sameENV_SCRUB=0posture), but worth verifying since the App is new.Notes for the reviewer
pull_request_targetruns from the base SHA, the true end-to-end (reviewer → arbiter → posts the right enum) is only exercisable after this lands onmain..github/only + docs → no changeset needed.pulls/{n}/files(GitHub caps at 3000); the job'sdraft == falsefilter makes the orchestrator'sforce-review-labelunreachable on drafts.🤖 Generated with Claude Code