feat(review): enforce reviewer read-only via tool allowlist (#260) by neversettle17-101 · Pull Request #278 · aoagents/ReverbCode

neversettle17-101 · 2026-06-17T08:00:58Z

Fixes #260

Summary

The reviewer's read-only guarantee was enforced only by the prompt. This makes it an enforced sandbox by launching the reviewer off bypassPermissions (which skips the permission system and ignores allow/deny rules) with a scoped tool allowlist.

ports.LaunchConfig gains AllowedTools []string / DisallowedTools []string. Empty = unrestricted, so worker sessions are unaffected.
claude-code agent adapter plumbs them to --allowedTools / --disallowedTools. Each list is comma-joined into one value so a rule containing spaces (e.g. Bash(git diff:*)) is not split into separate tool names.
Reviewer adapter launches off bypass with:
- allow: Read, Grep, Glob, Bash(gh:*), Bash(git diff:*), Bash(git log:*), Bash(git show:*), Bash(git status:*), Bash(ao review submit:*)
- deny (defense in depth): Edit, Write, NotebookEdit, Bash(git push:*), Bash(git commit:*)
review.md write flow resolved without granting Write: ao review submit gains --body-text for inline Markdown (--body <path> stays for humans; passing both is a usage error), and the prompt records the verdict inline instead of writing a file. No write tool is granted at all — a true read-only guarantee.

Test

go build ./..., full go test ./..., gofmt, go vet — all pass.
New tests: agent-adapter flag emission (joined values; omitted when unset), a hermetic reviewer test (off-bypass + correct allow/deny lists, no claude binary needed), CLI --body-text inline + mutual-exclusion error.

Notes

Accepted stall risk (flagged in enhancement(review): enforce reviewer read-only via a tool allowlist instead of bypassPermissions + prompt #260): the reviewer pane is interactive and headless. Off bypass, a tool that is neither allowed nor denied prompts — with no one to answer, it stalls. The scoped Bash allowlist means an unlisted bash command (cat, rg, ls) would stall. This is the tuning tradeoff the issue calls out; worth an end-to-end review run to confirm the allowlist is permissive enough.

🤖 Generated with Claude Code

The reviewer's read-only guarantee was prompt-only. Add AllowedTools/ DisallowedTools to ports.LaunchConfig and plumb them through the claude-code adapter to --allowedTools/--disallowedTools. Launch the reviewer off bypassPermissions (which ignores allow/deny rules) with an allowlist scoped to Read/Grep/Glob and the few Bash commands a reviewer needs (gh, git diff/log/show/status, ao review submit), and an explicit deny list for Edit/Write/NotebookEdit/git push/git commit. Resolve the review.md write flow without granting Write: ao review submit gains --body-text for inline Markdown and the prompt records the verdict inline instead of writing a file, so no write tool is granted at all. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

greptile-apps

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

greptile-apps

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.

…251) Drives the real useWorkspaceQuery + real SessionsBoard / PullRequestsPage for an ordinary project (from /api/v1/projects) whose session has an open PR, mocking only the HTTP client and router. Confirms PR facts fetched from /sessions/{id}/pr flow through the shared workspace cache into both the Board card ("PR #278 · open") and the PR page row. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…279) * fix(frontend): populate session.pullRequest from /pr endpoint (#251) WorkspaceSession.pullRequest was declared but never populated, so every Boolean(session.pullRequest) check was dead: the Summary tab gated its PR fetch on it (never ran), and the Board card and /prs page read it directly (always empty). Hydrate the field centrally in fetchWorkspaces — the single place that builds session objects for the workspace, board, PR page, and sidebar — by fetching GET /sessions/{sessionId}/pr per non-terminated session in parallel and attaching {number, state}. A per-session fetch error degrades to "no PR" rather than failing the whole workspace query; terminated sessions are skipped. GET /sessions/{sessionId}/pr stays the single source of truth, so no new backend endpoint and no changes to the consuming components. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore: format with prettier [skip ci] * test(frontend): verify PR hydration end-to-end for a normal project (#251) Drives the real useWorkspaceQuery + real SessionsBoard / PullRequestsPage for an ordinary project (from /api/v1/projects) whose session has an open PR, mocking only the HTTP client and router. Confirms PR facts fetched from /sessions/{id}/pr flow through the shared workspace cache into both the Board card ("PR #278 · open") and the PR page row. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(frontend): move PR hydration integration test into __tests__/integration Cross-component integration test belongs in a dedicated folder, separate from the co-located unit tests. No behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

neversettle17-101

This closes the gap from #260 (allow/deny lists are wired through and the review.md write is avoided), but the central guarantee — "launch off bypassPermissions" — isn't actually enforced, so the fix can silently no-op back to the exact vulnerability #260 describes.

Core issue: ReviewCommand leaves Permissions as the zero value and doesn't set Config at all. In GetLaunchCommand, an empty Permissions (and empty Config.Permissions) normalizes to PermissionModeDefault, which emits no --permission-mode flag — Claude's TUI then resolves the starting mode from the user's own ~/.claude/settings.json defaultMode. If that ambient default is bypassPermissions (plausible for AO operators who run agents autonomously), the reviewer launches in bypass mode, and per the adapter's own doc comment, "bypassPermissions ignores both lists" — so the allowlist/denylist do nothing and the reviewer is back to prompt-only enforcement, with no visible signal that the sandbox didn't take effect.

The new test (TestReviewCommandLaunchesReadOnlyOffBypass) doesn't catch this: it only asserts agent.got.Permissions != PermissionModeBypassPermissions, which is trivially true for the empty string, but doesn't verify the resolved launch is non-bypass.

To actually guarantee non-bypass (matching what #260 asked for: "the reviewer must launch in a non-bypass mode"), ReviewCommand should pass an explicit non-bypass mode (e.g. ports.PermissionModeAcceptEdits or ports.PermissionModeAuto) rather than relying on whatever defaultMode happens to be configured on the host. That makes the read-only guarantee independent of ambient user config, rather than a hope.

Everything else — the tool-flag plumbing, comma-joining to avoid splitting Bash(git diff:*)-style rules, the --body-text CLI flow, and the frontend reviewer-pane wiring — looks correct and well-tested.

neversettle17-101 · 2026-06-17T13:45:11Z

+		// Launch off bypassPermissions so the allow/deny lists are enforced:
+		// the reviewer's read-only guarantee is the sandbox, not just the
+		// prompt. Permissions is left unset (default mode) on purpose.
+		AllowedTools:    reviewerAllowedTools,


Permissions is left as the zero value here, which (per GetLaunchCommand) emits no --permission-mode flag and defers to the user's ~/.claude/settings.json defaultMode. If that default is bypassPermissions, this allow/deny list is silently ignored — the exact failure mode #260 wanted closed. Pass an explicit non-bypass mode (e.g. ports.PermissionModeAcceptEdits) instead of relying on ambient config.

greptile-apps

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.

neversettle17-101

This review is an approval. (Posted as a COMMENT-event review, not APPROVE, because GitHub does not allow the PR author to approve their own PR.)

This addresses the finding from my previous review (PR #278 at commit 39906b3) directly and correctly.

Fix verified: ReviewCommand now sets Permissions: ports.PermissionModeAuto explicitly, instead of leaving it unset. Per appendPermissionFlags, this emits --permission-mode auto, which guarantees the launch is non-bypass regardless of what the host's ~/.claude/settings.json defaultMode happens to be — closing the gap where an ambient bypassPermissions default would have silently disabled the allow/deny enforcement.

The test was updated in lockstep: TestReviewCommandLaunchesReadOnlyOffBypass now asserts agent.got.Permissions == ports.PermissionModeAuto instead of the previous != bypassPermissions check, which is the right strengthening (the old assertion was trivially satisfied by the empty string that caused the bug).

go build ./... and the reviewer/agent adapter test suites pass. No further findings — approving.

# Conflicts: # backend/internal/adapters/reviewer/claudecode/claudecode.go # backend/internal/review/prompt.go

greptile-apps

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.

harshitsinghbhandari

LGTM

neversettle17-101

This review is an approval. (Posted as a COMMENT-event review, not APPROVE, because GitHub does not allow the PR author to approve their own PR.)

This head (6372208) is a merge of origin/main into the PR branch, with conflicts in backend/internal/adapters/reviewer/claudecode/claudecode.go and backend/internal/review/prompt.go (both touched by upstream #259, "reviewer posts to GitHub and records its verdict autonomously").

Conflict resolution verified correct:

claudecode.go: the merge kept this PR's fix — Permissions: ports.PermissionModeAuto plus the allow/deny tool lists — rather than reverting to #259's bypassPermissions launch. The non-bypass sandbox guarantee from my earlier review is intact.
prompt.go: the merge correctly combined #259's "post to GitHub first, then record with AO" step ordering with this PR's "no review.md write, use --body-text" flow. Step numbering (1/2/3) and the "only if step 2 fails, still run step 3" fallback line are consistent and don't reference removed steps.
No leftover conflict markers (grep for <<<<<<</=======/>>>>>>> across backend/ is clean).

Diffing this PR's own files between the previously-reviewed head (283f3e3) and this one shows no new changes from this PR's own work — the rest of the diff (frontend/src/api/schema.ts, pr-hydration.test.tsx, useWorkspaceQuery.*) comes from the merged-in upstream commits (#251/#279, #230), not from this PR.

go build ./..., go vet ./..., gofmt -l ., and the full go test ./... backend suite all pass clean.

No further findings — approving.

Vaibhaav and others added 3 commits June 16, 2026 20:08

feat(frontend): show reviewer worker controls

6d81019

chore: format with prettier [skip ci]

a678421

greptile-apps Bot reviewed Jun 17, 2026

View reviewed changes

style(review): gofmt reviewer adapter test

39906b3

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

greptile-apps Bot reviewed Jun 17, 2026

View reviewed changes

neversettle17-101 mentioned this pull request Jun 17, 2026

bug: session inspector in summary section doesn't show PR details even though its been raised #251

Closed

neversettle17-101 requested a review from Vaibhaav-Tiwari June 17, 2026 10:11

Merge remote-tracking branch 'origin/main' into pr-278

5d03319

neversettle17-101 commented Jun 17, 2026

View reviewed changes

fix(review): force reviewer non-bypass permissions

283f3e3

greptile-apps Bot reviewed Jun 17, 2026

View reviewed changes

neversettle17-101 commented Jun 17, 2026

View reviewed changes

Merge remote-tracking branch 'origin/main' into pr-278

6372208

# Conflicts: # backend/internal/adapters/reviewer/claudecode/claudecode.go # backend/internal/review/prompt.go

greptile-apps Bot reviewed Jun 17, 2026

View reviewed changes

harshitsinghbhandari approved these changes Jun 17, 2026

View reviewed changes

neversettle17-101 mentioned this pull request Jun 17, 2026

bug(lifecycle): a sticky waiting_input session whose runtime pane died is never reaped to exited (stuck needs_input, frozen activity, dead terminal) #290

Open

neversettle17-101 commented Jun 17, 2026

View reviewed changes

neversettle17-101 mentioned this pull request Jun 17, 2026

bug(lifecycle): merge-conflict on a PR is detected but never surfaced — no nudge, no notification, no status (suppressed for waiting_input sessions) #295

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(review): enforce reviewer read-only via tool allowlist (#260)#278

feat(review): enforce reviewer read-only via tool allowlist (#260)#278
neversettle17-101 wants to merge 7 commits into
mainfrom
ao/agent-orchestrator-2

neversettle17-101 commented Jun 17, 2026

Uh oh!

greptile-apps Bot left a comment

Uh oh!

greptile-apps Bot left a comment

Uh oh!

neversettle17-101 left a comment

Uh oh!

neversettle17-101 Jun 17, 2026

Uh oh!

greptile-apps Bot left a comment

Uh oh!

neversettle17-101 left a comment

Uh oh!

greptile-apps Bot left a comment

Uh oh!

harshitsinghbhandari left a comment

Uh oh!

neversettle17-101 left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

neversettle17-101 commented Jun 17, 2026

Summary

Test

Notes

Uh oh!

greptile-apps Bot left a comment

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot left a comment

Choose a reason for hiding this comment

Uh oh!

neversettle17-101 left a comment

Choose a reason for hiding this comment

Uh oh!

neversettle17-101 Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot left a comment

Choose a reason for hiding this comment

Uh oh!

neversettle17-101 left a comment

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot left a comment

Choose a reason for hiding this comment

Uh oh!

harshitsinghbhandari left a comment

Choose a reason for hiding this comment

Uh oh!

neversettle17-101 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants