diff --git a/internal/orch/feedback.go b/internal/orch/feedback.go index 1c39a40..126f969 100644 --- a/internal/orch/feedback.go +++ b/internal/orch/feedback.go @@ -78,6 +78,28 @@ func (o *Orchestrator) ProcessFeedback(ctx context.Context, prID string) ([]*mod checkLogs = append(checkLogs, string(f.Kind)+": "+f.Body) } + sess, err := o.spawnPRFollowup(ctx, pr, role, strings.Join(checkLogs, "\n")) + if err != nil { + return nil, err + } + for _, f := range pending { + _ = o.st.MarkFeedbackHandled(f.ID, sess.ID) + } + o.audit(pr.ObjectiveID, sess.ID, "followup_spawned", + fmt.Sprintf("spawned %s for PR #%d", role, pr.Number), model.JSONMap{"pr_id": prID}) + return []*model.Session{sess}, nil +} + +// spawnPRFollowup creates a follow-up session bound to a PR and returns it. It is +// the single path for creating PR/CI follow-ups: it provisions the PR-branch +// workspace (the canonical checkout for the PR's branch, which update_pr pushes +// from), bakes the PR's internal id and branch into the goal, and records the +// pr_id in session metadata. Both the automatic feedback path (ProcessFeedback) +// and the manager's address_pr_feedback tool go through here, so a follow-up can +// never be created without the checkout it must push its fix back from. feedback +// is the body the agent acts on (review comments, check logs, or the manager's +// instructions). +func (o *Orchestrator) spawnPRFollowup(ctx context.Context, pr *model.PullRequest, role model.SessionRole, feedback string) (*model.Session, error) { // PR-branch workspace for the follow-up. Pick a schedulable target. target, err := o.SelectTarget(TargetRequest{}) if err != nil { @@ -102,7 +124,7 @@ func (o *Orchestrator) ProcessFeedback(ctx context.Context, prID string) ([]*mod BaseSHA: pr.HeadSHA, BranchName: pr.Branch, Status: model.WorkspacePreparing, - Metadata: prWorkspaceMeta(pr.Repo, prID, cloneURL, pushRepo), + Metadata: prWorkspaceMeta(pr.Repo, pr.ID, cloneURL, pushRepo), } if err := o.st.CreateWorkspace(ws); err != nil { return nil, err @@ -124,9 +146,9 @@ func (o *Orchestrator) ProcessFeedback(ctx context.Context, prID string) ([]*mod goal := fmt.Sprintf("Address feedback on PR #%d (%q) in repo %s.\n"+ "Use orcha pr_id=%q for your tool calls (update_pr / comment_pr take pr_id).\n"+ "This checkout is the PR branch %q with origin set.\n\nFeedback:\n%s\n\nPrior PR summary: %s", - pr.Number, pr.Title, pr.Repo, prID, pr.Branch, strings.Join(checkLogs, "\n"), pr.Summary) + pr.Number, pr.Title, pr.Repo, pr.ID, pr.Branch, feedback, pr.Summary) - sess, err := o.CreateSession(SpawnSpec{ + return o.CreateSession(SpawnSpec{ ObjectiveID: pr.ObjectiveID, Role: role, Agent: o.defaultAgent(), @@ -134,17 +156,8 @@ func (o *Orchestrator) ProcessFeedback(ctx context.Context, prID string) ([]*mod Title: fmt.Sprintf("Follow-up: PR #%d", pr.Number), Goal: goal, WorkspaceID: ws.ID, - Metadata: model.JSONMap{"pr_id": prID}, + Metadata: model.JSONMap{"pr_id": pr.ID}, }) - if err != nil { - return nil, err - } - for _, f := range pending { - _ = o.st.MarkFeedbackHandled(f.ID, sess.ID) - } - o.audit(pr.ObjectiveID, sess.ID, "followup_spawned", - fmt.Sprintf("spawned %s for PR #%d", role, pr.Number), model.JSONMap{"pr_id": prID}) - return []*model.Session{sess}, nil } // orchaBotMarker tags orcha's own PR comments so the monitor doesn't react to diff --git a/internal/orch/manager.go b/internal/orch/manager.go index 19bbc6e..852e463 100644 --- a/internal/orch/manager.go +++ b/internal/orch/manager.go @@ -10,7 +10,7 @@ import ( ) // The manager tool surface. These methods back the manager agent's tools -// (spawn_session, ask_user, publish_pr, update_pr, comment_pr, create_note, +// (spawn_session, ask_user, publish_pr, update_pr, comment_pr, address_pr_feedback, create_note, // mark_objective_done, cancel_session). Each acquires the objective_manager // lock so only one manager mutation per objective happens at a time. @@ -176,8 +176,8 @@ func (o *Orchestrator) MarkObjectiveDone(managerSessionID, summary string) error if len(open) > 0 { return fmt.Errorf("objective is not done: %d open PR(s) %v are not merged yet. "+ "It completes when its PRs merge — you are steered automatically then. If a PR "+ - "needs review replies, CI fixes, or conflict resolution, push fixes with update_pr "+ - "or spawn a follow-up; do NOT mark the objective done now", len(open), open) + "needs review replies, CI fixes, or conflict resolution, call address_pr_feedback "+ + "to push fixes; do NOT mark the objective done now", len(open), open) } } if err := o.withManagerLock(mgr.ObjectiveID, managerSessionID, func() error { diff --git a/internal/orch/manager_mcp.go b/internal/orch/manager_mcp.go index 6825d45..7e3f0d3 100644 --- a/internal/orch/manager_mcp.go +++ b/internal/orch/manager_mcp.go @@ -4,6 +4,8 @@ import ( "context" "fmt" "net/http" + "strconv" + "strings" "github.com/nathanwhit/orcha/internal/mcp" "github.com/nathanwhit/orcha/internal/model" @@ -24,9 +26,9 @@ func (o *Orchestrator) ManagerMCPHandler() http.Handler { s.AddTool(mcp.Tool{ Name: "spawn_session", - Description: "Spawn a scoped worker session under this objective. Returns the new session id. Use dependencies to make a session wait for others to succeed.", + Description: "Spawn a scoped worker session under this objective. Returns the new session id. Use dependencies to make a session wait for others to succeed. To address feedback or push a fix to an existing PR, use address_pr_feedback instead — not spawn_session.", InputSchema: obj(map[string]any{ - "role": map[string]any{"type": "string", "enum": []string{"implementer", "reviewer", "validator", "researcher", "pr_followup", "ci_followup", "custom"}}, + "role": map[string]any{"type": "string", "enum": []string{"implementer", "reviewer", "validator", "researcher", "custom"}}, "title": str, "goal": str, "agent_hint": map[string]any{"type": "string", "enum": []string{"claude", "codex"}}, @@ -72,10 +74,22 @@ func (o *Orchestrator) ManagerMCPHandler() http.Handler { }) s.AddTool(mcp.Tool{ Name: "comment_pr", - Description: "Leave a comment on a PR.", + Description: "Leave a comment on a PR. pr_id accepts the Orcha pr_id or the GitHub PR number.", InputSchema: obj(map[string]any{"pr_id": str, "body": str}, "pr_id", "body"), Handler: o.mcpCommentPR, }) + s.AddTool(mcp.Tool{ + Name: "address_pr_feedback", + Description: "Spawn a follow-up worker to address review feedback or push a fix to an existing PR. " + + "Use this for any PR follow-up instead of spawn_session: the worker gets a checkout of the PR " + + "branch and pushes its fix back to the same PR. pr_id accepts the Orcha pr_id or the GitHub PR number.", + InputSchema: obj(map[string]any{ + "pr_id": str, + "instructions": map[string]any{"type": "string", "description": "what the follow-up should do: the review comments to address or the fix to make"}, + "role": map[string]any{"type": "string", "enum": []string{"pr_followup", "ci_followup"}, "description": "ci_followup for CI/check failures; pr_followup (default) for review feedback"}, + }, "pr_id", "instructions"), + Handler: o.mcpAddressPRFeedback, + }) s.AddTool(mcp.Tool{ Name: "create_note", Description: "Record a note in the objective's shared memory (not stdout).", @@ -112,6 +126,14 @@ func (o *Orchestrator) mcpSpawnSession(ctx context.Context, args map[string]any) if err != nil { return "", err } + role := model.SessionRole(mcp.StringArg(args, "role")) + // PR/CI follow-ups need a PR-branch checkout that spawn_session cannot + // provision; routing them through address_pr_feedback is the only way they get + // one (without it the follow-up commits to a stranded branch and update_pr has + // nothing to push). Block them here so the enum can't be worked around. + if role == model.RolePRFollowup || role == model.RoleCIFollowup { + return "", fmt.Errorf("use address_pr_feedback (not spawn_session) for PR follow-ups: it provisions the PR-branch checkout the follow-up must push its fix from") + } agentHint := model.AgentKind(mcp.StringArg(args, "agent_hint")) meta := model.JSONMap{} if repo := mcp.StringArg(args, "repo"); repo != "" { @@ -137,7 +159,7 @@ func (o *Orchestrator) mcpSpawnSession(ctx context.Context, args map[string]any) meta = nil } sess, err := o.SpawnSession(mgr.ID, SpawnSpec{ - Role: model.SessionRole(mcp.StringArg(args, "role")), + Role: role, Title: mcp.StringArg(args, "title"), Goal: mcp.StringArg(args, "goal"), Agent: agentHint, @@ -175,6 +197,10 @@ func (o *Orchestrator) mcpPublishPR(ctx context.Context, args map[string]any) (s } func (o *Orchestrator) mcpUpdatePR(ctx context.Context, args map[string]any) (string, error) { + pr, err := o.resolvePR(ctx, mcp.StringArg(args, "pr_id")) + if err != nil { + return "", err + } // Default the pushing session to the caller (e.g. a follow-up pushing its own // checkout), so the agent need not know its own session/workspace id. sessionID := mcp.StringArg(args, "session_id") @@ -192,7 +218,7 @@ func (o *Orchestrator) mcpUpdatePR(ctx context.Context, args map[string]any) (st // so an agent that force-pushes after a rebase isn't blocked on wording. reason = "force update after history rewrite (e.g. rebase to resolve conflicts)" } - pr, err := o.UpdatePR(ctx, mcp.StringArg(args, "pr_id"), UpdateSpec{ + updated, err := o.UpdatePR(ctx, pr.ID, UpdateSpec{ SessionID: sessionID, WorkspaceID: workspaceID, Title: mcp.StringArg(args, "title"), @@ -204,16 +230,93 @@ func (o *Orchestrator) mcpUpdatePR(ctx context.Context, args map[string]any) (st if err != nil { return "", err } - return fmt.Sprintf("updated PR #%d (head %s)", pr.Number, pr.HeadSHA), nil + return fmt.Sprintf("updated PR #%d (head %s)", updated.Number, updated.HeadSHA), nil } func (o *Orchestrator) mcpCommentPR(ctx context.Context, args map[string]any) (string, error) { - if err := o.CommentPR(ctx, mcp.StringArg(args, "pr_id"), mcp.StringArg(args, "body")); err != nil { + pr, err := o.resolvePR(ctx, mcp.StringArg(args, "pr_id")) + if err != nil { + return "", err + } + if err := o.CommentPR(ctx, pr.ID, mcp.StringArg(args, "body")); err != nil { return "", err } return "comment posted.", nil } +func (o *Orchestrator) mcpAddressPRFeedback(ctx context.Context, args map[string]any) (string, error) { + pr, err := o.resolvePR(ctx, mcp.StringArg(args, "pr_id")) + if err != nil { + return "", err + } + role := model.RolePRFollowup + if mcp.StringArg(args, "role") == string(model.RoleCIFollowup) { + role = model.RoleCIFollowup + } + sess, err := o.spawnPRFollowup(ctx, pr, role, mcp.StringArg(args, "instructions")) + if err != nil { + return "", err + } + return fmt.Sprintf("spawned %s session %s for PR #%d on a checkout of branch %q; it will push its fix back to the PR.", + role, sess.ID, pr.Number, pr.Branch), nil +} + +// resolvePR resolves a pr_id tool argument to a PR. It accepts either the +// internal Orcha PR id (a UUID) or the GitHub PR number (e.g. "17"): the number +// is the identifier agents see on GitHub and in PR comments, so accepting it +// avoids the confusing "store: not found" an agent hits when it passes the number +// it knows. A bare number is resolved within the caller's objective. +func (o *Orchestrator) resolvePR(ctx context.Context, idArg string) (*model.PullRequest, error) { + idArg = strings.TrimSpace(idArg) + if idArg == "" { + return nil, fmt.Errorf("pr_id is required") + } + if pr, err := o.st.GetPR(idArg); err == nil { + return pr, nil + } + objID := o.callerObjective(ctx) + if n, convErr := strconv.Atoi(idArg); convErr == nil && n > 0 && objID != "" { + prs, _ := o.st.ListPRsByObjective(objID) + var match *model.PullRequest + for _, pr := range prs { + if pr.Number != n { + continue + } + // Prefer an open/draft PR if a number somehow appears more than once. + if match == nil || pr.Status == model.PROpen || pr.Status == model.PRDraft { + match = pr + } + } + if match != nil { + return match, nil + } + } + // Help the agent recover by listing the objective's PRs and their ids. + if objID != "" { + if prs, _ := o.st.ListPRsByObjective(objID); len(prs) > 0 { + var b strings.Builder + for _, pr := range prs { + fmt.Fprintf(&b, "\n #%d -> pr_id %s (%s)", pr.Number, pr.ID, pr.Status) + } + return nil, fmt.Errorf("no PR found for pr_id %q; this objective's PRs:%s", idArg, b.String()) + } + } + return nil, fmt.Errorf("no PR found for pr_id %q (pass the Orcha pr_id or the GitHub PR number)", idArg) +} + +// callerObjective returns the objective of the session bound to the MCP request, +// or "" if none is bound. +func (o *Orchestrator) callerObjective(ctx context.Context) string { + id := mcp.SessionFromContext(ctx) + if id == "" { + return "" + } + if s, err := o.st.GetSession(id); err == nil { + return s.ObjectiveID + } + return "" +} + func (o *Orchestrator) mcpCreateNote(ctx context.Context, args map[string]any) (string, error) { mgr, err := o.managerSession(ctx) if err != nil { diff --git a/internal/orch/orch_test.go b/internal/orch/orch_test.go index bed0f21..c33d6f3 100644 --- a/internal/orch/orch_test.go +++ b/internal/orch/orch_test.go @@ -1749,3 +1749,141 @@ func TestRecordUsage_IncrementsSessionAndProviderBucket(t *testing.T) { t.Fatalf("session used=%d after no-op calls, want 150", got) } } + +// ---- PR follow-up wiring (manager-spawned follow-ups & id resolution) ---- + +// update_pr/comment_pr must accept the GitHub PR number, not just the internal +// Orcha pr_id — agents naturally pass the number they see on GitHub. Regression +// for "store: not found" when a follow-up passed pr_id="17". +func TestUpdatePR_AcceptsGitHubNumber(t *testing.T) { + o, st := newTestOrch(t) + o.RegisterProvider(agent.NewFake(model.AgentClaude, true, nil)) + f := forge.NewFake() + o.SetForge(f) + obj, _, _ := o.CreateObjective(NewObjectiveSpec{Title: "x", Prompt: "p"}) + + ws := &model.Workspace{ObjectiveID: obj.ID, Kind: model.WorkspacePRBranch, ProjectPath: "octo/repo", + VCS: model.VCSGit, BranchName: "orcha/impl-x", Path: "/tmp/pr-7", Status: model.WorkspaceReady} + _ = st.CreateWorkspace(ws) + fu := &model.Session{ObjectiveID: obj.ID, Role: model.RolePRFollowup, Agent: model.AgentClaude, + Status: model.SessionRunning, WorkspaceID: ws.ID} + _ = st.CreateSession(fu) + pr := &model.PullRequest{ObjectiveID: obj.ID, Repo: "octo/repo", Number: 7, Branch: "orcha/impl-x", + BaseBranch: "main", Status: model.PROpen} + _ = st.CreatePR(pr) + + // Pass the GitHub number, not the UUID. + ctx := mcp.WithSession(context.Background(), fu.ID) + if _, err := o.mcpUpdatePR(ctx, map[string]any{"pr_id": "7"}); err != nil { + t.Fatalf("update_pr by GitHub number: %v", err) + } + if len(f.Pushes) == 0 { + t.Fatal("update_pr by GitHub number should have pushed the branch") + } +} + +// A bare unknown number returns a helpful error listing the objective's PRs. +func TestResolvePR_UnknownNumberListsPRs(t *testing.T) { + o, st := newTestOrch(t) + o.SetForge(forge.NewFake()) + obj, _, _ := o.CreateObjective(NewObjectiveSpec{Title: "x", Prompt: "p"}) + mgr := &model.Session{ObjectiveID: obj.ID, Role: model.RoleManager, Status: model.SessionRunning} + _ = st.CreateSession(mgr) + pr := &model.PullRequest{ObjectiveID: obj.ID, Repo: "octo/repo", Number: 7, Branch: "b", BaseBranch: "main", Status: model.PROpen} + _ = st.CreatePR(pr) + + ctx := mcp.WithSession(context.Background(), mgr.ID) + _, err := o.resolvePR(ctx, "999") + if err == nil { + t.Fatal("unknown number should error") + } + if !strings.Contains(err.Error(), "#7") || !strings.Contains(err.Error(), pr.ID) { + t.Fatalf("error should list the objective's PRs, got: %v", err) + } +} + +// spawn_session must refuse PR/CI follow-up roles: it can't provision the +// PR-branch checkout those roles need, so they must go through +// address_pr_feedback. This is the root-cause guard for the stranded-commit bug. +func TestSpawnSession_RejectsFollowupRoles(t *testing.T) { + o, st := newTestOrch(t) + obj, _, _ := o.CreateObjective(NewObjectiveSpec{Title: "x", Prompt: "p"}) + mgr := &model.Session{ObjectiveID: obj.ID, Role: model.RoleManager, Status: model.SessionRunning} + _ = st.CreateSession(mgr) + + ctx := mcp.WithSession(context.Background(), mgr.ID) + for _, role := range []string{"pr_followup", "ci_followup"} { + _, err := o.mcpSpawnSession(ctx, map[string]any{"role": role, "title": "t", "goal": "g"}) + if err == nil { + t.Fatalf("spawn_session must reject role %q", role) + } + if !strings.Contains(err.Error(), "address_pr_feedback") { + t.Fatalf("rejection should point at address_pr_feedback, got: %v", err) + } + } +} + +// address_pr_feedback provisions the PR-branch workspace and attaches the +// follow-up to the PR, accepting the GitHub number for pr_id. +func TestAddressPRFeedback_ProvisionsPRBranchWorkspace(t *testing.T) { + o, st := newTestOrch(t) + addTarget(t, st, "local", model.TargetLocal, 4) + o.RegisterProvider(agent.NewFake(model.AgentClaude, true, nil)) + o.SetForge(forge.NewFake()) + obj, _, _ := o.CreateObjective(NewObjectiveSpec{Title: "x", Prompt: "p"}) + mgr := &model.Session{ObjectiveID: obj.ID, Role: model.RoleManager, Status: model.SessionRunning} + _ = st.CreateSession(mgr) + pr := &model.PullRequest{ObjectiveID: obj.ID, Repo: "octo/repo", Number: 5, Branch: "feature", + BaseBranch: "main", HeadSHA: "sha1", Status: model.PROpen} + _ = st.CreatePR(pr) + + ctx := mcp.WithSession(context.Background(), mgr.ID) + if _, err := o.mcpAddressPRFeedback(ctx, map[string]any{"pr_id": "5", "instructions": "preserve unchanged fields"}); err != nil { + t.Fatalf("address_pr_feedback: %v", err) + } + + sessions, _ := st.ListSessionsByObjective(obj.ID) + var fu *model.Session + for _, s := range sessions { + if s.Role == model.RolePRFollowup { + fu = s + } + } + if fu == nil { + t.Fatal("address_pr_feedback should spawn a pr_followup session") + } + if fu.Metadata["pr_id"] != pr.ID { + t.Fatalf("follow-up must be attached to the PR, got pr_id=%v", fu.Metadata["pr_id"]) + } + if !strings.Contains(fu.Goal, "preserve unchanged fields") || !strings.Contains(fu.Goal, pr.ID) { + t.Fatalf("goal should carry the instructions and the pr_id, got: %q", fu.Goal) + } + ws, _ := st.GetWorkspace(fu.WorkspaceID) + if ws == nil || ws.Kind != model.WorkspacePRBranch || ws.BranchName != pr.Branch { + t.Fatalf("follow-up should use a PR-branch checkout, got %+v", ws) + } +} + +// UpdatePR must refuse to push when no checkout holds the PR branch, rather than +// running git in the orchestrator's own cwd. This is what turned a manager-spawned +// follow-up's stranded commit into a silent no-op. +func TestUpdatePR_NoCheckoutRefuses(t *testing.T) { + o, st := newTestOrch(t) + f := forge.NewFake() + o.SetForge(f) + obj, _, _ := o.CreateObjective(NewObjectiveSpec{Title: "x", Prompt: "p"}) + pr := &model.PullRequest{ObjectiveID: obj.ID, Repo: "octo/repo", Number: 4, Branch: "b", + BaseBranch: "main", Status: model.PROpen} + _ = st.CreatePR(pr) + + _, err := o.UpdatePR(context.Background(), pr.ID, UpdateSpec{SessionID: "x"}) + if err == nil { + t.Fatal("update_pr with no PR-branch checkout must error") + } + if !strings.Contains(err.Error(), "no PR-branch checkout") { + t.Fatalf("error should explain the missing checkout, got: %v", err) + } + if len(f.Pushes) != 0 { + t.Fatalf("no push should have happened, got %d", len(f.Pushes)) + } +} diff --git a/internal/orch/pr.go b/internal/orch/pr.go index a40ed52..5e45d1a 100644 --- a/internal/orch/pr.go +++ b/internal/orch/pr.go @@ -37,6 +37,44 @@ func (o *Orchestrator) forgeForWorkspace(ws *model.Workspace) forge.Forge { return o.forgeFor(tgt) } +// prBranchWorkspace resolves the checkout a PR follow-up should push from. It +// prefers an explicitly supplied workspace (the follow-up session's own checkout) +// when that workspace actually tracks the PR's branch, and otherwise finds the +// ready PR-branch workspace recorded for this PR. It returns nil when neither +// resolves, so UpdatePR can refuse to push instead of running git in the +// orchestrator's own cwd against the wrong (or a missing) branch. +func (o *Orchestrator) prBranchWorkspace(workspaceID string, pr *model.PullRequest) *model.Workspace { + isForPR := func(ws *model.Workspace) bool { + if ws == nil { + return false + } + if id, _ := ws.Metadata["pr_id"].(string); id == pr.ID { + return true + } + // A PR-branch checkout tracks the PR's branch even without pr_id metadata. + return ws.Kind == model.WorkspacePRBranch && ws.BranchName != "" && ws.BranchName == pr.Branch + } + if workspaceID != "" { + if ws, err := o.st.GetWorkspace(workspaceID); err == nil && isForPR(ws) { + return ws + } + } + wss, err := o.st.ListWorkspacesByObjective(pr.ObjectiveID) + if err != nil { + return nil + } + var best *model.Workspace + for _, ws := range wss { + if ws.Kind != model.WorkspacePRBranch || ws.Status != model.WorkspaceReady || !isForPR(ws) { + continue + } + if best == nil || ws.CreatedAt.After(best.CreatedAt) { + best = ws + } + } + return best +} + // PublishSpec carries the agent-supplied PR content. type PublishSpec struct { Title string @@ -195,12 +233,11 @@ func (o *Orchestrator) UpdatePR(ctx context.Context, prID string, spec UpdateSpe if err != nil { return nil, err } - // Run git/gh on the machine holding the PR-branch checkout (the follow-up - // workspace's target), not the orchestrator host. - var fws *model.Workspace - if spec.WorkspaceID != "" { - fws, _ = o.st.GetWorkspace(spec.WorkspaceID) - } + // Resolve the checkout that actually holds the PR branch and run git/gh on its + // target, not the orchestrator host. fws may be nil here — the state refresh + // and the merged/closed/force checks below don't need a checkout, so we defer + // the "no checkout" failure until just before the push. + fws := o.prBranchWorkspace(spec.WorkspaceID, pr) f := o.forgeForWorkspace(fws) // Refresh host state before deciding anything. @@ -235,6 +272,15 @@ func (o *Orchestrator) UpdatePR(ctx context.Context, prID string, spec UpdateSpe return pr, errors.New("orch: force push requires an explicit reason") } + // A push needs a checkout of the PR branch. Refuse rather than fall back to + // the orchestrator host's cwd (which would push the wrong branch, or nothing, + // silently). This is the case a manager-spawned follow-up with no PR-branch + // workspace used to hit — its fix would commit to a stranded local branch and + // update_pr would appear to do nothing. + if fws == nil { + return pr, fmt.Errorf("%w: no PR-branch checkout for PR #%d — spawn the follow-up with address_pr_feedback so its fix lands on the PR branch", ErrUnsafePublish, pr.Number) + } + // One updater per PR branch, keyed by PR id. lockKey := prBranchLockKey(prID) if err := o.st.AcquireLock(lockKey, model.LockPRBranch, spec.SessionID, "follow-up update"); err != nil { @@ -245,10 +291,7 @@ func (o *Orchestrator) UpdatePR(ctx context.Context, prID string, spec UpdateSpe } defer o.st.ReleaseLock(lockKey, spec.SessionID) - wsPath := "" - if fws != nil { - wsPath = fws.Path - } + wsPath := fws.Path // Fallback: commit any edits the agent left uncommitted (normally the agent // commits its own work with its own message, so this is a no-op). if wsPath != "" { diff --git a/internal/orch/steering.go b/internal/orch/steering.go index 8d29f8e..db43239 100644 --- a/internal/orch/steering.go +++ b/internal/orch/steering.go @@ -202,17 +202,18 @@ Finish with a brief summary of what you changed.` const managerSystemPreamble = `You are the MANAGER of an engineering team working toward an objective. You coordinate via your orcha MCP tools (named mcp__orcha*): spawn_session to delegate scoped work, ask_user when direction/credentials are unclear, publish_pr to ship -coherent slices, comment_pr/update_pr for follow-ups, create_note for shared -memory, and mark_objective_done when finished. Prefer several clean PR-sized +coherent slices, address_pr_feedback to push a fix or reply to an existing PR, +create_note for shared memory, and mark_objective_done when finished. Prefer several clean PR-sized slices over one giant PR. Keep working after publishing intermediate PRs unless truly blocked. A published PR is NOT a finished objective — the work lands when the PR MERGES. After publishing, do NOT call mark_objective_done while any PR is still open; it will be refused. Wait — you are steered automatically when a PR merges, gets review comments, fails CI, or has merge conflicts. To resolve conflicts or -address feedback, push fixes with update_pr or spawn a follow-up; do not merge -PRs yourself. Call mark_objective_done only once every PR has merged (or there -was never a PR to open). +address feedback, call address_pr_feedback (NOT spawn_session) — it gives the +follow-up a checkout of the PR branch so its fix is pushed back to the same PR; +do not merge PRs yourself. Call mark_objective_done only once every PR has merged +(or there was never a PR to open). When the objective names a repo, you are running in a fresh checkout of it: explore the code first and scope workers' goals precisely, with verified file references. Do NOT code, commit, or push yourself — workers do the coding in