From 876d3ac281a2f1dd7a929d1cab384f415edebcb3 Mon Sep 17 00:00:00 2001 From: Aditi Chauhan Date: Wed, 17 Jun 2026 00:32:20 +0530 Subject: [PATCH 1/2] fix(review): make the reviewer post to GitHub and record its verdict autonomously MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The claude-code reviewer never completed a review on its own. Three defects in the reviewer launch + flow: - It launched with no permission mode, so a headless pane stalled on the first tool-permission prompt and never ran gh/ao. Launch with bypassPermissions (read-only is enforced by the prompt, not a sandbox). - The reviewer pane got no pinned PATH, so `ao review submit` resolved to a foreign `ao` on the inherited PATH and failed. Pin PATH to the daemon's own dir the same way worker sessions do — export HookPATH and reuse it in the launcher. - The prompt did not enforce ordering. Make it post the review on the PR via gh first, then run `ao review submit`. Fixes #258 Co-Authored-By: Claude Opus 4.8 --- .../reviewer/claudecode/claudecode.go | 4 ++++ backend/internal/review/launcher.go | 22 ++++++++++++++++++- backend/internal/review/prompt.go | 6 +++-- backend/internal/session_manager/manager.go | 9 ++++---- .../session_manager/provision_test.go | 8 +++---- 5 files changed, 38 insertions(+), 11 deletions(-) diff --git a/backend/internal/adapters/reviewer/claudecode/claudecode.go b/backend/internal/adapters/reviewer/claudecode/claudecode.go index 5573553f..8f595e58 100644 --- a/backend/internal/adapters/reviewer/claudecode/claudecode.go +++ b/backend/internal/adapters/reviewer/claudecode/claudecode.go @@ -39,6 +39,10 @@ func (r *Reviewer) ReviewCommand(ctx context.Context, inv ports.ReviewInvocation WorkspacePath: inv.WorkspacePath, Prompt: inv.Prompt, SystemPrompt: inv.SystemPrompt, + // The reviewer runs headless with no human to approve tool prompts; it + // is read-only by prompt and must run gh/ao on its own, so bypass the + // permission gate rather than stall on the first prompt. + Permissions: ports.PermissionModeBypassPermissions, }) if err != nil { return ports.ReviewCommandSpec{}, err diff --git a/backend/internal/review/launcher.go b/backend/internal/review/launcher.go index 28e312f1..fef9d0b3 100644 --- a/backend/internal/review/launcher.go +++ b/backend/internal/review/launcher.go @@ -3,9 +3,11 @@ package review import ( "context" "fmt" + "os" "github.com/aoagents/agent-orchestrator/backend/internal/domain" "github.com/aoagents/agent-orchestrator/backend/internal/ports" + sessionmanager "github.com/aoagents/agent-orchestrator/backend/internal/session_manager" ) // Launcher spawns, re-notifies, and probes a reviewer over a worker's worktree. @@ -97,7 +99,7 @@ func (l *agentLauncher) Spawn(ctx context.Context, spec LaunchSpec) (string, err SessionID: domain.SessionID(handleID), WorkspacePath: spec.WorkspacePath, Argv: cmd.Argv, - Env: cmd.Env, + Env: pinnedEnv(cmd.Env), }) if err != nil { return "", fmt.Errorf("reviewer runtime: %w", err) @@ -105,6 +107,24 @@ func (l *agentLauncher) Spawn(ctx context.Context, spec LaunchSpec) (string, err return handle.ID, nil } +// pinnedEnv returns the reviewer command's env with PATH pinned to the daemon's +// own directory, so the bare `ao` the reviewer runs (e.g. `ao review submit`) +// resolves to this daemon's CLI rather than a foreign `ao` first on the +// inherited PATH. Mirrors the worker-session pin in the session manager. +// Best-effort: an unpinnable daemon (not named "ao") keeps the inherited PATH. +func pinnedEnv(base map[string]string) map[string]string { + path, err := sessionmanager.HookPATH(os.Executable, os.Getenv, base) + if err != nil { + return base + } + env := make(map[string]string, len(base)+1) + for k, v := range base { + env[k] = v + } + env["PATH"] = path + return env +} + func (l *agentLauncher) Notify(ctx context.Context, handleID string, spec LaunchSpec) error { reviewer, ok := l.reviewers.Reviewer(spec.Harness) if !ok { diff --git a/backend/internal/review/prompt.go b/backend/internal/review/prompt.go index a054fbfd..8448e57b 100644 --- a/backend/internal/review/prompt.go +++ b/backend/internal/review/prompt.go @@ -20,11 +20,13 @@ Post your review on the pull request using the available review tooling (request prompt = fmt.Sprintf(`Review pull request %s (head commit %s). -When done, write your full review to review.md and record the result with AO by running exactly: +Do these steps in order: +1. Post your review on the pull request with `+"`gh`"+` — request changes or approve, with inline comments for specific findings. +2. Write your full review to review.md and record the result with AO by running exactly: ao review submit --session %s --run %s --verdict --body review.md -If you cannot post the review on the provider, still run the command above so the result is recorded.`, +Only if step 1 genuinely fails on the provider, still run step 2 so the result is recorded.`, spec.PRURL, spec.TargetSHA, spec.WorkerID, spec.RunID) return prompt, systemPrompt } diff --git a/backend/internal/session_manager/manager.go b/backend/internal/session_manager/manager.go index af1aa594..624b1044 100644 --- a/backend/internal/session_manager/manager.go +++ b/backend/internal/session_manager/manager.go @@ -720,7 +720,7 @@ func spawnEnv(id domain.SessionID, project domain.ProjectID, issue domain.IssueI // logged so the degradation isn't silent. func (m *Manager) runtimeEnv(id domain.SessionID, project domain.ProjectID, issue domain.IssueID, projectEnv map[string]string) map[string]string { env := spawnEnv(id, project, issue, m.dataDir, projectEnv) - path, err := hookPATH(m.executable, os.Getenv, projectEnv) + path, err := HookPATH(m.executable, os.Getenv, projectEnv) if err != nil { m.logger.Warn("session PATH not pinned to the daemon binary; `ao hooks` callbacks may resolve to a different ao and activity tracking will stall", "session", id, "error", err) @@ -730,13 +730,14 @@ func (m *Manager) runtimeEnv(id domain.SessionID, project domain.ProjectID, issu return env } -// hookPATH builds the PATH value pinned into a spawned session: the daemon +// HookPATH builds the PATH value pinned into a spawned session: the daemon // executable's directory prepended to the base PATH (the project's PATH // override when set, else the daemon's inherited PATH — matching what the // runtime would have exported anyway). An error means the pin cannot be // applied: the executable is unresolvable, or is not named "ao", in which case -// prepending its directory would not change what `ao` resolves to. -func hookPATH(executable func() (string, error), getenv func(string) string, projectEnv map[string]string) (string, error) { +// prepending its directory would not change what `ao` resolves to. Exported so +// the reviewer launcher can pin its pane's PATH the same way. +func HookPATH(executable func() (string, error), getenv func(string) string, projectEnv map[string]string) (string, error) { exe, err := executable() if err != nil { return "", fmt.Errorf("resolve daemon executable: %w", err) diff --git a/backend/internal/session_manager/provision_test.go b/backend/internal/session_manager/provision_test.go index ba04606a..d82d6e35 100644 --- a/backend/internal/session_manager/provision_test.go +++ b/backend/internal/session_manager/provision_test.go @@ -82,18 +82,18 @@ func TestHookPATH(t *testing.T) { } return "" } - got, err := hookPATH(tc.executable, getenv, tc.projectEnv) + got, err := HookPATH(tc.executable, getenv, tc.projectEnv) if tc.wantErr { if err == nil { - t.Fatalf("hookPATH = %q, want error", got) + t.Fatalf("HookPATH = %q, want error", got) } return } if err != nil { - t.Fatalf("hookPATH: %v", err) + t.Fatalf("HookPATH: %v", err) } if got != tc.want { - t.Fatalf("hookPATH = %q, want %q", got, tc.want) + t.Fatalf("HookPATH = %q, want %q", got, tc.want) } }) } From eca07ce8cdb0c227a279e6282a04efe8449ebe0a Mon Sep 17 00:00:00 2001 From: Aditi Chauhan Date: Wed, 17 Jun 2026 00:45:06 +0530 Subject: [PATCH 2/2] fix(review): fall back to a comment review when self-approval is rejected GitHub does not let an author approve their own PR, so a reviewer running under the same account can't post an `approve`. Tell the reviewer to post the approval as a regular comment review (COMMENT event stating it is an approval) when the provider rejects the self-approval, instead of failing. Co-Authored-By: Claude Opus 4.8 --- backend/internal/review/prompt.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/backend/internal/review/prompt.go b/backend/internal/review/prompt.go index 8448e57b..1a5ee248 100644 --- a/backend/internal/review/prompt.go +++ b/backend/internal/review/prompt.go @@ -21,7 +21,9 @@ Post your review on the pull request using the available review tooling (request prompt = fmt.Sprintf(`Review pull request %s (head commit %s). Do these steps in order: -1. Post your review on the pull request with `+"`gh`"+` — request changes or approve, with inline comments for specific findings. +1. Post your review on the pull request with `+"`gh`"+`, with inline comments for specific findings: + - If changes are needed, request changes. + - If it is ready, approve it. GitHub does not let you approve a PR you opened — if the approval is rejected because you are the PR author, post the same review as a regular comment instead (a COMMENT-event review whose body states it is an approval). 2. Write your full review to review.md and record the result with AO by running exactly: ao review submit --session %s --run %s --verdict --body review.md