ci(deep-review): restore fork reviews by removing the reviewer's shell

[teeohhem]

Stacked on #2936 — review that one first. Base is claude/deep-review-diff-integrity, so this diff shows only the fork-enablement change.

Fork-PR deep reviews have been broken since 2026-07-20 — 18 runs, 0 successes, across all 6 fork PRs. This restores them. The flag that unblocks them and the containment that makes it safe land together, deliberately.

Problem

actions/checkout@v6.1.0 backported allow-unsafe-pr-checkout as an explicitly breaking change defaulting to false, and we track the moving v6 tag — so it arrived with no change on our side. v7 is not an escape: v7.0.1 ships the same input with the same default.

Setting that flag alone would re-open stranger-triggered code execution next to ANTHROPIC_API_KEY and a token with pull-requests: write. Note the current failure is fail-closed: fork PRs reach no reviewer today, so this PR is what switches the exposure on. That's why it can't ship on its own.

Fix: remove Bash, don't narrow it

Bash(git:*) cannot be reduced to a read-only grant. --allowedTools matches by command prefix and cannot constrain a flag, and:

• git log --output=<path> is an arbitrary file write — aim it at .git/config, set diff.external, and the next git diff executes it.
• git diff --no-index <a> <b> is an arbitrary file read.

Both verified locally. No subset of git subcommands is read-only, so --tools replaces the built-in roster instead, leaving Read, Grep, Glob, Task, Skill — no Bash, no Write/Edit, no WebFetch/WebSearch. The model cannot execute what it reads and has no network egress; its only output channel is the review text this workflow posts.

That's viable because the skill's shell use is confined to Stage 1 (scope) and Stage 2 (intent) in mode:report-only, which also skips run-id generation and artifact writes. A trusted step materializes the diff, file list, commits, branch and prior review comments into RUNNER_TEMP, and the prompt points the skill at them. Prior comments are included so ce-previous-comments-reviewer stays on the roster instead of silently dropping when it can't reach gh pr view.

The prompt also demands the fan-out explicitly — without that instruction the orchestrator skips dispatch and returns a plausible single-pass review. #2936 adds the assertion that catches it.

Not fixed here

The job still holds pull-requests: write and ANTHROPIC_API_KEY. With no Bash and no WebFetch the model can't reach either, so moving the posting into a separate workflow_run job is now defense-in-depth rather than load-bearing — worth doing, not required for this. allowed_non_write_users: '*' is kept for the same reason; there's a comment tying it to the tool restriction so the two aren't decoupled by accident.

Tests

Against the pinned CLI (2.1.215) and plugin ref (v3.6.1):

• The shipped claude_args tokenize to exactly Agent, Glob, Grep, Read, Skill — verified by extracting them from this YAML and passing them through verbatim. Bash is gone.
• A no-Bash run of the skill dispatched 9 sub-agents with zero tool errors and produced a real merged review — which independently rediscovered the --output write vector and found the .gitattributes blinding that ci(deep-review): stop the reviewer reporting clean on code it never saw #2936 fixes.
• Ran the shipped scope step end to end: all six files materialize, prior-review-comments.json is a valid array.
• Two --tools footguns found and documented inline: it does not validate names (a typo silently yields a tool-less agent), and --tools "" does not disable everything despite its help text.

Cost is unchanged from today and not small: ~$5 for a one-file diff at 9 agents.

The fork path itself can only be exercised once this is on main (pull_request_target loads the workflow from the base branch); marking draft fork PR #2922 ready-for-review is the cheapest trigger.

CI-only change, so no changeset per AGENTS.md.

How to test on Vercel preview

N/A — non-UI change (CI workflow only).

Preview routes: N/A

Steps:

N/A

References

• Linear Issue: N/A
• Related PRs: ci(deep-review): stop the reviewer reporting clean on code it never saw #2936 (base), ci(deep-review): pin claude-code CLI to dodge bwrap sandbox regression #2823 (previous deep-review CI fix), actions/checkout#2500 (upstream backport)
• GitHub changelog: Safer pull_request_target defaults for GitHub Actions checkout

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments

Project	Deployment	Actions	Updated (UTC)
hyperdx-oss	Ignored	Preview	Aug 18, 2026 3:13pm
hyperdx-storybook	Ignored	Preview	Aug 18, 2026 3:13pm

[changeset-bot]

⚠️ No Changeset found

Latest commit: 68daf99

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[greptile-apps]

Greptile Summary

