Skip to content

fix(argus): make the AI PR review actually run, and fail the job when it doesn't - #42

Merged
bokelley merged 3 commits into
mainfrom
fix/argus-review-crash
Oct 16, 2026
Merged

bokelley merged 3 commits into
mainfrom
fix/argus-review-crash

Conversation

@bokelley

@bokelley bokelley commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Closes #41

What was wrong

claude_args passed --output-format json. claude-code-action forwards unrecognised args verbatim to the claude CLI — which it already runs with --input-format stream-json --output-format stream-json. The CLI rejects the pair and exits 1 in ~0.3s, before it reads the diff:

Error: --input-format=stream-json requires output-format=stream-json.

The step is continue-on-error: true and nothing downstream failed the job, so every run reported success. Argus has not posted a single review since the fork-PR hardening landed on 2026-05-28 (#29) — PRs #38 and #40 both show zero reviews from aao-ipr-bot[bot].

The Anthropic client constructor text in the issue is a red herring. It is Bun printing source context around the agent SDK's own Claude Code process exited with code 1 throw in sdk.mjs (lines 55–60, error at 60:11499). The directory mismatch … tsconfig.json line is a separate Bun runtime notice. Neither is the cause; the child's real stderr was hidden by show_full_output: false.

Reproduced, not inferred

Downloaded Claude Code 2.1.150 (the version the pinned action installs; checksum verified against the release manifest) and ran it with the workflow's exact argument set under an isolated HOME:

CLI args result
2.1.150 workflow args with --output-format json Error: --input-format=stream-json requires output-format=stream-json., exit 1
2.1.150 same without it starts normally, reaches the API
2.1.300 with --output-format json exit 1
2.1.300 without it starts normally; permissionMode=dontAsk, model=claude-opus-5-5, deny list applied

(v1.0.251 installs CLI 2.1.301; 2.1.300 is the closest build available locally. The argument-validation path in question is unchanged between them, and the end-to-end run below used 2.1.300.)

Bumping the action alone would not have fixed this. I checked base-action/src/parse-sdk-options.ts at v1.0.251: unknown flags still go through as extraArgs unchanged.

What changed

The fix

  • Drop --output-format json. --json-schema alone is what populates structured_output.
  • A review that did not happen now fails the job. Everything that can fail is continue-on-error so the notice comment still posts; a final !cancelled() step reads verify.outputs.review_posted and exits 1 when no review landed.

Asked for in the issue

  • Repin anthropics/claude-code-action 787c5a0c (2026-05-23) → 374f254f (v1.0.251, 2026-10-15), full SHA.
  • The model moves to a workflow-level ARGUS_MODEL, checked against GET /v1/models/{id} before the review runs. Hard-fails on 401/404 (revoked key, retired model); warns and continues on 403/000 so a transport blip or a key scoped away from /v1/models can't block every PR.

Expert-review findings (code-reviewer + security-reviewer, run before this PR opened; both reports fully addressed)

  • The self-modification gate now covers scripts/check-ai-review-workflow.cjs and validate-ai-review-config.yml too — otherwise a PR could delete the guard and have Argus approve its own defanging — and it fails closed when gh pr diff fails (it 406s on very large diffs; "unknown" was reading as "safe").
  • always() → ${{ !cancelled() }} on the four terminal steps: always() is true on cancellation, so a superseded push would mint a write-capable App token and leave a "review could not complete" notice on a run nobody failed. Paired with a new concurrency: argus-<pr> group.
  • The failure notice is deduped by marker and updated in place rather than appended once per synchronize.
  • settings.permissions.deny now mirrors --disallowedTools (subagents launched via Task inherit the permission rules, not the CLI flag), and WebFetch/WebSearch are denied — a code reviewer has no need for network egress.
  • The generated prompt now explicitly fences .argus/* as untrusted PR-author content. PR title, body and diff are attacker-controlled and reach the model; the post step only validates the shape of the JSON.
  • The API key goes to curl on stdin (-H @-) rather than argv.
  • Stale Replace / NotebookEditCell tool names replaced with the current NotebookEdit.

Proof the fixed invocation actually reviews a diff

The self-modification gate pauses Argus on this PR by design, so the CI proof has to come from a follow-up PR. To avoid merging on faith, I ran the fixed invocation end-to-end locally first: CLI 2.1.300, the exact post-fix claude_args and settings, the real base-SHA Argus prompt built by the same sed/awk pipeline, and .argus/* snapshotted from this PR via gh pr view / gh pr diff.

TOOL: Read   .argus/pr-view.json
TOOL: Read   .argus/files.txt
TOOL: Read   .argus/pr.diff
TOOL: Grep   'D21|D18' ROADMAP.md
TOOL: Read   .github/workflows/ai-review.yml
TOOL: Agent  subagent_type=code-reviewer
TOOL: Agent  subagent_type=security-reviewer
TOOL: Glob   {.claude/**,.agents/**,.github/ai-review/**,.github/CODEOWNERS}
TOOL: StructuredOutput {'event': 'COMMENT', 'body': ...}
RESULT subtype=success is_error=False turns=9
HAS structured_output: True

It read the diff, used the repo's subagents, and returned schema-valid {event, body} — the first time this path has worked since 2026-05-28. It independently chose COMMENT rather than APPROVE, citing the self-modification gate.

Its review found real problems in the first two commits, and 4a9e989 fixes them:

  • The gate missed .claude/agents/, .agents/roles/ and .github/CODEOWNERS. Argus reads all of those from the base SHA, so a PR rewriting a subagent is judged by the old one — approve it and every later PR is judged by the weakened one. Same argument that already guards the workflow. Patterns are now anchored with $ too; they were prefix matches.
  • The paused notice claimed "this PR modifies Argus's own configuration" even on the fail-closed branch, and named two of what are now six guarded paths.
  • listComments read only the first page in both notice steps, so the dedupe marker could be missed on a long PR.
  • The stale "could not complete" notice was never cleared once a later push got reviewed.
  • Four validator defects, all of which made it weaker than advertised: - id:-leading steps folded into their predecessor; comment lines attributed to the step above; directive checks satisfied by a comment mentioning if: !cancelled(); on.*.types list items counted as steps; guarded paths matched anywhere in the step rather than inside the grep -E pattern; the deny-list comparison only ran one direction; and /untrusted/i matched a comment rather than the echoed prompt.

Both this and the security-reviewer pass flagged the same Medium: Read/Glob/Grep are unrestricted and the sandbox is inert, so a successful injection could read a credential and quote it in a review body published on a public PR, where log masking does not apply. 4a9e989 closes both halves — Read is denied on /proc, /etc, /root, $RUNNER_TEMP and the runner dot-directories (verified against the real CLI: the rules parse, Read still works for the workspace, and /etc/hostname comes back as a permission_denial), and the post step refuses bodies containing credential-shaped content.

Tests

scripts/check-ai-review-workflow.cjs — dependency-free, run by a new always-on CI job. Every assertion was mutation-tested; each of these reverts turns CI red:

mutation caught by
re-add --output-format json (or =json) output-format check
delete the "did Argus post a review" gate gate presence
make the gate continue-on-error/if: false/drop !cancelled() gate conditions
rewire verify to if false so it always reports success verify-step check
delete the verify step review_posted check
hardcode --model, or drop --json-schema args checks
delete the model API check model-verification check
unpin the action to @v1 SHA-pin check
drop a tool from permissions.deny but not the CLI flag deny-parity check
remove WebFetch/WebSearch from the deny list read-only reviewer check
drop any guarded path from the self-mod gate, or make it fail open self-mod gate checks
remove the untrusted-input fence from the prompt fence check

Also ran locally: ./gradlew build :build-logic:test (green, no skips), npx changeset status --since=origin/main, npx commitlint, and a YAML parse of both workflows. No hooks skipped.

Notes for the maintainer

  • This PR cannot be reviewed by Argus. It touches ai-review.yml, so the self-modification gate pauses Argus by design and a human must take it. Proof that the fix works comes from a separate test PR against main after this merges — the issue explicitly allows this. I'll post the run link and the log line showing the diff was reviewed.
  • The model bump is a deliberate upgrade riding along, not part of the remediation. claude-opus-4-7 is still served and was not a cause of the crash; claude-opus-5-5 is the current Opus and is cheaper ($4/$20 vs $5/$25 per MTok).
  • The sandbox block in settings is inert on GitHub runners and I left it in place with a comment saying so. The action only installs bubblewrap/socat when allowed_non_write_users is set, so Claude Code logs "Sandbox disabled" and continues, and sandbox.filesystem.denyRead would not govern the Read tool even if it worked. The permissions.deny rules are the real control, which is why the path-scoped Read(...) denies live there.
  • Known gap, not fixed here: if the job hits timeout-minutes: 20, the !cancelled() steps are skipped, so the PR gets no notice — though the job still ends red, so it is not a silent pass. Our end-to-end run took 9 turns, well inside the budget. Worth a follow-up if it ever bites.
  • An Argus approval cannot satisfy branch protection on its own. The org ruleset requires a CODEOWNERS review, and .github/CODEOWNERS lists only @bokelley @MichielDean.
  • validate-ai-review-config.yml uses tag refs for actions/checkout/setup-node, matching validate-agent-roles-sync.yml. It has no secrets and a read-only token. Note it runs the PR's own copy of the script, so it guards against honest mistakes, not hostile PRs — the self-modification gate is what covers that case.
  • The validator asserts actions are SHA-shaped, not that a SHA matches the release its comment names. I verified by hand that 374f254f… is exactly what tag v1.0.251 dereferences to (chore: bump Claude Code to 2.1.301 and Agent SDK to 0.3.301, 2026-10-15). Automating that would need network access in the validator.

🤖 Generated with Claude Code

bokelley and others added 2 commits October 16, 2026 15:43
Argus has not posted a review since 2026-05-28. `claude_args` passed
`--output-format json`, which claude-code-action forwards verbatim to the
`claude` CLI. The action already runs the CLI with `--input-format stream-json
--output-format stream-json`, so the CLI rejected the conflicting pair
("--input-format=stream-json requires output-format=stream-json") and exited 1
about 0.3s in, before reading the diff. The step is `continue-on-error`, so the
job still reported success and a silent no-review looked like a pass.

Reproduced locally against both the pinned Claude Code 2.1.150 and current
2.1.300: with `--output-format json` the CLI exits 1 immediately; without it,
the same argument set starts normally.

- drop `--output-format json`; `--json-schema` alone populates structured_output
- repin anthropics/claude-code-action 787c5a0c (2026-05-23) to 374f254f (v1.0.251)
- hoist the model into `env.ARGUS_MODEL` (claude-opus-5-5, up from the
  generation-old claude-opus-4-7) and verify it against the Anthropic Models
  API before the review runs, so a retired model or revoked key fails loudly
- deny WebFetch/WebSearch and drop the stale `Replace` / `NotebookEditCell`
  tool names from `--disallowedTools` in favour of the current `NotebookEdit`
- fail the job when no review was posted, instead of only warning
- add scripts/check-ai-review-workflow.cjs and a CI job that enforces all of
  the above, so the regression cannot come back unnoticed

Closes #41

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Round-2 changes from the code-reviewer and security-reviewer passes on the
previous commit.

Gate correctness:
- the self-modification gate now also covers scripts/check-ai-review-workflow.cjs
  and .github/workflows/validate-ai-review-config.yml; without that a PR could
  delete the guard and have Argus approve its own defanging
- the gate fails closed: `gh pr diff` 406s on very large diffs, and an
  undetermined answer was reading as "safe to review"
- `always()` -> `${{ !cancelled() }}` on the four terminal steps, so a
  superseded push no longer mints an App token, posts a "review could not
  complete" notice, and reds a run nobody failed
- the failure notice carries a marker and is updated in place instead of
  piling up one comment per `synchronize`
- `concurrency: argus-<pr>` with cancel-in-progress, which is what makes the
  `!cancelled()` conditions meaningful

Hardening:
- the model check now hard-fails only on 401/404 and warns on 403/000, so a
  transport blip or a key scoped away from /v1/models cannot block every PR
- the API key goes to curl on stdin (`-H @-`) instead of argv
- settings.permissions.deny mirrors --disallowedTools; subagents launched via
  Task inherit the permission rules, not the CLI flag
- the generated prompt now fences `.argus/*` as untrusted PR-author content

Validator additions, each mutation-tested:
- the two deny layers must agree, and Bash/Edit/Write/WebFetch/WebSearch must
  stay denied
- the self-mod gate must match every guarded path and must fail closed
- the verify step must read the reviews API and be able to emit
  review_posted=false (previously `if false` passed validation)
- the prompt must fence its inputs as untrusted
- `!cancelled()` accepted alongside `always()` on the gate

The validator workflow also drops its `paths:` filter — a path-filtered job
never reports on unrelated PRs, which makes it unusable as a required check.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@bokelley
bokelley requested a review from MichielDean as a code owner October 16, 2026 15:54
@aao-ipr-bot

aao-ipr-bot Bot commented Oct 16, 2026

Copy link
Copy Markdown

🛑 Argus paused on this PR.

This PR modifies Argus's own configuration (.github/workflows/ai-review.yml or .github/ai-review/**). Argus will not post an approving review on changes to its own gates — a human reviewer must take this PR.

Workflow run

Automated by the Argus self-modification gate.

Ran the fixed invocation end to end against this PR's real diff before
merging: CLI 2.1.300, the exact post-fix claude_args, the real base-SHA
prompt, and `.argus/*` snapshotted from PR #42. It read all three `.argus`
files, delegated to the code-reviewer and security-reviewer subagents, and
returned schema-valid structured output in 9 turns — the first time this
path has worked since 2026-05-28. Its findings are applied here.

Self-modification gate:
- also guards `.claude/agents/`, `.agents/roles/` and `.github/CODEOWNERS`.
  Argus loads all of these from the base SHA, so a PR that rewrites a
  subagent is judged by the old one; approve it and every later PR is judged
  by the weakened one — the same argument that already guards the workflow
- filename patterns are anchored with `$`; they were prefix matches
- the paused notice no longer claims the PR edited `ai-review.yml`, which was
  wrong for the fail-closed branch and listed two of the now six paths

Credential exposure through the published review:
- the post step refuses bodies containing credential-shaped content. The
  review is published on a public PR, where Actions log masking does not
  apply, so a successful injection would otherwise have a publication channel
- `Read` is denied on /proc, /etc, /root, $RUNNER_TEMP and the runner's dot
  directories. Verified against the real CLI: the rules parse, `Read` stays
  available for the workspace, and `/etc/hostname` comes back as a
  permission_denial

Notices:
- both notice steps paginate `listComments`; the marker could fall off page
  one on a long PR and produce duplicates
- the stale "could not complete" notice is deleted once a later push is
  reviewed, instead of sitting next to the real review contradicting it

Validator (each fix mutation-tested):
- steps split on the list dash, not `- name:`/`- uses:`, so a step leading
  with `- id:` is no longer folded into its predecessor, and comment lines
  attach to the step below them. Directive assertions now run against the
  step with comments stripped — a `# if: !cancelled()` comment used to
  satisfy the gate check
- the scan starts at the job's `steps:` key; `on.*.types` list items were
  being counted as steps
- guarded paths are checked inside the `grep -E` pattern, not anywhere in the
  step text
- the deny-list comparison runs both ways, ignoring path-scoped `Read(...)`
  rules that only exist on the settings side
- the untrusted-input fence must be echoed into the prompt; `/untrusted/i`
  anywhere in the file used to pass
- new: the post step must screen for credential-shaped bodies

validate-ai-review-config.yml pins checkout by SHA and drops setup-node
(ubuntu-latest ships Node 22). Its comment no longer implies it should become
a required check: that would change D21's required-check set and needs a
ROADMAP decision row first.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@bokelley
bokelley merged commit eef1e03 into main Oct 16, 2026
11 checks passed
@bokelley
bokelley deleted the fix/argus-review-crash branch October 16, 2026 16:22
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.

Argus AI PR review crashes in claude-code-action before reading the diff

1 participant