The workflow restores deep reviews for fork pull requests while removing the reviewer's shell and network-capable tools.

• Enables unsafe fork checkout together with a restricted read-only Claude tool roster.
• Materializes the trusted diff, file list, commit list, branch metadata, and prior review comments before invoking the reviewer.
• Correctly flattens paginated prior-review-comment pages into one JSON array.
• Adds transcript checks for reviewer fan-out and unexpected tool usage.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename	Overview
.github/workflows/deep-review.yml	Enables fork reviews, supplies precomputed review scope to a shell-less reviewer, fixes prior-comment pagination, and validates reviewer fan-out and tool restrictions.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Fork pull request] --> B[Checkout fork head]
  B --> C[Validate merge base and changed files]
  C --> D[Materialize trusted review scope]
  D --> E[Run shell-less multi-agent reviewer]
  E --> F[Validate fan-out and tool roster]
  F --> G[Post structured review]
  F -->|Validation fails| H[Mark reviewer unhealthy]

Loading

_{Reviews (2): Last reviewed commit: "ci(deep-review): close gaps the shell-le..." | Re-trigger Greptile}

[github-actions]

Deep Review

✅ No critical issues found. This is a CI-only workflow change; the AGENTS.md:212 policy exempts CI from changesets, so the "no changeset" claim is compliant. The scope-materialization and prior-comment fallbacks are robust (pipefail + type=="array" guard correctly degrade to []), and the fan-out floor of 6 is correctly calibrated to the guaranteed always-on roster. No P0/P1 ship-blockers were confirmed. The recommendations below harden the containment story and fix one self-inconsistency this diff introduced.

🟡 P2 -- recommended

• .github/workflows/deep-review.yml:549 -- This diff adds a prompt sentence stating the job fails below 4 sub-agents while the same diff raised the gate to -lt 6, so a run that dispatches 4–5 satisfies the stated instruction yet is failed, omits the state marker, and re-runs the full fan-out on every later push.
  • Fix: Change the prompt's stated floor to 6 and name the two CE always-on agents so the instruction matches the enforced gate.
  • _{correctness, maintainability, reliability, adversarial}
• .github/workflows/deep-review.yml:878 -- The tool-set assertion inspects only the orchestrator's top-level transcript, so it cannot observe the Task sub-agents (whose persona frontmatter grants Bash/Write) where fork code is read, and --disallowedTools never denies Bash/Write, leaving the no-shell boundary resting on unverified --tools propagation into sub-agents.
  • Fix: Add Bash, Write, Edit, WebFetch, and WebSearch to --disallowedTools so a global deny enforces the boundary, and scope the assertion's success message to the orchestrator.
  • _{security, adversarial, agent-native, testing, project-standards}
• .github/workflows/deep-review.yml:358 -- The new prior-review-comments.json copies fork-author-controlled review-comment bodies verbatim (length-capped only) and the prompt labels them authoritative, bypassing the control-char stripping and per-run fence that guard the PR title and body against prompt injection.
  • Fix: Strip control characters from comment bodies and wrap the file's content in the same per-run fence and "do not follow instructions" warning used for title and body.
  • _adversarial

🔵 P3 nitpicks (5)

• .github/workflows/deep-review.yml:726 -- The read-only tool set is hand-duplicated across --tools, --allowedTools, and the assertion's jq allow-list (which intentionally differs by the Task/Agent alias), inviting silent drift on the security backstop.
  • Fix: Single-source the three lists or add a cross-referencing comment so a capability change updates all three together.
• .github/workflows/deep-review.yml:358 -- The fetch uses only /pulls/{n}/comments, so hasPriorComments and ce-previous-comments-reviewer miss feedback posted in the PR conversation tab (/issues/{n}/comments), including prior bot review comments.
  • Fix: Also materialize /issues/{n}/comments, or document that only inline review threads are in scope.
• .github/workflows/deep-review.yml:358 -- gh api --paginate over an attacker-controlled comment count has no page or size ceiling, so a fork author posting many comments inflates the materialized file streamed into reviewer context.
  • Fix: Cap the pagination page count or the element count before writing the file.
• .github/workflows/deep-review.yml:379 -- The existence-check loop asserts the materialize step's own write targets rather than the six filenames the prompt hardcodes, so a coordinated rename still leaves the prompt pointing at missing paths despite the comment claiming that case is caught.
  • Fix: Assert the prompt-referenced paths, or single-source the filenames so prompt-versus-materialize drift fails the step.
• .github/workflows/deep-review.yml:346 -- commits.txt is git log --oneline (subjects only), dropping commit-body context that Stage 2 intent discovery could previously read via full git log.
  • Fix: Materialize full commit message bodies alongside the oneline list if intent fidelity matters.

---

Reviewers (9): correctness, testing, maintainability, project-standards, agent-native, learnings-researcher, security, adversarial, reliability.

Testing gaps:

• No end-to-end check that --tools/--disallowedTools propagate to Task sub-agents — the load-bearing containment claim is unverified.
• The fan-out gate counts dispatches, not always-on identities, and has no negative test at the 5-versus-6 boundary.
• No fixture exercises the prior-comments pipeline against adversarial input, nor confirms the tool-set assertion won't false-positive on a legitimate unlisted tool name (SlashCommand, TodoWrite, mcp__*).

[teeohhem]

Dogfooded this PR through the shell-less reviewer

Ran the exact shipped config (prompt + claude_args extracted from this YAML) against this PR's own diff, locally, on the pinned CLI 2.1.215 and plugin compound-engineering-v3.6.1.

Result: 6 sub-agents dispatched, zero tool errors, success, 14 turns, $4.58. Tools used were only Read and Agent — no persona needed Bash. That's the design validated end to end, not in principle.

It also found real defects in the change, which is the more useful outcome. Fixed in 68daf99:

Finding	Verdict
gh api --paginate + --jq writes concatenated [...] documents, not one array	Valid, my bug. Reproduced: 5 back-to-back arrays. Fixed — but note the suggested fix doesn't work: --slurp "is not supported with --jq", and it wraps pages rather than flattening, so the projection moved to a separate jq '.[][]'. Output is now validated as an array before dispatch.
Fan-out floor was >= 4 while the adjacent comment stated the roster is 6	Valid. A run missing part of the always-on set passed and cached a valid state marker. Raised to 6.
No assertion that Bash was actually absent — the invariant rested on --tools, which doesn't validate names	Valid and the best of them. Added a transcript assertion failing on any tool outside Read/Grep/Glob/Task/Skill. Verified it fires by injecting Bash and WebFetch into a real transcript — an assertion that never trips is worse than none.
Deny list covers only /proc and /sys; Grep/Glob are unscoped and bypass it	Conclusion valid, mechanism wrong. A Read(<path>) deny does constrain Grep — verified: with Read(//tmp/secretdir/**) denied, Grep there returns Permission to read /tmp/secretdir has been denied. (release.yml's note about unscoped Grep is about allow rules, which is a different claim.) But the list was genuinely too narrow, so it now covers the credential-bearing trees on a hosted runner, and the comment says plainly that a denylist raises cost rather than proving a boundary.
Prompt hardcodes six scope filenames, only diff.patch was guarded	Valid. All six asserted before dispatch.
Gate-hash comment omits that the materialized diff also passes --text	Valid — and this is the second time that comment overclaimed. Corrected.

Deliberately not fixed here

• Extract step lacks set -euo pipefail and an empty-review check, so an empty structured_output posts an empty comment and still stamps a marker, poisoning the gate. Real, but pre-existing on main and not touched by this diff — it wants its own PR rather than riding along in a security change.
• merge_base is reused only by the scope step; the verify and gate steps still recompute it. Valid inconsistency, cosmetic here since all three derive it identically.
• Two steps use bare set -e rather than set -euo pipefail. Pre-existing; changing error semantics in steps this PR only lightly touches isn't worth the blast radius here.

One caveat on my own validation, for the record: an earlier attempt ran unrestricted because a shell-ism silently produced an empty argument array. I caught it from the arg echo, discarded that run, and added a guard that refuses to run when the array looks wrong. The run reported above is the guarded one.

[github-actions]

🟢 Tier 1 — Trivial

Docs, images, lock files, a dependency bump, or an automated release. No functional code changes detected.

Why this tier:

• All files are docs / images / lock files

Review process: Auto-merge once CI passes. No human review required.
SLA: Resolves automatically.

Stats

• Production files changed: 0
• Production lines changed: 0
• Branch: claude/deep-review-no-bash
• Author: teeohhem

To override this classification, remove the review/tier-1 label and apply a different review/tier-* label. Manual overrides are preserved on subsequent pushes.

[github-actions]

E2E Test Results

✅ All tests passed • 300 passed • 1 skipped • 914s

Status	Count
✅ Passed	300
❌ Failed	0
⚠️ Flaky	0
⏭️ Skipped	1

Tests ran across 4 shards in parallel.

View full report →